Module file name rule autofix

[sconwayaus]

Added a simple autofix for [module-filename] lint rule to rename the module to match the filename.

[corco]

Will it work on closing module name like:

module a();

endmodule : a

[sconwayaus]

No, but I'll look into it.

On Tue, 16 Apr 2024, 10:38 pm Jonathan Drolet, @.***> wrote:

Will it work on closing module name like:

module a();

endmodule : a

— Reply to this email directly, view it on GitHub https://github.com/chipsalliance/verible/pull/2162#issuecomment-2058989979, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABVJSIZ44D7X3YH6A2IJ7VTY5ULTDAVCNFSM6AAAAABGJIEAJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANJYHE4DSOJXHE . You are receiving this because you authored the thread.Message ID: @.***>

[IEncinas10]

I checked this out of curiosity, sharing in case it is useful @sconwayaus

diff --git a/verilog/analysis/checkers/module_filename_rule.cc b/verilog/analysis/checkers/module_filename_rule.cc
index 8e1d9cc1e0c1..369602379973 100644
--- a/verilog/analysis/checkers/module_filename_rule.cc
+++ b/verilog/analysis/checkers/module_filename_rule.cc
@@ -121,14 +121,24 @@ void ModuleFilenameRule::Lint(const TextStructureView &text_structure,
   }

   // Only report a violation on the last module declaration.
-  const auto *last_module_id = GetModuleName(*module_cleaned.back().match);
+  const verible::Symbol &last_module = *module_cleaned.back().match;
+  const auto *last_module_id = GetModuleName(last_module);
   if (!last_module_id) LOG(ERROR) << "Couldn't extract module name";
   if (last_module_id) {
-    std::string autofix_msg =
+    const std::string autofix_msg =
         absl::StrCat("Rename module to '", unitname, "' to match filename");
+
+    verible::AutoFix autofix{autofix_msg, {last_module_id->get(), unitname}};
+
+    const verible::SyntaxTreeLeaf *last_module_end_label =
+        GetModuleEndLabel(last_module);
+    if (last_module_end_label) {
+      autofix.AddEdits({{last_module_end_label->get(), unitname}});
+    }
+
     verible::LintViolation violation = verible::LintViolation(
         last_module_id->get(), absl::StrCat(kMessage, "\"", unitname, "\""),
-        {verible::AutoFix(autofix_msg, {last_module_id->get(), unitname})});
+        {autofix});

     violations_.insert(violation);
   }

quickhack.txt

git apply quickhack.txt should work with your current version

Edit: Just in case you want to quickly check autofix stuff, after recompiling verilog-verible-lint (bazel build -c opt //verilog/tools/lint:verible-verilog-lint). You can do stuff like:

$ verible-verilog-lint --autofix=patch-interactive name.sv --ruleset=all
name.sv:1:8-12: Declared module does not match the first dot-delimited component of file name: "name" [Style: file-names] [module-filename]
[ Rename module to 'name' to match filename ]
@@ -1,2 +1,2 @@
-module namee;
-endmodule : namee
+module name;
+endmodule : name
Autofix is available. Apply? [y,n,a,d,A,D,p,P,?] n

[sconwayaus]

Now updates the endmodule identifier (if present).

I should point out is the rule now raises a violation for all modules (excluding inner modules) if no matches are found. The rational is that the checker doesn't know what module to rename with the autofix, so it now suggests an autofix for all modules.

[IEncinas10]

I should point out is the rule now raises a violation for all modules (excluding inner modules) if no matches are found.

I don't understand this change. I see how defaulting to the last module isn't particularly "smart" but I don't see any strict improvement over the default.

The rational is that the checker doesn't know what module to rename with the autofix, so it now suggests an autofix for all modules.

It does know, right? it will apply/suggest the autofix for the last module in the file.

By the way, this change can lead to very strange situations if you apply all suggested fixes by the tool:

verible-verilog-lint --autofix=patch-interactive name.sv --ruleset=all
name.sv:1:8-12: Declared module does not match the first dot-delimited component of file name: "name" [Style: file-names] [module-filename]
[ Rename module to 'name' to match filename ]
@@ -1,3 +1,3 @@
-module namee;
-endmodule : namee
+module name;
+endmodule : name

Autofix is available. Apply? [y,n,a,d,A,D,p,P,?] ?
y - apply fix
n - reject fix
a - apply this and all remaining fixes for violations of this rule
d - reject this and all remaining fixes for violations of this rule
A - apply this and all remaining fixes
D - reject this and all remaining fixes
p - show fix
P - show fixes applied in this file so far
? - print this help and prompt again

Autofix is available. Apply? [y,n,a,d,A,D,p,P,?] A
name.sv:4:8-10: Declared module does not match the first dot-delimited component of file name: "name" [Style: file-names] [module-filename]
--- a/name.sv
+++ b/name.sv
@@ -1,5 +1,5 @@
-module namee;
-endmodule : namee
+module name;
+endmodule : name

-module asd;
-endmodule : asd
+module name;
+endmodule : name

[sconwayaus]

The intention was to present the user with all the options, and let the user choose the "right" moudle to rename. Seems to work well in VSCode.

I do see your point on users accepting all autofixes from the CLI.

If you feel strongly that this was a poor choice I can revert to the original behaviour.

[hzeller]

I am wondering if the filename should take the precedence here, imposing the name to the module.

Usually one would write stuff, comes up with a better name of a module and then rather would have the filename be renamed to match the new name of the module.

I am not sure though if that can be auto-fixed easily (I think there is no easy 'rename file edit' that can be send to the LSP), and it would probably also create other issues like having to update build-systems, having to deal with filenames that already exists etc.

Changing the module name to the filename is simple to implement, but does it also adhere to the principle of least surprise ?

[corco]

I am wondering if the filename should take the precedence here, imposing the name to the module.

Usually one would write stuff, comes up with a better name of a module and then rather would have the filename be renamed to match the new name of the module.

I am not sure though if that can be auto-fixed easily (I think there is no easy 'rename file edit' that can be send to the LSP), and it would probably also create other issues like having to update build-systems, having to deal with filenames that already exists etc.

Changing the module name to the filename is simple to implement, but does it also adhere to the principle of least surprise ?

I disagree about ever changing the filename on disk. This could create conflict if two separate files have a module with the same name (like, if you copy a.sv to b.sv).

It is always safe to rename the module to match the filename.

[sconwayaus]

Usually one would write stuff, comes up with a better name of a module and then rather would have the filename be renamed to match the new name of the module.

Agree, this is how I tend to work, but sometimes I go the other way too.

Using the Language Server a bit more I really should rename the symbol so usages get fixed too. With this in mind this change is of little value for my own work flow, and I'm not sure it's of much use to others.

Happy to make changes to this PR if others think it would be useful, otherwise I might close it next week without merging.

[IEncinas10]

Using the Language Server a bit more I really should rename the symbol so usages get fixed too.

It is somewhat supported: https://github.com/chipsalliance/verible/pull/2081 but the "machinery" doesn't really run so smoothly yet.

I still think the autofix is valuable (more so if there is a label matching the module). I wouldn't get too caught up on discussing whether this is that useful. There are autofixes to remove trailing whitespaces, redundant semicolons...

I think the small change is worthwhile, although I wouldn't change the old behaviour of only complaining about the last module.

[sconwayaus]

Reverted back to raising violation on the last module.

[hzeller]

If we offer potential fixes, maybe we could also offer just adding a waive-comment (watching myself: that is typically what I actually do when I run into this lint message)

// verilog_lint: waive module-filename

Having multiple choices for autofixes vs. a single one can also help the user see that this is something to choose from, not an ultimate 'this is the only possible fix' way.

I will probably have time to review this more thoroughly on Friday, but good work so far! I love seeing these improved autofixes.