From 4e5a56c5e0969977243c413f1edcfbf66cd5b2ed Mon Sep 17 00:00:00 2001 From: Evan Typanski Date: Thu, 14 Aug 2025 09:29:50 -0400 Subject: [PATCH 1/2] Only allow `&optional` in records There was some confusing behavior with &optional and locals, so this should get rid of that by making it an error. However, there is a case where function parameters are still allowed to have &optional - this is because there are checks for &default in parameters as well. --- src/Attr.cc | 6 +++--- .../language.attr-default-global-set-error/out | 2 +- .../Baseline/language.invalid-optional-attr/out | 3 +++ testing/btest/language/invalid-optional-attr.zeek | 12 ++++++++++++ 4 files changed, 19 insertions(+), 4 deletions(-) create mode 100644 testing/btest/Baseline/language.invalid-optional-attr/out create mode 100644 testing/btest/language/invalid-optional-attr.zeek diff --git a/src/Attr.cc b/src/Attr.cc index 52c7c461b6..998d48e9b5 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -297,10 +297,10 @@ bool Attributes::CheckAttr(Attr* a) { case ATTR_IS_USED: break; case ATTR_OPTIONAL: - if ( global_var ) - return AttrError("&optional is not valid for global variables"); + if ( ! in_record ) + return AttrError("&optional is only valid for record fields"); - if ( in_record && Find(ATTR_DEFAULT) ) + if ( Find(ATTR_DEFAULT) ) return AttrError("Using &default and &optional together results in &default behavior"); break; diff --git a/testing/btest/Baseline/language.attr-default-global-set-error/out b/testing/btest/Baseline/language.attr-default-global-set-error/out index 431b2d445b..683c0c0db5 100644 --- a/testing/btest/Baseline/language.attr-default-global-set-error/out +++ b/testing/btest/Baseline/language.attr-default-global-set-error/out @@ -2,5 +2,5 @@ error in <...>/attr-default-global-set-error.zeek, line 4: &default is not valid for global variables except for tables (&default=0) error in <...>/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=10) error in <...>/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=9) -error in <...>/attr-default-global-set-error.zeek, line 9: &optional is not valid for global variables (&optional) +error in <...>/attr-default-global-set-error.zeek, line 9: &optional is only valid for record fields (&optional) error in <...>/attr-default-global-set-error.zeek, line 10: &default is not valid for global variables except for tables (&default=set()) diff --git a/testing/btest/Baseline/language.invalid-optional-attr/out b/testing/btest/Baseline/language.invalid-optional-attr/out new file mode 100644 index 0000000000..e862d4bb9b --- /dev/null +++ b/testing/btest/Baseline/language.invalid-optional-attr/out @@ -0,0 +1,3 @@ +### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. +error in <...>/invalid-optional-attr.zeek, line 5: &optional is only valid for record fields (&optional) +error in <...>/invalid-optional-attr.zeek, line 11: &optional is only valid for record fields (&optional) diff --git a/testing/btest/language/invalid-optional-attr.zeek b/testing/btest/language/invalid-optional-attr.zeek new file mode 100644 index 0000000000..338408caf4 --- /dev/null +++ b/testing/btest/language/invalid-optional-attr.zeek @@ -0,0 +1,12 @@ +# @TEST-EXEC-FAIL: zeek -b %INPUT >out 2>&1 +# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out + +# Invalid on globals +global a: int &optional; + +# TODO: Invalid on parameters +function f(a: int &optional) + { + # Invalid in locals + local b: int &optional; + } From 4445bc1dafc8bed88bf26cf57221a3f768db7fde Mon Sep 17 00:00:00 2001 From: Evan Typanski Date: Thu, 14 Aug 2025 11:47:20 -0400 Subject: [PATCH 2/2] Fix parameter attributes pretending to be records Parameters relied on is_record for a couple of validations, but they are not records and should not be treated as such. This way we can validate &optional better. --- src/Attr.cc | 18 +++++++----------- src/Attr.h | 11 +++++++++-- src/parse.y | 8 ++++---- .../language.invalid-optional-attr/out | 1 + .../btest/language/invalid-optional-attr.zeek | 2 +- 5 files changed, 22 insertions(+), 18 deletions(-) diff --git a/src/Attr.cc b/src/Attr.cc index 998d48e9b5..9a8aa3396a 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -166,13 +166,9 @@ detail::TraversalCode Attr::Traverse(detail::TraversalCallback* cb) const { HANDLE_TC_ATTR_POST(tc); } -Attributes::Attributes(TypePtr t, bool arg_in_record, bool is_global) - : Attributes(std::vector{}, std::move(t), arg_in_record, is_global) {} - -Attributes::Attributes(std::vector a, TypePtr t, bool arg_in_record, bool is_global) : type(std::move(t)) { +Attributes::Attributes(std::vector a, TypePtr t, bool in_record, bool is_global, bool is_param) + : type(std::move(t)), in_record(in_record), global_var(is_global), is_param(is_param) { attrs.reserve(a.size()); - in_record = arg_in_record; - global_var = is_global; SetLocationInfo(&start_location, &end_location); @@ -331,7 +327,7 @@ bool Attributes::CheckAttr(Attr* a) { return AttrError("&default and &default_insert cannot be used together"); std::string err_msg; - if ( ! check_default_attr(a, type, global_var, in_record, err_msg) && ! err_msg.empty() ) + if ( ! check_default_attr(a, type, global_var, in_record || is_param, err_msg) && ! err_msg.empty() ) return AttrError(err_msg.c_str()); break; } @@ -344,7 +340,7 @@ bool Attributes::CheckAttr(Attr* a) { return AttrError("Using &default and &optional together results in &default behavior"); std::string err_msg; - if ( ! check_default_attr(a, type, global_var, in_record, err_msg) && ! err_msg.empty() ) + if ( ! check_default_attr(a, type, global_var, in_record || is_param, err_msg) && ! err_msg.empty() ) return AttrError(err_msg.c_str()); break; } @@ -586,7 +582,7 @@ bool Attributes::operator==(const Attributes& other) const { return true; } -bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool in_record, std::string& err_msg) { +bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool record_like, std::string& err_msg) { ASSERT(a->Tag() == ATTR_DEFAULT || a->Tag() == ATTR_DEFAULT_INSERT); std::string aname = attr_name(a->Tag()); // &default is allowed for global tables, since it's used in @@ -598,7 +594,7 @@ bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool in_r const auto& atype = a->GetExpr()->GetType(); - if ( type->Tag() != TYPE_TABLE || (type->IsSet() && ! in_record) ) { + if ( type->Tag() != TYPE_TABLE || (type->IsSet() && ! record_like) ) { if ( same_type(atype, type) ) // Ok. return true; @@ -628,7 +624,7 @@ bool check_default_attr(Attr* a, const TypePtr& type, bool global_var, bool in_r TableType* tt = type->AsTableType(); const auto& ytype = tt->Yield(); - if ( ! in_record ) { // &default applies to the type itself. + if ( ! record_like ) { // &default applies to the type itself. if ( same_type(atype, ytype) ) return true; diff --git a/src/Attr.h b/src/Attr.h index e44de83b20..15cb471c20 100644 --- a/src/Attr.h +++ b/src/Attr.h @@ -108,8 +108,14 @@ protected: // Manages a collection of attributes. class Attributes final : public Obj { public: - Attributes(std::vector a, TypePtr t, bool in_record, bool is_global); - Attributes(TypePtr t, bool in_record, bool is_global); + Attributes(std::vector a, TypePtr t, bool in_record, bool is_global, bool is_param); + + Attributes(TypePtr t, bool in_record, bool is_global) + : Attributes(std::vector{}, std::move(t), in_record, is_global, false) {} + + Attributes(std::vector a, TypePtr t, bool in_record, bool is_global) + : Attributes(a, std::move(t), in_record, is_global, false) {} + ~Attributes() override = default; @@ -142,6 +148,7 @@ protected: bool in_record; bool global_var; + bool is_param; }; // Checks whether default attribute "a" is compatible with the given type. diff --git a/src/parse.y b/src/parse.y index 0220e64615..ea4fc7ae26 100644 --- a/src/parse.y +++ b/src/parse.y @@ -257,11 +257,11 @@ static void extend_record(ID* id, std::unique_ptr fields, std::u } } -static AttributesPtr make_attributes(std::vector* attrs, TypePtr t, bool in_record, bool is_global) { +static AttributesPtr make_attributes(std::vector* attrs, TypePtr t, bool in_record, bool is_param) { if ( ! attrs ) return nullptr; - auto rval = make_intrusive(std::move(*attrs), std::move(t), in_record, is_global); + auto rval = make_intrusive(std::move(*attrs), std::move(t), in_record, /*is_global=*/false, is_param); delete attrs; return rval; } @@ -1296,7 +1296,7 @@ type_decl: TOK_ID ':' type opt_attr ';' { set_location(@1, @4); - auto attrs = make_attributes($4, {NewRef{}, $3}, in_record > 0, false); + auto attrs = make_attributes($4, {NewRef{}, $3}, in_record > 0, /*is_param=*/false); $$ = new TypeDecl($1, {AdoptRef{}, $3}, std::move(attrs)); if ( in_record > 0 && cur_decl_type_id ) @@ -1327,7 +1327,7 @@ formal_args_decl: TOK_ID ':' type opt_attr { set_location(@1, @4); - auto attrs = make_attributes($4, {NewRef{}, $3}, true, false); + auto attrs = make_attributes($4, {NewRef{}, $3}, /*in_record=*/false, /*is_param=*/true); $$ = new TypeDecl($1, {AdoptRef{}, $3}, std::move(attrs)); } ; diff --git a/testing/btest/Baseline/language.invalid-optional-attr/out b/testing/btest/Baseline/language.invalid-optional-attr/out index e862d4bb9b..b9e20e582c 100644 --- a/testing/btest/Baseline/language.invalid-optional-attr/out +++ b/testing/btest/Baseline/language.invalid-optional-attr/out @@ -1,3 +1,4 @@ ### BTest baseline data generated by btest-diff. Do not edit. Use "btest -U/-u" to update. Requires BTest >= 0.63. error in <...>/invalid-optional-attr.zeek, line 5: &optional is only valid for record fields (&optional) +error in <...>/invalid-optional-attr.zeek, line 8: &optional is only valid for record fields (&optional) error in <...>/invalid-optional-attr.zeek, line 11: &optional is only valid for record fields (&optional) diff --git a/testing/btest/language/invalid-optional-attr.zeek b/testing/btest/language/invalid-optional-attr.zeek index 338408caf4..33d6903f91 100644 --- a/testing/btest/language/invalid-optional-attr.zeek +++ b/testing/btest/language/invalid-optional-attr.zeek @@ -4,7 +4,7 @@ # Invalid on globals global a: int &optional; -# TODO: Invalid on parameters +# Invalid on parameters function f(a: int &optional) { # Invalid in locals