Merge remote-tracking branch 'origin/topic/etyp/fix-optional-attr-errors'

* origin/topic/etyp/fix-optional-attr-errors:
  Fix parameter attributes pretending to be records
  Only allow `&optional` in records

CHANGES

@@ -1,3 +1,18 @@
+8.1.0-dev.87 | 2025-08-18 14:39:10 -0400
+
+  * Fix parameter attributes pretending to be records (Evan Typanski, Corelight)
+
+    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.
+
+  * Only allow `&optional` in records (Evan Typanski, Corelight)
+
+    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.
+
 8.1.0-dev.83 | 2025-08-18 09:40:41 -0700
 
   * Add `record_type_to_vector` deprecation to NEWS (Evan Typanski, Corelight)

NEWS

@@ -11,6 +11,9 @@ We would like to thank ... for their contributions to this release.
 Breaking Changes
 ----------------
 
+- The ``&optional`` script attribute will now error when applied to anything that's
+  not a record field. Previously, this would have surprising behavior.
+
 New Functionality
 -----------------
 

VERSION

@@ -1 +1 @@
-8.1.0-dev.83
+8.1.0-dev.87

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<AttrPtr>{}, std::move(t), arg_in_record, is_global) {}
-
-Attributes::Attributes(std::vector<AttrPtr> a, TypePtr t, bool arg_in_record, bool is_global) : type(std::move(t)) {
+Attributes::Attributes(std::vector<AttrPtr> 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);
 
@@ -297,10 +293,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;
@@ -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;
 

src/Attr.h

@@ -108,8 +108,14 @@ protected:
 // Manages a collection of attributes.
 class Attributes final : public Obj {
 public:
-    Attributes(std::vector<AttrPtr> a, TypePtr t, bool in_record, bool is_global);
-    Attributes(TypePtr t, bool in_record, bool is_global);
+    Attributes(std::vector<AttrPtr> a, TypePtr t, bool in_record, bool is_global, bool is_param);
+
+    Attributes(TypePtr t, bool in_record, bool is_global)
+        : Attributes(std::vector<AttrPtr>{}, std::move(t), in_record, is_global, false) {}
+
+    Attributes(std::vector<AttrPtr> 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.

src/parse.y

@@ -257,11 +257,11 @@ static void extend_record(ID* id, std::unique_ptr<type_decl_list> fields, std::u
     }
 }
 
-static AttributesPtr make_attributes(std::vector<AttrPtr>* attrs, TypePtr t, bool in_record, bool is_global) {
+static AttributesPtr make_attributes(std::vector<AttrPtr>* attrs, TypePtr t, bool in_record, bool is_param) {
     if ( ! attrs )
         return nullptr;
 
-    auto rval = make_intrusive<Attributes>(std::move(*attrs), std::move(t), in_record, is_global);
+    auto rval = make_intrusive<Attributes>(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));
 			}
 	;

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())

testing/btest/Baseline/language.invalid-optional-attr/out

@@ -0,0 +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)

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;
+
+# Invalid on parameters
+function f(a: int &optional)
+	{
+	# Invalid in locals
+	local b: int &optional;
+	}