Make change signature more async and remove UI dialogs from the service #47768

dibarbet · 2020-09-16T23:22:33Z

When debugging roslyn master, I noticed that change signature was throwing in TryCreateUpdatedSolution as it expected to be called on the UI thread. However, this code path is triggered by GetOperationsAsync which is explicitly not running on the UI thread. The AbstractChangeSignatureService currently uses the UI thread as it pops up error messages and dialog boxes in certain cases.

There are two parts to the change:

Make TryCreateUpdatedSolution (and bits above it) in AbstractChangeSignatureService async. This is mostly done in
3c9b43c, but not quite all (I should have started with 2 first but oh well).
Move the error message /dialog box handling outside of the AbstractChangeSignatureService. This is done in
5144713

Note that most of the file changes are replacing WpfFact with Fact, the meat of the changes are in the following files
AbstractChangeSignatureService.cs
ChangeSignatureCodeAction.cs
ChangeSignatureCodeActionOperation.cs
AbstractChangeSignatureCommandHandler.cs

Resolves #44198
Resolves #41121
Resolves #41783

dibarbet · 2020-09-16T23:23:22Z

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

@@ -97,7 +97,7 @@ internal ChangeSignatureResult ChangeSignature(Document document, int position,
            switch (context)
            {
                case ChangeSignatureAnalysisSucceededContext changeSignatureAnalyzedSucceedContext:
-                    return ChangeSignatureWithContext(changeSignatureAnalyzedSucceedContext, cancellationToken);
+                    return ChangeSignatureWithContextAsync(changeSignatureAnalyzedSucceedContext, cancellationToken).WaitAndGetResult_CanCallOnBackground(cancellationToken);


In the next commit, I make this async as well and move the wait up to the command handler

dibarbet · 2020-09-16T23:23:43Z

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

@@ -387,7 +385,7 @@ internal ChangeSignatureResult ChangeSignatureWithContext(ChangeSignatureAnalysi
                var notificationService = context.Solution.Workspace.Services.GetRequiredService<INotificationService>();
                if (!notificationService.ConfirmMessageBox(FeaturesResources.This_symbol_has_related_definitions_or_references_in_metadata_Changing_its_signature_may_result_in_build_errors_Do_you_want_to_continue, severity: NotificationSeverity.Warning))


this is moved in the next commit to the proper locations

src/EditorFeatures/Core/Implementation/ChangeSignature/AbstractChangeSignatureCommandHandler.cs

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

src/Features/Core/Portable/ChangeSignature/ChangeSignatureCodeActionOperation.cs

src/Features/Core/Portable/ChangeSignature/ChangeSignatureResult.cs

CyrusNajmabadi

Overall LGTM. But def a few things i'd like tweaked before i sign off. but this is sooo much better :)

src/Features/Core/Portable/ChangeSignature/CannotChangeSignatureReason.cs

jasonmalinowski · 2020-12-03T22:13:57Z

@dibarbet: I think this will close these bugs, right?

#44198
#41121

It looks like we may have changed the offending WaitAndGetResult to a _CanCallOnBackground but you've cleaned it up properly now.

src/EditorFeatures/DiagnosticsTestUtilities/ChangeSignature/ChangeSignatureTestState.cs

src/EditorFeatures/Core/Implementation/ChangeSignature/AbstractChangeSignatureCommandHandler.cs

jasonmalinowski · 2020-12-03T22:15:42Z

src/EditorFeatures/Core/Implementation/ChangeSignature/AbstractChangeSignatureCommandHandler.cs

-                {
-                    return true;
-                }
+                    context.OperationContext.UserCancellationToken).WaitAndGetResult(context.OperationContext.UserCancellationToken);


So where in this process does the options dialog get displayed? Because now that this is async, how are we getting back to the UI thread to display the dialog? Does this need a JTF-friendly wait here as opposed to WaitAndGetResult?

@jasonmalinowski see the HandleResult method that runs after this WaitAndGetResult
https://github.com/dotnet/roslyn/pull/47768/files#diff-becc7ba48505ea7db15e854f845d4702e0d44964ec9a05d20b65c26572049596R86
The service just returns the failure kind and doesn't do any UI thread operations, so WaitAndGetResult is fine. Then when it completes since we're already on the UI thread, HandleResult can just display the notification without any switching.

And to add to that - the preview window is also part of HandleResult. If the result has preview changes then it will use the preview service to display them from the UI thread.

dibarbet · 2020-12-08T02:04:55Z

@dibarbet: I think this will close these bugs, right?

#44198
#41121

It looks like we may have changed the offending WaitAndGetResult to a _CanCallOnBackground but you've cleaned it up properly now.

Yes it will! Updated the PR description.

allisonchou · 2020-12-15T01:02:12Z

@dibarbet I think this may also resolve #41783

src/Features/Core/Portable/ChangeSignature/ChangeSignatureCodeAction.cs

jasonmalinowski

Looks good, or at least much better than what was there. I can't say I care for the pattern where the GetOptions() call runs even if the analysis fails and returns null, and then the actual refactoring operation runs even if options are null and then returns a different failure object. It seems like we're having to propogate failures when instead we could have stopped earlier and not had to have code dealing with failure modes. I'm happy for that to be addressed in a follow-up PR though (or not, if it makes a mess of something else) since this is a vast improvement for what's there.

jasonmalinowski · 2020-12-16T01:18:03Z

src/EditorFeatures/VisualBasicTest/ChangeSignature/ReorderParameters.MultiFile.vb

@@ -54,7 +54,7 @@ class C
            Using testState = ChangeSignatureTestState.Create(workspace)
                Dim history = testState.Workspace.GetService(Of ITextUndoHistoryRegistry)().RegisterHistory(testState.Workspace.Documents.First().GetTextBuffer())
                testState.TestChangeSignatureOptionsService.UpdatedSignature = permutation
-                Dim result = testState.ChangeSignature()
+                Dim result = Await testState.ChangeSignatureAsync().ConfigureAwait(False)


The ConfigureAwait() shouldn't be required in tests -- did the analyzer run here?

jasonmalinowski · 2020-12-17T22:52:44Z

src/EditorFeatures/CSharpTest/ChangeSignature/RemoveParametersTests.cs

@@ -248,10 +248,10 @@ void M()

            using var testState = ChangeSignatureTestState.Create(XElement.Parse(workspaceXml));
            testState.TestChangeSignatureOptionsService.UpdatedSignature = updatedSignature;
-            var result = testState.ChangeSignature();
+            var result = await testState.ChangeSignatureAsync().ConfigureAwait(false);


You don't need the ConfigureAwait(false) in tests. If the analyzer was flagging this, that's a bug.

jasonmalinowski · 2020-12-17T22:54:24Z

src/EditorFeatures/Core/Implementation/ChangeSignature/AbstractChangeSignatureCommandHandler.cs

-                    return true;
-                }
+                    restrictToDeclarations: false,
+                    cancellationToken).WaitAndGetResult(context.OperationContext.UserCancellationToken);


Suggested change

cancellationToken).WaitAndGetResult(context.OperationContext.UserCancellationToken);

cancellationToken).WaitAndGetResult(cancellationToken);

Just to be consistent with which cancellation token is in use. Not that there aren't ways to use different ones, but those are tricky.

jasonmalinowski · 2020-12-17T22:54:55Z

src/EditorFeatures/Core/Implementation/ChangeSignature/AbstractChangeSignatureCommandHandler.cs


-                var finalSolution = result.UpdatedSolution;
+                // UI thread bound operation to show the change signature dialog.
+                var changeSignatureOptions = AbstractChangeSignatureService.GetChangeSignatureOptions(changeSignatureContext);


Does this need to be null checked if the cancel button is clicked?

jasonmalinowski · 2020-12-17T23:22:22Z

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

+            if (context is not ChangeSignatureAnalysisSucceededContext succeededContext)
+            {
+                return null;
+            }


It feels a bit strange that the callers don't just check for the failure and immediately abort.

jasonmalinowski · 2020-12-17T23:26:29Z

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

@@ -81,39 +79,14 @@ internal abstract class AbstractChangeSignatureService : ILanguageService

        public async Task<ImmutableArray<ChangeSignatureCodeAction>> GetChangeSignatureCodeActionAsync(Document document, TextSpan span, CancellationToken cancellationToken)


I admit I'm surprised this function is here versus just being inlined into the actual code fix provider. Not urgent to fix though.

jasonmalinowski · 2020-12-17T23:30:59Z

src/Features/Core/Portable/ChangeSignature/AbstractChangeSignatureService.cs

@@ -248,23 +230,21 @@ internal ChangeSignatureResult ChangeSignatureWithContext(ChangeSignatureAnalysi

 #nullable enable


Is there anything left in this file that isn't #nullable enabled or isn't easily updated?

jasonmalinowski · 2020-12-17T23:31:44Z

src/Features/Core/Portable/ChangeSignature/ChangeSignatureCodeAction.cs

@@ -26,19 +29,19 @@ public ChangeSignatureCodeAction(AbstractChangeSignatureService changeSignatureS
        public override object? GetOptions(CancellationToken cancellationToken)
            => AbstractChangeSignatureService.GetChangeSignatureOptions(_context);

-        protected override Task<IEnumerable<CodeActionOperation>> ComputeOperationsAsync(object options, CancellationToken cancellationToken)
+        protected override async Task<IEnumerable<CodeActionOperation>> ComputeOperationsAsync(object options, CancellationToken cancellationToken)
        {
            if (options is ChangeSignatureOptionsResult changeSignatureOptions && changeSignatureOptions != null)


Suggested change

if (options is ChangeSignatureOptionsResult changeSignatureOptions && changeSignatureOptions != null)

if (options is ChangeSignatureOptionsResult changeSignatureOptions)

If the first test is true, it can't be null.

dibarbet · 2020-12-18T00:08:18Z

merging, will address small feedback in followup

Start making change signature compute operations async

3c9b43c

dibarbet added the Area-IDE label Sep 16, 2020

dibarbet requested a review from CyrusNajmabadi September 16, 2020 23:22

dibarbet requested a review from a team as a code owner September 16, 2020 23:22

dibarbet force-pushed the async_change_sig branch from bf57c7b to bf5ad54 Compare September 16, 2020 23:22

dibarbet commented Sep 16, 2020

View reviewed changes

Move UI notifications out of the change signature service

5144713

dibarbet force-pushed the async_change_sig branch from bf5ad54 to 5144713 Compare September 16, 2020 23:26

davidwengier approved these changes Sep 17, 2020

View reviewed changes

dibarbet added 2 commits September 21, 2020 16:21

Merge remote-tracking branch 'upstream/master' into async_change_sig

4f8e39d

fix nullable annotation

88ae5a2