Wrap calls to CompletionProvider extensions such that they go through our extension manager #70843
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| or CodeRefactoringProvider | ||
| or CodeRefactorings.FixAllProvider | ||
| or CodeFixes.FixAllProvider | ||
| or CompletionProvider) |
There was a problem hiding this comment.
added CompletionProvider here. That way this participates in teh yellow-bar, telling the user about the completion provider that thrw the exception and if htey want to enable it.
| { | ||
| if (!IsIgnored(provider) && | ||
| _globalOptions.GetOption(ExtensionManagerOptions.DisableCrashingExtensions)) |
There was a problem hiding this comment.
this option was always true, and had no way to configure it. removed it.
| await task.ConfigureAwait(false); | ||
|
|
||
| if (actions.Count == 0) | ||
| return extensionManager.PerformFunctionAsync( |
There was a problem hiding this comment.
moved all explicit try/catch code with extension manager to use the existing helper functions that already do the right thing.
src/Features/Core/Portable/Completion/CompletionService_GetCompletions.cs
Outdated
Show resolved
Hide resolved
| return false; | ||
| } | ||
| protected override void HandleNonCancellationException(object provider, Exception exception) | ||
| => _logger.Log(LogLevel.Error, exception, $"{provider.GetType()} threw an exception."); |
There was a problem hiding this comment.
@dibarbet Despite having no UI for ut, we're still not disabling the providers when they crash. IMO, we should do that in LSP, otherwise we can just continue calling into them, and they will continue crashing. Thoughts?
There was a problem hiding this comment.
Talked offline - fine with this for this PR. Tracking issue here #70844 to enable a similar UI that VS has.
| @@ -4,127 +4,121 @@ | |||
|
|
|||
There was a problem hiding this comment.
view with whitespace off.
| [method: ImportingConstructor] | ||
| [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| internal sealed class TestExtensionManager( | ||
| [Import] TestExtensionErrorHandler errorHandler) : AbstractExtensionManager |
There was a problem hiding this comment.
this import seems sus
|
|
||
| void HandleException(object provider, Exception exception); |
There was a problem hiding this comment.
merged these into one helper. much simpler and cleaner. especially as some CanHandleException helpers were stateful!
|
@CyrusNajmabadi also fixes #70786 no? |
|
@jbevain yup! |
ghost
added this to the 
|
|
||
| public override void HandleException(object provider, Exception exception) | ||
| if (provider is CodeFixProvider | ||
| or CodeRefactoringProvider |
Fixes #70830
Fixes #70786