search

suoto / hdl_checker

Repurposing existing HDL tools to help writing better code
GNU General Public License v3.0
192 stars 22 forks source link

Empty Range for Diagnostics #92

Closed SethGower closed 3 years ago

SethGower commented 3 years ago

Issue details

It seems that the Server is sending empty ranges for diagnostics. This wasn't an issue for me until recently, when CoC updated to stop compensating for this. See my issue I created on CoC. Below is the output of CocCommand workspace.showOutput for the server, in a small test file. It shows that the range is 0:0, which breaks the spec since the Range is 0 based with an end position that is exclusive. So 0:0 isn't the 0th character, it is empty. Test file:

f
entity a is
end entity a;
architecture Behavioral of a is
begin
end Behavioral;

Output from the server to the client:

{
    "uri": "file:///home/sgower/test/test2.vhd",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 0,
                    "character": 0
                },
                "end": {
                    "line": 0,
                    "character": 0
                }
            },
            "message": "near \"f\": syntax error",
            "severity": 1,
            "code": null,
            "source": "HDL Checker/msim",
            "relatedInformation": null
        }
    ]
}

I believe I know how to fix this, I am just making an issue to make sure this is documented. I will work on it tonight or tomorrow night probably, and make a PR. I want to try and help contribute to this awesome project any way I can.

EDIT: Doing some testing in VSCode it seems that this bug doesn't show up, since it seems that VSCode might compensate for it, by just highlighting either the space on which the range , or by highlighting the word on which it is (the start of the word).

Output of the language server in VSCode:

[Trace - 11:26:44 AM] Sending notification 'textDocument/didOpen'.
Params: {
    "textDocument": {
        "uri": "file:///home/sgower/test/test2.vhd",
        "languageId": "vhdl",
        "version": 16,
        "text": "bad_name\nentity a is\nend entity a;\narchitecture Behavioral of a is\nbegin\nend Behavioral;\n"
    }
}

[Trace - 11:26:44 AM] Received notification 'textDocument/publishDiagnostics'.
Params: {
    "uri": "file:///home/sgower/test/test2.vhd",
    "diagnostics": [
        {
            "range": {
                "start": {
                    "line": 0,
                    "character": 0
                },
                "end": {
                    "line": 0,
                    "character": 0
                }
            },
            "message": "near \"bad_name\": syntax error",
            "severity": 1,
            "code": null,
            "source": "HDL Checker/msim",
            "relatedInformation": null
        }
    ]
}

image

SethGower commented 3 years ago

I have made a bit of progress. Got the static checker working properly with the ranges. Will be working on seeing if I can get the builders to do it appropriately.

https://github.com/SethGower/hdl_checker/tree/range-fixes

suoto commented 3 years ago

Hi I think the best solution is to just send end column = start column + 1.

ModelSim, GHDL and Vivado have no message where they return both start and end columns and I could not think of any way to guess the end column reliably without slowing down the entire server.

I saw you're using the text in quotes to do that, but a quick test illustrates what I mean. This code

entity some_entity is
end some_entity_name;

Produces the message

Line 1, column 21: Labels do not match: 'some_entity' and 'some_entity_name'.

So if you take column 21 (it's in the end) and use the length of some_entity to infer where the range starts, you'd get column 10 and the editor would show image which is also not correct.

All things considered, the error text showing up next to the line that's causing it is good enough -- adding better column ranges is only marginally better.

SethGower commented 3 years ago

Ok, that makes sense. What builder were you using for that example? I tried using GHDL (don't have vivado installed, and msim doesn't provide column numbers). GHDL doesn't give the column of the end of the token, but the beginning. 

test1.vhd:2:5:error: misspelling, "some_entity" expected
ghdl:error: compilation error

image

Output of ghdl --version

GHDL 1.0.0 (tarball) [Dunoon edition]
 Compiled with GNAT Version: 11.1.0
 llvm code generator
Written by Tristan Gingold.

Copyright (C) 2003 - 2021 Tristan Gingold.
GHDL is free software, covered by the GNU General Public License.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
suoto commented 3 years ago

I used modelsim for that example.

The error message parsing won't really care where the column is, the only difference is that using ghdl would result in an error in column 5 while modelsim would mark column 21, which I find acceptable to be honest.

You can think of a hierarchy of messages:

That being said, there's some static checks (for example unused signals, ports, etc) which are parsed in Python and in that case there can be a real range rather than a single character.

(hopefully I'm not making things more confusing!)

SethGower commented 3 years ago

That makes sense. I feel like it can be done so that it won't cause errors with the other builders. Like if we know the behavior from modelsim (which column it says) it can be modified in the msim.py so that it doesn't affect anything else.

Now that you mention the static checks, I have that working and I believe that should be appropriate ranges. Currently only have a bug that it doesn't calculate them properly when more than one signal is declared on one line, like

signal a,b,c : std_logic;

I believe it's an issue with the regex, since it gives the range as being the start column where the name is, and then the length of the whole block of signal names + start as the end. So the diagnostic for that example would extend 5 characters after c. It's weird, but I am sure I can figure it out. I was on vacation last week, so I didn't do any work on it.

My big problem was the columns provided by the server break the LSP spec. So if you want to just do the first column as the location of an error, then the range should be 0:1.