Add FixAll support for code refactorings (approach #2) #60906

mavasani · 2022-04-22T13:09:26Z

No public APIs are being added in this PR. #60703 tracks designing and exposing public APIs for this feature.

…ted changes

…to add FixAll support

…date namespace

…ks making them public in future. Also move some helper types from shared layer to workspaces layer as a result of internalizing these APIs

… up the added fix all integration tests

Closes dotnet#32461 No public APIs are being added in this PR. dotnet#60703 tracks designing and exposing public APIs for this feature.

…ng_2

…/roslyn into FixAllCodeRefactoring_2

CyrusNajmabadi · 2022-04-25T22:50:23Z

At this point i'm overall very happy with the state of things. The last set of comments is mostly cleanup/docs (Though there are at least a couple of important things i think should be looked at closely). Once that's addressed, i think we're good to go in :)

mavasani · 2022-04-27T09:37:58Z

note: you can still skip getting the root. you can figure the length out from teh text of the document.

Yes, that is correct. But we do the computation now at the point where we are just about to invoke the refactoring provider to fix all instances in the document, and the provider is going to request for syntax nodes to compute the fixes, so we are not really saving anything.

…ng_2

CyrusNajmabadi · 2022-04-27T16:24:24Z

Yes, that is correct. But we do the computation now at the point where we are just about to invoke the refactoring provider to fix all instances in the document, and the provider is going to request for syntax nodes to compute the fixes, so we are not really saving anything.

This is not actually true. Consider if we needed to do fix-all on 100s to thousands of files. This will go through and parse them all (just to find the length). However, these trees are not held onto by anything. Internally, we just have a weak-reference (so that if one is alive, and another asks for it, they'll get the same tree). So if a single GC happens (which is virtually guaranteed in a fix-all scenario), then all those trees will be garbage collected and we'll have to parse them all over again as we actually fix each file.

You're only parsing/loading-text just to say "i want to do the whole file". there's got to be a cleaner way of representing that doesn't involve pre-parsing and potentially redoing all that work later :)

mavasani · 2022-04-27T16:28:16Z

You're only parsing/loading-text just to say "i want to do the whole file". there's got to be a cleaner way of representing that doesn't involve pre-parsing and potentially redoing all that work later :)

The only other alternative is to pass in an empty array of spans to the refactoring provider, and it needs to check for empty array and deduce that it needs to fix the entire document. If that is fine to keep as a contract, we can skip this part in the engine.

CyrusNajmabadi · 2022-04-27T19:41:53Z

The only other alternative is to pass in an empty array of spans to the refactoring provider

Right. FOr each document it could have a Nullable span, where null means: the entire doc.

mavasani · 2022-04-28T05:37:13Z

Right. FOr each document it could have a Nullable span, where null means: the entire doc.

The API actually takes an immutable array of spans, as we have to account for partial definitions for ContainingType fix all scope. Probably we can pass in an Optional immutable array. I’ll go ahead with that.

mavasani · 2022-04-28T06:52:54Z

@CyrusNajmabadi I was able to avoid explicitly parsing/loading the document text for this scenario with 265022a. The final call made to each refactoring provider has a SyntaxEditor, which already has the root, so we perform the empty spans check right before that call and insert the full span.

CyrusNajmabadi · 2022-04-29T16:00:54Z

src/Features/Core/Portable/CodeFixesAndRefactorings/AbstractFixAllCodeAction.cs

+    /// </summary>
+    internal abstract class AbstractFixAllCodeAction : CodeAction
+    {
+        private bool _showPreviewChangesDialog;


mutable field is very strange. but if this is how it already was, then that's ok.

CyrusNajmabadi · 2022-04-29T16:03:18Z

...eatures/LanguageServer/Protocol/Features/UnifiedSuggestions/UnifiedSuggestedActionsSource.cs

@@ -427,8 +428,14 @@ private static bool IsBulkConfigurationAction(CodeAction action)

            var filteredRefactorings = FilterOnAnyThread(refactorings, selection, filterOutsideSelection);

-            return filteredRefactorings.SelectAsArray(
-                r => OrganizeRefactorings(workspace, r));
+            using var _ = ArrayBuilder<UnifiedSuggestedActionSet>.GetInstance(filteredRefactorings.Length, out var orderedRefactorings);


no biggy, but there's also SelectAsArrayAsync

CyrusNajmabadi · 2022-04-29T16:07:26Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/FixAllLogger.cs

+                    break;
+
+                case FixAllKind.Refactoring:
+                    functionId = FunctionId.Refactoring_FixAllOccurrencesContext;


tbh, i'm not sure if we care that much, right?

we're already going to say which provider we have... so we already distinguish on what matters. having a distinct id to bucket fix all for fixes vs refactorings doesn't seem relevant (afaict).

Let me sync up with @mikadumont if we care about differentiating between a FixAll for code fixes vs FixAll for refactorings with respect to telemetry. For example, do we want to find out how many FixAll for refactorings were invoked by users over a time frame?

CyrusNajmabadi · 2022-04-29T16:08:28Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/FixAllProviderInfo.cs

+            }
+
+            public override bool CanBeFixed(Diagnostic diagnostic)
+                => throw ExceptionUtilities.Unreachable;


this should be doc'ed. that's basically my preference for anything with inheritance where we say something is unreachable. it's not always evident, so the docs should help the next person understand why we think this.

CyrusNajmabadi · 2022-04-29T16:09:29Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/IFixAllContext.cs

+        IFixAllState State { get; }
+        IFixAllProvider FixAllProvider { get; }
+        Solution Solution { get; }
+        Project Project { get; }


nit, SOlution seems redundant (can be => property with => Project.Solution;.

oh, we don't have DIM. ignore this. it could technically be an extension method. but that's also not ideal. oh well.

CyrusNajmabadi · 2022-04-29T16:09:48Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/IFixAllState.cs

+        FixAllKind FixAllKind { get; }
+        Document? Document { get; }
+        Project Project { get; }
+        Solution Solution { get; }


same comment here.

CyrusNajmabadi · 2022-04-29T16:10:09Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/IFixAllState.cs

+        /// <summary>
+        /// Underlying code fix provider or code refactoring provider for the fix all occurrences fix.
+        /// </summary>
+        object Provider { get; }


nit, woudl be nice to have common internal base interface (Even if empty) to make sure we're passing in the right type of thing, and not some random value.

CyrusNajmabadi · 2022-04-29T16:12:03Z

src/Workspaces/Core/Portable/CodeRefactorings/FixAllOccurences/FixAllProvider.cs

+    /// </remarks>
+    internal abstract class FixAllProvider : IFixAllProvider
+    {
+        private protected static ImmutableArray<FixAllScope> DefaultSupportedFixAllScopes


Suggested change

private protected static ImmutableArray<FixAllScope> DefaultSupportedFixAllScopes

private protected static readonly ImmutableArray<FixAllScope> DefaultSupportedFixAllScopes

CyrusNajmabadi · 2022-04-29T16:16:01Z

src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/WorkspaceExtensions.projitems

-    <None Include="$(MSBuildThisFileDirectory)WorkspaceExtensionsResources.resx" />
+    <None Include="$(MSBuildThisFileDirectory)WorkspaceExtensionsResources.resx">
+      <SubType>Designer</SubType>
+    </None>


odd change.

Will revert, this keeps coming back.

CyrusNajmabadi · 2022-04-29T16:16:33Z

.../SharedUtilitiesAndExtensions/Workspace/VisualBasic/VisualBasicWorkspaceExtensions.projitems

@@ -43,7 +43,6 @@
    <Compile Include="$(MSBuildThisFileDirectory)Indentation\VisualBasicIndentationService.vb" />
    <Compile Include="$(MSBuildThisFileDirectory)LanguageServices\VisualBasicBlockFactsService.vb" />
    <Compile Include="$(MSBuildThisFileDirectory)LanguageServices\VisualBasicFileBannerFactsService.vb" />
-    <Compile Include="$(MSBuildThisFileDirectory)LanguageServices\VisualBasicFixAllSpanMappingService.vb" />


where did this go?

This is not required in the CodeStyle layer, so moved to the Workspaces layer.

CyrusNajmabadi

awesome! if you think any of this needs cleanup, feel free to do in a future PR.

mavasani · 2022-04-29T16:21:05Z

awesome! if you think any of this needs cleanup, feel free to do in a future PR.

Thanks, I have been struggling to get the tests passing on the PR, so would like to merge this. I plan to immediately do a follow-up PR for #60956. I'll address the latest round of feedback as part of it.

mavasani added 30 commits February 28, 2022 04:42

FixAll public API + core fix all related types + other workspace rela…

d9f5725

…ted changes

Features layer changes for FixAll support for refactorings

7812bec

Unified suggestion related changes for FixAll support for refactorings

26c35ef

EditorFeatures layer changes for FixAll support for refactorings

c6cc64f

VisualStudio layer changes for FixAll support for refactorings

4630883

Proof of concept - "Invert conditional" code refactoring enhancement …

96cd412

…to add FixAll support

Merge remote-tracking branch 'upstream/main' into FixAllCodeRefactoring

f73b283

Remove FixAllScope.Selection

c1adefd

Nullable enable

b463620

Share code

b41f729

Add code refactoring specific API to IFixAllSpanMappingService and up…

7ef03b2

…date namespace

Cleanup to make the APIs consistent with FixAll APIs for code fixes

80dc735

Make FixAll APIs for refactorings internal for now. dotnet#60703 trac…

6e86cd6

…ks making them public in future. Also move some helper types from shared layer to workspaces layer as a result of internalizing these APIs

Fix function ID

b27b6ca

Fixes

09d595c

Add unit tests and integration tests

b2b7cf8

Add integration test support for FixAll for code refactorings and fix…

4f20ef8

… up the added fix all integration tests

Merge remote-tracking branch 'upstream/main' into FixAllCodeRefactoring

d97f240

Fixes from merge

bd85c93

Fix IDE0044

a1e77fe

Add FixAll support for code refactorings

38d73f3

Closes dotnet#32461 No public APIs are being added in this PR. dotnet#60703 tracks designing and exposing public APIs for this feature.

Merge remote-tracking branch 'upstream/main' into FixAllCodeRefactori…

27c54a2

…ng_2

Cleanup

3500a61

Cleanup

3b92633

Cleanup

6786c47

Cleanup

824012d

Comment

3fe5ebb

Merge branch 'FixAllCodeRefactoring_2' of https://github.com/mavasani…

a4b67fd

…/roslyn into FixAllCodeRefactoring_2

Revert

8bd4f16

Revert

3a311d0

This was referenced Apr 26, 2022

Revert FixAll support for Invert conditional refactoring #60956

Closed

note: teh CT being in FixAllContext was somewhat a mistake #60957

Open

Address feedback

a54ee1d

Merge remote-tracking branch 'upstream/main' into FixAllCodeRefactori…

98e8d83

…ng_2

Address feedback - do not parse/load text for documents

265022a

CyrusNajmabadi reviewed Apr 29, 2022

View reviewed changes

CyrusNajmabadi approved these changes Apr 29, 2022

View reviewed changes

mavasani merged commit 18305a1 into dotnet:main Apr 29, 2022

ghost added this to the Next milestone Apr 29, 2022

mavasani deleted the FixAllCodeRefactoring_2 branch April 29, 2022 16:21

Cosifne modified the milestones: Next, 17.3 P2 May 31, 2022

Youssef1313 mentioned this pull request Nov 14, 2022

Add IDExxx rule for converting multiline string to raw string #65405

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FixAll support for code refactorings (approach #2) #60906

Add FixAll support for code refactorings (approach #2) #60906

mavasani commented Apr 22, 2022 •

edited

Loading

CyrusNajmabadi commented Apr 25, 2022

mavasani commented Apr 27, 2022

CyrusNajmabadi commented Apr 27, 2022

mavasani commented Apr 27, 2022

CyrusNajmabadi commented Apr 27, 2022

mavasani commented Apr 28, 2022

mavasani commented Apr 28, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

mavasani Apr 29, 2022

CyrusNajmabadi Apr 29, 2022 •

edited

Loading

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

mavasani Apr 29, 2022

CyrusNajmabadi Apr 29, 2022

mavasani Apr 29, 2022

CyrusNajmabadi left a comment

mavasani commented Apr 29, 2022

	private protected static ImmutableArray<FixAllScope> DefaultSupportedFixAllScopes
	private protected static readonly ImmutableArray<FixAllScope> DefaultSupportedFixAllScopes

Add FixAll support for code refactorings (approach #2) #60906

Add FixAll support for code refactorings (approach #2) #60906

Conversation

mavasani commented Apr 22, 2022 • edited Loading

CyrusNajmabadi commented Apr 25, 2022

mavasani commented Apr 27, 2022

CyrusNajmabadi commented Apr 27, 2022

mavasani commented Apr 27, 2022

CyrusNajmabadi commented Apr 27, 2022

mavasani commented Apr 28, 2022

mavasani commented Apr 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi Apr 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi left a comment

Choose a reason for hiding this comment

mavasani commented Apr 29, 2022

mavasani commented Apr 22, 2022 •

edited

Loading

CyrusNajmabadi Apr 29, 2022 •

edited

Loading