false-positive for CamelCase parameter in a SV module

[jdhaliwa]

Describe the bug I was running the verible linting rules on my SystemVerilog code but I noticed a small bug while linting parameter variables. I am running the linting with the following rules for parameters and localparams:

+parameter-name-style=parameter_style:ALL_CAPS
+parameter-name-style=localparam_style:ALL_CAPS

But apparently, only the second line properly applies the rule while the first one also admits the CamelCase. NOTE: The snake_case is not allowed by both (correct behavior). For example:

module test #(
    parameter int Apple,
    localparam int Banana
);
endmodule

The parameter Apple is correct as well as APPLE (false-positive). If I try with apple it shows the following error:

Non-type parameter names must be styled with CamelCase or ALL_CAPS [Style: constants][parameter-name-style]

On the other hand, Banana is not correct because it has to be ALL_CAPS and it shows the following error:

Non-type localparam names must be styled with ALL_CAPS [Style: constants][parameter-name-style]

[snsokolov]

I suspect the second style overrides the first one, should it be a single line something like this?

+parameter-name-style=parameter_style:ALL_CAPS;localparam_style:ALL_CAPS

[IEncinas10]

Indeed, the last value is the one that "wins". https://github.com/chipsalliance/verible/blob/d9163ae08d481fdea6660777a9005b876b32a840/verilog/analysis/verilog_linter_configuration.cc#L163

In any case, I think checking and warning users about this would be desirable. What do you think of something like this @jdhaliwa?

diff --git a/verilog/analysis/verilog_linter_configuration.cc b/verilog/analysis/verilog_linter_configuration.cc
index 15ecb300..98a01b68 100644
--- a/verilog/analysis/verilog_linter_configuration.cc
+++ b/verilog/analysis/verilog_linter_configuration.cc
@@ -155,6 +155,15 @@ bool RuleBundle::ParseConfiguration(absl::string_view text, char separator,
       parsed_correctly = false;
       continue;
     }
+
+    if (rules.count(*rule_iter)) {
+      absl::StrAppend(error, error->empty() ? "" : "\n", kRepeatedFlagMessage,
+                      " \"", rule_name, "\" = ", setting.configuration);
+      parsed_correctly = false;
+    }
+
     // Map keys must use canonical registered string_views for guaranteed
     // lifetime, not just any string-equivalent copy.
     rules[*rule_iter] = setting;
diff --git a/verilog/analysis/verilog_linter_configuration.h b/verilog/analysis/verilog_linter_configuration.h
index 471da65e..7917fffb 100644
--- a/verilog/analysis/verilog_linter_configuration.h
+++ b/verilog/analysis/verilog_linter_configuration.h
@@ -42,6 +42,12 @@ struct RuleSetting {
 // encountered while parsing a configuration
 inline constexpr absl::string_view kInvalidFlagMessage = "[ERR] Invalid flag";

+// Warning to be shown when we parse a configuration file that configures the
+// same rule more than once.
+inline constexpr absl::string_view kRepeatedFlagMessage =
+    "[WARN] Repeated flag in the configuration. Last provided value will be "
+    "used";
+
 // Warning to be shown when an stray comma is
 // encountered while parsing a configuration
 inline constexpr absl::string_view kStrayCommaWarning =

You should get this value in your editor's LSP log after setting the correct verbosity:

[ERROR][2024-03-09 10:49:43] .../vim/lsp/rpc.lua:798    "rpc"   "/home/ier/cosas/contributing/verible/bazel-bin/verilog/tools/ls/verible-verilog-ls"    "stderr"    'Using a partial version from /home/ier/cosas/contributing/verible/.rules.verible_lint. Found the following issues: [WARN] Repeated flag in the configuration. Last provided value will be used "parameter-name-style" = localparam_style:ALL_CAPS'

Edit: https://github.com/IEncinas10/verible/tree/warn-about-repeated-rules. If you're happy I'll open a PR

[jdhaliwa]

Yes, that was the case. Thanks @snsokolov, I solved it in this way. Indeed @IEncinas10 it is useful to put a check on this, thanks for fixing it! You can go ahead with the PR.

[IEncinas10]

Looks like we're done here! Thanks for filing the issue.