Rename API handles refactor notify #41126

ryzngard · 2020-01-21T22:30:30Z

This change makes it so that the rename API we surface pubilicly marks symbol definitions with an annotation.

An editor workspace implementation can use those annotations as hints to what has been refactored, and handle these changes if needed. For the case of VS, that includes calling IVsRefactorNotify on TryApplyChanges

This allows IVsRefactorNotify to be handled automatically when calling Solution.TryApplyChanges

jasonmalinowski

Overall the direction here looks great. A few general thoughts:

It'd be nice if the various code action tests just tested the existence of the annotation, and we had a single test that tested that the Workspace does the right thing when it encouters it; this keeps all the individual features from having to set up the refactor notify stuff directly.
We may need to refactor the stuff into TryApplyChanges better -- right now there's duplicate code between TestWorkspace and VisualStudioWorkspace. It also looks like there's a bug in the VisualStudioWorkspace variant that might prevent it from working, so that's worrisome -- the unit tests may pass but it'll fail in the actual product.
We might be able to clean up more in Rename tracking too -- that also is directly invoking IRefactorNotify which might now be unnecessary. It actually might mean we're firing the events twice now.
Ditto for ContainedLanguageCodeSupport.cs which isn't here: that's also firing events directly. At this point with this PR I'd guess you can just search on IRefactorNotifySymbol and only see the workspace stuff directly interacting with it -- anybody else still firing it manually is probably a duplicate invocation.

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/NamingStylesTests.cs

jasonmalinowski · 2020-02-06T00:50:09Z

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/NamingStylesTests.cs

@@ -1217,17 +1231,42 @@ public async Task TestRefactorNotify()
            var testParameters = new TestParameters(options: options.ClassNamesArePascalCase);

            using var workspace = CreateWorkspaceFromOptions(markup, testParameters);
+
+            var refactorNotify = workspace.GetService<IRefactorNotifyService>() as TestRefactorNotify;


At this point, is there some way to make these tests not have to grovel with IRefactorNotifyService and just have a helper that asserts the rename annotation was added? And have a standalone test that does all the refactor notify stuff directly?

jasonmalinowski · 2020-02-06T00:50:38Z

src/EditorFeatures/CSharpTest/MoveToNamespace/MoveToNamespaceTests.cs

@@ -21,7 +22,7 @@ public class MoveToNamespaceTests : AbstractMoveToNamespaceTests
    {
        private static readonly IExportProviderFactory ExportProviderFactory =
            ExportProviderCache.GetOrCreateExportProviderFactory(
-                TestExportProvider.EntireAssemblyCatalogWithCSharpAndVisualBasic.WithPart(typeof(TestMoveToNamespaceOptionsService)));
+                TestExportProvider.EntireAssemblyCatalogWithCSharpAndVisualBasic.WithPart(typeof(TestMoveToNamespaceOptionsService)).WithPart(typeof(TestRefactorNotify)));


Why? What happens if it's not there?

src/Workspaces/Core/Portable/Rename/RenameSymbolAnnotation.cs

jasonmalinowski · 2020-02-06T01:27:17Z

src/Workspaces/Core/Portable/Rename/RenameSymbolAnnotation.cs

+                        var currentSemanticModel = await oldDocument.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
+
+                        var oldSymbol = ResolveSymbol(annotation, currentSemanticModel.Compilation);
+                        var newSymbol = newSemanticModel.GetDeclaredSymbol(node);


GetDeclaredSymbol could return null, why did it let you pass that to changedSymbols.Add?

It can, but shouldn't in this case right? Assuming we're always annotating something that declares a symbol (which is the useful point of this). Maybe just add a contract throw?

I'm more worried why the compiler didn't yell at you. Is this a bug in nullable somewhere?

hmm... the extension method is annotated as return a non-null value. Potentially by design but not desired...

SemanticModel.GetDeclaredSymbolForNode might be annotated wrong?

src/Workspaces/Core/Portable/Rename/RenameSymbolAnnotation.cs

src/EditorFeatures/CSharpTest/Diagnostics/NamingStyles/NamingStylesTests.cs

jasonmalinowski · 2020-02-06T01:46:46Z

Tagging @dpoeschl to also look at this, since he has background in both rename tracking and IRefactorNotifyService; there might be some other nice cleanups that happen too. I could also imagine things breaking too: if the rename tracking is now running through the common code, that might also trigger the dismissal of the session in some way that might interact poorly.

ryzngard · 2020-02-06T20:26:45Z

It'd be nice if the various code action tests just tested the existence of the annotation, and we had a single test that tested that the Workspace does the right thing when it encouters it; this keeps all the individual features from having to set up the refactor notify stuff directly.

Agreed. I'll work on a test helper for this. I think having a mock refactornotify test is worthwhile still, but more general use would be to check annotation.

We may need to refactor the stuff into TryApplyChanges better -- right now there's duplicate code between TestWorkspace and VisualStudioWorkspace. It also looks like there's a bug in the VisualStudioWorkspace variant that might prevent it from working, so that's worrisome -- the unit tests may pass but it'll fail in the actual product.

hmm... maybe good chance to add more integration tests?

We might be able to clean up more in Rename tracking too -- that also is directly invoking IRefactorNotify which might now be unnecessary. It actually might mean we're firing the events twice now.

Same as above :)

Ditto for ContainedLanguageCodeSupport.cs which isn't here: that's also firing events directly. At this point with this PR I'd guess you can just search on IRefactorNotifySymbol and only see the workspace stuff directly interacting with it -- anybody else still firing it manually is probably a duplicate invocation.

I'll dig into these implementations. Having multiple IRefactorNotifySymbol is not necessarily bad, but they shouldn't all be calling into the VS layer. We should have 1 per endpoint that needs to be called.

CyrusNajmabadi · 2020-06-04T18:46:47Z

@ryzngard What would you like to do with this PR?

Modify rename to always annotate symbol declarations that were renamed.

17c380e

This allows IVsRefactorNotify to be handled automatically when calling Solution.TryApplyChanges

ryzngard force-pushed the rename_does_refactor_notify branch from 4abad93 to 17c380e Compare January 23, 2020 00:36

ryzngard marked this pull request as ready for review January 28, 2020 18:52

ryzngard requested a review from a team as a code owner January 28, 2020 18:52

ryzngard requested review from CyrusNajmabadi and jasonmalinowski January 28, 2020 18:52

jinujoseph added the Area-IDE label Jan 28, 2020

ryzngard mentioned this pull request Jan 29, 2020

WIP: Add IDocumentRefactoringService #40173

Closed

3 tasks

jasonmalinowski changed the base branch from master to release/dev16.6-preview1 January 31, 2020 21:08

jasonmalinowski reviewed Feb 6, 2020

View reviewed changes

ryzngard added 2 commits February 6, 2020 11:51

Merge branch 'master' into rename_does_refactor_notify

c6601ee

Small cleanups from PR feedback

fb201dd

More missed cleanup places

181f210

ryzngard requested review from a team as code owners February 6, 2020 20:35

Merge branch 'master' into rename_does_refactor_notify

94de5bf

ryzngard requested a review from a team as a code owner February 6, 2020 20:36

ryzngard changed the base branch from release/dev16.6-preview1 to master February 6, 2020 20:37

ryzngard and others added 7 commits February 6, 2020 13:20

Use GetRequiredDocument

9f4c76e

Update header to MIT license

14b6c26

Cleanup a lot of uses of IRefactorNotifyService still lingering

de7f62a

Remove more refactorNotify code

718ed6b

Fix duplicate annotation

3efc4d0

Merge branch 'master' into rename_does_refactor_notify

218545d

Nullability fixes

2f1bcb4

ryzngard closed this Sep 9, 2020

ryzngard deleted the rename_does_refactor_notify branch September 9, 2020 20:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename API handles refactor notify #41126

Rename API handles refactor notify #41126

ryzngard commented Jan 21, 2020 •

edited

Loading

jasonmalinowski left a comment

jasonmalinowski Feb 6, 2020

jasonmalinowski Feb 6, 2020

jasonmalinowski Feb 6, 2020

ryzngard Feb 6, 2020

jasonmalinowski Feb 6, 2020

ryzngard Feb 6, 2020

jasonmalinowski commented Feb 6, 2020

ryzngard commented Feb 6, 2020

CyrusNajmabadi commented Jun 4, 2020

Rename API handles refactor notify #41126

Rename API handles refactor notify #41126

Conversation

ryzngard commented Jan 21, 2020 • edited Loading

jasonmalinowski left a comment

Choose a reason for hiding this comment

jasonmalinowski Feb 6, 2020

Choose a reason for hiding this comment

jasonmalinowski Feb 6, 2020

Choose a reason for hiding this comment

jasonmalinowski Feb 6, 2020

Choose a reason for hiding this comment

ryzngard Feb 6, 2020

Choose a reason for hiding this comment

jasonmalinowski Feb 6, 2020

Choose a reason for hiding this comment

ryzngard Feb 6, 2020

Choose a reason for hiding this comment

jasonmalinowski commented Feb 6, 2020

ryzngard commented Feb 6, 2020

CyrusNajmabadi commented Jun 4, 2020

ryzngard commented Jan 21, 2020 •

edited

Loading