WIP: Add IDocumentRefactoringService #40173

ryzngard · 2019-12-05T19:56:35Z

IDocumentRefactoringService is intended to allow refactoring behavior
provided by roslyn on documents directly. Currently it has one method,
UpdateAfterInfoChangeAsync, which is intended to update named types
to sync to the document name and location.

Also modify the public rename surface so it tags symbols that are
renamed. This is consumed by VisualStudioWorkspace.TryApplyChanges
to call IVsRefactorNotify for symbols that changed

To do:

IDocumentRefactoringService tests
VisualStudioWorkspace test for refactor notify
Namespace modification

IDocumentRefactoringService is intended to allow refactoring behavior provided by roslyn on documents directly. Currently it has one method, UpdateAfterInfoChangeAsync, which is intended to update named types to sync to the document name and location. Also modify the public rename surface so it tags symbols that are renamed. This is consumed by VisualStudioWorkspace.TryApplyChanges to call IVsRefactorNotify for symbols that changed

src/EditorFeatures/CSharpTest/DocumentRefactoringService/DocumentRefactoringServiceTests.cs

jasonmalinowski

Blocking primarily because it looks like this will break TryApplyChanges if there's an additional file or analyzer config file being updated. But many other questions. I think the high level design of the annotation informing TryApplyChanges seems really solid though, this is just drilling into details.

jasonmalinowski · 2019-12-16T20:40:32Z

src/EditorFeatures/Test2/Rename/RenameEngineResult.vb

+                End If
+            Next
+
+            Assert.True(False, "Did not find any rename annotated nodes")


Suggested change

Assert.True(False, "Did not find any rename annotated nodes")

throw new Exception("Did not find any rename annotated nodes")

This results in less head-scratching when somebody sees an "Expected: true, Actual: false" line which is otherwise pointless.

So, a little rant about this: Assertions throw exceptions from xunit. We have AssertEx.Fail which also will throw an exception. Both will by caught in the Try/catch surrounding this code, which makes sad pandas like me when I'm trying to investigate why a test failed and it's because in the dispose method of something later an assertion failed while in the middle of catching an exception thrown by this code.

TLDR: AssertEx.Fail is probably more correct here? Not really sure. Either way, the message will not be what surfaces when a test fails

src/VisualStudio/Core/Def/Implementation/DocumentRefactoring/DocumentRefactoringService.cs

jasonmalinowski · 2019-12-16T20:43:45Z

src/VisualStudio/Core/Def/Implementation/DocumentRefactoring/DocumentRefactoringService.cs

+{
+    [Export(typeof(DocumentRefactoringService)), PartNotDiscoverable]
+    [Export(typeof(IDocumentRefactoringService))]
+    [ExportWorkspaceService(typeof(IDocumentRefactoringService)), Shared]


Does this need to be a Workspace service or can it be just a straight-up MEF service? I ask only because @tmat is beginning to do some audits of our workspace services for what type they are, and I'm not sure if he has guidance yet on this.

Honestly I'm not sure what the difference will be in accessing it. Straight service should be fine? Would it still be workspace.GetService?

src/VisualStudio/Core/Def/Implementation/DocumentRefactoring/DocumentRefactoringService.cs

jasonmalinowski · 2019-12-16T22:38:32Z

src/VisualStudio/Core/Test/DocumentRefactoring/DocumentRefactoringServiceTests.vb

+                        .DescendantNodes()
+
+            Assert.True(typeDeclarationPairs.Any())
+            Dim matchingTypeDeclarationPair = typeDeclarationPairs.Where(Function(p) String.Equals(p.Item2, Path.GetFileNameWithoutExtension(document.FilePath), StringComparison.OrdinalIgnoreCase))


Are we worried this test is just doing a copy/paste of the same code in the actual feature? Is it actually testing anything then?

Would it be better to just do a markup test of before/after? Might make sense

src/VisualStudio/Core/Test/DocumentRefactoring/DocumentRefactoringServiceTests.vb

jasonmalinowski · 2019-12-16T22:40:24Z

src/Workspaces/Core/Portable/Workspace/Host/DocumentService/IDocumentRefactoringService.cs

+        /// Updates the contents of a document after some document info change, such as file name or path. May
+        /// prompt a user for confirmation on the changes.


Hmm, this is a data API -- it really shouldn't be prompting. (I wish you luck when this is on a server...)

If it needs use input, this seems like the best place for that information to be obtained. How does this directly vary from CodeActionWithOptions or similar?

ping @jasonmalinowski

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

ryzngard · 2019-12-19T19:41:13Z

Marking for personal review while I rip out the old rename code action

…t can be shared

…name_api

ryzngard · 2020-01-09T22:39:22Z

Marking for design review of public interface

ryzngard · 2020-01-09T23:38:52Z

Corresponds to dotnet/project-system#5436

src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs

src/Features/Core/Portable/CodeFixes/NamingStyle/NamingStyleCodeFixProvider.cs

CyrusNajmabadi · 2020-01-10T20:10:43Z

src/Features/Core/Portable/CodeRefactorings/MoveType/AbstractMoveTypeService.cs

-            var editor = Editor.GetEditor(operationKind, (TService)this, state, suggestedFileNames.FirstOrDefault(), cancellationToken);
+            var suggestedFileName = operationKind switch
+            {
+                MoveTypeOperationKind.RenameType => Path.GetFileNameWithoutExtension(document.Name),


slightly confsued . if we're renaming the type... the suggested file name is the name the document already is? can you doc what is going on here?

CyrusNajmabadi · 2020-01-10T20:11:15Z

src/Features/Core/Portable/CodeRefactorings/MoveType/AbstractMoveTypeService.cs

+            var suggestedFileName = operationKind switch
+            {
+                MoveTypeOperationKind.RenameType => Path.GetFileNameWithoutExtension(document.Name),
+                MoveTypeOperationKind.MoveTypeNamespaceScope => document.Name,


also don't understand this. if we're moving the type... why do we just get he doc name, and why aren't ee trimming the extension like above? this needs commenting.

CyrusNajmabadi · 2020-01-10T20:11:28Z

src/Features/Core/Portable/CodeRefactorings/MoveType/AbstractMoveTypeService.cs

+            {
+                MoveTypeOperationKind.RenameType => Path.GetFileNameWithoutExtension(document.Name),
+                MoveTypeOperationKind.MoveTypeNamespaceScope => document.Name,
+                _ => GetSuggestedFileNames(


can we be explicit about what case this is for, instead of _

CyrusNajmabadi · 2020-01-10T20:12:30Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

+                    syntaxFacts.StringComparer);
+
+            if (defaultNamespaceFromProjects.Count != 1
+                || defaultNamespaceFromProjects.First() == null)


the null check seems unnecessary.

CyrusNajmabadi · 2020-01-10T20:13:11Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs

-                var compilation = await document.Project.GetCompilationAsync(cancellationToken).ConfigureAwait(false);
-                var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
+                var compilation = await document.Project.GetRequiredCompilationAsync(cancellationToken).ConfigureAwait(false);
+                var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
                root = addImportService.AddImports(compilation, root, contextLocation, imports, placeSystemNamespaceFirst, cancellationToken);
                document = document.WithSyntaxRoot(root);
            }


i skipped the rest of this file. note: you could consider a separate PR to make all the nullable/GetRequired changes to make this easier to review.

CyrusNajmabadi · 2020-01-10T20:13:39Z

src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs

@@ -51,5 +53,10 @@ internal interface IChangeNamespaceService : ILanguageService
        /// a no-op and original solution will be returned.
        /// </remarks>
        Task<Solution> ChangeNamespaceAsync(Document document, SyntaxNode container, string targetNamespace, CancellationToken cancellationToken);
+
+        /// <summary>
+        /// Gets the target namespace for a given document by using the path and any root namespace 


can you give an example?

can you doc when you would get null back and waht that means?

CyrusNajmabadi · 2020-01-10T20:14:14Z

src/VisualStudio/Core/Def/Implementation/DocumentRefactoring/DocumentRefactoringService.cs

+            return solution;
+        }
+    }
+}


skipped this file for now. i don't understand the design here.

CyrusNajmabadi · 2020-01-10T20:15:21Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

+                {
+                    if (refactorNotifyService.Value.TryOnBeforeGlobalSymbolRenamed(this, changedDocumentIds, oldSymbol, newSymbol.ToDisplayString(), throwOnFailure: false))
+                    {
+                        refactorNotifyService.Value.TryOnAfterGlobalSymbolRenamed(this, changedDocumentIds, oldSymbol, newSymbol.ToDisplayString(), throwOnFailure: false);


same comment as in the test version. this logic seems bizarre to me. what's the point of Before/After when we're not actually doing anything in between?

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

CyrusNajmabadi · 2020-01-10T20:15:46Z

src/VisualStudio/Core/Def/Implementation/ProjectSystem/VisualStudioWorkspaceImpl.cs

+                return;
+            }
+
+            var changedSymbols = RenameSymbolAnnotation.GatherChangedSymbolsInDocumentsAsync(changedDocumentIds, CurrentSolution, oldSolution, cancellationToken).WaitAndGetResult(cancellationToken);


WaitAndGetResult here seems really bad.

src/VisualStudio/Core/Def/Implementation/VisualStudioRenameTrackingDismissService.cs

CyrusNajmabadi · 2020-01-10T20:16:42Z

src/VisualStudio/Core/Def/Implementation/VisualStudioRenameTrackingDismissService.cs

+namespace Microsoft.VisualStudio.LanguageServices.Implementation
+{
+    /// <summary>
+    /// Handles dismissing the rename tracker when a symbol has changed


... i feel like this needs a ton more information. why woudl we need this at all?

src/VisualStudio/Core/Def/Implementation/VisualStudioRenameTrackingDismissService.cs

src/VisualStudio/Core/Test/DocumentRefactoring/DocumentRefactoringServiceTests.vb

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

CyrusNajmabadi · 2020-01-10T20:20:11Z

Definitely need more info here. ..

ryzngard · 2020-01-10T20:59:50Z

@CyrusNajmabadi this is still marked for personal review

CyrusNajmabadi · 2020-01-10T21:00:34Z

Oh. Sorry :) could you potentially use WIP in the titel? github email notifications don't include this info :(

ryzngard · 2020-01-29T19:00:02Z

I've split this work into two PRs:

#41126 for the rename = refactor notify work
#41267 for the new API design

ryzngard added Area-IDE PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. labels Dec 5, 2019

ryzngard force-pushed the prototype/post_rename_api branch from be6691a to 0875e09 Compare December 6, 2019 22:21

ryzngard removed the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 6, 2019

ryzngard marked this pull request as ready for review December 6, 2019 23:57

ryzngard requested a review from a team as a code owner December 6, 2019 23:57

dpoeschl reviewed Dec 9, 2019

View reviewed changes

src/EditorFeatures/CSharpTest/DocumentRefactoringService/DocumentRefactoringServiceTests.cs Outdated Show resolved Hide resolved

ryzngard added 5 commits December 9, 2019 18:34

Fix tests so MEF export works correctly. Remove casing constraint

a71edfc

Assert single type matches the file name

34f7d30

Add more tests to verify rename still happens in various circumstances

3037973

More test stuff

177f983

Change to notempty assert

396cebf

jasonmalinowski requested changes Dec 16, 2019

View reviewed changes

ryzngard added 3 commits December 18, 2019 10:43

Feedback. Update tests to actually test changes...

02c763a

Add namespace implementation and cleanup some code

6cc09bc

Remove symbol renamed code action

7510ea2

ryzngard added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Dec 19, 2019

ryzngard added 6 commits December 19, 2019 13:42

WIP: Move gathering of changed symbols to RenameSymbolAnnotation so i…

5556137

…t can be shared

Get MoveToNamespace tests passing

4e5021a

Merge remote-tracking branch 'upstream/master' into prototype/post_re…

451835c

…name_api

Fix nullable semantic model usage after merge

6e2522d

Undo an unintentional change

b0f3b3a

Add check for ShouldAnnotateSymbol

4134d4d

ryzngard added the Need Design Review The end user experience design needs to be reviewed and approved. label Jan 9, 2020