New rule that ensures module/interface/package/program identifier matches the filename it's in
Closes dalance/svlint#252
The pass testcase is slightly unintuitive in that the module name does not actually match the file it's currently in, but works due to how build.rs modifies the filenames for testing.
Nice one @ShantanuPSinha :)
The . was used in the testcase filenames specifically because it's a character that can't appear in an identifier!
To get the testcases working, I'd like to suggest a different approach (not replacing . with _): Just match everything in the filename up to the first non-identifier character.
As well as Foo.1of5.sv (like the testcases), that would allow these filenames to contain a module Foo: Foo.v, Foo.bugfix.sv, Foo-whatever.weird, etc. for all those places that have weird and wonderful tool flows.
Thanks @DaveMcEwan!
Just a couple of clarifying questions.
-
module foo;is allowed to live in any file of formatFoo<Non-Identifier><whatever else>and the stopping point for string matching has to be a non-identifier character? This wayFooBar.svis an invalid location, butFoo-Bar.svis a valid filename formodule Foo;. -
For
cargo test, after restoring the periods in test file names in build.rs, the filename now looks likesyntaxrules.module_identifier_matches_filename.pass.1of1Matching everything in the filename up to the first non-identifier character, would now only capturesyntaxrules. The testcases, again, can be modified such that the pass testcase ismodule syntaxrules;, but this may make for unintuitive reading of the manual. Do you have any thoughts or ideas on this? -
Should I enforce case-sensitivity (i.e.
module foo;has to be infoo.svandFoo.svwill fail) or is that up to TextRules?
Thanks for your time
Updates:
- Made documentation explanations clearer.
- File Identifier is now a substring from the filename up to the first non-identifier. This file identifier is then compared to the module/package/interface/program identifier.
- Testcases are updated with an explanation for why
module syntaxrules;passes the testcase. I believe this explanation resolves the issue of the manual feeling unintuitive.
Notes: I am using str::eq_ignore_ascii_case to compare the file identifier with the test identifier. This is case insensitive. Changing it to account for case sensitivity is trivial, however, does Case matter in the context of this rule? I do not see a scenario where a user has module foo; and module Foo; in the same scope and intends for them to be two distinct modules.
Hey @DaveMcEwan, do you have any feedback/suggestions?
@DaveMcEwan @dalance, just following up on this
I think this PR seems to be ready, so I'll merge it.