Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistent filterText and textEdit are returned, violating the LSP spec #1348

Closed
yyoncho opened this issue Feb 14, 2020 · 12 comments
Closed

Comments

@yyoncho
Copy link
Contributor

yyoncho commented Feb 14, 2020

Completing the following line:

        java.util.concurrent.atomic.

In this case the server returns:

[Trace - 08:10:19 AM] Received response 'textDocument/completion - (166)' in 37ms.
Result: {
  "items": [
    {
      "data": {
        "uri": "file:///home/kyoncho/Sources/lsp/dap-mode/features/fixtures/test-project/src/main/java/temp/App.java",
        "rid": "20",
        "pid": "0",
        "decl_signature": "Ljava.util.concurrent.atomic.AtomicBoolean;"
      },
      "textEdit": {
        "newText": "java.util.concurrent.atomic.AtomicBoolean",
        "range": {
          "end": {
            "character": 36,
            "line": 9
          },
          "start": {
            "character": 8,
            "line": 9
          }
        }
      },
      "insertTextFormat": 2,
      "insertText": "AtomicBoolean",
      "filterText": "AtomicBoolean",
      "sortText": "999999180",
      "detail": "java.util.concurrent.atomic.AtomicBoolean",
      "kind": 7,
      "label": "AtomicBoolean - java.util.concurrent.atomic"
    },
....

As per spec the clients are supposed to use the textEdit range to find out where the completion has started and then filter it against filterText. In this case, the completion of prefix is calculated as java.util.concurrent.atomic. while the filter text is AtomicBoolean.

@akaroml
Copy link
Contributor

akaroml commented Feb 15, 2020

The textEdit is for applying text changes when the completion item is selected. And the filterText is for the client to further filter the completion items as users keep on typing. I don't see those two related. @fbricon what do you think?

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 15, 2020

is for the client to further filter the completion items

Filter against what? Note that the servers may return whatever filterText they want - e. g. xml language server returns <tagName and the html language server returns tagName.

here it is eclipse lsp4e prefix calculation: https://git.eclipse.org/c/lsp4e/lsp4e.git/tree/org.eclipse.lsp4e/src/org/eclipse/lsp4e/operations/completion/LSIncompleteCompletionProposal.java#n378

For the record, when you do import java.util.concurent.| JDT LS returns the proper textEdit and filter text.

@akaroml
Copy link
Contributor

akaroml commented Feb 16, 2020

@Eskibear could you take a look here?

@Eskibear
Copy link
Contributor

I just tried:

  1. import a class,
  2. in function body, type full qualified name of the class.

According to the log, jdtls returns the same response in both cases. However, in Case 1 target class was shown in the completion list, while in Case 2 only sub packages were shown.

E.g. I wanted to refer java.util.concurrent.AbstractExecutorService. See below:

Case 1

Screen Shot 2020-02-17 at 10 47 33 AM

{
  "label": "AbstractExecutorService - java.util.concurrent",
  "kind": 6,
  "detail": "java.util.concurrent.AbstractExecutorService",
  "sortText": "999999212",
  "filterText": "AbstractExecutorService", // <-- same filter text
  "insertText": "AbstractExecutorService",
  "insertTextFormat": 2,
  "textEdit": {
    "range": {
      "start": {
        "line": 2,
        "character": 28
      },
      "end": {
        "line": 2,
        "character": 28
      }
    },
    "newText": "AbstractExecutorService;"// <-- different new text
  },
  ...
},

Case 2

Screen Shot 2020-02-17 at 10 46 58 AM

{
  "label": "AbstractExecutorService - java.util.concurrent",
  "kind": 6,
  "detail": "java.util.concurrent.AbstractExecutorService",
  "sortText": "999999180",
  "filterText": "AbstractExecutorService", // <-- same filter text
  "insertText": "AbstractExecutorService",
  "insertTextFormat": 2,
  "textEdit": {
    "range": {
      "start": {
        "line": 9,
        "character": 8
      },
      "end": {
        "line": 9,
        "character": 29
      }
    },
    "newText": "java.util.concurrent.AbstractExecutorService" // <-- different new text
  },
  ...
},

I guess it's because in Case 2, instead of an empty string, vscode filtered the items using the full line java.util.concurrent.. I think @jrieken might know what's happening behind. Is that a bug of vscode?

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 17, 2020

I guess it's because in Case 2, instead of an empty string, vscode filtered the items using the full line java.util.concurrent.. I think @jrieken might know what's happening behind. Is that a bug of vscode?

That is because JDT LS returns incorrect text range in Case 2 - the clients use Completion.textEdit.range to calculate what to filter against.

@Eskibear
Copy link
Contributor

You are right. So I can see two possible solutions for this case:

  • a) change filterText to the full qualified name.
  • b) change textEdit.range and corresponding textEdit.newText.

And I think b) makes more sense.

@Eskibear
Copy link
Contributor

Well, I just had a glance, the range was directly converted from CompletionProposal, which is calculated by JDT core. I'm afraid that a fix in upstream is required.

@jrieken
Copy link

jrieken commented Feb 17, 2020

the clients use Completion.textEdit.range to calculate what to filter against.

That's correct - the range serves as the range that is replaced upon insertion and selects the prefix that's used for filtering and sorting.

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 18, 2020

@Eskibear

Well, I just had a glance, the range was directly converted from CompletionProposal, which is calculated by JDT core. I'm afraid that a fix in upstream is required.

I guess option a) is still on the table? IMO it won't make any difference for the clients unless they are putting the completion dialog under the range.start position.

@Eskibear
Copy link
Contributor

Eskibear commented Feb 26, 2020

I was trying to update filterText to match textEdit for this case. This fix is easy and the item does show up in the completion list, but it just looks a little bit wired in vscode.

Screen Shot 2020-02-26 at 7 59 48 PM

The highlighted part of label doesn't make much sense, because now the prefix is "java.util.concurrent.atomic.AtomicB". But that's a client issue, about how vscode highlights the prefix when it is different from label. fyi @jrieken

Back to this issue, I think the perfect solution would be, to use a textEdit with more reasonable range and newText, which is far more complicated. To some extent, it does unblock the use case... not perfect though.

@yyoncho
Copy link
Contributor Author

yyoncho commented Feb 26, 2020

@Eskibear what about having the filterText patch merged and then when upstream is fixed it will automatically resolve the highlighting issue?

how vscode highlights the prefix when filterText is different from label. fyi @jrieken

We (at emacs side) do it the same way. IMO the issue is not that the filterText is different from the label but the fact that the prefix is different(the prefix is calculated based on the textEdit).

@Eskibear
Copy link
Contributor

@yyoncho you're right. I've updated the original statement to avoid confusion.

@fbricon fbricon added this to the Early March 2020 milestone Feb 28, 2020
@fbricon fbricon closed this as completed Feb 28, 2020
@fbricon fbricon changed the title JDT ls does not follow the spec and return the inconsistent filterText and textEdit Inconsistent filterText and textEdit are returned, violating the LSP spec Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants