completion returns wrong and according to the spec invalid results

max397574 commented 3 months ago

How are you using the lua-language-server?

NeoVim

Which OS are you using?

MacOS

What is the issue affecting?

Completion

Expected Behaviour

Get back a completion item which does nothing which is explicitly forbidden in the spec and actually is correct

Actual Behaviour

Returns a completion item with a textEdit which makes no sense and additionalTextEdits which wouldn't even be allowed by the spec

Reproduction steps

I completed with the following context (notice that the cursor position uses different based indexing because neovim being like it is)

local entry_context = {
    bufnr = 1,
    cursor = { col = 35, row = 1 },
    line = "    completion_item.textEdit.range.",
    line_before_cursor = "    completion_item.textEdit.range.",
    previous = nil,
    reason = 1,
}

I get back an entry like this

local completion_item = {
    additionalTextEdits = {
        {
            newText = "",
            range = {
                ["end"] = { character = 35, line = 69 },
                start = { character = 34, line = 69 },
            },
        },
    },
    data = { id = 119, uri = "not relevant" },
    detail = "lsp.Position",
    documentation = { kind = "markdown", value = "not relevant" },
    insertTextFormat = 2,
    kind = 5,
    label = '"end"',
    sortText = "0001",
    textEdit = {
        newText = '["end"]',
        range = {
            ["end"] = { character = 35, line = 69 },
            start = { character = 4, line = 71 },
        },
    },
}

Additional Notes

I have no idea why the text edit would modify 3 different lines when just a single statement would have to be inserted. About additionalTextEdits from the lsp specification:

/**

An optional array of additional text edits that are applied when

selecting this completion. Edits must not overlap (including the same

insert position) with the main edit nor with themselves.

Additional text edits should be used to change text unrelated to the

current cursor position (for example adding an import statement at the

top of the file if the completion item will insert an unqualified type). */

In this case the additionalTextEdit overlaps with the edit and also doesn't what's it intended for (2nd paragraph)

Log File

No response

max397574 commented 2 months ago

@sumneko @CppCXY @tomlau10 could you help me with this or point me in the direction where I could fix this in the code myself?

max397574 commented 2 months ago

still happening with latests version is there anything I can do about this?

tomlau10 commented 2 months ago

I use vscode and I don't quite understand what this issue is talking about.

I also cannot follow your Reproduction steps 😅

What do you mean by context when giving the local entry_context example? Is it the file content?
What errors are you seeing? Or do you mean you are having incorrect completion result list when triggering auto completion?
Can you provide some code snippets where triggering auto completion will return incorrect result list? Similar to the descriptions in this issue: #2783
- the code snippet
- the cursor position when trigger auto completion
- the expected and actual result after triggering auto completion
Can you reproduce this on vscode? If yes then this issue is related to LuaLS (the language server itself), otherwise this maybe neovim specific 🤔

I'd be happy to help if you can elaborate more on this issue, though I can't guarantee I'll be able to find a solution 😄 @max397574

max397574 commented 2 months ago

I'm writing a completion engine for neovim and there I get completions when I have a type with an end field (which is the case for lsp ranges)

the entry context is the context in which completion was requested/received from the server I0m not seeing any errors. I just get a completion item which does really wrong things.

I'll try to create a minimal example

I could try this on vscode But perhaps it won't be reproducible because e..g some other completion engine I used in neovim didn't even return the entry because it was filtered out as a duplicate of simply .end

tomlau10 commented 2 months ago

I have a type with an end field

I just crafted a range type with the field start and end, and I experienced some strange issue when auto completing the end field. 😕 As you may know end is a lua keyword and we just can't write t.end, we have to write it as t["end"].

---@class range
---@field start integer
---@field end integer

---@type range
local r

r["|"] --< `|` is the cursor and I trigger completion there

the completion list does includes start and end ✅
- but when I select start, the applied result is incorrect => r["tar"] ❌
- also when I select end, the applied result is wrong 🙄 => r["n"] ❌
it seems to me that when completing literal inside "", the range index doesn't adjust for the surrounding double quotes "" => causing the applied results have the first and last character stripped.

However this is different from what you're encountering. And there maybe more bugs in LuaLS side of it's completion function. 😂

max397574 commented 2 months ago

minimal example

---@class Range
---@field start integer
---@field end integer

---@type Range
local x = { ["end"] = 3, start = 5 }
x.|
local y = 3

cursor is the | select the completion "end" (notice the quotes) it seems relevant that there is code after the line on which you complete

this leads to the invalid completion item as noted above (overlapping, multiple lines)

tomlau10 commented 2 months ago

I just tried with you example:

in vscode, only end and start is provided when i trigger completion
and you're right that "it seems relevant that there is code after the line on which you complete" because after I remove the local y = 3 and trigger completion again, another "end" is on the list

Maybe in the 1st case, there's the wrong completion item and vscode will filter it out silently.

With some digging in the code base, I believe the logic of handling completion request is here:

the callback when receiving a textDocument/completion request https://github.com/LuaLS/lua-language-server/blob/aaf16240f77bb75a3d8db89c2a99934c8b6022ee/script/provider/provider.lua#L609-L636
then it triggers core.completion which returns a result[] https://github.com/LuaLS/lua-language-server/blob/aaf16240f77bb75a3d8db89c2a99934c8b6022ee/script/core/completion/completion.lua#L2388-L2398
it triggers the tryCompletions() there and each step will push result into the results[]

That's all I know, maybe you can start debugging from there 🤔

max397574 commented 2 months ago

well but the language server does also return items in the case with the local y which are just filtered out by vscode then it seems. And it looks like they are only problematic in this case. Otherwise I also get the completion and it also is correct.

max397574 commented 2 months ago

I'll try to look into the code parts you sent

tomlau10 commented 2 months ago

I forgot to mention that, you can start LuaLS with --rpcjson=true argument, and LuaLS will logs the json data that received and sent in the log file.

the language server does also return items in the case with the local y

And yes I confirmed in the case with the local y, LuaLS also return the wrong completion item, as seen from the log. Below is the beautified json data:

{
  "id": 20,
  "jsonrpc": "2.0",
  "result": {
    "isIncomplete": false,
    "items": [
      {
        "additionalTextEdits": [
          {
            "newText": "",
            "range": {
              "end": {
                "character": 2,
                "line": 6
              },
              "start": {
                "character": 1,
                "line": 6
              }
            }
          }
        ],
        "data": {
          "id": 1,
          "uri": "file:///c%3A/Users/TomLau/test/test.lua"
        },
        "insertTextFormat": 2,
        "kind": 13,
        "label": "\"end\"",
        "sortText": "0001",
        "textEdit": {
          "newText": "[\"end\"]",
          "range": {
            "end": {
              "character": 2,
              "line": 6
            },
            "start": {
              "character": 0,
              "line": 8
            }
          }
        }
      },
      {
        "data": {
          "id": 2,
          "uri": "file:///c%3A/Users/TomLau/test/test.lua"
        },
        "insertTextFormat": 2,
        "kind": 5,
        "label": "end",
        "sortText": "0002"
      },
      {
        "data": {
          "id": 3,
          "uri": "file:///c%3A/Users/TomLau/test/test.lua"
        },
        "insertTextFormat": 2,
        "kind": 13,
        "label": "start",
        "sortText": "0003"
      }
    ]
  }
}

tomlau10 commented 2 months ago

For the case when a completion item's newText is a ["field"], I believe it is handled by checkFieldFromFieldToIndex() https://github.com/LuaLS/lua-language-server/blob/aaf16240f77bb75a3d8db89c2a99934c8b6022ee/script/core/completion/completion.lua#L449-L477

Maybe the offset calculation here is incorrect 🤔

tomlau10 commented 2 months ago

(Off topic)

Oh for the incorrect completion inside quoted string that I mentioned in this comment: https://github.com/LuaLS/lua-language-server/issues/2762#issuecomment-2282695433, I found that you have reported it in another issue: #2688.

Maybe during your debugging journey, you can fix both of them 😄

max397574 commented 1 month ago

@sumneko I tried to debug this myself but couldn't figure it out. Could you take a look at this?

tomlau10 commented 1 month ago

I don't think author have time to debug this, given that seems no others are encountering this issue, so this have a very low low priority ☹️ .

As many editors will just ignore invalid entries on client side, maybe in the meantime you can filter invalid result in your client side as well? Another approach that I can think of is to filter invalid result on LuaLS side before it returns the result items, but this just hides the problem ... 😇

So seems the best way is you tackle it in your client side, and leave this issue open in LuaLS repo. Maybe in someday it will get solved finally, by author or by others, (or maybe not...) 😂 @max397574

CppCXY commented 1 month ago

Some parts of the specification are difficult to understand. We understood some things through translation software, and the actual implementation of the functionality was simply tested by continuously checking whether VSCode worked correctly at the time.

tomlau10 commented 1 month ago

I have spent some time to debug this issue and I am 100% sure the related logic resides in checkFieldFromFieldToIndex(). Here are some findings 🤔

Purpose of `additionalTextEdits`

The additionalTextEdits that created by the above logic, is trying to change the .field syntax into ["field"] syntax Consider the following case:
```
---@class Range
---@field start integer
---@field end integer

---@type Range
local x = { ["end"] = 3, start = 5 }
x.    e| --< trigger completion here
```
- in vscode both end and "end" are shown, after choosing "end", the line becomes
```
x    ["end"]
```
- expected ✅ the dot is removed to adjust for the ["field"] syntax, while keeping the original spaces in between
Now consider the next case, where the starting word is on a new line
```
x.
e| --< trigger completion here
```
- also work as expected ✅ both end and "end" are shown, and if choosing the later one, it becomes
```
x
["end"]
```

Problem when triggering completion in middle

On same line
```
x.|    e --< trigger completion at `|`
```
- on vscode three items are shown end / "end" / start, but the "end" entry is actually somewhat invalid ❌ : it's start offset is larger then the finish offset
On next line
```
x.|  --< trigger completion at `|`
e
```
- now vscode will only show end and start, because the "end" entry is totally invalid ❌: the start offset is on the next line, while the finish offset is on the original line

The unknown logic of finding next word offset

This implies that the bug is triggered when completing immediately after a dot, and there are words following the cursor. In this case (completing immediately after a dot) the word value in checkFieldFromFieldToIndex() is empty string. Therefore the following extra logic is being executed: https://github.com/LuaLS/lua-language-server/blob/c9d819305b6f88b53de294d54c820681518430c8/script/core/completion/completion.lua#L461-L467

I do not understand what this if branch trying to achieve currently 😇
state.lua is the current file's lua text, and I assume startOffset is the cursor's position
%S means any non space character (including \n), ()%S together will capture nothing and just return the matched position
- => so that's the next word character position
but how is the next word position related to the current text edit? Just a few lines below, the wordStartPos is assigned to textEdit.start, without changing the textEdit.finish => it will always cause this textEdit to have an invalid start / finish position (start > finish) https://github.com/LuaLS/lua-language-server/blob/c9d819305b6f88b53de294d54c820681518430c8/script/core/completion/completion.lua#L471-L477
If I just remove this extra if branch (i.e. only keeping local wordStartOffset = offset - #word) then seems everything will work as expected 🤔

My proposed solution

Remove this entire if case in the following logic: https://github.com/LuaLS/lua-language-server/blob/c9d819305b6f88b53de294d54c820681518430c8/script/core/completion/completion.lua#L460-L470

Change it to just the following
```
local wordStartOffset = offset - #word
```

max397574 commented 1 month ago

I opened a pr with this change here https://github.com/LuaLS/lua-language-server/pull/2844

LuaLS / lua-language-server