Fix completion at the start of readonly documents #33830
Conversation
sharwell
force-pushed
the
readonly-completion
branch
from
f1092e7
to
85b5f31
Compare
| @@ -18,7 +18,7 @@ public override bool ShouldTriggerCompletion(SourceText text, int position, Comp | |||
| { | |||
| switch (trigger.Kind) | |||
| { | |||
| case CompletionTriggerKind.Insertion: | |||
| case CompletionTriggerKind.Insertion when position > 0: | |||
| var insertedCharacterPosition = position - 1; | |||
| return this.IsInsertionTrigger(text, insertedCharacterPosition, options); | |||
There was a problem hiding this comment.
it feels like we should have a normal unit test for this. I'm actually surprised we don't as i thought we explicitly tested a whole bunch of edge cases for completion.
There was a problem hiding this comment.
Will file a follow-up issue to add one.
There was a problem hiding this comment.
I like the change. It seem that IsInsertionTrigger has checks for position > 0 as well but it is nice to have the check here earlier. Thank you!
The code works for both: old and new completions. So, the issue might occur from the very beginning. I would support it with a unit test now if we need to push it into 16.0.
There was a problem hiding this comment.
Any reason not to add a unit test now? Would this have also broken interactive entirely where you're regularly typing at the very start?
There was a problem hiding this comment.
Any reason not to add a unit test now?
I'm not sure where a unit test would go to hit this path, plus our existing test coverage of the feature is at least as good as what a unit test would provide.
Would this have also broken interactive entirely where you're regularly typing at the very start?
It only happens when you type a character to trigger completion, but something blocks that character from getting added to the IDE.
There was a problem hiding this comment.
I think we used to have tests for this. @dpoeschl recall anywhere those might have been?
If it was a completion integration test covering this I wouldn't be too terribly worried, but I could easily imagine somebody falling into the "oh, this is just testing Edit and Continue" and not realize this is our only coverage for something like this.
There was a problem hiding this comment.
@jasonmalinowski providing a unit test is not easy for the scenario. Here is my repro:
- Create a solution of two indenendent console applications.
- Do not build application 1
- Start debugging application 2 and stop at a break point
- Go to application 1 in editor and set cursor at position = 0
- Try to type anything.
- Get "Changes are not allowed if the project wan't built when debugging started"
- Hit Enter many times: once to close the dialog and the next one to attempt editing again (the cursor must be in the beginning of the file) , again and again.
I will file an issue to construct a unit test
There was a problem hiding this comment.
Sure, but you don't need a debugger here right? Just create a editor read-only span across the buffer? It's only a few lines extra compared to a normal test...?
There was a problem hiding this comment.
@jasonmalinowski Yes, we do not need a debugger here. However, it is hard to reproduce the issue. I had to hit enter 20 times or so to get a NFW. I think we need a more elegant repro.
|
I think we should not expect to be called with position = 0 and reason = insertion. I have added an issue on Editor: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/809690 |
ivanbasov
added
the
Tenet-Reliability
Fixes #33829