dalance / svlint

SystemVerilog linter
MIT License
315 stars 33 forks source link

Integrate with Vim's linter plugin ALE #67

Closed poetaman closed 2 years ago

poetaman commented 3 years ago

Vim has a popular plugin ALE that works with various languages. Add a file like other linters to integrate svlint in Vim: https://github.com/dense-analysis/ale/tree/master/ale_linters/verilog, and perhaps also enable it for System Verilog.

dalance commented 3 years ago

Thank you for your suggestion. I think svls, which is a language server of svlint, can be integrated more easily than svlint. The following implementation may be helpful.

https://github.com/dense-analysis/ale/pull/2804

I don't have time to do it. So any contribution is welcome.

poetaman commented 3 years ago

@dalance svls is a language server, right? ALE has an option to disable language server feature, and just use its lint feature. That way, I believe users have a choice to use other language servers if they like.

What one would need is understanding the syntax of output of svlint, and parse it in a file like this: https://github.com/dense-analysis/ale/blob/master/ale_linters/verilog/iverilog.vim

I see all lines on an example file I have start with Fail: when I run svlint -1 filename.v.

What's categories does svlint have? Error/Warning/Info? etc. It would be nice to have this information in the output messages, one option could be like Fail(E) for Error, Fail(W) for warning, etc. Whats the source of your style-guide? There are two I came across online: https://google.github.io/verible/lint.html, and https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md.

Also, does svlint detect circular logic? Thanks!

dalance commented 3 years ago

ALE seems to have LSP support as linter: https://github.com/dense-analysis/ale/blob/master/autoload/ale/handlers/hdl_checker.vim I think svls integration may be easier than svlint integration because there is no need to parse output of svlint. Of course, svlint integration is no problem.

What's categories does svlint have? Error/Warning/Info? etc. It would be nice to have this information in the output messages, one option could be like Fail(E) for Error, Fail(W) for warning, etc. Whats the source of your style-guide? There are two I came across online: https://google.github.io/verible/lint.html, and https://github.com/lowRISC/style-guides/blob/master/VerilogCodingStyle.md.

Also, does svlint detect circular logic? Thanks!

There is no category. I think all lint errors should be fixed, and if there are "Warning/Info" level rules, they should be disabled because users can't take care of the many "Warning/Info" messages at all times. It seems to be a best practices of modern programing language. The rules are based on the internal rules of my company, and some user requests have been added. svlint is a syntactic linter, so semantic error like "circular logic" can't be detected.

poetaman commented 3 years ago

@dalance A lot of users don't use ALE for LSP, but just for lint (given it already supports so many linters out of the box & gives a global option to disable LSP). So if I were to code, I would rather parse the output of svlint.

Not true. Most good linters I use differentiate between Error and Warning. Some of them just report first error, so user can directly jump to that & incrementally keep fixing them. The danger of not doing so is this: Let's say if there is some real error (like unmatched bracket), not keeping it fixed can (depending on language) generate spurious linter warnings where there is no problem. So it is always a good practice for user to forget about rest of the Warnings/Errors, and focus on the first error encountered.

If you can add E/W characters to appropriate lines returned by svlint -1 filename.v, parsing it is not that big a deal. Also, a bonus would be to add an argument that when passed will just return first Error (if there are errors), or will return list of Warnings. That way I need not filter one error that occurs after many warnings.

It would be nice if you can publish all the warning/error rules that your linter checks for like google's style guide does for so many languages.

dalance commented 3 years ago

Do you say about syntax error not lint error? If there is a syntax error like unmatched bracket, svlint will stop to parse the file and show the first syntax error only. If there is no syntax error, all lint error are shown because they are real lint error. If you want to distinguish the error type, the first word "Error"/"Fail" can be used like below:

$ svlint -1 test.sv
Error   test.sv:9:1     endmodule       hint: parse error
$ svlint -1 test.sv
Fail    test.sv:1:1     module A;       hint: `` `default_nettype none`` should be at the top of source code
poetaman commented 3 years ago

@dalance Ok, cool! Vim's ALE linter distinguishes between Error & Warning. So perhaps I can parse syntax error as Error, and lint error as Warning... I also found the place where you have listed all rules on the GitHub page. Thanks.

DaveMcEwan commented 2 years ago

@reportaman Do you have any updates on this? Is this still an issue?

DaveMcEwan commented 2 years ago

@dalance Given that svls supports by several Vim and Neovim LSP clients, I think this can be closed.

dalance commented 2 years ago

I think so too.