Skip to content

Commit

Permalink
Merge pull request #2230 from hzeller/feature-20240805-rebase-790
Browse files Browse the repository at this point in the history
Add variable shadowing rule.
  • Loading branch information
hzeller authored Aug 5, 2024
2 parents 65168e7 + d5dcbe6 commit 79f6290
Show file tree
Hide file tree
Showing 10 changed files with 554 additions and 11 deletions.
44 changes: 40 additions & 4 deletions common/analysis/lint_rule_status.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down
38 changes: 33 additions & 5 deletions common/analysis/lint_rule_status.h
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
47 changes: 45 additions & 2 deletions common/analysis/lint_rule_status_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down Expand Up @@ -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",
Expand Down
44 changes: 44 additions & 0 deletions verilog/analysis/checkers/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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"],
Expand Down
Loading

0 comments on commit 79f6290

Please sign in to comment.