diff --git a/verilog/CST/parameters.cc b/verilog/CST/parameters.cc index 34f54135d..1453e7f3a 100644 --- a/verilog/CST/parameters.cc +++ b/verilog/CST/parameters.cc @@ -59,6 +59,14 @@ verilog_tokentype GetParamKeyword(const verible::Symbol &symbol) { return static_cast(leaf->get().token_enum()); } +const verible::TokenInfo *GetParameterToken(const verible::Symbol &symbol) { + const auto *param_keyword_symbol = + verible::GetSubtreeAsSymbol(symbol, NodeEnum::kParamDeclaration, 0); + if (param_keyword_symbol == nullptr) return nullptr; + const auto *leaf = down_cast(param_keyword_symbol); + return &leaf->get(); +} + const verible::Symbol *GetParamTypeSymbol(const verible::Symbol &symbol) { return verible::GetSubtreeAsSymbol(symbol, NodeEnum::kParamDeclaration, 1); } diff --git a/verilog/CST/parameters.h b/verilog/CST/parameters.h index 778b565fc..4e449fa8b 100644 --- a/verilog/CST/parameters.h +++ b/verilog/CST/parameters.h @@ -71,6 +71,9 @@ std::vector FindAllNamedParams( // kParamDeclaration (either TK_parameter or TK_localparam). verilog_tokentype GetParamKeyword(const verible::Symbol &); +// Returns the token for kParamDeclaration symbol +const verible::TokenInfo *GetParameterToken(const verible::Symbol &symbol); + // Returns a pointer to either TK_type or kParamType node, which holds the param // type, id, and dimensions info for that parameter. const verible::Symbol *GetParamTypeSymbol(const verible::Symbol &); diff --git a/verilog/CST/parameters_test.cc b/verilog/CST/parameters_test.cc index 25b49f16e..408d02ea9 100644 --- a/verilog/CST/parameters_test.cc +++ b/verilog/CST/parameters_test.cc @@ -153,6 +153,34 @@ TEST(GetParamKeywordTest, MultipleParamsDeclared) { } } +// Tests that GetParameterToken correctly returns the token of the +// parameter. +TEST(GetParameterTokenTest, BasicTests) { + constexpr std::pair kTestCases[] = { + {"module foo; parameter Bar = 1; endmodule", "parameter"}, + {"module foo; localparam Bar_1 = 1; endmodule", "localparam"}, + {"module foo; localparam int HelloWorld = 1; endmodule", "localparam"}, + {"module foo #(parameter int HelloWorld1 = 1); endmodule", "parameter"}, + {"class foo; parameter HelloWorld_1 = 1; endclass", "parameter"}, + {"class foo; localparam FooBar = 1; endclass", "localparam"}, + {"class foo; localparam int Bar_1_1 = 1; endclass", "localparam"}, + {"package foo; parameter BAR = 1; endpackage", "parameter"}, + {"package foo; parameter int HELLO_WORLD = 1; endpackage", "parameter"}, + {"package foo; localparam BAR = 1; endpackage", "localparam"}, + {"package foo; localparam int HELLO_WORLD = 1; endpackage", "localparam"}, + {"parameter int Bar = 1;", "parameter"}, + }; + for (const auto &test : kTestCases) { + VerilogAnalyzer analyzer(test.first, ""); + ASSERT_OK(analyzer.Analyze()); + const auto &root = analyzer.Data().SyntaxTree(); + const auto param_declarations = FindAllParamDeclarations(*root); + const auto name_token = + GetParameterToken(*param_declarations.front().match); + EXPECT_EQ(name_token->text(), test.second); + } +} + // Tests that GetParamTypeSymbol correctly returns the kParamType node. TEST(GetParamTypeSymbolTest, BasicTests) { constexpr absl::string_view kTestCases[] = { diff --git a/verilog/analysis/checkers/BUILD b/verilog/analysis/checkers/BUILD index b032b4bd4..b90f2fe1e 100644 --- a/verilog/analysis/checkers/BUILD +++ b/verilog/analysis/checkers/BUILD @@ -1748,6 +1748,7 @@ cc_library( "//common/text:config-utils", "//common/text:symbol", "//common/text:syntax-tree-context", + "//common/text:token-info", "//verilog/CST:context-functions", "//verilog/CST:parameters", "//verilog/CST:verilog-matchers", diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc index 3d5f36bb9..f34f967cb 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.cc @@ -24,6 +24,7 @@ #include "common/text/config_utils.h" #include "common/text/symbol.h" #include "common/text/syntax_tree_context.h" +#include "common/text/token_info.h" #include "verilog/CST/context_functions.h" #include "verilog/CST/parameters.h" #include "verilog/CST/verilog_matchers.h" @@ -34,9 +35,9 @@ namespace verilog { namespace analysis { +using verible::AutoFix; using verible::LintRuleStatus; using verible::LintViolation; -using verible::SyntaxTreeContext; using verible::matcher::Matcher; // Register ProperParameterDeclarationRule @@ -58,6 +59,11 @@ static constexpr absl::string_view kLocalParamAllowPackageMessage = static absl::string_view kParameterMessage = kParameterNotInPackageMessage; static absl::string_view kLocalParamMessage = kLocalParamAllowPackageMessage; +static absl::string_view kAutoFixReplaceParameterWithLocalparam = + "Replace 'parameter' with 'localparam'"; +static absl::string_view kAutoFixReplaceLocalparamWithParameter = + "Replace 'localparam' with 'parameter'"; + const LintRuleDescriptor &ProperParameterDeclarationRule::GetDescriptor() { static const LintRuleDescriptor d{ .name = "proper-parameter-declaration", @@ -107,9 +113,37 @@ static const Matcher &ParamDeclMatcher() { return matcher; } +void ProperParameterDeclarationRule::AddParameterViolation( + const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) { + const verible::TokenInfo *token = GetParameterToken(symbol); + + if (token == nullptr) { + return; + } + + AutoFix autofix = + AutoFix(kAutoFixReplaceParameterWithLocalparam, {*token, "localparam"}); + violations_.insert( + LintViolation(*token, kParameterMessage, context, {autofix})); +} + +void ProperParameterDeclarationRule::AddLocalparamViolation( + const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) { + const verible::TokenInfo *token = GetParameterToken(symbol); + + if (token == nullptr) { + return; + } + + AutoFix autofix = + AutoFix(kAutoFixReplaceLocalparamWithParameter, {*token, "parameter"}); + violations_.insert( + LintViolation(*token, kLocalParamMessage, context, {autofix})); +} + // TODO(kathuriac): Also check the 'interface' and 'program' constructs. void ProperParameterDeclarationRule::HandleSymbol( - const verible::Symbol &symbol, const SyntaxTreeContext &context) { + const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) { verible::matcher::BoundSymbolManager manager; if (ParamDeclMatcher().Matches(symbol, &manager)) { const auto param_decl_token = GetParamKeyword(symbol); @@ -118,13 +152,13 @@ void ProperParameterDeclarationRule::HandleSymbol( // kFormalParameterList. if (ContextIsInsideClass(context) && !ContextIsInsideFormalParameterList(context)) { - violations_.insert(LintViolation(symbol, kParameterMessage, context)); + AddParameterViolation(symbol, context); } else if (ContextIsInsideModule(context) && !ContextIsInsideFormalParameterList(context)) { - violations_.insert(LintViolation(symbol, kParameterMessage, context)); + AddParameterViolation(symbol, context); } else if (ContextIsInsidePackage(context)) { if (!package_allow_parameter_) { - violations_.insert(LintViolation(symbol, kParameterMessage, context)); + AddParameterViolation(symbol, context); } } } else if (param_decl_token == TK_localparam) { @@ -132,12 +166,10 @@ void ProperParameterDeclarationRule::HandleSymbol( // module, report violation. if (!ContextIsInsideClass(context) && !ContextIsInsideModule(context)) { if (!ContextIsInsidePackage(context)) { - violations_.insert( - LintViolation(symbol, kLocalParamMessage, context)); + AddLocalparamViolation(symbol, context); } else { if (!package_allow_localparam_) { - violations_.insert( - LintViolation(symbol, kLocalParamMessage, context)); + AddLocalparamViolation(symbol, context); } } } diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule.h b/verilog/analysis/checkers/proper_parameter_declaration_rule.h index 410459154..e645b5f33 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule.h +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule.h @@ -37,6 +37,12 @@ class ProperParameterDeclarationRule : public verible::SyntaxTreeLintRule { static const LintRuleDescriptor &GetDescriptor(); + void AddParameterViolation(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context); + + void AddLocalparamViolation(const verible::Symbol &symbol, + const verible::SyntaxTreeContext &context); + void HandleSymbol(const verible::Symbol &symbol, const verible::SyntaxTreeContext &context) final; diff --git a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc index 2ad3c69d4..5ecb9e5d4 100644 --- a/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc +++ b/verilog/analysis/checkers/proper_parameter_declaration_rule_test.cc @@ -27,6 +27,7 @@ namespace analysis { namespace { using verible::LintTestCase; +using verible::RunApplyFixCases; using verible::RunConfiguredLintTestCases; using verible::RunLintTestCases; @@ -224,6 +225,88 @@ TEST(ProperParameterDeclarationRuleTest, AllowPackageParameters) { "package_allow_parameter:true;package_allow_localparam:false"); } +TEST(ProperParameterDeclarationRuleTest, AutoFixAllowLocalParamInPackage) { + const std::initializer_list kTestCases = { + {"package foo; parameter int Bar = 1; endpackage", + "package foo; localparam int Bar = 1; endpackage"}, + // TODO (sconwayaus): Commented out as the linter_test_util dosn't handle + // multiple violations. linter_test_utils.h:137] Check failed: + // violations.size() == 1 (2 vs. 1) TODO: apply multi-violation fixes + // {"package foo; parameter int Bar = 1; parameter int Bar2 = 2; " + // "endpackage", + // "package foo; localparam int Bar = 1; localparam int Bar2 = 2; " + // "endpackage"}, + {"module foo; parameter int Bar = 1; endmodule", + "module foo; localparam int Bar = 1; endmodule"}, + {"class foo; parameter int Bar = 1; endclass", + "class foo; localparam int Bar = 1; endclass"}, + {"package foo; class bar; endclass parameter int HelloWorld = 1; " + "endpackage", + "package foo; class bar; endclass localparam int HelloWorld = 1; " + "endpackage"}, + {"package foo; class bar; parameter int HelloWorld = 1; endclass " + "endpackage", + "package foo; class bar; localparam int HelloWorld = 1; endclass " + "endpackage"}, + {"module foo #(parameter int Bar = 1); parameter int HelloWorld = 1; " + "endmodule", + "module foo #(parameter int Bar = 1); localparam int HelloWorld = 1; " + "endmodule"}, + {"module foo #(parameter type Bar); parameter type Bar2; endmodule", + "module foo #(parameter type Bar); localparam type Bar2; endmodule"}, + {"module foo #(parameter type Bar);module innerFoo #(parameter type " + "innerBar, localparam int j = 2)();parameter int i = 1; localparam int " + "j = 2; endmodule endmodule", + "module foo #(parameter type Bar);module innerFoo #(parameter type " + "innerBar, localparam int j = 2)();localparam int i = 1; localparam int " + "j = 2; endmodule endmodule"}, + }; + RunApplyFixCases(kTestCases); +} + +TEST(ProperParameterDeclarationRuleTest, AutoFixAllowParametersInPackage) { + const std::initializer_list kTestCases = { + {"package foo; localparam int Bar = 1; endpackage", + "package foo; parameter int Bar = 1; endpackage"}, + {"module foo; parameter int Bar = 1; endmodule", + "module foo; localparam int Bar = 1; endmodule"}, + {"class foo; parameter int Bar = 1; endclass", + "class foo; localparam int Bar = 1; endclass"}, + {"package foo; class bar; parameter int HelloWorld = 1; endclass " + "endpackage", + "package foo; class bar; localparam int HelloWorld = 1; endclass " + "endpackage"}, + {"module foo #(parameter int Bar = 1); parameter int HelloWorld = 1; " + "endmodule", + "module foo #(parameter int Bar = 1); localparam int HelloWorld = 1; " + "endmodule"}, + {"module foo #(parameter type Bar); parameter type Bar2; endmodule", + "module foo #(parameter type Bar); localparam type Bar2; endmodule"}, + {"module foo #(parameter type Bar);" + "module innerFoo #(" + "parameter type innerBar, localparam int j = 2)();parameter int i = 1;" + "localparam int j = 2;" + "endmodule " + "endmodule", + "module foo #(parameter type Bar);" + "module innerFoo #(" + "parameter type innerBar, localparam int j = 2)();localparam int i = 1;" + "localparam int j = 2;" + "endmodule " + "endmodule"}, + {"localparam int Bar = 1;", "parameter int Bar = 1;"}, + {"package foo; localparam int Bar = 1; endpackage", + "package foo; parameter int Bar = 1; endpackage"}, + {"package foo; class bar; endclass localparam int HelloWorld = 1; " + "endpackage", + "package foo; class bar; endclass parameter int HelloWorld = 1; " + "endpackage"}, + }; + RunApplyFixCases( + kTestCases, + "package_allow_parameter:true;package_allow_localparam:false"); +} + } // namespace } // namespace analysis } // namespace verilog