Allow rule aliasing

[wsipak]

This is a proposal of a solution for #902.
With these changes, we can use an alternative name for a rule.
This is how aliases can be defined:

VERILOG_REGISTER_LINT_RULE(UndersizedBinaryLiteralRule);
VERILOG_ALIAS_LINT_RULE(UndersizedBinaryLiteralRule, "undersized-numeric-literal", "some-other-alias");

--help_rules=all prints the aliases for each rule where they exist.
When --help_rules option is used with a specific name, the result is the same regardless of if the original name or alias was used.

[codecov-commenter]

Codecov Report

Merging #935 (888ad16) into master (19ed159) will decrease coverage by 0.01%.
The diff coverage is 82.14%.

@@            Coverage Diff             @@
##           master     #935      +/-   ##
==========================================
- Coverage   92.72%   92.71%   -0.02%     
==========================================
  Files         316      316              
  Lines       20577    20604      +27     
==========================================
+ Hits        19081    19103      +22     
- Misses       1496     1501       +5     

Impacted Files	Coverage Δ
verilog/analysis/verilog_linter.cc	91.12% <37.50%> (-1.51%)	⬇️
verilog/analysis/lint_rule_registry.cc	100.00% <100.00%> (ø)
verilog/analysis/verilog_linter_configuration.cc	89.14% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 19ed159...888ad16. Read the comment docs.

[hzeller]

We might also need configuration.

The issue in #902 is not completely hashed out yet. The problem we'd like to solve is

• we start out with a rule with a particular name that does something
• Later, some more functionality is added as it is closely related ,and this can then be switched and configured using the configuration.
• Now, the original name does not necessarily fit anymore, but we want to keep it around as people might use it. Also, we probably have to keep most of the configuration disabled to not confuse existing users
• Ideally, we now can define different rules with a different name that are essentially just call the old rule, but with a potentially different configuration.
• There might be all kinds of corner-cases that have to be thought of: in messages for instance, we'd need the new name to display. Maybe by having a different LintRuleDescriptor ? So from that perspective, it just looks like a different rule, but goal of Internal: Consider ways to extend and rename/alias rules #902 should be to reduce the necessary boilerplate to do so.

So before we can come to a definitive implementation conclusion, we should discuss if there are other corner cases to consider, how would we like the such aliases define etc.

[mglb]

* There might be all kinds of corner-cases that have to be thought of: in messages for instance, we'd need the new name to display.

"New name" is always the primary name of the rule implementation, right? So assuming undersized-binary-literal ::= undersized-numeric-literal with configuration="bin:true", do both +undersized-binary-literal and +undersized-numeric-literal=bin:true would display undersized-numeric-literal?

If so, translating aliases in a configuration reading code should cover all cases. The rest of codebase wouldn't even know that alias has been used.

Maybe by having a different LintRuleDescriptor?

Or something like:

struct LintRuleAliasDescriptor {
  LintRuleId name;             // ID/name of the alias.
  absl::string_view topic;     // section in style-guide
  absl::string_view dv_topic;  // section in design verification style-guide
  std::string desc;            // Detailed description
  std::vector<std::pair<absl::string_view, absl::string_view>>
      param_defaults;  // List of param values set by the alias
};

// ...

const std::vector<LintRuleAliasDescriptor>&
UndersizedNumericLiteralRule::GetAliasDescriptors() {
  static const std::vector<LintRuleAliasDescriptor> d{
      {
          .name = "undersized-binary-literal",
          // Empty .topic -> the same as the original rule's
          // Empty .desc -> something like:
          //   "Alias for <original_rule_name> with config=<param>:<value>"
          .param_defaults =
              {
                  {"bin", "true"},
              },
      },
  };
  return d;
}

VERILOG_REGISTER_LINT_RULE could even use SFINAE to detect presence of GetAliasDescriptors function, limiting registration to just one macro call.

[wsipak]

1. The current state solves the problem of configuration and doesn't yetsolve the problem of printing messages with the right names. Rules can be aliased without changing the way they're registered. Following @mglb's ideas, aliasing a rule requires adding a function that returns a vector of alias descriptors.

2. (about mixing original names with aliases in configuration)

Right now it's legal to use the same rule several times in configuration. You can disable it, enable, disable again, then apply addiontional changes on top of it using the comandline arguments. This is processed in order, the defaults first, then the config file, then the arguments.

Should we disallow this (or warn about this) when we introduce aliases?

Example case:

+rule1         // rule1 is enabled
-rule1-alias   // rule1 is disabled
+rule1         // rule1 is enabled

It's okay, would work the same as without aliases.

Another case:

+rule1-alias   // rule1 is enabled, with default parameters of the alias
-rule1         // rule1 is disabled

Now, if we enable the rule again, either by its original name or an alias, should we care about the previously set parameter values? Should we delete them? Of course this case may make no sense in practical view, but as Verible allows to enable and disable rules multiple times, do we need to address this additionally?

3. (about printing messages with the right name)

Aliases are translated to the original names early, so that there's no need to create double rules and keep track of whether the original one, or the aliased one is enabled or disabled.

The name of a rule (printed when Verible finds violations) is stored in a const structure, like this one:
https://cs.opensource.google/verible/verible/+/master:verilog/analysis/checkers/undersized_binary_literal_rule.cc;l=56

In order to implement printing the right name (alias, if alias was used, the original name otherwise) we could change this to variable and overwrite the .name field, but I expect that It would be better to keep it as a constant.

We could add a field for the name that would actually be printed in logs (like printed_name) and assign the value of either .name from the descriptor of the rule, or descriptor of an alias, depending on which was used in configuration.

Alternatively, we can store information about which name shall be printed in an additional container, (not in each rule).

[hzeller]

I like this implementation so far. I'll play around with it a bit and report back.

[hzeller]

I think it got more complicated by having a GetAliasDescriptors() function. It looks like adding the alias descriptors just to the toplevel descriptor (which can be left uninitialized if a rule does not have an alias) should make it more easy to maintain.

[hzeller]

For the 'numeric' literal, we should now actually switch all bin, oct and hex to 'true', while the undersized-binary-literal would have bin=true and oct=false, hex=false. That way we can have the advantage of having a good easy-to-discover al lthe features-rule and the limited to what the old binary rule to not surprise existing users-rule.

This would be also the typical application of the aliases: rules evolve and will get more features, but somtimes we don't want to expose these features in a rule that had a very specific name (like in this case where 'binary' kindof indicates that it only deals with one type of number).

So let's have here all features turned on and in the binary rule the ones not bin turned off.

[hzeller]

We should probably rename the whole class to be UndersizedNumericLiteralRule, also the naming of the header and test.

This should probably be a separate PR after this is done, so that we don't have too many changes at once.

[hzeller]

std::pairs have somewhat arbitrary first, second members, so it is hard to read. Almost always it is better to define a struct for that with a good name. Adds a few lines, but also makes it much more readable in every place it is used.

Maybe we can re-use the name/value thing we have going on in LintRuleConfigParameterDescriptor, but decompose it to only use the part with name/value in the alias while the base rule has also description.

So have a little hierarchy (usually, I'd be in favor of composition instead of inheritance, but we already have a bunch of descriptors defined that all would need changing; it is not too bad

struct LintConfigParameterNameValue {
  absl::string_view name;
  std::string default_value;
};

// The descriptor also has a human readable description of this parameter
struct LintConfigParameterDescriptor : public LintConfigParameterNameValue {
  std::string description;
};

Then, replace the pair with our new name/value struct:

struct LintRuleAliasDescriptor {
  LintRuleId name;  // ID/name of the alias.                                                                                                                                                                  
  std::vector<LintConfigParameterNameValue>  param_defaults;  // List of param values set by the alias                                                                                                                                               
};

[hzeller]

Instead of having a separate function GetAliasDescriptors() (which then requires some SFINAE-magic), I suggest to add a new field std::vector<LintRuleAliasDescriptor> aliases at the end of our struct LintRuleDescriptor.
That way, everything can be brace-initialized more compactly in one go in the descriptor and we require less support code for the alias rules as we can just look through the existing descriptor if there are aliases.

(Recently I cleaned up the rules that had multiple static functions for their descriptions to just return one descriptor which made things simpler; I'd like to avoid it getting more compilcated again)