Merge remote-tracking branch 'origin/topic/timw/174-duplicate-attributes'

- Replaced "tag" with "attribute" in the error message since the former
  is not exactly the same concept/meaning and also not user-facing
  terminology

* origin/topic/timw/174-duplicate-attributes:
  Allow duplicate attributes in full redefs
  Short-circuit checking of whether attr exists
  Expanded check for other tag types, fixed btest to cover more tags
  GH-174: Add warning for duplicate attributes

CHANGES

@@ -1,4 +1,12 @@
 
+3.3.0-dev.221 | 2020-08-31 17:31:21 -0700
+
+  * GH-174: Treat ambiguous attribute duplication as an error (Tim Wojtulewicz, Corelight)
+
+    For example, a &default=1 and a &default=2 attribute are not valid when
+    used together, but two duplicate &log attributes together are acceptable
+    although redundant.
+
 3.3.0-dev.216 | 2020-08-31 14:57:57 -0700
 
   * Simplify a broker btest (Jon Siwek, Corelight)

VERSION

@@ -1 +1 @@
-3.3.0-dev.216
+3.3.0-dev.221

src/Attr.cc

@@ -193,8 +193,35 @@ Attributes::Attributes(std::vector<AttrPtr> a,
 		AddAttr(std::move(attr));
 	}
 
-void Attributes::AddAttr(AttrPtr attr)
+void Attributes::AddAttr(AttrPtr attr, bool is_redef)
 	{
+	auto acceptable_duplicate_attr = [](const AttrPtr& attr, const AttrPtr& existing) -> bool
+		{
+		AttrTag new_tag = attr->Tag();
+
+		if ( new_tag == ATTR_DEPRECATED )
+			{
+			if ( ! attr->DeprecationMessage().empty() ||
+			     ( existing && ! existing->DeprecationMessage().empty() ) )
+				return false;
+
+			return true;
+			}
+
+		return new_tag == ATTR_LOG || new_tag == ATTR_OPTIONAL || new_tag == ATTR_REDEF ||
+			new_tag == ATTR_BROKER_STORE_ALLOW_COMPLEX || new_tag == ATTR_RAW_OUTPUT ||
+			new_tag == ATTR_ERROR_HANDLER;
+		};
+
+	// A `redef` is allowed to overwrite an existing attribute instead of
+	// flagging it as ambiguous.
+	if ( ! is_redef )
+		{
+		auto existing = Find(attr->Tag());
+		if ( existing && ! acceptable_duplicate_attr(attr, existing) )
+			reporter->Error("Duplicate %s attribute is ambiguous", attr_name(attr->Tag()));
+		}
+
 	// We overwrite old attributes by deleting them first.
 	RemoveAttr(attr->Tag());
 	attrs_list.push_back(attr.get());
@@ -224,16 +251,16 @@ void Attributes::AddAttr(AttrPtr attr)
 		}
 	}
 
-void Attributes::AddAttrs(const AttributesPtr& a)
+void Attributes::AddAttrs(const AttributesPtr& a, bool is_redef)
 	{
 	for ( const auto& attr : a->GetAttrs() )
-		AddAttr(attr);
+		AddAttr(attr, is_redef);
 	}
 
-void Attributes::AddAttrs(Attributes* a)
+void Attributes::AddAttrs(Attributes* a, bool is_redef)
 	{
 	for ( const auto& attr : a->GetAttrs() )
-		AddAttr(attr);
+		AddAttr(attr, is_redef);
 
 	Unref(a);
 	}

src/Attr.h

@@ -115,12 +115,12 @@ public:
 
 	~Attributes() override = default;
 
-	void AddAttr(AttrPtr a);
+	void AddAttr(AttrPtr a, bool is_redef = false);
 
-	void AddAttrs(const AttributesPtr& a);
+	void AddAttrs(const AttributesPtr& a, bool is_redef = false);
 
 	[[deprecated("Remove in v4.1. Pass IntrusivePtr instead.")]]
-	void AddAttrs(Attributes* a);	// Unref's 'a' when done
+	void AddAttrs(Attributes* a, bool is_redef = false);	// Unref's 'a' when done
 
 	[[deprecated("Remove in v4.1. Use Find().")]]
 	Attr* FindAttr(AttrTag t) const;

src/ID.cc

@@ -316,10 +316,10 @@ std::string ID::GetDeprecationWarning() const
 		return util::fmt("deprecated (%s): %s", Name(), result.c_str());
 	}
 
-void ID::AddAttrs(AttributesPtr a)
+void ID::AddAttrs(AttributesPtr a, bool is_redef)
 	{
 	if ( attrs )
-		attrs->AddAttrs(a);
+		attrs->AddAttrs(a, is_redef);
 	else
 		attrs = std::move(a);
 

src/ID.h

@@ -121,7 +121,7 @@ public:
 	bool IsRedefinable() const;
 
 	void SetAttrs(AttributesPtr attr);
-	void AddAttrs(AttributesPtr attr);
+	void AddAttrs(AttributesPtr attr, bool is_redef = false);
 	void RemoveAttr(AttrTag a);
 	void UpdateValAttrs();
 

src/Var.cc

@@ -207,7 +207,7 @@ static void make_var(const IDPtr& id, TypePtr t, InitClass c, ExprPtr init,
 	id->SetType(t);
 
 	if ( attr )
-		id->AddAttrs(make_intrusive<Attributes>(std::move(*attr), t, false, id->IsGlobal()));
+		id->AddAttrs(make_intrusive<Attributes>(std::move(*attr), t, false, id->IsGlobal()), dt == VAR_REDEF);
 
 	if ( init )
 		{

testing/btest/Baseline/language.attr-default-global-set-error/out

@@ -1,5 +1,6 @@
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 4: &default is not valid for global variables except for tables (&default=0)
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=10)
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=9)
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: &optional is not valid for global variables (&default=9, &optional)
-error in /home/jon/pro/zeek/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 10: &default is not valid for global variables except for tables (&default=set())
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 4: &default is not valid for global variables except for tables (&default=0)
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=10)
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: Duplicate &default attribute is ambiguous
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: &default is not valid for global variables except for tables (&default=9)
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: &optional is not valid for global variables (&default=9, &optional)
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 10: &default is not valid for global variables except for tables (&default=set())

testing/btest/Baseline/language.duplicate-attributes/out

@@ -0,0 +1,3 @@
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 4: Duplicate &default attribute is ambiguous
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 6: Duplicate &deprecated attribute is ambiguous
+error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 7: Duplicate &deprecated attribute is ambiguous

testing/btest/language/duplicate-attributes.zeek

@@ -0,0 +1,8 @@
+# @TEST-EXEC-FAIL: zeek -b %INPUT >out 2>&1
+# @TEST-EXEC: TEST_DIFF_CANONIFIER=$SCRIPTS/diff-remove-abspath btest-diff out
+
+global a: table[count] of count &default = 10 &default = 9;
+global b: table[count] of count &deprecated &deprecated;
+global c: table[count] of count &deprecated="a" &deprecated;
+global d: table[count] of count &deprecated &deprecated="a";
+global e: table[count] of count &redef &redef;