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

Completion response says "isIncomplete": true even when the total number of completion items is two #7649

Closed
bstaletic opened this issue Feb 12, 2021 · 6 comments

Comments

@bstaletic
Copy link
Contributor

Yesterday (so 11. 02. 2021.) I updated rust-analyzer to the latest version available in rustup. This is a part of the log that shows the completion request/response:

2021-02-11 14:00:01 - DEBUG - TX: Sending message: b'Content-Length: 211\r\n\r\n{"id":22,"jsonrpc":"2.0","method":"textDocument/completion","params":{"position":{"character":12,"line":13},"textDocument":{"uri":"file:///home/bstaletic/work/ycmd/ycmd/tests/rust/testdata/common/src/main.rs"}}}'
2021-02-11 14:00:01 - DEBUG - RX: Received message: b'{"jsonrpc":"2.0","id":22,"result":{"isIncomplete":true,"items":[{"label":"into","kind":2,"detail":"-> T","documentation":{"kind":"markdown","value":"Performs the conversion."},"deprecated":false,"filterText":"into","insertTextFormat":1,"textEdit":{"range":{"start":{"line":13,"character":12},"end":{"line":13,"character":12}},"newText":"into"},"additionalTextEdits":[]},{"label":"build_rocket","kind":2,"detail":"-> ()","documentation":{"kind":"markdown","value":"Do not try at home"},"deprecated":false,"filterText":"build_rocket","insertTextFormat":1,"textEdit":{"range":{"start":{"line":13,"character":12},"end":{"line":13,"character":12}},"newText":"build_rocket"},"additionalTextEdits":[]},{"label":"build_shuttle","kind":2,"detail":"-> ()","deprecated":false,"filterText":"build_shuttle","insertTextFormat":1,"textEdit":{"range":{"start":{"line":13,"character":12},"end":{"line":13,"character":12}},"newText":"build_shuttle"},"additionalTextEdits":[]}]}}'

The project used for testing: https://github.com/ycm-core/ycmd/tree/master/ycmd/tests/rust/testdata/common
The specific file open: https://github.com/ycm-core/ycmd/blob/master/ycmd/tests/rust/testdata/common/src/main.rs#L14
The expected completions: https://github.com/ycm-core/ycmd/blob/master/ycmd/tests/rust/testdata/common/src/test.rs#L7-L8

The new rust-analyzer does work, but if it is always returning true for isIncomplete, it circumvents ycmd's cache and impacts performance.

On the bright side, this exposed a bug in ycmd.

@flodiebold
Copy link
Member

flodiebold commented Feb 12, 2021

We set isIncomplete to true unconditionally, because with the fuzzy autoimport completions the set may change with each letter. In some situations that's overly careful of course, but since the goal is for our completions to be fast enough to be requested on every keystroke (#7542) we'll probably not put effort into implementing more complicated logic there.

One possible consideration would be to only set it to true if import autocompletions are actually enabled, though.

@bstaletic
Copy link
Contributor Author

I'm concerned that 68ms is a performance regression for our users. Hitting the ycmd cache, right now, roundtrips in <6ms.

@lnicola
Copy link
Member

lnicola commented Feb 12, 2021

Those 68ms are on a cold RA cache. A second invocation should be fast.

@bstaletic
Copy link
Contributor Author

Alright, thanks for correcting me. That doesn't sound too bad.

@flodiebold
Copy link
Member

I think we can close this?

@bstaletic
Copy link
Contributor Author

Sure! I should have closed it yesterday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants