diff --git a/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc b/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc index f1c47aeedf..d3050f81cd 100644 --- a/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc +++ b/verilog/analysis/checkers/forbid_implicit_declarations_rule.cc @@ -87,8 +87,10 @@ void ForbidImplicitDeclarationsRule::DetectDeclarations(const verible::Symbol& s // Top of the matched tree stack CHECK_GE(context.size(), 1); const auto& top = context.top(); // scope - CHECK_EQ(static_cast(top.Tag().tag), - verilog::NodeEnum::kModuleItemList); + const auto top_tag = static_cast(top.Tag().tag); + + CHECK((top_tag == NodeEnum::kModuleItemList) || + (top_tag == NodeEnum::kGenerateItemList)); // This could be omitted if we had matchers for begining and ending of scopes // e.g. something like kBegin/kEnd. But we would have to modify parser grammar for @@ -114,18 +116,12 @@ void ForbidImplicitDeclarationsRule::DetectDeclarations(const verible::Symbol& s } } -void ForbidImplicitDeclarationsRule::DetectReference(const verible::Symbol& symbol, - const SyntaxTreeContext& context) { - verible::matcher::BoundSymbolManager manager; - - if (net_assignment_matcher_.Matches(symbol, &manager)) { - const auto* lval = ABSL_DIE_IF_NULL(manager.GetAs("lval")); - const verible::TokenInfo& lval_token = lval->get(); +void ForbidImplicitDeclarationsRule::AnalyzeLHS(const verible::SyntaxTreeLeaf& lval, + const verible::SyntaxTreeContext& context) { + const verible::TokenInfo& lval_token = lval.get(); CHECK_EQ(lval_token.token_enum, SymbolIdentifier); const auto& identifier = lval_token.text; - CheckAndPopScope(symbol, context); - CHECK_GE(scopes.size(), 1); auto& scope = scopes.top().declared_nets_; const auto* node = scope[identifier]; @@ -135,8 +131,46 @@ void ForbidImplicitDeclarationsRule::DetectReference(const verible::Symbol& symb // TODO: Check necessity of call to ContainsAncestor(). CheckAndPopScope() // is propably enough. if ((node == nullptr) || (!ContainsAncestor(node, context))) { - violations_.insert(LintViolation(*lval, kMessage, context)); + violations_.insert(LintViolation(lval, kMessage, context)); } +} + +void ForbidImplicitDeclarationsRule::DetectReference(const verible::Symbol& symbol, + const SyntaxTreeContext& context) { + verible::matcher::BoundSymbolManager manager; + + if (net_assignment_matcher_.Matches(symbol, &manager)) { + if (!context.IsInside(NodeEnum::kContinuousAssignmentStatement)) { + return ; + } + + const auto* lpval = ABSL_DIE_IF_NULL( + manager.GetAs("lpval")); + + const auto& children = lpval->children(); + CHECK_EQ(children.size(), 1); + const auto& lhs = *ABSL_DIE_IF_NULL(children[0]); + + CheckAndPopScope(symbol, context); + + if (net_reference_matcher_.Matches(lhs, &manager)) { + // Matches simple LPvalue, e.g. assign a = 1'b0; + const auto& lval = *ABSL_DIE_IF_NULL( + manager.GetAs("lval")); + AnalyzeLHS(lval, context); + } else if (net_openrange_matcher_.Matches(lhs, &manager)) { + // Matches concatenated LPvalue, e.g. assign {a,b,c} = 3'b101; + const auto* clist = ABSL_DIE_IF_NULL( + manager.GetAs("clist")); + + for (const auto& itr : clist->children()) { + if (net_expression_reference_matcher_.Matches(*itr, &manager)) { + const auto& lval = *ABSL_DIE_IF_NULL( + manager.GetAs("lval")); + AnalyzeLHS(lval, context); + } + } + } // TODO: Should log if we don't get any matches? } } diff --git a/verilog/analysis/checkers/forbid_implicit_declarations_rule.h b/verilog/analysis/checkers/forbid_implicit_declarations_rule.h index d6f18f2510..d082a4562d 100644 --- a/verilog/analysis/checkers/forbid_implicit_declarations_rule.h +++ b/verilog/analysis/checkers/forbid_implicit_declarations_rule.h @@ -61,6 +61,9 @@ class ForbidImplicitDeclarationsRule : public verible::SyntaxTreeLintRule { void CheckAndPopScope(const verible::Symbol& symbol, const verible::SyntaxTreeContext& context); + void AnalyzeLHS(const verible::SyntaxTreeLeaf& lval, + const verible::SyntaxTreeContext& context); + // Pushes new scopes on stack void DetectScope(const verible::Symbol& symbol, const verible::SyntaxTreeContext& context); @@ -92,17 +95,25 @@ class ForbidImplicitDeclarationsRule : public verible::SyntaxTreeLintRule { // Matches scopes with nets // TODO: description, other scopes const Matcher net_scope_matcher_ = verible::matcher::AnyOf( - NodekModuleItemList()); + NodekModuleItemList(), + NodekGenerateItemList()); // Matches nets declarations const Matcher net_declaration_matcher_ = NodekNetDeclaration(PathkNetVariableDeclarationAssign().Bind("decls")); - // Detects nets assignments - const Matcher net_assignment_matcher_ = NodekContinuousAssignmentStatement( - PathkAssignmentList(PathkNetVariableAssignment( - LValueOfAssignment( - PathkReference(UnqualifiedReferenceHasId().Bind("lval")))))); + const Matcher net_assignment_matcher_ = + NodekNetVariableAssignment(PathkLPValue().Bind("lpval")); + + const Matcher net_openrange_matcher_ = + NodekBraceGroup(PathkOpenRangeList().Bind("clist")); + + // TODO: Any smart way to merge those two? + const Matcher net_reference_matcher_ = NodekReferenceCallBase( + PathkReference(UnqualifiedReferenceHasId().Bind("lval"))); + const Matcher net_expression_reference_matcher_ = NodekExpression( + PathkReferenceCallBase(PathkReference( + UnqualifiedReferenceHasId().Bind("lval")))); std::set violations_; diff --git a/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc b/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc index 6507d74742..8e1e0bcd9c 100644 --- a/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc +++ b/verilog/analysis/checkers/forbid_implicit_declarations_rule_test.cc @@ -43,6 +43,105 @@ TEST(ForbidImplicitDeclarationsRule, FunctionFailures) { {"module m;\nwire a1;\nbegin\nend\nassign a1 = 1'b0;\nendmodule"}, {"module m;\nwire a1;\nbegin\nassign ", {kToken, "a1"}, " = 1'b0;\nend\nassign a1 = 1'b0;\nendmodule"}, {"module m;\nwire a1;\nbegin\nwire a1; assign a1 = 1'b0;\nend\nassign a1 = 1'b0;\nendmodule"}, + + // multiple declarations + {"module m;\nwire a0, a1;\nassign a0 = 1'b0;\nassign a1 = 1'b1;\nendmodule"}, + {"module m;\nwire a0, a2;\nassign a0 = 1'b0;\nassign ", {kToken, "a1"}, " = 1'b1;\nendmodule"}, + + // multiple net assignments + {"module m;\nassign ", {kToken, "a"}, " = b, ", {kToken, "c"}, " = d;\nendmodule"}, + {"module m;\nassign ", {kToken, "a1"}, " = 1'b0;wire a1;\nendmodule"}, + + // out-of-order + {"module m;\nassign ", {kToken, "a1"}, " = 1'b0;\nwire a1;\nassign a1 = 1'b1;\nendmodule"}, + + // concatenated + {"module m;\nassign {", {kToken, "a"}, "} = 1'b0;\nendmodule"}, + {"module m;\nassign {", {kToken, "a"}, ",", {kToken, "b"}, "} = 2'b01;\nendmodule"}, + {"module m;\nassign {", {kToken, "a"}, ",", {kToken, "b"}, ",", {kToken, "c"}, "} = 3'b010;\nendmodule"}, + {"module m;\nwire b;assign {", {kToken, "a"}, ", b,", {kToken, "c"}, "} = 3'b010;\nendmodule"}, + + // around scope + {"module m;assign ", {kToken, "a1"}, " = 1'b1;\nbegin\nend\nendmodule"}, + {"module m;begin\nend\nassign ", {kToken, "a1"}, " = 1'b1;endmodule"}, + + // declaration and assignement separated by begin-end block + {"module m;\nwire a1;\nbegin\nend\nassign a1 = 1'b1;\nendmodule"}, + + // out-of-scope + {"module m;\nbegin wire a1;\nend\nassign ", {kToken, "a1"}, " = 1'b1;\nendmodule"}, + {"module m;\nbegin wire a1;\nassign a1 = 1'b0;\nend\nassign ", {kToken, "a1"}, " = 1'b1;\nendmodule"}, + {"module m;\nwire a1;begin assign ", {kToken, "a1"}, " = 1'b0;\nend\nassign a1 = 1'b1;\nendmodule"}, + + // multi-level begin-end blocks + {"module m;\n" + " wire x1;\n" + " begin\n" + " wire x2;\n" + " begin\n" + " wire x3;\n" + " begin\n" + " wire x4;\n" + " assign x4 = 1'b0;\n" + " assign ", {kToken, "x3"}, " = 1'b0;\n" + " assign ", {kToken, "x2"}, " = 1'b0;\n" + " assign ", {kToken, "x1"}, " = 1'b0;\n" + " end\n" + " assign ", {kToken, "x4"}, " = 1'b0;\n" + " assign x3 = 1'b1;\n" + " assign ", {kToken, "x2"}, " = 1'b0;\n" + " assign ", {kToken, "x1"}, " = 1'b0;\n" + " end\n" + " assign ", {kToken, "x4"}, " = 1'b0;\n" + " assign ", {kToken, "x3"}, " = 1'b0;\n" + " assign x2 = 1'b0;\n" + " assign ", {kToken, "x1"}, " = 1'b0;\n" + " end\n" + " assign ", {kToken, "x4"}, " = 1'b0;\n" + " assign ", {kToken, "x3"}, " = 1'b0;\n" + " assign ", {kToken, "x2"}, " = 1'b0;\n" + " assign x1 = 1'b1;\n" + "endmodule"}, + + // generate block, TODO: multi-level + {"module m;\ngenerate\nendgenerate\nendmodule"}, + {"module m;\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " generate\n" + " endgenerate\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + {"module m;\n" + " generate\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " endgenerate\n" + "endmodule"}, + {"module m;\n" + " generate\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + " endgenerate\n" + "endmodule"}, + {"module m;\n" + " generate\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " endgenerate\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + "endmodule"}, + {"module m;\n" + " wire a1;\n" + " assign a1 = 1'b1;\n" + " generate\n" + " assign ", {kToken, "a1"}, " = 1'b1;\n" + " endgenerate\n" + " assign a1 = 1'b0;\n" + "endmodule"}, + + // TODO: module scope + + // TODO: nets declared inside terminal/port connection list }; RunLintTestCases(