From 989531826fb77c3f5edae616ff05cd32451f439e Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Mon, 24 Aug 2020 08:24:12 -0700 Subject: [PATCH 1/4] GH-174: Add warning for duplicate attributes --- src/Attr.cc | 6 ++++++ .../language.attr-default-global-set-error/out | 11 ++++++----- .../btest/Baseline/language.duplicate-attributes/out | 2 ++ testing/btest/language/duplicate-attributes.zeek | 6 ++++++ 4 files changed, 20 insertions(+), 5 deletions(-) create mode 100644 testing/btest/Baseline/language.duplicate-attributes/out create mode 100644 testing/btest/language/duplicate-attributes.zeek diff --git a/src/Attr.cc b/src/Attr.cc index 8f6b54a13c..a5046dd764 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -195,6 +195,12 @@ Attributes::Attributes(std::vector a, void Attributes::AddAttr(AttrPtr attr) { + // Display a warning for duplicated tags on a type. &log tags are intentionally + // ignored here because duplicate log tags on record types are valid, and don't + // cause any significant breakage for other types. + if ( attr->Tag() != ATTR_LOG && Find(attr->Tag()) ) + reporter->Warning("Found duplicate tag %s", attr_name(attr->Tag())); + // We overwrite old attributes by deleting them first. RemoveAttr(attr->Tag()); attrs_list.push_back(attr.get()); 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 ddcc6a8345..27ebbf3356 100644 --- a/testing/btest/Baseline/language.attr-default-global-set-error/out +++ b/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 /mnt/data/tim/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 /mnt/data/tim/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) +warning in /mnt/data/tim/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: Found duplicate tag &default +error in /mnt/data/tim/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 /mnt/data/tim/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 /mnt/data/tim/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()) diff --git a/testing/btest/Baseline/language.duplicate-attributes/out b/testing/btest/Baseline/language.duplicate-attributes/out new file mode 100644 index 0000000000..422d5f94e1 --- /dev/null +++ b/testing/btest/Baseline/language.duplicate-attributes/out @@ -0,0 +1,2 @@ +warning in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 6: Found duplicate tag &default +warning in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 6: Found duplicate tag &read_expire diff --git a/testing/btest/language/duplicate-attributes.zeek b/testing/btest/language/duplicate-attributes.zeek new file mode 100644 index 0000000000..b25a45d033 --- /dev/null +++ b/testing/btest/language/duplicate-attributes.zeek @@ -0,0 +1,6 @@ +# @TEST-EXEC: 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 + &read_expire = 5 sec &read_expire = 1 min; From 36e3ab7177dac651b7229f354ff26b2d05eab341 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Thu, 27 Aug 2020 15:04:10 -0700 Subject: [PATCH 2/4] Expanded check for other tag types, fixed btest to cover more tags --- src/Attr.cc | 25 +++++++++++++++++-- .../out | 12 ++++----- .../language.duplicate-attributes/out | 5 ++-- .../btest/language/duplicate-attributes.zeek | 10 +++++--- 4 files changed, 38 insertions(+), 14 deletions(-) diff --git a/src/Attr.cc b/src/Attr.cc index a5046dd764..4da14e4962 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -195,11 +195,32 @@ Attributes::Attributes(std::vector a, void Attributes::AddAttr(AttrPtr attr) { + auto acceptable_duplicate_tag = [&](AttrPtr attr) -> bool + { + AttrTag new_tag = attr->Tag(); + + if ( new_tag == ATTR_DEPRECATED ) + { + if ( ! attr->DeprecationMessage().empty() ) + return false; + + auto existing = Find(new_tag); + if ( 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; + }; + // Display a warning for duplicated tags on a type. &log tags are intentionally // ignored here because duplicate log tags on record types are valid, and don't // cause any significant breakage for other types. - if ( attr->Tag() != ATTR_LOG && Find(attr->Tag()) ) - reporter->Warning("Found duplicate tag %s", attr_name(attr->Tag())); + if ( ! acceptable_duplicate_tag(attr) && Find(attr->Tag()) ) + reporter->Error("Duplicate %s tag is ambiguous", attr_name(attr->Tag())); // We overwrite old attributes by deleting them first. RemoveAttr(attr->Tag()); 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 27ebbf3356..280704c371 100644 --- a/testing/btest/Baseline/language.attr-default-global-set-error/out +++ b/testing/btest/Baseline/language.attr-default-global-set-error/out @@ -1,6 +1,6 @@ -error in /mnt/data/tim/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 /mnt/data/tim/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) -warning in /mnt/data/tim/zeek/testing/btest/.tmp/language.attr-default-global-set-error/attr-default-global-set-error.zeek, line 9: Found duplicate tag &default -error in /mnt/data/tim/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 /mnt/data/tim/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 /mnt/data/tim/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 tag 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()) diff --git a/testing/btest/Baseline/language.duplicate-attributes/out b/testing/btest/Baseline/language.duplicate-attributes/out index 422d5f94e1..9765ce19e5 100644 --- a/testing/btest/Baseline/language.duplicate-attributes/out +++ b/testing/btest/Baseline/language.duplicate-attributes/out @@ -1,2 +1,3 @@ -warning in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 6: Found duplicate tag &default -warning in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 6: Found duplicate tag &read_expire +error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 4: Duplicate &default tag is ambiguous +error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 6: Duplicate &deprecated tag is ambiguous +error in /Users/tim/Desktop/projects/zeek/testing/btest/.tmp/language.duplicate-attributes/duplicate-attributes.zeek, line 7: Duplicate &deprecated tag is ambiguous diff --git a/testing/btest/language/duplicate-attributes.zeek b/testing/btest/language/duplicate-attributes.zeek index b25a45d033..f8114c5f77 100644 --- a/testing/btest/language/duplicate-attributes.zeek +++ b/testing/btest/language/duplicate-attributes.zeek @@ -1,6 +1,8 @@ -# @TEST-EXEC: zeek -b %INPUT >out 2>&1 +# @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 - &read_expire = 5 sec &read_expire = 1 min; +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; From 9b2f26c0aa2dbf312b55df63306246822cc97d9e Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 28 Aug 2020 09:37:36 -0700 Subject: [PATCH 3/4] Short-circuit checking of whether attr exists --- src/Attr.cc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/Attr.cc b/src/Attr.cc index 4da14e4962..483c98f0ea 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -195,17 +195,14 @@ Attributes::Attributes(std::vector a, void Attributes::AddAttr(AttrPtr attr) { - auto acceptable_duplicate_tag = [&](AttrPtr attr) -> bool + auto acceptable_duplicate_tag = [&](const AttrPtr& attr, const AttrPtr& existing) -> bool { AttrTag new_tag = attr->Tag(); if ( new_tag == ATTR_DEPRECATED ) { - if ( ! attr->DeprecationMessage().empty() ) - return false; - - auto existing = Find(new_tag); - if ( existing && ! existing->DeprecationMessage().empty() ) + if ( ! attr->DeprecationMessage().empty() || + ( existing && ! existing->DeprecationMessage().empty() ) ) return false; return true; @@ -219,7 +216,8 @@ void Attributes::AddAttr(AttrPtr attr) // Display a warning for duplicated tags on a type. &log tags are intentionally // ignored here because duplicate log tags on record types are valid, and don't // cause any significant breakage for other types. - if ( ! acceptable_duplicate_tag(attr) && Find(attr->Tag()) ) + auto existing = Find(attr->Tag()); + if ( existing && ! acceptable_duplicate_tag(attr, existing) ) reporter->Error("Duplicate %s tag is ambiguous", attr_name(attr->Tag())); // We overwrite old attributes by deleting them first. From 9106f3f72208eaa4cbb8bd84c6c787dde8699582 Mon Sep 17 00:00:00 2001 From: Tim Wojtulewicz Date: Fri, 28 Aug 2020 09:55:40 -0700 Subject: [PATCH 4/4] Allow duplicate attributes in full redefs --- src/Attr.cc | 19 +++++++++++-------- src/Attr.h | 6 +++--- src/ID.cc | 4 ++-- src/ID.h | 2 +- src/Var.cc | 2 +- 5 files changed, 18 insertions(+), 15 deletions(-) diff --git a/src/Attr.cc b/src/Attr.cc index 483c98f0ea..dc6ef6e380 100644 --- a/src/Attr.cc +++ b/src/Attr.cc @@ -193,7 +193,7 @@ Attributes::Attributes(std::vector a, AddAttr(std::move(attr)); } -void Attributes::AddAttr(AttrPtr attr) +void Attributes::AddAttr(AttrPtr attr, bool is_redef) { auto acceptable_duplicate_tag = [&](const AttrPtr& attr, const AttrPtr& existing) -> bool { @@ -216,9 +216,12 @@ void Attributes::AddAttr(AttrPtr attr) // Display a warning for duplicated tags on a type. &log tags are intentionally // ignored here because duplicate log tags on record types are valid, and don't // cause any significant breakage for other types. - auto existing = Find(attr->Tag()); - if ( existing && ! acceptable_duplicate_tag(attr, existing) ) - reporter->Error("Duplicate %s tag is ambiguous", attr_name(attr->Tag())); + if ( ! is_redef ) + { + auto existing = Find(attr->Tag()); + if ( existing && ! acceptable_duplicate_tag(attr, existing) ) + reporter->Error("Duplicate %s tag is ambiguous", attr_name(attr->Tag())); + } // We overwrite old attributes by deleting them first. RemoveAttr(attr->Tag()); @@ -249,16 +252,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); } diff --git a/src/Attr.h b/src/Attr.h index 5c110bfe7f..51344ffff7 100644 --- a/src/Attr.h +++ b/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; diff --git a/src/ID.cc b/src/ID.cc index 46dcd50efd..ab4d34bf9e 100644 --- a/src/ID.cc +++ b/src/ID.cc @@ -314,10 +314,10 @@ std::string ID::GetDeprecationWarning() const return zeek::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); diff --git a/src/ID.h b/src/ID.h index 2ea5a41134..a24724013b 100644 --- a/src/ID.h +++ b/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(); diff --git a/src/Var.cc b/src/Var.cc index 174704e069..b98c89c317 100644 --- a/src/Var.cc +++ b/src/Var.cc @@ -213,7 +213,7 @@ static void make_var(const zeek::detail::IDPtr& id, zeek::TypePtr t, id->SetType(t); if ( attr ) - id->AddAttrs(zeek::make_intrusive(std::move(*attr), t, false, id->IsGlobal())); + id->AddAttrs(zeek::make_intrusive(std::move(*attr), t, false, id->IsGlobal()), dt == VAR_REDEF); if ( init ) {