Make completion handler more resilient #69795
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| } | ||
| catch (Exception e) when (FatalError.ReportAndCatchUnlessCanceled(e)) | ||
| { | ||
| // In case of exception, we simply return DisplayText with default span as the change. |
There was a problem hiding this comment.
Does DisplayText have the risk of containing stars in the case of an IntelliCode suggestion?
There was a problem hiding this comment.
Ah, you are right. I thought I had moved them to use DisplayTextPrefix, turns out I never did.
There was a problem hiding this comment.
So is this going to insert stars into the file in that case? Is there a change to make here or should we just be moving to DisplayTextPrefix?
There was a problem hiding this comment.
Yes, this will insert the star if call to GetChange crashes, which is unlikely to happen with current implementation though, since the change was computed during the calculation of CompletionItem, GetChange simply retrieve it from the property bag (see here). We should take this change as is (i.e. fallback to using DisplayText). I will make a separate change in Pythia to use DisplayTextPrefix for the star (because it's seems to be the right thing to do anyway)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Based on my review of the original issue, I believe the handling and presentation of the error is reasonable (no change needed). We should instead fix the override completion provider to not produce this error, either by avoiding the use of the NotImplementedException symbol altogether, or by falling back to construction from a fully qualified identifier in the case where it's null.
|
@sharwell I have reset your review since I have added the fix for override completion provider, and I think having a general defense against CompletionProvider crash is an improvement. |
ghost
added this to the 
This contains two individual fixes.