Bug: Textrule header_copyright only checks the first file passed

[skjdbg]

When passing multiple files at once to svlint, header_copyright ignores all files after the first.
For instance with:
file1.sv

module foo;
endmodule

and file2.sv

module bar;
endmodule

Calling svlint -v path/to/file1.sv path/to/file2.sv returns:

Fail: header_copyright
   --> path/to/file1.sv:1:1
  |
1 | module foo;
  | ^^^^^^^^^^^ hint  : Copyright notice must be present on line 1.
  |             reason: Copyright notices are required for legal purposes.

Info: pass 'path/to/file2.sv'

I would expect both files to fail instead.

Looking at the implementation of header_copyright, I see that linenum is never reset to 1. which seems to be the cause of the bug.
This brings me to the next issue: There seems to be no way for a TextRule (or a SyntaxRule for all that matters) to detect the end of a file, which could be used to trigger the reset of linenum.

[skjdbg]

Note that my example assumes that a .svlint.toml file enabling header_copyright was found.

[DaveMcEwan]

That's a bit of an oversight from me, sorry!
I had originally thought of passing a line number through textrules_check(), but went for the local variable linenum instead because I was concerned about error-prone redundancy between that and beg (byte offset of first character).

Option1: Change the API only for TextRule to include a line number, i.e. in linter.rs, fn textrule_check(..., linenum: usize, ...) and in src/textrules/*.rs fn check(..., linenum: usize, ...).
Pros: 1) Simple; 2) Existing API is very new so not too much disturbance;
Cons: 1) Doesn't help syntaxrules; 2) Any API disturbance is annoying;

Option2: Change the API for both TextRule and SyntaxRule to somehow include a signal for EOF, then let rules deal with it individually.
Pros: 1) Helps both textrules and syntaxrules; 2) No change for any plugins which rely on current behaviour (none are known to me);
Cons: 1) Perhaps less intuitive for new contributors; 2) It's an easy gotcha (not dealing with EOF) when writing rules;

Option3: Change main.rs to reinitialize linter for every file.
Pros: 1) Simplest coding change; 2) No change to APIs or svlint-plugin-sample or svls; 3) Avoids gotcha and provides rule with a clean start for each file;
Cons: 1) Extra latency to reinitialize linter for each file; 2) May break (currently hypothetical) plugins that rely on non-resetting state machines;

Can you think of some more options? Personally, I'm leaning towards Option3.

[skjdbg]

Option4 could be to give the name of the file being parsed to the linter.
By detecting a change in the name, the rule could reset the line number on its own.
Pros and cons would depend on how it's implemented (as in Option1 or Option2).
An additional pro is that it would help with #252.

Maybe Option2 could be implemented by passing an Option<str> to TextRule and an Option<NodeEvent> to SyntaxRule ? Or Maybe directly create an enum for each of them to provision for other "signals" we could pass ? For instance a variant ChangeFile(&Path) which would give the path of the next file.

I do have an issue with Option 3 because I wrote a (syntax) rule that relies on the fact that it is not reset between file.
I'm trying to build an inheritance tree of classes to know whether a class somehow extends a UVM class (not sure if that's the best way to do it, but it seems to be working as of now) which wouldn't work if it is reset.

I would also point that the filename, line number, and EOF "signal" are not exclusive features - even if reducing how much we break backwards compatibility is important. While the same result can be achieved from simply passing the filename and letting each rule deduce the EOF or the line number, those seem like they could become common among future rules. Giving all those informations would prevent every rule to reimplement the line number tracking and such.

Lastly, for Option1, I would pass a structure (maybe named ParsingState) that would contain all such informations (line number, filename, path, and so on) since extending this structure would no longer break backwards compatibility.

I prefer Option1, especially if we pass a structure instead.

[DaveMcEwan]

I do have an issue with Option 3 because I wrote a (syntax) rule that relies on the fact that it is not reset between file.
I'm trying to build an inheritance tree of classes to know whether a class somehow extends a UVM class (not sure if that's the best way to do it, but it seems to be working as of now) which wouldn't work if it is reset.

Interesting :) An alternative could be to only look for class foo extends bar and apply a regex to foo only when bar matches another regex. That way you don't need to understand the inheritance tree. For example using a rule like the re_forbidden_ ones, the config would look like:

option.re_forbidden_class_extends = ".*"
option.re_forbidden_class_extends_match ="^uvm_"
syntaxrules.re_forbidden_class_extends = true

Pehaps I misunderstand the need for building the tree...

[DaveMcEwan]

I have assumed that plugins should be compatible with svls, where you can only see one file at a time.
Under that assumption, resetting the linter for each file is fine and I think that's what svls does, but maybe it's dependent on the editor/client.
AFAIK, there's currently no way to use plugin rules with svls, but this could be added as a configuration option.

One problem I see with Option 1/2/4 is that commands like svlint foo.sv foo.sv could give confusing output where one passes and the other fails. Naturally, a small command like that is obviously strange but in large projects using deeply nested filelists or symlinks...

Keeping state between files means that each rule is effectively a little independent compiler (like your class tree builder)! That is cool but I'm not sure if this is the right place.
I've been thinking about how to do "proper compiler" rules (called comprules and elabrules in #237) though I'm on my 4th or 5th attempt now :p - apparently it's quite a lot of work to do properly!

I think we need @dalance to give an opinion here.

1. Should rules allow/expect/require a clean state for each file?
2. Should plugins be usable with svls? (If so, I can happily open a PR later.)

[skjdbg]

Pehaps I misunderstand the need for building the tree...

Well if it was just what I'd call "1st order inheritance", there would be no issue. I do that to detect "nth order inheritance".
Since most of our classes are defined in a separate file, I wouldn't be able to check anything beyond 1st order if the linter was reset.
Obviously there is the solution of hard-coding the name of all class that I know extend uvm, but that would require a lot of maintaining. Having a naming convention for classes that extend uvm could also work (I believe that is what you expected in you first comment ?), but I don't believe we want to have to do that for the rule to work (classes that extend uvm have no particular naming convention at the moment).

As an example:
1st order would be

class X extends uvm;

2nd order would be

class Y extends uvm;
...
class X extends Y;

In both cases, I would want my rule to do something based on the knowledge that X extends uvm

I have assumed that plugins should be compatible with svls, where you can only see one file at a time.
Under that assumption, resetting the linter for each file is fine and I think that's what svls does, but maybe it's dependent on the editor/client.
AFAIK, there's currently no way to use plugin rules with svls, but this could be added as a configuration option.

If plugin rules can't be used with svls there is no reason for them to be compatible. That being said, I would be happy to use my plugins with svls =).
In that case and if svls cannot get rid of that one-file-at-a-time limitation - be cause it would take too long otherwise, I suppose ? - I think it could be a valid stance to say that svls has limitations because it is meant to be a fast, reactive linter and as such does not support the more complex rules, while manually calling svlint would be aimed at more in-depth linting.
However that is simply my opinion, and I would prefer not having any restriction.

One problem I see with Option 1/2/4 is that commands like svlint foo.sv foo.sv could give confusing output where one passes and the other fails. Naturally, a small command like that is obviously strange but in large projects using deeply nested filelists or symlinks...

I do agree that simply the path would not work in that case, but I don't see how the line number and the EOF signal would fail ?
Besides of everything is given, then a file change can be detected by a reset of line number, while rules could memorize files already parsed to prevent issues related to parsing the same file twice.

I've been thinking about how to do "proper compiler" rules (called comprules and elabrules in #237) though I'm on my 4th or 5th attempt now :p - apparently it's quite a lot of work to do properly!

I'm rooting for you =)

[DaveMcEwan]

I do that to detect "nth order inheritance".

Cool. That makes sense.

If plugin rules can't be used with svls there is no reason for them to be compatible.

Good point! I've just modified a PR for that along with the new API dalance/svls#216.
If that is merged, then you'll be able to use your other plugins with svls. Perhaps you could see if your plugins+editor setup works with your UVM class plugin? It looks like simple/intuitive behaviour for me with (Vim, vim-lsp, svls) when I open only one file vim foo.sv, but missing/repeated messages when opening multiple files vim -p foo.sv bar.sv... I suspect it depends on editor setup.

[DaveMcEwan]

I do agree that simply the path would not work in that case, but I don't see how the line number and the EOF signal would fail ?

I'm thinking of other rules with similar logic to your UVM class one like Only one top module allowed or Duplicate (package|module|class|program) name detected.

The path is already passed to syntaxrules in the Locate nodes of syntax_tree.
I don't think passing the path is good for syntaxrules because the preprocessor can be used (and is) in confusing ways for tool workarounds like:

`ifdef MODULE_NOT_PACKAGE
module mod_foo;
`else
package pkg_foo;
`endif

  localparam HELLO = 123;
  // ... lots more localparams

`ifdef MODULE_NOT_PACKAGE
endmodule
`else
endpackage
`endif

... and maybe processed twice with define and undef directives. Totally sane to spec a vague text processor into the LRM, right!

[DaveMcEwan]

I think we're converging on a choice between two solutions.

• Option1: Change the API for TextRule to pass each line as Option<&str>, and promise to call textrules_check() with None before line iterator.
  • https://github.com/DaveMcEwan/svlint/tree/bugfix257_Option1
  • Rules must chose if they retain or reset their state when line.is_none().
  • Enables more complex/indepth analysis.
  • May produce confusing output when user gives the same file multiple times.
  • May produce different output when user gives files in different order (and preprocessed code is equivalent).
  • Breaks the API again, but probably not too disruptive since v0.8.0 is very recent.
  • Requires matching changes to Use updated API for svlint v0.8.0 svlint-plugin-sample#7
  • Requires matching changes to Feature: Support svlint v0.8.0 with TextRule and SyntaxRule. svls#216
  • Preferred approach by @skjdbg.
• Option3: Re-initialize the linter for every file.
  • https://github.com/DaveMcEwan/svlint/tree/bugfix257_Option3
  • Simplest approach, easiest to reason about.
  • No changes to other svlint-plugin-sample or svls.
  • Performance penalty of reinitialising the linter.
  • Breaks compatibility with @skjdbg's plugin, and maybe more.

...Now I'm starting to prefer Option1 too.
@skjdbg If you're happy with my Option1 branch, I'll move it into a PR.

[skjdbg]

The path is already passed to syntaxrules in the Locate nodes of syntax_tree.

I hadn't ever realised that!

If that is merged, then you'll be able to use your other plugins with svls. Perhaps you could see if your plugins+editor setup works with your UVM class plugin? It looks like simple/intuitive behaviour for me with (Vim, vim-lsp, svls) when I open only one file vim foo.sv, but missing/repeated messages when opening multiple files vim -p foo.sv bar.sv... I suspect it depends on editor setup.

I won't be able to look into that before next week.

If you're happy with my Option1 branch, I'll move it into a PR.

I notice that in the implementation of textrules_check, if line is None, you ignore a textrule fail. How would you write a rule that expects something before the end of the file ? For instance checking that every file ends with a newline (c.f. this).

[DaveMcEwan]

No rush man!

I notice that in the implementation of textrules_check, if line is None, you ignore a textrule fail. How would you write a rule that expects something before the end of the file ? For instance checking that every file ends with a newline (c.f. this).

I went for a SOF signal instead of an EOF because I think it's a bit easier to think about writing rules. Using an EOF signal would mean we need to think about both the initialisation and the EOF, but using an SOF signal we only need to think about the initialisation.

How I wish more people would think about POSIX lines haha! However, to avoid issues of different behaviour between Windows and everything else, I purposefully stripped the \r and \n to hide the line endings. It's also a sad fact that lots of people now use VSCode et al which don't finish files with a newline. So I'd say that not being able to check newlines is a usability feature of the API.

[skjdbg]

I purposefully stripped the \r and \n to hide the line endings.

Oh, right, I was confused! When there is a newline at the end of a file, text editors effectively show another line, so I was expecting to see an empty string before EOF. Indeed, besides that I don't see a particular need for reporting errors on SOF/EOF.

It's also a sad fact that lots of people now use VSCode et al which don't finish files with a newline.

It's at least not true of VSCode! I configured mine to remove trailing whitespaces and add a newline at the end of the file whenever I save the file. Although it should probaly be enabled by default...

I think you could go ahead with you PR. =)

[dalance]

I caught up on the discussion. I agree the Option1 is better.
The meaning of None which used as SOF signal may be bit confusing.
How about using enum like enum TextRuleEvent { StartOfFile, Line(&str) }?

[DaveMcEwan]

How about using enum like enum TextRuleEvent { StartOfFile, Line(&str) }?

That sounds less confusing :) I'll make the changes now.

[skjdbg]

If that is merged, then you'll be able to use your other plugins with svls. Perhaps you could see if your plugins+editor setup works with your UVM class plugin? It looks like simple/intuitive behaviour for me with (Vim, vim-lsp, svls) when I open only one file vim foo.sv, but missing/repeated messages when opening multiple files vim -p foo.sv bar.sv... I suspect it depends on editor setup.

I tried that out on VSCode on windows and I have a couple issues:

• Giving a Windows style path to the plugin doesn't work (fails parsing in backend.rs in generate_config ). Although replacing all \ with / works, even when giving the disk letter (C:/path/to/plugin.dll works but C:\path\to\plugin.dll doesn't)
• Simply pulling Feature: Support svlint v0.8.0 with TextRule and SyntaxRule. svls#216 and trying to use it doesn't give errors, but my plugins don't seem to be run at all (no linting errors are shown) while replacing the svlint dependency with my local version works. What troubles me is that the only difference I could find is a grand total of 4 debug print I added in main.rs.
• I also saw that my UVM plugin didn't thrown an error when it should have, but I didn't have time to look into that any deeper.

I'll try to go deeper tomorrow!

[DaveMcEwan]

• Simply pulling Feature: Support svlint v0.8.0 with TextRule and SyntaxRule. svls#216 and trying to use it doesn't give errors, but my plugins don't seem to be run at all (no linting errors are shown) while replacing the svlint dependency with my local version works. What troubles me is that the only difference I could find is a grand total of 4 debug print I added in main.rs.

That is strange. I've been testing with svlint-plugin-sample and see all the expected messages about initial, disable fork, and XXX. Maybe you need to update Cargo.toml?

[DaveMcEwan]

• Giving a Windows style path to the plugin doesn't work (fails parsing in backend.rs in generate_config ). Although replacing all \ with / works, even when giving the disk letter (C:/path/to/plugin.dll works but C:\path\to\plugin.dll doesn't)

Interesting edge case. If it's failing to parse .svls.toml, the same issue will likely apply to include_paths.
Have you used ' (single quote) to delimit the paths in Literal String, as opposed to " (double quote) which delimits TOML Basic strings?

As long as using / works I'd say that's a "good enough" workaround for now.

[skjdbg]

That is strange. I've been testing with svlint-plugin-sample and see all the expected messages about initial, disable fork, and XXX. Maybe you need to update Cargo.toml?

Ok so I restarted from scratch, and it seems I had broken something while dealing with the windows path issue. It seems to work out of the box.

Interesting edge case. If it's failing to parse .svls.toml, the same issue will likely apply to include_paths.
Have you used ' (single quote) to delimit the paths in Literal String, as opposed to " (double quote) which delimits TOML Basic strings?

I tested using single quotes and doubling all the \ in the path, and both solve the issue. I guess it's just a problem of escaping characters.

I also saw that my UVM plugin didn't thrown an error when it should have, but I didn't have time to look into that any deeper.

What happened was that my textrules worked fine, but a parsing error occurred before the syntaxrules could run. It would be useful if svls gave an error message in those cases. Arguably, svlint already knows what/where the error is so that could be shown in the editor as any other error ? (for reference the error was an undefined macro)

Finally, it seems everytime svls reruns the linter on the file (upon modification), errors originating from plugins seem to accumulate.
For instance: I have two rules. one textrule checks that checks the indentation, and my uvm rule. Both fail in one place each. If I fix the indentation, the indentation error disappears, but I have the UVM error twice. If I undo my previous correction, I end up with 3 times the UVM error, and 3 time the indentation error.
In the meantime the style_textwidth rule shows only 1 error without being duplicated.

[DaveMcEwan]

Ok so I restarted from scratch, and it seems I had broken something while dealing with the windows path issue. It seems to work out of the box.

Phew! :) Good to hear.

I tested using single quotes and doubling all the \ in the path, and both solve the issue. I guess it's just a problem of escaping characters.

Again, phew! Just a TOML thing (not a bug).

I have seen similar types of strange behaviour. When I open Vim+vim-lsp with (1 file, 1 buffer, 1 tab) everything works as expected, but when I open different numbers of files, buffers, and tabs then I sometimes get repeated messages. I haven't done a full range of experiments yet, but it looks quite dependent on editor setup.
Since you seem to have a better handle on this, would you maybe start an issue on svls? At this point, I think the svlint pieces look good.

[skjdbg]

svlint does seem to work fine!

I'm not sure opening an issue on svls is the right thing to do since the issue seems related to plugins which are added by dalance/svls#216 which isn't merged yet. Shouldn't I just reference this issue in the pull request ? Again, I'm not particularly used to how thing go on github... ^^'

[DaveMcEwan]

I'm not sure opening an issue on svls is the right thing to do since the issue seems related to plugins which are added by dalance/svls#216 which isn't merged yet.

Good point, let's wait to see what @dalance thinks and if the PRs get merged.
For me, you seem pretty easy to work with on GitHub :)

[DaveMcEwan]

@skjdbg With v0.9.0 released, I think this can be closed?

[skjdbg]

Sorry, I missed it !