imc-trading / svlangserver

MIT License
95 stars 13 forks source link

File column parsed as part of diagnostics message #8

Closed chrbirks closed 2 years ago

chrbirks commented 2 years ago

Hi I see an issue in Emacs with lsp-mode where the textDocument/publishDiagnostics message starts with the column number (17) of the warning. The line number (75) is parsed fine. As far as I can see this applies to all warnings and errors published by svlangserver and I have seen it for some time.

lsp-mode trace log:

[Trace - 10:18:37 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
  "uri": "file:///home/cbs/etc/example_code/systemverilog/crc32_8.sv",
  "diagnostics": [
    {
      "severity": 2,
      "range": {
        "start": {
          "line": 75,
          "character": 0
        },
        "end": {
          "line": 75,
          "character": 1.7976931348623157e+308
        }
      },
      "message": "17 Operator ASSIGNDLY expects 8 bits on the Assign RHS, but Assign RHS's CONST '7'h7f' generates 7 bits.",
      "code": "verilator",
      "source": "verilator"
    }
  ]
}

Output of Verilator directly:

$ verilator --lint-only -Wall crc32_8.sv
%Warning-WIDTH: crc32_8.sv:76:17: Operator ASSIGNDLY expects 8 bits on the Assign RHS, but Assign RHS's CONST '7'h7f' generates 7 bits.
                                : ... In instance crc32_8
   76 |         crc     <= 7'hFF;
      |                 ^~
                ... For warning description see https://verilator.org/warn/WIDTH?v=4.212
                ... Use "/* verilator lint_off WIDTH */" and lint_on around source to disable this message.
%Error: Exiting due to 1 warning(s)

Other info

$ svlangserver -v
Version is 0.3.4

$ verilator --version
Verilator 4.212 2021-09-01 rev UNKNOWN.REV

$ emacs --version
GNU Emacs 29.0.50
Copyright (C) 2021 Free Software Foundation, Inc.
GNU Emacs comes with ABSOLUTELY NO WARRANTY.
You may redistribute copies of GNU Emacs
under the terms of the GNU General Public License.
For more information about these matters, see the file named COPYING.

lsp-mode: 20210927.714
chrbirks commented 2 years ago

I just realized that older versions of Verilator don't report the column as part of the error message so it's not actually an error in svlangserver. I don't know how complicated it would be to check which Verilator version is installed from inside the application but another fix could be to check how many elements in terms is returned from the _splitTerms function and use that number to determine if that version of Verilator returns a column.

$ verilator --version
Verilator 3.916 2017-11-25 rev verilator_3_914-65-g0478dbd

$ verilator --lint-only -Wall crc32_8.sv
%Error: crc32_8.sv:69: Duplicate declaration of signal: plus_one
%Error: crc32_8.sv:68: ... Location of original declaration
%Error: Exiting due to 1 error(s)
%Error: Command Failed /usr/bin/verilator_bin --lint-only -Wall crc32_8.sv
kkanhere commented 2 years ago

Thanks for reporting this! I will investigate both the approaches you have suggested, unless you want to take a stab at it. The version approach might be more difficult than it looks because user provided verilator command might have more args and I don't know how verilator would work in those cases if --version is added.

chrbirks commented 2 years ago

I can see if I can make a pull request for a fix this weekend. I'm no typescript expert but it shouldn't be very complicated if we can go for the solution of checking the size of the terms array to decide if column is present.

kkanhere commented 2 years ago

Fixed in release 0.3.5