You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The Roslyn BatchFixAllProvider often fails to fix several diagnostics in a document because it merges code changes. The Roslyn team discussed the pro and cons of several approaches for FixAllProvider and I think there is a sweet-spot between the Sequential and the Batch approaches that can be used for a variety of fixes.
I propose to create a custom FixAllProvider with the following characteristics:
Runs for all documents in the scope in parallel.
Runs the fixes for a single document in sequence like that:
Get all fixable diagnostics for the document
Track the nodes associated with the diagnostic with SyntaxAnnotations
This new custom FixAllProvider would have the limitation, that the fix for a diagnostic is only allowed to change the current document (otherwise there would be merge conflicts).
To make the FixAllProvider reusable I propose the following architecture:
The IntroduceFieldFromConstructorCodeFixProviderAll reuses the CodeFixProvider logic by calling the static IntroduceFieldFromConstructorCodeFixProvider.IntroduceFieldFromConstructor method.
This would be abstracted by introducing an interface that the CodeFixProvider needs to implement.
This interface might look like that:
/// <summary>/// This interface must be implemented by the associated CodeFixProvider. The CodeFixProvider must operate on a single document and/// should only change the document. This limits the possible operations of the CodeFixProvider to change only document internals without/// effecting other parts of the solution./// </summary>publicinterfaceIFixDocumentInternalsOnly{Task<Document>FixDocumentAsync(SyntaxNodenodeWithDiagnostic,Documentdocument,CancellationTokencancellationToken);}
The custom FixAllProvider would operate on all the Documents in scope in parallel:
privateasyncstaticTask<Solution>GetFixedDocumentsAsync(FixAllContextfixAllContext,IEnumerable<Document>documents){varsolution=fixAllContext.Solution;varnewDocuments=documents.ToDictionary(d =>d.Id, d =>GetFixedDocumentAsync(fixAllContext,d));awaitTask.WhenAll(newDocuments.Values).ConfigureAwait(false);varchangedDocuments=fromkvpinnewDocumentswherekvp.Value.Result!=nullselectnew{DocumentId=kvp.Key,Document=kvp.Value.Result};foreach(varnewDocumentinchangedDocuments)solution=solution.WithDocumentSyntaxRoot(newDocument.DocumentId,awaitnewDocument.Document.GetSyntaxRootAsync().ConfigureAwait(false));returnsolution;}
A single document is fixed like that:
privateasyncstaticTask<Document>GetFixedDocumentAsync(FixAllContextfixAllContext,Documentdocument){varcodeFixer=fixAllContext.CodeFixProviderasIFixDocumentInternalsOnly;if(codeFixer==null)thrownewArgumentException("This CodeFixAllProvider requires that your CodeFixProvider implements the IFixDocumentInternalsOnly.");vardiagnostics=awaitfixAllContext.GetDocumentDiagnosticsAsync(document).ConfigureAwait(false);if(diagnostics.Length==0)returnnull;varroot=awaitdocument.GetSyntaxRootAsync(fixAllContext.CancellationToken).ConfigureAwait(false);varnodes=diagnostics.Select(d =>root.FindNode(d.Location.SourceSpan)).Where(n =>!n.IsMissing);varannotations=newList<SyntaxAnnotation>();varnewRoot=root.ReplaceNodes(nodes,(original,rewritten)=>{varannotation=newSyntaxAnnotation(SyntaxAnnotationKey);annotations.Add(annotation);varnewNode=original.WithAdditionalAnnotations(annotation);returnnewNode;});varnewDocument=document.WithSyntaxRoot(newRoot);newDocument=awaitFixCodeForAnnotatedNodesAsync(newDocument,codeFixer,annotations,fixAllContext.CancellationToken).ConfigureAwait(false);newDocument=awaitRemoveAnnontationsAsync(newDocument,annotations).ConfigureAwait(false);returnnewDocument;}privatestaticasyncTask<Document>FixCodeForAnnotatedNodesAsync(Documentdocument,IFixDocumentInternalsOnlycodeFixer,IEnumerable<SyntaxAnnotation>annotations,CancellationTokencancellationToken){foreach(varannotationinannotations){varnewRoot=awaitdocument.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);varannotatedNodes=newRoot.GetAnnotatedNodes(annotation);varnode=annotatedNodes.FirstOrDefault();if(node==null)continue;document=awaitcodeFixer.FixDocumentAsync(node,document,cancellationToken).ConfigureAwait(false);}returndocument;}privatestaticasyncTask<Document>RemoveAnnontationsAsync(Documentdocument,IEnumerable<SyntaxAnnotation>annotations){varroot=awaitdocument.GetSyntaxRootAsync().ConfigureAwait(false);varnodes=annotations.SelectMany(annotation =>root.GetAnnotatedNodes(annotation));root=root.ReplaceNodes(nodes,(original,rewritten)=>original.WithoutAnnotations(annotations));varnewDocument=document.WithSyntaxRoot(root);returnnewDocument;}
I started a local branch for such a FixAllProvider for the new analyzer #904 that is already working on my machine.
If there is interest in such a FixAllProvider we need to discuss
Where it should be located (CodeCracker.Common.dll or CodeCracker.CSharp.dll)
The proposed architecture (especially the interface approach)
How to proceed with the consumers (I think it is okay to first merge Pull Request New Analyzer CC0125 - getter only auto property #904 than create another Pull Request with the new FixAllProvider and related test and then change IntroduceFieldFromConstructorCodeFixProvider and ReplaceWithGetterOnlyAutoPropertyCodeFixProvider to use the new FixAllProvider)
As I worked on the implementation I came across some issues I would like to document:
The fixer might need the SemanticModel of the SyntaxTree. That's why I used the Document instead of the RootSyntaxTree for the parameters in the interface (The SemanticModel is only available via Document.GetSemanticModelAsync).
This approach will fail on partial classes that are defined in two ore more documents (the Fixer will not be able to operate on nodes in the other documents).
Related to #352 (Test fix all providers)
The Roslyn BatchFixAllProvider often fails to fix several diagnostics in a document because it merges code changes. The Roslyn team discussed the pro and cons of several approaches for
FixAllProviderand I think there is a sweet-spot between the Sequential and the Batch approaches that can be used for a variety of fixes.I propose to create a custom
FixAllProviderwith the following characteristics:SyntaxAnnotationsSyntaxAnnotationsfrom the tree.The IntroduceFieldFromConstructorCodeFixProviderAll already works like that.
This new custom
FixAllProviderwould have the limitation, that the fix for a diagnostic is only allowed to change the current document (otherwise there would be merge conflicts).To make the
FixAllProviderreusable I propose the following architecture:The
IntroduceFieldFromConstructorCodeFixProviderAllreuses the CodeFixProvider logic by calling the staticIntroduceFieldFromConstructorCodeFixProvider.IntroduceFieldFromConstructormethod.This would be abstracted by introducing an interface that the
CodeFixProviderneeds to implement.This interface might look like that:
The custom
FixAllProviderwould operate on all theDocumentsin scope in parallel:A single document is fixed like that:
I started a local branch for such a
FixAllProviderfor the new analyzer #904 that is already working on my machine.If there is interest in such a
FixAllProviderwe need to discussCodeCracker.Common.dllorCodeCracker.CSharp.dll)interfaceapproach)FixAllProviderand related test and then changeIntroduceFieldFromConstructorCodeFixProviderandReplaceWithGetterOnlyAutoPropertyCodeFixProviderto use the newFixAllProvider)As I worked on the implementation I came across some issues I would like to document:
SemanticModelof theSyntaxTree. That's why I used theDocumentinstead of theRootSyntaxTreefor the parameters in the interface (TheSemanticModelis only available viaDocument.GetSemanticModelAsync).partialclasses that are defined in two ore more documents (theFixerwill not be able to operate on nodes in the other documents).