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

Incomplete suggestion results cause re-triggering when typing w/o further context. #28400

Closed
yurriy opened this issue Jun 10, 2017 · 33 comments
Closed
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug suggest IntelliSense, Auto Complete verified Verification succeeded
Milestone

Comments

@yurriy
Copy link

yurriy commented Jun 10, 2017

extension cpptools is installed.
VS Code toggle quick suggestions popup, after I type "i++;", but it shouldn't.

  • VSCode Version: last
  • OS Version: Ubuntu 16.04

Steps to Reproduce:
With cpptools enabled, type "i++;" in any cpp file.

@ramya-rao-a
Copy link
Contributor

This issue was moved to microsoft/vscode-cpptools#818

@bobbrow
Copy link
Member

bobbrow commented Jun 12, 2017

Hi there,

We do not register for a textDocument/completion callback when semicolon is typed. We think this is a VS Code bug with the "editor.quickSuggestions" setting. It is popping up after semicolon is typed instead of waiting for a new line. I repro this on Windows too.

Please reopen and investigate.

@ramya-rao-a ramya-rao-a reopened this Jun 12, 2017
@ramya-rao-a ramya-rao-a added the suggest IntelliSense, Auto Complete label Jun 12, 2017
@jrieken jrieken added the info-needed Issue requires more information from poster label Jun 13, 2017
@jrieken
Copy link
Member

jrieken commented Jun 13, 2017

We think this is a VS Code bug with the "editor.quickSuggestions" setting.

@bobbrow What makes you think this? Quick-suggestions has nothing to do with trigger characters which will always go straight to your provider. Quick-suggestios is about suggesting while you type words. Please share your word definition, it might include ;

@bobbrow
Copy link
Member

bobbrow commented Jun 13, 2017

Our word definition? Do you mean the C++ grammar? I can go take a look and see if something changed there. What's strange is that we get a textDocument/completion event from VS Code only once the ; is typed. It's not a continuation of the previous word/symbol.

@jrieken
Copy link
Member

jrieken commented Jun 13, 2017

No, not the grammar but the wordPattern extensions either define via an API call or via the language-config file

@jrieken
Copy link
Member

jrieken commented Jun 13, 2017

Actually, this doesn't repro for me... @yurriy Please provide more details

jun-13-2017 18-26-41

@bobbrow
Copy link
Member

bobbrow commented Jun 13, 2017

We don't set a LanguageConfiguration anywhere in our extension. I honestly didn't know such a thing existed.

@sean-mcmanus
Copy link
Contributor

sean-mcmanus commented Jun 13, 2017

@jrieken Bob and I have both reproed the bug where the quickSuggestion popup occurs after the semicolon, but the repro appears to be random (timing related?) and we don't yet know a good way to force a repro. We don't think our extension is causing the issue.

@yurriy
Copy link
Author

yurriy commented Jun 14, 2017

This bug happened every time, but only in a particular directory, even if I cleaned all files within it (including .vscode), and created new 1.cpp. It didn't repro for copy of this dir. I deleted it and created the dir with the same name, and now it doesn't repro. Hence, it seems like this bug depends on time of creation of directory.

@jrieken
Copy link
Member

jrieken commented Jun 14, 2017

@yurriy what is the list of snippets you have? what is the list of suggestion you see? can you attach a screen shot?

@yurriy
Copy link
Author

yurriy commented Jun 14, 2017

Suggestions list: https://www.dropbox.com/s/xezqz2noymhgg80/Screenshot%20from%202017-06-10%2003-53-05.png?dl=0
I didn't change standard snippets

@jrieken
Copy link
Member

jrieken commented Jun 14, 2017

Weird, still not able to repro. Please provide more accurate steps

@yurriy
Copy link
Author

yurriy commented Jun 14, 2017

I succeeded to repro from the new folder, with .vscode/c_cpp_properties.json
after adding file 1.c
suggestions popup appears after typing "__;" (in 1.cpp)

@jrieken
Copy link
Member

jrieken commented Jun 14, 2017

Thanks. With those files I was able to reproduce. So, this is what happens when typing __;:

  1. When typing the first underscore we detect that a new word is started and trigger quick suggestions. The cpp-extension returns a completion list, but that list is marked as incomplete.

  2. Next, when the second underscore is being typed we re-evaluate things. Because we are already inside an active session we should just filter down the list of completions we have, unless the list is marked as incomplete. The semantics that are, 'Hey, I just gave some results and there will be more if you type more'. Typical scenarios are suggestions that are queried from a database (e.g. a package registry like Nuget or npm) which have to many results when querying for something like a.

  3. Because of the aforementioned semantics of incomplete we re-trigger completions (instead of filtering down) and because the result after the second underscore is also marked as incomplete we also re-trigger completions after typing the semi-colon etc. etc.

@bobbrow @sean-mcmanus I am unsure what you are trying to achieve by using incomplete but the current behaviour is incorrect/causes this behaviour.

@jrieken jrieken added *caused-by-extension Issue identified to be caused by an extension and removed info-needed Issue requires more information from poster labels Jun 14, 2017
@jrieken
Copy link
Member

jrieken commented Jun 14, 2017

I'd suggest to re-open microsoft/vscode-cpptools#818, the suggestion logic in VS Code does what the extension tells it to do.

@jrieken jrieken closed this as completed Jun 14, 2017
@bobbrow
Copy link
Member

bobbrow commented Jun 14, 2017

Thanks @yurriy for finding a repro and @jrieken taking a look. We return "incomplete" when our search yields more than 1000 results. The double underscore query in our database returns more that 1000 results so that's why we return "incomplete." We didn't have a check to terminate completion offers when semicolon is sent. This should be easy enough to fix. We'll reopen our issue.

@bobbrow
Copy link
Member

bobbrow commented Jun 14, 2017

@jrieken, I did some more digging. It looks like there was a behavior change in VS Code. This issue does not repro in 1.11.2 and earlier builds of VS Code. It used to be the case that completion requests were not sent to the extension after a grammar scope change. There are a number of cases I can see that our extension won't handle correctly anymore because we are not keeping track of the current token being edited.

Since VS Code is tracking the tokens via the language grammar, shouldn't it also terminate the completion requests when it detects that the scope has changed? It is valid for us to offer completion results after a semicolon is typed, so it would be wrong for us to suppress completion offers in this case because we don't know if the user manually requested it via CTL+Space or if it is a continuation of a previous request. I think this is a regression in VS Code.

@bobbrow
Copy link
Member

bobbrow commented Jun 20, 2017

Will you please reopen this? It is a regression in VS Code as per my comment above.

@ramya-rao-a ramya-rao-a reopened this Jun 20, 2017
@ramya-rao-a ramya-rao-a removed the *caused-by-extension Issue identified to be caused by an extension label Jun 20, 2017
@jrieken
Copy link
Member

jrieken commented Jun 21, 2017

It used to be the case that completion requests were not sent to the extension after a grammar scope change.

That is not true. Suggest isn't looking at tokens at all. The only case in which we look at (simplified) tokens is to know if we are inside a string, comment, or other code. That's btw all the token information there is. There is no termination when reaching certain tokens or anything like that.

@bobbrow The problem here is that incomplete result sets are returned. Why are you doing that and when you are doing so why don't you respect the former anchor point? Please provide meaningful information here, attach a small repo with extension code that demos what you think is a bug

@jrieken jrieken added the info-needed Issue requires more information from poster label Jun 21, 2017
@bobbrow
Copy link
Member

bobbrow commented Jun 21, 2017

That is not true. Suggest isn't looking at tokens at all

Ok, that may be the case, but what I'm getting at is that this worked correctly in 1.11.2 and earlier versions of VS Code, so something changed. If you want to point me at the code that manages this stuff I can take a look at the history and see if I can figure it out.

Why are you doing that and when you are doing so why don't you respect the former anchor point?

As we mentioned earlier in the thread, we send incomplete results because our search turned up more than 1000 matches. What anchor point are you referring to? There is no guarantee from the language server protocol that subsequent completion requests are tied to any previous request. The user can cancel a request at any time, move the cursor, etc. These are stateless requests.

Consider the repro @yurriy outlined (with "editor.quickSuggestions" enabled):

  1. Create a new C++ source file (e.g. test.cpp)
  2. On a new line:
  • type _
  • then type _
  • then type ;
  1. Press ESC to cancel the popup, and delete what you just typed.
  2. Then, on a new line:
  • type _
  • then type _
  • then press ESC
  • then type ;
  • then manually invoke suggestions with CTL+Space

In both cases, we will get the exact same 3 requests from VS Code. The "anchor point" you refer to would be different in both these cases, but there's no way for the extension to know that since we don't have enough information to make that distinction.

If we were to assume that the 3rd request is tied to an "anchor point" before the semicolon (e.g. step 2 above), then the user would get no results when manually invoking suggestions after the semicolon (e.g. step 4) and think that the extension was broken.

Does this help explain our situation any better?

Appendix:

These are the exact messages (minus uri scrubbing) I got when running through the repro steps above:

{
    "jsonrpc": "2.0",
    "id": 11,
    "method": "textDocument/completion",
    "params": {
        "textDocument": {
            "uri": "file:///.../test.cpp"
        },
        "position": {
            "line": 0,
            "character": 1
        }
    }
}
{
    "jsonrpc": "2.0",
    "id": 13,
    "method": "textDocument/completion",
    "params": {
        "textDocument": {
            "uri": "file:///.../test.cpp"
        },
        "position": {
            "line": 0,
            "character": 2
        }
    }
}
{
    "jsonrpc": "2.0",
    "id": 15,
    "method": "textDocument/completion",
    "params": {
        "textDocument": {
            "uri": "file:///.../test.cpp"
        },
        "position": {
            "line": 0,
            "character": 3
        }
    }
}

@jrieken
Copy link
Member

jrieken commented Jun 21, 2017

earlier versions of VS Code, so something changed

Obviously things change, we fix various bugs and implement features every milestone. The assumption that we broke this extension, without a repro etc, is pretty bold.

As we mentioned earlier in the thread, we send incomplete results because our search turned up more than 1000 matches.

Does that mean you cap it at 1000 results and want to return more results when you have more prefix-text? For instance ab<trigger_here> with 1e10 results for the ab-prefix where with abc<trigger_here> there are just 1e2 results for the abc-prefix?

respect the former anchor point?

By "anchor point" the current prefix left of the cursor is meant. So, you have member completion for foo where foo has the bar, far, and boo members. So, now with foo.b<trigger_here> look at what is left of the cursor and find as anchor point the position before b (after the dot) and then they tell the completions to overwrite one character. Now, with foo.ba they stay at the same anchor point and tell the completions to overwrite two characters. That's what I mean by anchor point and what this extension does is telling to re-compute after having a list of suggestion (that means do not filter down the list of suggestions). Re-computing is supported and works as designed however, this extension doesn't seem to look at any anchor point and always seems to return something, for instance after having typed a semi-colon.

@jrieken
Copy link
Member

jrieken commented Jun 21, 2017

{
    "jsonrpc": "2.0",
    "id": 15,
    "method": "textDocument/completion",
    "params": {
        "textDocument": {
            "uri": "file:///.../test.cpp"
        },
        "position": {
            "line": 0,
            "character": 3
        }
    }
}

That request (auto-triggered by this extension singling incomplete results) is requesting completion after the ; in the __; text. Why does your extension return a result here?

@bobbrow
Copy link
Member

bobbrow commented Jun 21, 2017

That request (auto-triggered by this extension singling incomplete results) is requesting completion after the ; in the __; text. Why does your extension return a result here?

Why wouldn't it? It is valid C++ to add a new identifier immediately after a semicolon. Stylistically, I would never do it, but that doesn't make it any less valid. Replace ; with + in the repro above and you get the same behavior. I certainly don't have any qualms with developers wanting to put identifiers right after a +.

@bobbrow
Copy link
Member

bobbrow commented Jun 21, 2017

Does that mean you cap it at 1000 results and want to return more results when you have more prefix-text? For instance ab<trigger_here> with 1e10 results for the ab -prefix where with abc<trigger_here> there are just 1e2 results for the abc -prefix?

Yes, as the user types more, the number of results returned should eventually get down under 1000 and we'll mark our results as "complete."

That's what I mean by anchor point and what this extension does is telling to re-compute after having a list of suggestion (that means do not filter down the list of suggestions).

We have no choice but to recompute because there is no state (that I am aware of) manifested in the chain of textDocument/completion requests. We cannot assume that any new requests we receive are tied to a previous request that we marked as incomplete.

@jrieken
Copy link
Member

jrieken commented Jun 22, 2017

Yes, as the user types more, the number of results returned should eventually get down under 1000 and we'll mark our results as "complete."

But why is that? Can you not retrieve more than 1000 suggestions from your backend or are you afraid the LSP or VS Code cannot handle 1000+ items? Wrt VS Code and its suggest logic (rendering, filtering, and sorting) we try to make sure things are snappy with ~10000 items. So there is room left.

@jrieken jrieken changed the title Quick suggestions work incorrect Incomplete suggestion results cause re-triggering when typing w/o further context. Jun 22, 2017
@jrieken jrieken added under-discussion Issue is under discussion for relevance, priority, approach and removed info-needed Issue requires more information from poster labels Jun 22, 2017
@jrieken
Copy link
Member

jrieken commented Jun 22, 2017

Ok, I have updated the title of this issue to more accurately reflect what is happening here. Today, the definition and implementation for incomplete suggestion results is that each character typed after receiving a result trigger a re-compute of the list. Since the document is the only one holding state and since that might not be enough more information is needed. Makes this request very similar to #752.

Sorry for taking so long to understand this. My suggestion for now is to return more than 1000 results, e.g all results if possible, e.g not incomplete. Idea is to add a context param to the provide call tho that needs careful thinking, we want to keep the cancelation token the last param and we don't want to break the API

@jrieken jrieken added the api label Jun 22, 2017
@sean-mcmanus
Copy link
Contributor

1000 was chosen as an arbitrary number to prevent an unbounded delay, but we don't have performance data on the effect of increasing the number (our backend has processing to do). We could investigate increasing this number, but for large C++ projects, I've seen autocomplete at global scale generate over 40000 results.

We might be able to fix this via saving the anchor position when an incomplete result is returned and clearing the state when the selection changes to a different line/file, although it might not solve all scenarios.

@jrieken
Copy link
Member

jrieken commented Jun 22, 2017

Ok, I'd go for a higher limit to begin with and see how well or not that goes. With 10000 it should still be OK, the 40000 maybe a little slower. The rendering is all virtualised, the expensive bit is data serialisation and filtering/scoring. I'd give it a try, everyone would benefit if you guys can uncover some hidden perf issues with this.

We might be able to fix this via saving the anchor position when an incomplete result is returned and clearing the state when the selection changes to a different line/file, although it might not solve all scenarios.

It could work - the UI only allows to invoke IntelliSense in one editor at a time but your provider can also be asked via API-commands. But the assumption will hold for most cases...

Another thing you could try is to filter down the list on the server, e.g. when having a huge result set and with a forgiving continuous substring matcher.

@sean-mcmanus
Copy link
Contributor

@jrieken We're hitting problems when incomplete == true -- there appears to be no way to cancel the completion when a ";" or other identifier breaking character is is encountered, even if we save the anchor position. The result is that it shows "No suggestions" instead of disappearing. Also, when the user hits Esc to cancel the completion popup, we also get no cancelRequest set, so we can't reset the "anchor" correctly. It sounds like you're saying this "incomplete" parameter is not really usable in a correct way? Or are we misunderstanding something?

@bobbrow
Copy link
Member

bobbrow commented Jun 27, 2017

@sean-mcmanus we don't have enough context from vscode to do anything right now besides increasing our upper limit. We can try 10000 and see if that helps.

@jrieken jrieken added this to the June 2017 milestone Jun 28, 2017
@jrieken jrieken added bug Issue identified by VS Code Team member as probable bug and removed under-discussion Issue is under discussion for relevance, priority, approach labels Jun 28, 2017
@jrieken
Copy link
Member

jrieken commented Jun 28, 2017

Ok, plan for June is to cancel suggestions when a result is marked as incomplete and when the cursor isn't on a word anymore. That might discriminate suggestions that contain non-word-characters and aren't there yet (still in the incomplete portion) but we believe that's the smaller problem.

@jrieken
Copy link
Member

jrieken commented Jun 28, 2017

I have pushed a change that implements aforementioned behaviour. Closing this issue, exposing more information to suggestion providers is tracked here: #752.

@jrieken jrieken closed this as completed Jun 28, 2017
@jrieken jrieken removed the api label Jun 28, 2017
@bobbrow
Copy link
Member

bobbrow commented Jun 28, 2017

Thanks @jrieken!

@roblourens roblourens added the verified Verification succeeded label Jun 30, 2017
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug suggest IntelliSense, Auto Complete verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

6 participants