From d5dcbe624c9052e80bfb7a9c61f78dff2b0701ef Mon Sep 17 00:00:00 2001 From: Henner Zeller Date: Mon, 5 Aug 2024 05:41:26 -0700 Subject: [PATCH] Add variable shadowing rule. This is a rebase of #790 to compile with head. Fixes #247 --- common/analysis/lint_rule_status.cc | 44 ++++- common/analysis/lint_rule_status.h | 38 +++- common/analysis/lint_rule_status_test.cc | 47 ++++- verilog/analysis/checkers/BUILD | 44 +++++ .../analysis/checkers/instance_shadow_rule.cc | 144 ++++++++++++++ .../analysis/checkers/instance_shadow_rule.h | 59 ++++++ .../checkers/instance_shadow_rule_test.cc | 179 ++++++++++++++++++ verilog/analysis/default_rules.h | 1 + verilog/tools/lint/BUILD | 1 + .../tools/lint/testdata/shadow_parameter.sv | 8 + 10 files changed, 554 insertions(+), 11 deletions(-) create mode 100644 verilog/analysis/checkers/instance_shadow_rule.cc create mode 100644 verilog/analysis/checkers/instance_shadow_rule.h create mode 100644 verilog/analysis/checkers/instance_shadow_rule_test.cc create mode 100644 verilog/tools/lint/testdata/shadow_parameter.sv diff --git a/common/analysis/lint_rule_status.cc b/common/analysis/lint_rule_status.cc index 72c99c0a7..dac335098 100644 --- a/common/analysis/lint_rule_status.cc +++ b/common/analysis/lint_rule_status.cc @@ -15,15 +15,18 @@ #include "common/analysis/lint_rule_status.h" #include +#include #include #include #include #include #include +#include #include #include #include "absl/strings/str_cat.h" +#include "absl/strings/str_replace.h" #include "absl/strings/string_view.h" #include "common/strings/line_column_map.h" #include "common/text/concrete_syntax_leaf.h" @@ -76,12 +79,14 @@ static TokenInfo SymbolToToken(const Symbol& root) { LintViolation::LintViolation(const Symbol& root, absl::string_view reason, const SyntaxTreeContext& context, - const std::vector& autofixes) + const std::vector& autofixes, + const std::vector& related_tokens) : root(&root), token(SymbolToToken(root)), reason(reason), context(context), - autofixes(autofixes) {} + autofixes(autofixes), + related_tokens(related_tokens) {} void LintStatusFormatter::FormatLintRuleStatus(std::ostream* stream, const LintRuleStatus& status, @@ -94,6 +99,35 @@ void LintStatusFormatter::FormatLintRuleStatus(std::ostream* stream, } } +std::string LintStatusFormatter::FormatWithRelatedTokens( + const std::vector& tokens, absl::string_view message, + absl::string_view path, absl::string_view base) const { + if (tokens.empty()) { + return std::string(message); + } + size_t beg_pos = 0; + size_t end_pos = message.find("@", beg_pos); + std::ostringstream s; + for (const auto& token : tokens) { + if (end_pos == absl::string_view::npos) { + s << message.substr(beg_pos); + break; + } + if (!(end_pos != 0 && message[end_pos - 1] == '\\')) { + s << message.substr(beg_pos, end_pos - beg_pos); + s << path << ":"; + s << line_column_map_.GetLineColAtOffset(base, token.left(base)); + } else { + s << message.substr(beg_pos, end_pos - beg_pos + 1); + } + + beg_pos = end_pos + 1; + end_pos = message.find("@", beg_pos); + } + + return absl::StrReplaceAll(s.str(), {{"\\@", "@"}}); +} + void LintStatusFormatter::FormatLintRuleStatuses( std::ostream* stream, const std::vector& statuses, absl::string_view base, absl::string_view path, @@ -137,8 +171,10 @@ void LintStatusFormatter::FormatViolation(std::ostream* stream, line_column_map_.GetLineColAtOffset(base, violation.token.left(base)), line_column_map_.GetLineColAtOffset(base, violation.token.right(base))}; - (*stream) << path << ':' << range << ' ' << violation.reason << ' ' << url - << " [" << rule_name << ']'; + (*stream) << path << ':' << range << " " + << FormatWithRelatedTokens(violation.related_tokens, + violation.reason, path, base) + << ' ' << url << " [" << rule_name << ']'; } // Formats and outputs violation to a file stream in a syntax accepted by diff --git a/common/analysis/lint_rule_status.h b/common/analysis/lint_rule_status.h index cb110632f..8d4df30fc 100644 --- a/common/analysis/lint_rule_status.h +++ b/common/analysis/lint_rule_status.h @@ -92,15 +92,31 @@ class AutoFix { struct LintViolation { // This construct records a token stream lint violation. LintViolation(const TokenInfo& token, absl::string_view reason, - const std::vector& autofixes = {}) - : token(token), reason(reason), context(), autofixes(autofixes) {} + const std::vector& autofixes = {}, + const std::vector& related_tokens = {}) + : token(token), + reason(reason), + context(), + autofixes(autofixes), + related_tokens(related_tokens) {} + + // This construct records a token stream lint violation. + // with additional tokens that might be related somehow with vulnerable token + LintViolation(const TokenInfo& token, absl::string_view reason, + const std::vector& tokens) + : token(token), reason(reason), context(), related_tokens(tokens) {} // This construct records a syntax tree lint violation. // Use this variation when the violation can be localized to a single token. LintViolation(const TokenInfo& token, absl::string_view reason, const SyntaxTreeContext& context, - const std::vector& autofixes = {}) - : token(token), reason(reason), context(context), autofixes(autofixes) {} + const std::vector& autofixes = {}, + const std::vector& related_tokens = {}) + : token(token), + reason(reason), + context(context), + autofixes(autofixes), + related_tokens(related_tokens) {} // This construct records a syntax tree lint violation. // Use this variation when the range of violation is a subtree that spans @@ -108,7 +124,8 @@ struct LintViolation { // the left-most leaf of the subtree. LintViolation(const Symbol& root, absl::string_view reason, const SyntaxTreeContext& context, - const std::vector& autofixes = {}); + const std::vector& autofixes = {}, + const std::vector& related_tokens = {}); // root is a reference into original ConcreteSyntaxTree that // linter was run against. LintViolations should not outlive this tree. @@ -127,6 +144,9 @@ struct LintViolation { const std::vector autofixes; + // Additional tokens that are related somehow to offending token. + const std::vector related_tokens; + bool operator<(const LintViolation& r) const { // compares addresses of violations, which correspond to substring // locations @@ -238,6 +258,14 @@ class LintStatusFormatter { const LintViolation& violation, absl::string_view base, absl::string_view path, absl::string_view rule_name) const; + // Substitute the markers \@ with tokens location + // this allows us to create custom reason msg + // with different token location that are related to found + // vulnerable token. It is important to note that all the tokens + // must come from the same file. + std::string FormatWithRelatedTokens( + const std::vector& tokens, absl::string_view message, + absl::string_view path, absl::string_view base) const; private: // Translates byte offsets, which are supplied by LintViolations via diff --git a/common/analysis/lint_rule_status_test.cc b/common/analysis/lint_rule_status_test.cc index d5e708b80..7a5f253e7 100644 --- a/common/analysis/lint_rule_status_test.cc +++ b/common/analysis/lint_rule_status_test.cc @@ -75,6 +75,7 @@ struct LintViolationTest { std::string reason; TokenInfo token; std::string expected_output; + std::vector related_tokens; }; // Struct for checking expected formatting of a LintRuleStatus @@ -96,8 +97,9 @@ void RunLintStatusTest(const LintStatusTest& test) { status.url = test.url; status.lint_rule_name = test.rule_name; for (const auto& violation_test : test.violations) { - status.violations.insert( - LintViolation(violation_test.token, violation_test.reason)); + status.violations.insert(LintViolation(violation_test.token, + violation_test.reason, {}, + violation_test.related_tokens)); } std::ostringstream ss; @@ -135,6 +137,47 @@ TEST(LintRuleStatusFormatterTest, SimpleOutput) { RunLintStatusTest(test); } +TEST(LintRuleStatusFormatterTest, HelperTokensReplacmentWithTokensLocation) { + SymbolPtr root = Node(); + static const int dont_care_tag = 0; + constexpr absl::string_view text( + "This is some code\n" + "That you are looking at right now\n" + "It is nice code, make no mistake\n" + "Very nice"); + LintStatusTest test = { + "test-rule", + "http://foobar", + "some/path/to/somewhere.fvg", + text, + {{"reason1 @", + TokenInfo(dont_care_tag, text.substr(0, 5)), + "some/path/to/somewhere.fvg:1:1-5: reason1 @ http://foobar [test-rule]", + {}}, + {"reason2", + TokenInfo(dont_care_tag, text.substr(6, 2)), + "some/path/to/somewhere.fvg:1:7-8: reason2 http://foobar [test-rule]", + {TokenInfo(dont_care_tag, text.substr(0, 5))}}, + {"reason3 \\@", + TokenInfo(dont_care_tag, text.substr(8, 2)), + "some/path/to/somewhere.fvg:1:9-10: reason3 @ http://foobar " + "[test-rule]", + {TokenInfo(dont_care_tag, text.substr(0, 5))}}, + {"reason4 @", + TokenInfo(dont_care_tag, text.substr(15, 4)), + "some/path/to/somewhere.fvg:1:16:2:1: reason4 " + "some/path/to/somewhere.fvg:1:1 http://foobar [test-rule]", + {TokenInfo(dont_care_tag, text.substr(0, 5))}}, + {"@ reason5 @", + TokenInfo(dont_care_tag, text.substr(21, 4)), + "some/path/to/somewhere.fvg:2:4-7: some/path/to/somewhere.fvg:1:10 " + "reason5 some/path/to/somewhere.fvg:2:4 http://foobar [test-rule]", + {TokenInfo(dont_care_tag, text.substr(9, 4)), + TokenInfo(dont_care_tag, text.substr(21, 4))}}}}; + + RunLintStatusTest(test); +} + TEST(LintRuleStatusFormatterTest, NoOutput) { SymbolPtr root = Node(); LintStatusTest test = {"cool-rule", diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index 71c668705..83f3d3723 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -37,6 +37,7 @@ cc_library( ":forbidden-symbol-rule", ":generate-label-prefix-rule", ":generate-label-rule", + ":instance-shadow-rule", ":interface-name-style-rule", ":legacy-generate-region-rule", ":legacy-genvar-declaration-rule", @@ -2053,6 +2054,49 @@ cc_test( ], ) +cc_library( + name = "instance-shadow-rule", + srcs = ["instance_shadow_rule.cc"], + hdrs = ["instance_shadow_rule.h"], + deps = [ + "//common/analysis:citation", + "//common/analysis:lint-rule-status", + "//common/analysis:syntax-tree-lint-rule", + "//common/analysis:syntax-tree-search", + "//common/analysis/matcher", + "//common/analysis/matcher:bound-symbol-manager", + "//common/text:symbol", + "//common/text:syntax-tree-context", + "//common/text:tree-utils", + "//common/util:iterator-adaptors", + "//verilog/CST:identifier", + "//verilog/CST:verilog-matchers", + "//verilog/CST:verilog-nonterminals", + "//verilog/analysis:descriptions", + "//verilog/analysis:lint-rule-registry", + "@com_google_absl//absl/strings:string_view", + ], + alwayslink = 1, +) + +cc_test( + name = "instance-shadow-rule-test", + srcs = ["instance_shadow_rule_test.cc"], + deps = [ + ":instance-shadow-rule", + "//common/analysis:linter-test-utils", + "//common/analysis:syntax-tree-linter", + "//common/analysis:syntax-tree-linter-test-utils", + "//common/util:logging", + "//verilog/analysis:verilog-analyzer", + "//verilog/parser:verilog-token-enum", + "@com_google_absl//absl/status", + "@com_google_absl//absl/strings:string_view", + "@com_google_googletest//:gtest", + "@com_google_googletest//:gtest_main", + ], +) + cc_library( name = "legacy-genvar-declaration-rule", srcs = ["legacy_genvar_declaration_rule.cc"], diff --git a/verilog/analysis/checkers/instance_shadow_rule.cc b/verilog/analysis/checkers/instance_shadow_rule.cc new file mode 100644 index 000000000..064302fef --- /dev/null +++ b/verilog/analysis/checkers/instance_shadow_rule.cc @@ -0,0 +1,144 @@ +// Copyright 2017-2020 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/instance_shadow_rule.h" + +#include +#include +#include + +#include "absl/strings/string_view.h" +#include "common/analysis/citation.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/matcher/bound_symbol_manager.h" +#include "common/analysis/matcher/matcher.h" +#include "common/analysis/syntax_tree_search.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "common/text/tree_utils.h" +#include "common/util/iterator_adaptors.h" +#include "verilog/CST/identifier.h" +#include "verilog/CST/verilog_matchers.h" // IWYU pragma: keep +#include "verilog/CST/verilog_nonterminals.h" +#include "verilog/analysis/descriptions.h" +#include "verilog/analysis/lint_rule_registry.h" + +namespace verilog { +namespace analysis { + +using verible::GetStyleGuideCitation; +using verible::LintRuleStatus; +using verible::LintViolation; +using verible::SyntaxTreeContext; +using verible::matcher::Matcher; + +// Register InstanceShadowRule +VERILOG_REGISTER_LINT_RULE(InstanceShadowRule); + +absl::string_view InstanceShadowRule::Name() { return "instance-shadowing"; } +const char InstanceShadowRule::kTopic[] = "mark-shadowed-instances"; +const char InstanceShadowRule::kMessage[] = + "Instance shadows the already declared variable"; + +const LintRuleDescriptor& InstanceShadowRule::GetDescriptor() { + static const LintRuleDescriptor d{ + .name = "instance-shadowing", + .topic = "mark-shadowed-instances", + .desc = + "Warns if there are multiple declarations in the same scope " + "that shadow each other with the same name."}; + return d; +} + +static const Matcher& InstanceShadowMatcher() { + static const Matcher matcher(SymbolIdentifierLeaf()); + return matcher; +} +static bool isInAllowedNode(const SyntaxTreeContext& ctx) { + return ctx.IsInside(NodeEnum::kSeqBlock) || + ctx.IsInside(NodeEnum::kGenvarDeclaration) || + ctx.IsInside(NodeEnum::kReference); +} + +void InstanceShadowRule::HandleSymbol(const verible::Symbol& symbol, + const SyntaxTreeContext& context) { + verible::matcher::BoundSymbolManager manager; + bool omit_node = false; + if (!InstanceShadowMatcher().Matches(symbol, &manager)) { + return; + } + const auto labels = FindAllSymbolIdentifierLeafs(symbol); + if (labels.empty()) return; + // if the considered symbol name is not a declaration + if (context.IsInside(NodeEnum::kReference)) return; + + // TODO: don't latch on to K&R-Style form in which the same symbol shows + // up twice. + + const auto rcontext = reversed_view(context); + const auto* const rdirectParent = *std::next(rcontext.begin()); + // we are looking for the potential labels that might overlap the considered + // declaration. We are searching all the labels within the visible scope + // until we find the node or we reach the top of the scope + for (const auto* node : rcontext) { + for (const verible::SymbolPtr& child : node->children()) { + if (child == nullptr) { + continue; + } + const auto overlappingLabels = FindAllSymbolIdentifierLeafs(*child); + for (const verible::TreeSearchMatch& omatch : overlappingLabels) { + const auto& overlappingLabel = SymbolCastToLeaf(*omatch.match); + // variable in different scopes or this is not a + // vulnerable declaration + if (isInAllowedNode(omatch.context)) { + omit_node = true; + break; + } + const auto& label = SymbolCastToLeaf(*labels[0].match); + // if found label has the same adress as considered label + // we found the same node so we don't + // want to look further + if (omatch.match == labels[0].match) { + omit_node = true; + break; + } + // if considered label is the last node + // this is the ending node with label + if (rdirectParent->back() == node->back()) { + return; + } + + if (overlappingLabel.get().text() == label.get().text()) { + std::stringstream ss; + ss << "Symbol `" << overlappingLabel.get().text() + << "` is shadowing symbol `" << label.get().text() + << "` defined at @"; + violations_.insert(LintViolation(symbol, ss.str(), context, {}, + {overlappingLabel.get()})); + return; + } + } + if (omit_node) { + omit_node = false; + break; + } + } + } +} + +LintRuleStatus InstanceShadowRule::Report() const { + return LintRuleStatus(violations_, Name(), GetStyleGuideCitation(kTopic)); +} +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/checkers/instance_shadow_rule.h b/verilog/analysis/checkers/instance_shadow_rule.h new file mode 100644 index 000000000..0db62e483 --- /dev/null +++ b/verilog/analysis/checkers/instance_shadow_rule.h @@ -0,0 +1,59 @@ +// Copyright 2017-2020 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef VERIBLE_VERILOG_ANALYSIS_CHECKERS_INSTANCE_SHADOW_RULE_H_ +#define VERIBLE_VERILOG_ANALYSIS_CHECKERS_INSTANCE_SHADOW_RULE_H_ + +#include + +#include "absl/strings/string_view.h" +#include "common/analysis/lint_rule_status.h" +#include "common/analysis/syntax_tree_lint_rule.h" +#include "common/text/symbol.h" +#include "common/text/syntax_tree_context.h" +#include "verilog/analysis/descriptions.h" + +namespace verilog { +namespace analysis { + +// InstanceShadowRule determines if a variable name shadows an already +// existing instance in that scope. +class InstanceShadowRule : public verible::SyntaxTreeLintRule { + public: + using rule_type = verible::SyntaxTreeLintRule; + static absl::string_view Name(); + + // Returns the description of the rule implemented formatted for either the + // helper flag or markdown depending on the parameter type. + static const LintRuleDescriptor& GetDescriptor(); + + void HandleSymbol(const verible::Symbol& symbol, + const verible::SyntaxTreeContext& context) override; + + verible::LintRuleStatus Report() const override; + + private: + // Link to style guide rule. + static const char kTopic[]; + + // Diagnostic message. + static const char kMessage[]; + + std::set violations_; +}; + +} // namespace analysis +} // namespace verilog + +#endif // VERIBLE_VERILOG_ANALYSIS_CHECKERS_INSTANCE_SHADOW_RULE_H_ diff --git a/verilog/analysis/checkers/instance_shadow_rule_test.cc b/verilog/analysis/checkers/instance_shadow_rule_test.cc new file mode 100644 index 000000000..b5fdb5e27 --- /dev/null +++ b/verilog/analysis/checkers/instance_shadow_rule_test.cc @@ -0,0 +1,179 @@ +// Copyright 2017-2020 The Verible Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "verilog/analysis/checkers/instance_shadow_rule.h" + +#include +#include + +#include "absl/status/status.h" +#include "absl/strings/string_view.h" +#include "common/analysis/linter_test_utils.h" +#include "common/analysis/syntax_tree_linter.h" +#include "common/analysis/syntax_tree_linter_test_utils.h" +#include "common/util/logging.h" +#include "gtest/gtest.h" +#include "verilog/analysis/verilog_analyzer.h" +#include "verilog/parser/verilog_token_enum.h" + +namespace verilog { +namespace analysis { +namespace { + +using verible::LintTestCase; +using verible::RunLintTestCases; + +TEST(InstanceShadowingTest, FunctionPass) { + const std::initializer_list kInstanceShadowingTestCases = { + {""}, + {"module foo; logic a; endmodule"}, + {"module foo; logic a, b, c; endmodule:foo;"}, + {"module foo; logic a; class boo; endclass:boo; endmodule:foo;"}, + {"module foo; logic a; begin boo; bit var1; end:boo; endmodule:foo;"}, + {"module foo; logic a; begin boo;", "if(a) begin baz\n", + "$display(\"\t Value of i = %0d\",i);", "\nend:baz", "\nend:boo;", + "endmodule:foo;"}, + {"module foo; logic a; begin boo;", + "loop: for(int i=0;i<5;i++) begin baz\n", + "$display(\"\t Value of i = %0d\",i);", "\nend:baz", "\nend:boo;", + "endmodule:foo;"}, + { + "module foo;\n", + "genvar i;\n", + "generate\n", + "initial begin\n", + "for(int i=0;i<5;i++) $display(\"\t i = %d\",i);\n", + "end\n", + "endgenerate\n", + "endmodule:foo;\n", + }, + { + "module foo;\n", + "initial begin\n", + "int a;\n", + "end\n", + "initial begin\n", + "int a;\n", + "end\n", + "endmodule:foo;\n", + }, + { + "module foo;\n", + "initial begin\n", + "int a;\n", + "end\n", + "int a;\n", + "endmodule:foo;\n", + }, + }; + RunLintTestCases( + kInstanceShadowingTestCases); +} + +TEST(InstanceShadowingTest, FunctionFailures) { + constexpr int kToken = SymbolIdentifier; + // clang-format off + const std::initializer_list kInstanceShadowingTestCases = { + {"module foo; logic ", {kToken, "foo"}, "; endmodule:foo;"}, + {"module foo; logic a; logic ", {kToken, "a"}, "; endmodule:foo;"}, + {"module foo; logic a; class ", + {kToken, "foo"}, + "; endclass:foo; endmodule:foo;"}, + {"module foo; logic a; class boo; logic ", + {kToken, "boo"}, + "; endclass:boo; endmodule:foo;"}, + {"module foo; logic a; class boo; logic ", + {kToken, "foo"}, + "; endclass:boo; endmodule:foo;"}, + { + "module foo;\n", + "int i;\n", + "initial begin\n", + " for(int ", {kToken, "i"}, "=0;i<5;i++) $display(\"i = %d\",i);\n", + "end\n", + "endmodule:foo;\n", + }, + { + "module foo;\n", + "int a;\n", + "initial begin\n", + " int ", {kToken, "a"}, ";\n" + "end\n", + "endmodule:foo;\n", + }, + { + "module foo;\n", + "initial begin\n", + " int a;\n", + " int ", {kToken, "a"},";\n", + "end\n", + "endmodule:foo;\n", + }, + { + "function automatic int foo (input bit in);\n", + " bit ", {kToken, "in"}, ";\n", + "endfunction\n" + }, + { + "function automatic int foo (input bit in, " + " input bit ", {kToken, "in"}, ");\n", + " bit out;\n", + "endfunction\n" + }, + // Currently, we do not recongize the "K&R-Style" declare+define pattern + { + "module foo(a, b);" + " input wire ", {kToken, "a"}, ";" // Warning despite this actually ok + " output reg ", {kToken, "b"}, ";" + "endmodule" + }, + }; + // clang-format on + RunLintTestCases( + kInstanceShadowingTestCases); +} + +TEST(InstanceShadowingTest, CorrectLocationTest) { + constexpr int kToken = SymbolIdentifier; + const std::initializer_list kInstanceShadowingTestCases = { + {"module ", {kToken, "foo"}, "; logic foo;\n endmodule:foo;"}, + {"module foo; logic ", {kToken, "a"}, "; logic a; endmodule:foo;"}, + }; + auto rule_generator = [&]() -> std::unique_ptr { + std::unique_ptr instance(new InstanceShadowRule()); + absl::Status config_status = instance->Configure(""); + CHECK(config_status.ok()) << config_status.message(); + return instance; + }; + for (const auto& test : kInstanceShadowingTestCases) { + VerilogAnalyzer analyzer(test.code, "<>"); + absl::Status unused_parser_status = analyzer.Analyze(); + verible::SyntaxTreeLinter linter_; + linter_.AddRule(rule_generator()); + linter_.Lint(*ABSL_DIE_IF_NULL(analyzer.Data().SyntaxTree())); + CHECK_EQ(linter_.ReportStatus().size(), 1); + CHECK_EQ(linter_.ReportStatus()[0].violations.size(), 1); + + // Report detailed differences, if any. + const absl::string_view base_text = analyzer.Data().Contents(); + absl::string_view foo = test.FindImportantTokens(base_text)[0].text(); + absl::string_view bar = + linter_.ReportStatus()[0].violations.begin()->token.text(); + ASSERT_TRUE(foo == bar); + } +} + +} // namespace +} // namespace analysis +} // namespace verilog diff --git a/verilog/analysis/default_rules.h b/verilog/analysis/default_rules.h index f58e237a4..593dd4952 100644 --- a/verilog/analysis/default_rules.h +++ b/verilog/analysis/default_rules.h @@ -75,6 +75,7 @@ inline constexpr const char* kDefaultRuleSet[] = { // TODO: "banned-declared-name-patterns", // TODO: "port-name-suffix", // "endif-comment", + // "instance-shadowing", }; // LINT.ThenChange(../tools/lint/BUILD) // Update integration tests for rulesets. diff --git a/verilog/tools/lint/BUILD b/verilog/tools/lint/BUILD index 75e2ca46a..3d8d5dace 100644 --- a/verilog/tools/lint/BUILD +++ b/verilog/tools/lint/BUILD @@ -71,6 +71,7 @@ _linter_test_configs = [ ("generate-label", "generate_label_module", True), ("generate-label", "generate-label-module-body", True), # uses parse directive ("generate-label-prefix", "generate_label_prefix", True), + ("instance-shadowing", "shadow_parameter", False), ("invalid-system-task-function", "psprintf", True), ("interface-name-style", "interface_type_name_style", True), ("legacy-genvar-declaration", "legacy_genvar_declaration", False), diff --git a/verilog/tools/lint/testdata/shadow_parameter.sv b/verilog/tools/lint/testdata/shadow_parameter.sv new file mode 100644 index 000000000..a5ce1e826 --- /dev/null +++ b/verilog/tools/lint/testdata/shadow_parameter.sv @@ -0,0 +1,8 @@ +class jumbo_packet; // the shadowed instance + const int max_size = 9 * 1024; + byte payload []; + function new( int size ); + payload = new[ size > max_size ? max_size : size ]; + int jumbo_packet = 0; // shadowing the class name, error + endfunction +endclass