Add FixAll support for code refactorings #59809
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| #nullable disable | ||
|
|
||
| using System; | ||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using System.Threading; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.CodeFixes; | ||
| using Microsoft.CodeAnalysis.Host; | ||
| using Microsoft.CodeAnalysis.Host.Mef; | ||
| using Microsoft.CodeAnalysis.Internal.Log; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.Implementation.Suggestions | ||
| { | ||
| [ExportWorkspaceServiceFactory(typeof(IFixAllCodeFixGetFixesService), ServiceLayer.Host), Shared] | ||
| internal class FixAllCodeFixGetFixesService : IFixAllCodeFixGetFixesService, IWorkspaceServiceFactory | ||
| { | ||
| [ImportingConstructor] | ||
| [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| public FixAllCodeFixGetFixesService() | ||
| { | ||
| } | ||
|
|
||
| public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices) | ||
| => this; | ||
|
There was a problem hiding this comment. doesn't use workspaceServices, make into IWorkspaceService (no Factory) instead? There was a problem hiding this comment. Existing code, but I can cleanup |
||
|
|
||
| public async Task<Solution> GetFixAllChangedSolutionAsync(FixAllContext fixAllContext) | ||
| { | ||
| var codeAction = await GetFixAllCodeActionAsync(fixAllContext).ConfigureAwait(false); | ||
| if (codeAction == null) | ||
| { | ||
| return fixAllContext.Solution; | ||
| } | ||
|
There was a problem hiding this comment. Existing code, but I can doc. |
||
|
|
||
| fixAllContext.CancellationToken.ThrowIfCancellationRequested(); | ||
| return await codeAction.GetChangedSolutionInternalAsync(cancellationToken: fixAllContext.CancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| public async Task<ImmutableArray<CodeActionOperation>> GetFixAllOperationsAsync( | ||
| FixAllContext fixAllContext, bool showPreviewChangesDialog) | ||
| { | ||
| var codeAction = await GetFixAllCodeActionAsync(fixAllContext).ConfigureAwait(false); | ||
| if (codeAction == null) | ||
| { | ||
| return ImmutableArray<CodeActionOperation>.Empty; | ||
| } | ||
|
|
||
| return await FixAllGetFixesServiceHelper.GetFixAllOperationsAsync( | ||
| codeAction, fixAllContext.State.Project, fixAllContext.State.CorrelationId, | ||
| FunctionId.CodeFixes_FixAllOccurrencesPreviewChanges, | ||
| showPreviewChangesDialog, fixAllContext.CancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private static async Task<CodeAction> GetFixAllCodeActionAsync(FixAllContext fixAllContext) | ||
| { | ||
| using (Logger.LogBlock( | ||
| FunctionId.CodeFixes_FixAllOccurrencesComputation, | ||
| KeyValueLogMessage.Create(LogType.UserAction, m => | ||
| { | ||
| m[FixAllLogger.CorrelationId] = fixAllContext.State.CorrelationId; | ||
| m[FixAllLogger.FixAllScope] = fixAllContext.State.Scope.ToString(); | ||
| }), | ||
| fixAllContext.CancellationToken)) | ||
| { | ||
| CodeAction action = null; | ||
| try | ||
| { | ||
| action = await fixAllContext.FixAllProvider.GetFixAsync(fixAllContext).ConfigureAwait(false); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| FixAllLogger.LogComputationResult(fixAllContext.State.CorrelationId, completed: false); | ||
| } | ||
| finally | ||
| { | ||
| if (action != null) | ||
| { | ||
| FixAllLogger.LogComputationResult(fixAllContext.State.CorrelationId, completed: true); | ||
| } | ||
| else | ||
| { | ||
| FixAllLogger.LogComputationResult(fixAllContext.State.CorrelationId, completed: false, timedOut: true); | ||
|
There was a problem hiding this comment. Again, all of this is existing code. Just renaming the type and splitting out some common code into a shared type led Github to show file as new code. |
||
| } | ||
| } | ||
|
|
||
| return action; | ||
| } | ||
| } | ||
|
|
||
| internal static Solution PreviewChanges( | ||
| Solution currentSolution, | ||
| Solution newSolution, | ||
| string fixAllPreviewChangesTitle, | ||
| string fixAllTopLevelHeader, | ||
| string languageOpt, | ||
| Workspace workspace, | ||
| int? correlationId = null, | ||
| CancellationToken cancellationToken = default) | ||
|
There was a problem hiding this comment. please don't make internal methods have optional cancellation tokens. That just means it's easy for us to forget to pass them along. There was a problem hiding this comment. Existing code, I just refactored it move out common code to a shared type. |
||
| => FixAllGetFixesServiceHelper.PreviewChanges( | ||
| currentSolution, newSolution, fixAllPreviewChangesTitle, | ||
| fixAllTopLevelHeader, languageOpt, workspace, | ||
| FunctionId.CodeFixes_FixAllOccurrencesPreviewChanges, | ||
| correlationId, cancellationToken); | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
| // See the LICENSE file in the project root for more information. | ||
|
|
||
| using System; | ||
| using System.Collections.Immutable; | ||
| using System.Composition; | ||
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.CodeRefactorings; | ||
| using Microsoft.CodeAnalysis.Host; | ||
| using Microsoft.CodeAnalysis.Host.Mef; | ||
| using Microsoft.CodeAnalysis.Internal.Log; | ||
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.Implementation.Suggestions | ||
| { | ||
| [ExportWorkspaceServiceFactory(typeof(IFixAllCodeRefactoringGetFixesService), ServiceLayer.Host), Shared] | ||
| internal class FixAllCodeRefactoringGetFixesService : IFixAllCodeRefactoringGetFixesService, IWorkspaceServiceFactory | ||
|
There was a problem hiding this comment.
There was a problem hiding this comment.
So the core issue was that similar types like FixAllContext, FixAllProvider, DocumentBasedFixAllProvider are all public types. Adding a common abstract type is not possible, unless we make it public, which would seem odd. One thing we can do is possibly move all the code to shared internal types like CommonFixAllContext, CommonFixAllProvider, etc. and then have conversions between them OR use composition - I'll see which works out better. I think it might be better to just create a separate PR for that approach |
||
| { | ||
| [ImportingConstructor] | ||
| [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] | ||
| public FixAllCodeRefactoringGetFixesService() | ||
| { | ||
| } | ||
|
|
||
| public IWorkspaceService CreateService(HostWorkspaceServices workspaceServices) | ||
| => this; | ||
|
|
||
| public async Task<Solution?> GetFixAllChangedSolutionAsync(FixAllContext fixAllContext) | ||
| { | ||
| var codeAction = await GetFixAllCodeActionAsync(fixAllContext).ConfigureAwait(false); | ||
| if (codeAction == null) | ||
| { | ||
| return fixAllContext.Project.Solution; | ||
|
There was a problem hiding this comment. why is this returning .Project.Solution. Code in prior class returned just .Solution. There was a problem hiding this comment. I thought I removed Solution property from this context because it was just returning Project.Solution. Let me clean this up using the Generics/Type hierarchy suggestion you provided and make it consistent. |
||
| } | ||
|
|
||
| fixAllContext.CancellationToken.ThrowIfCancellationRequested(); | ||
| return await codeAction.GetChangedSolutionInternalAsync(cancellationToken: fixAllContext.CancellationToken).ConfigureAwait(false); | ||
|
There was a problem hiding this comment. i hope all the fixAllContext.CancellationTokens are being used properly :) |
||
| } | ||
|
|
||
| public async Task<ImmutableArray<CodeActionOperation>> GetFixAllOperationsAsync(FixAllContext fixAllContext) | ||
| { | ||
| var codeAction = await GetFixAllCodeActionAsync(fixAllContext).ConfigureAwait(false); | ||
| if (codeAction == null) | ||
| { | ||
| return ImmutableArray<CodeActionOperation>.Empty; | ||
| } | ||
|
|
||
| return await FixAllGetFixesServiceHelper.GetFixAllOperationsAsync( | ||
| codeAction, fixAllContext.State.Project, fixAllContext.State.CorrelationId, | ||
| FunctionId.Refactoring_FixAllOccurrencesPreviewChanges, | ||
| showPreviewChangesDialog: true, fixAllContext.CancellationToken).ConfigureAwait(false); | ||
| } | ||
|
|
||
| private static async Task<CodeAction?> GetFixAllCodeActionAsync(FixAllContext fixAllContext) | ||
| { | ||
| using (Logger.LogBlock( | ||
| FunctionId.CodeFixes_FixAllOccurrencesComputation, | ||
| KeyValueLogMessage.Create(LogType.UserAction, m => | ||
| { | ||
| m[FixAllLogger.CorrelationId] = fixAllContext.State.CorrelationId; | ||
| m[FixAllLogger.FixAllScope] = fixAllContext.State.FixAllScope.ToString(); | ||
| }), | ||
| fixAllContext.CancellationToken)) | ||
| { | ||
| CodeAction? action = null; | ||
| try | ||
| { | ||
| action = await fixAllContext.FixAllProvider.GetFixAsync(fixAllContext).ConfigureAwait(false); | ||
| } | ||
| catch (OperationCanceledException) | ||
| { | ||
| FixAllLogger.LogComputationResult(fixAllContext.State.CorrelationId, completed: false); | ||
| } | ||
| finally | ||
| { | ||
| if (action != null) | ||
| { | ||
| FixAllLogger.LogComputationResult(fixAllContext.State.CorrelationId, completed: true); | ||
| } | ||
| else | ||
| { | ||
| FixAllLogger.LogComputationResult(fixAllContext.State.CorrelationId, completed: false, timedOut: true); | ||
| } | ||
| } | ||
|
|
||
| return action; | ||
| } | ||
| } | ||
| } | ||
| } | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an existing type that was renamed from
FixAllGetFixesServicetoFixAllCodeFixGetFixesServiceand some code split into a helper typeFixAllGetFixesServiceHelperwhich is shared betweenFixAllCodeFixGetFixesServiceandFixAllCodeRefactoringGetFixesService.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it a workspaceservicefactory, instead of just an IWorkspaceService?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is existing code, and I have mimic'ed the same for
FixAllCodeRefactoringGetFixesService. I can clean this up.