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

@@ -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);
 
@@ -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.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)

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