Merge pull request chipsalliance#2230 from hzeller/feature-20240805-r…

…ebase-790

Add variable shadowing rule.

common/analysis/lint_rule_status.cc

@@ -15,15 +15,18 @@
 #include "common/analysis/lint_rule_status.h"
 
 #include <algorithm>
+#include <cstddef>
 #include <functional>
 #include <iostream>
 #include <iterator>
 #include <ostream>
 #include <set>
+#include <sstream>
 #include <string>
 #include <vector>
 
 #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<AutoFix>& autofixes)
+                             const std::vector<AutoFix>& autofixes,
+                             const std::vector<TokenInfo>& 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<verible::TokenInfo>& 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<LintRuleStatus>& 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

common/analysis/lint_rule_status.h

@@ -92,23 +92,40 @@ class AutoFix {
 struct LintViolation {
   // This construct records a token stream lint violation.
   LintViolation(const TokenInfo& token, absl::string_view reason,
-                const std::vector<AutoFix>& autofixes = {})
-      : token(token), reason(reason), context(), autofixes(autofixes) {}
+                const std::vector<AutoFix>& autofixes = {},
+                const std::vector<TokenInfo>& 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<TokenInfo>& 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<AutoFix>& autofixes = {})
-      : token(token), reason(reason), context(context), autofixes(autofixes) {}
+                const std::vector<AutoFix>& autofixes = {},
+                const std::vector<TokenInfo>& 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
   // multiple tokens.  The violation will be reported at the location of
   // the left-most leaf of the subtree.
   LintViolation(const Symbol& root, absl::string_view reason,
                 const SyntaxTreeContext& context,
-                const std::vector<AutoFix>& autofixes = {});
+                const std::vector<AutoFix>& autofixes = {},
+                const std::vector<TokenInfo>& 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<AutoFix> autofixes;
 
+  // Additional tokens that are related somehow to offending token.
+  const std::vector<TokenInfo> 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<verible::TokenInfo>& tokens, absl::string_view message,
+      absl::string_view path, absl::string_view base) const;
 
  private:
   // Translates byte offsets, which are supplied by LintViolations via

common/analysis/lint_rule_status_test.cc

@@ -75,6 +75,7 @@ struct LintViolationTest {
   std::string reason;
   TokenInfo token;
   std::string expected_output;
+  std::vector<TokenInfo> 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",

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"],