Lint: Make acceptance of anonymous nested structs configurable.

This adds 'allow_anonymous_nested' configuration to the 'typedef-structs-unions'
rule to allow not to require a typedef for inner structs.

In the course of that, implement SyntaxTreeContext::IsInsideStartingFrom()
which starts looking backward the stack starting with an offset.

(As a side effect, make IsInside() to start looking from top of stack
instead of bottom, which is conceptually more aligned of what we expect,
even though it makes only a difference in this case (offset 0))

Fixes #128

PiperOrigin-RevId: 302738764

common/text/syntax_tree_context.h

@@ -65,15 +65,25 @@ class SyntaxTreeContext {
   const_reverse_iterator rend() const { return stack_.rend(); }
 
   // IsInside returns true if there is a node of the specified
-  // tag on the TreeContext stack.  Search occurs from the bottom of the
-  // stack and returns on the first match found.
+  // tag on the TreeContext stack.  Search traverses from the top of the
+  // stack starting with offset and returns on the first match found.
   // Type parameter E can be a language-specific enum or plain integer type.
   template <typename E>
-  bool IsInside(E tag_enum) const {
+  bool IsInsideStartingFrom(E tag_enum, size_t reverse_offset) const {
+    if (size() <= reverse_offset) return false;
     const auto iter = std::find_if(
-        begin(), end(),
+        rbegin() + reverse_offset, rend(),
         [=](const SyntaxTreeNode* node) { return node->MatchesTag(tag_enum); });
-    return iter != end();
+    return iter != rend();
+  }
+
+  // IsInside returns true if there is a node of the specified
+  // tag on the TreeContext stack.  Search occurs from the top of the
+  // stack and returns on the first match found.
+  // Type parameter E can be a language-specific enum or plain integer type.
+  template <typename E>
+  bool IsInside(E tag_enum) const {
+    return IsInsideStartingFrom(tag_enum, 0);
   }
 
   // Returns true if current context is directly inside one of the includes

common/text/syntax_tree_context_test.cc

@@ -69,12 +69,15 @@ TEST(SyntaxTreeContextTest, IteratorsTest) {
   }
 }
 
-// Test that IsInside correctly reports whether context matches.
+// Test that IsInside and IsInsideStartingFrom correctly reports whether
+// context matches.
 TEST(SyntaxTreeContextTest, IsInsideTest) {
   SyntaxTreeContext context;
   EXPECT_FALSE(context.IsInside(1));
   EXPECT_FALSE(context.IsInside(2));
   EXPECT_FALSE(context.IsInside(3));
+  EXPECT_FALSE(context.IsInsideStartingFrom(1, 1));
+  EXPECT_FALSE(context.IsInsideStartingFrom(1, 0));
   {
     SyntaxTreeNode node1(1);
     SyntaxTreeContext::AutoPop p1(&context, node1);
@@ -93,6 +96,14 @@ TEST(SyntaxTreeContextTest, IsInsideTest) {
         EXPECT_TRUE(context.IsInside(1));
         EXPECT_TRUE(context.IsInside(2));
         EXPECT_TRUE(context.IsInside(3));
+        // With an offset, we won't see some of these elements
+        EXPECT_TRUE(context.IsInsideStartingFrom(3, 0));
+        EXPECT_FALSE(context.IsInsideStartingFrom(3, 1));
+        EXPECT_TRUE(context.IsInsideStartingFrom(2, 1));
+        EXPECT_FALSE(context.IsInsideStartingFrom(2, 2));
+        // Check stack boundary
+        EXPECT_FALSE(context.IsInsideStartingFrom(2, 3));
+        EXPECT_FALSE(context.IsInsideStartingFrom(2, 4));
       }
     }
   }

verilog/analysis/checkers/BUILD

@@ -688,6 +688,7 @@ cc_test(
         "//common/analysis:syntax_tree_linter_test_utils",
         "//common/text:symbol",
         "//verilog/analysis:verilog_analyzer",
+        "@com_google_absl//absl/strings",
         "@com_google_googletest//:gtest_main",
     ],
 )

verilog/analysis/checkers/forbidden_anonymous_structs_unions_rule.cc

@@ -17,6 +17,7 @@
 #include <set>
 #include <string>
 
+#include "absl/strings/match.h"
 #include "absl/strings/str_cat.h"
 #include "common/analysis/citation.h"
 #include "common/analysis/lint_rule_status.h"
@@ -48,28 +49,62 @@ const char ForbiddenAnonymousStructsUnionsRule::kMessageUnion[] =
 
 std::string ForbiddenAnonymousStructsUnionsRule::GetDescription(
     DescriptionType description_type) {
-  return absl::StrCat(
+  static std::string basic_desc = absl::StrCat(
       "Checks that a Verilog ", Codify("struct", description_type), " or ",
       Codify("union", description_type), " declaration is named using ",
       Codify("typedef", description_type), ". See ",
-      GetStyleGuideCitation(kTopic), ".");
+      GetStyleGuideCitation(kTopic), ".\n");
+  if (description_type == DescriptionType::kHelpRulesFlag) {
+    return absl::StrCat(basic_desc, "Parameters: allow_anonymous_nested:false");
+  } else {
+    return absl::StrCat(basic_desc, "##### Parameters\n",
+                        "  * allow_anonymous_nested Default: false");
+  }
+}
+
+absl::Status ForbiddenAnonymousStructsUnionsRule::Configure(
+    absl::string_view configuration) {
+  if (configuration.empty()) return absl::OkStatus();
+  // TODO(hzeller): make general parser of lint rule configuration parameters
+  // to avoid ad-hoc parsings such as the following.
+  constexpr char kParamName[] = "allow_anonymous_nested";
+  if (absl::StartsWith(configuration, kParamName)) {
+    const absl::string_view suffix = configuration.substr(strlen(kParamName));
+    allow_anonymous_nested_type_ = suffix.empty() || suffix == ":true";
+    if (!allow_anonymous_nested_type_ && suffix != ":false") {
+      return absl::InvalidArgumentError(
+          "allow_anonymous_nested allowed value: "
+          "'true' or 'false'");
+    }
+    return absl::OkStatus();
+  }
+  return absl::InvalidArgumentError(
+      absl::StrCat("Only supported parameter 'allow_anonymous_nested'; got '",
+                   configuration, "'"));
+}
+
+static bool IsPreceededByTypedef(const verible::SyntaxTreeContext& context) {
+  return context.DirectParentsAre(
+      {NodeEnum::kDataTypePrimitive, NodeEnum::kTypeDeclaration});
+}
+
+static bool NestedInStructOrUnion(const verible::SyntaxTreeContext& context) {
+  return context.IsInsideStartingFrom(NodeEnum::kDataTypePrimitive, 1);
+}
+
+bool ForbiddenAnonymousStructsUnionsRule::IsRuleMet(
+    const verible::SyntaxTreeContext& context) const {
+  return IsPreceededByTypedef(context) ||
+         (allow_anonymous_nested_type_ && NestedInStructOrUnion(context));
 }
 
 void ForbiddenAnonymousStructsUnionsRule::HandleSymbol(
     const verible::Symbol& symbol, const verible::SyntaxTreeContext& context) {
   verible::matcher::BoundSymbolManager manager;
-  if (matcher_struct_.Matches(symbol, &manager)) {
-    // Check if it is preceded by a typedef
-    if (!context.DirectParentsAre(
-            {NodeEnum::kDataTypePrimitive, NodeEnum::kTypeDeclaration})) {
-      violations_.insert(LintViolation(symbol, kMessageStruct, context));
-    }
-  } else if (matcher_union_.Matches(symbol, &manager)) {
-    // Check if it is preceded by a typedef
-    if (!context.DirectParentsAre(
-            {NodeEnum::kDataTypePrimitive, NodeEnum::kTypeDeclaration})) {
-      violations_.insert(LintViolation(symbol, kMessageUnion, context));
-    }
+  if (matcher_struct_.Matches(symbol, &manager) && !IsRuleMet(context)) {
+    violations_.insert(LintViolation(symbol, kMessageStruct, context));
+  } else if (matcher_union_.Matches(symbol, &manager) && !IsRuleMet(context)) {
+    violations_.insert(LintViolation(symbol, kMessageUnion, context));
   }
 }
 

verilog/analysis/checkers/forbidden_anonymous_structs_unions_rule.h

@@ -46,6 +46,14 @@ namespace analysis {
 //      secondSignal,
 //    } my_instance;
 //
+// If 'waive_nested' configuration is provided, anonymous structs within
+// nested typedefs are allowed
+//
+// Allowed with 'allow_anonymous_nested'
+// typedef struct {
+//    struct { logic x; logic y; } foo;
+// } outer_t;
+//
 class ForbiddenAnonymousStructsUnionsRule : public verible::SyntaxTreeLintRule {
  public:
   using rule_type = verible::SyntaxTreeLintRule;
@@ -55,12 +63,17 @@ class ForbiddenAnonymousStructsUnionsRule : public verible::SyntaxTreeLintRule {
   // helper flag or markdown depending on the parameter type.
   static std::string GetDescription(DescriptionType);
 
+  absl::Status Configure(absl::string_view configuration) override;
+
   void HandleSymbol(const verible::Symbol& symbol,
                     const verible::SyntaxTreeContext& context) override;
 
   verible::LintRuleStatus Report() const override;
 
  private:
+  // Tests if the rule is met, taking waiving condition into account.
+  bool IsRuleMet(const verible::SyntaxTreeContext& context) const;
+
   // Link to style guide rule.
   static const char kTopic[];
 
@@ -70,8 +83,10 @@ class ForbiddenAnonymousStructsUnionsRule : public verible::SyntaxTreeLintRule {
 
   using Matcher = verible::matcher::Matcher;
 
-  Matcher matcher_struct_ = NodekStructType();
-  Matcher matcher_union_ = NodekUnionType();
+  const Matcher matcher_struct_ = NodekStructType();
+  const Matcher matcher_union_ = NodekUnionType();
+
+  bool allow_anonymous_nested_type_ = false;
 
   // Collection of found violations.
   std::set<verible::LintViolation> violations_;

verilog/analysis/checkers/forbidden_anonymous_structs_unions_rule_test.cc

@@ -17,6 +17,7 @@
 #include <initializer_list>
 
 #include "gtest/gtest.h"
+#include "absl/strings/match.h"
 #include "common/analysis/syntax_tree_linter_test_utils.h"
 #include "common/text/symbol.h"
 #include "verilog/analysis/verilog_analyzer.h"
@@ -26,8 +27,27 @@ namespace analysis {
 namespace {
 
 using verible::LintTestCase;
+using verible::RunConfiguredLintTestCases;
 using verible::RunLintTestCases;
 
+TEST(ForbiddenAnonymousStructsUnionsTest, Configuration) {
+  ForbiddenAnonymousStructsUnionsRule rule;
+  absl::Status s;
+  EXPECT_TRUE((s = rule.Configure("")).ok()) << s.message();
+  EXPECT_TRUE((s = rule.Configure("allow_anonymous_nested")).ok())
+      << s.message();
+  EXPECT_TRUE((s = rule.Configure("allow_anonymous_nested:true")).ok())
+      << s.message();
+  EXPECT_TRUE((s = rule.Configure("allow_anonymous_nested:false")).ok())
+      << s.message();
+
+  EXPECT_FALSE((s = rule.Configure("foo")).ok());
+  EXPECT_TRUE(absl::StrContains(s.message(), "supported parameter"));
+
+  EXPECT_FALSE((s = rule.Configure("allow_anonymous_nested:bogus")).ok());
+  EXPECT_TRUE(absl::StrContains(s.message(), "allowed value"));
+}
+
 // Tests that properly typedef'ed struct passes.
 TEST(ForbiddenAnonymousStructsUnionsTest, AcceptsTypedefedStructs) {
   const std::initializer_list<LintTestCase> kTestCases = {
@@ -54,6 +74,59 @@ TEST(ForbiddenAnonymousStructsUnionsTest, RejectsAnonymousStructs) {
       kTestCases);
 }
 
+TEST(ForbiddenAnonymousStructsUnionsTest,
+     AcceptInnerAnonymousStructsIfConfigured) {
+  {
+    // Without waived nested configuration we expect complains at
+    // every anonymous struct/union, inside or nested.
+    const std::initializer_list<LintTestCase> kTestCases = {
+        // outer and inner struct/union is complained about.
+        {{TK_struct, "struct"},
+         " {byte a;",
+         {TK_struct, "struct"},
+         " { byte x, y;} b;} z;"},
+        {{TK_union, "union"},
+         " {byte a;",
+         {TK_struct, "struct"},
+         " { byte x, y;} b;} z;"},
+        {"typedef struct {byte a;",
+         {TK_struct, "struct"},
+         " { byte x, y;} b;} foo_t;\nfoo_t z;"},
+        {"typedef struct {byte a;",
+         {TK_union, "union"},
+         " { byte x, y;} b;} foo_t;\nfoo_t z;"},
+        {"typedef union {byte a;",
+         {TK_union, "union"},
+         " { byte x, y;} b;} foo_t;\nfoo_t z;"},
+    };
+    for (const auto not_waived_cfg : {"", "allow_anonymous_nested:false"}) {
+      RunConfiguredLintTestCases<VerilogAnalyzer,
+                                 ForbiddenAnonymousStructsUnionsRule>(
+          kTestCases, not_waived_cfg);
+    }
+  }
+
+  {
+    // With waived nested configuration, lint will only complain about outer
+    // anonymous struct/unions.
+    const std::initializer_list<LintTestCase> kTestCases = {
+        // Just outer struct/union is complained about, but anonymous inner ok
+        {{TK_struct, "struct"}, " {byte a; struct { byte x, y;} b;} z;"},
+        {{TK_union, "union"}, " {byte a; struct { byte x, y;} b;} z;"},
+        // Here we are entirely happy: outside with typedef, inner w/o fine.
+        {"typedef struct {byte a; struct { byte x, y;} b;} foo_t;\nfoo_t z;"},
+        {"typedef struct {byte a; union { byte x, y;} b;} foo_t;\nfoo_t z;"},
+        {"typedef union {byte a; union { byte x, y;} b;} foo_t;\nfoo_t z;"},
+    };
+    for (const auto waived_cfg :
+         {"allow_anonymous_nested", "allow_anonymous_nested:true"}) {
+      RunConfiguredLintTestCases<VerilogAnalyzer,
+                                 ForbiddenAnonymousStructsUnionsRule>(
+          kTestCases, waived_cfg);
+    }
+  }
+}
+
 // Tests that properly typedef'ed union passes.
 TEST(ForbiddenAnonymousStructsUnionsTest, AcceptsTypedefedUnions) {
   const std::initializer_list<LintTestCase> kTestCases = {