Merge pull request #2198 from sconwayaus/proper_parameter_declaration…

…_autofix

Added an autofix to the proper-parameter-declaration rule

verilog/CST/parameters.cc

@@ -59,6 +59,14 @@ verilog_tokentype GetParamKeyword(const verible::Symbol &symbol) {
   return static_cast<verilog_tokentype>(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<const SyntaxTreeLeaf *>(param_keyword_symbol);
+  return &leaf->get();
+}
+
 const verible::Symbol *GetParamTypeSymbol(const verible::Symbol &symbol) {
   return verible::GetSubtreeAsSymbol(symbol, NodeEnum::kParamDeclaration, 1);
 }

verilog/CST/parameters.h

@@ -71,6 +71,9 @@ std::vector<verible::TreeSearchMatch> 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 &);

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<absl::string_view, absl::string_view> 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[] = {

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

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,26 +152,24 @@ 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) {
       // Raise violation if the context is not inside a class, package or
       // 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);
           }
         }
       }

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;
 

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<verible::AutoFixInOut> 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<VerilogAnalyzer, ProperParameterDeclarationRule>(kTestCases);
+}
+
+TEST(ProperParameterDeclarationRuleTest, AutoFixAllowParametersInPackage) {
+  const std::initializer_list<verible::AutoFixInOut> 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<VerilogAnalyzer, ProperParameterDeclarationRule>(
+      kTestCases,
+      "package_allow_parameter:true;package_allow_localparam:false");
+}
+
 }  // namespace
 }  // namespace analysis
 }  // namespace verilog