Merge pull request #40018 from genlu/FixChangeNamespace

Fix move to namespace refactoring

src/EditorFeatures/CSharpTest/MoveToNamespace/MoveToNamespaceTests.cs

@@ -7,6 +7,7 @@
 using Microsoft.CodeAnalysis.Editor.UnitTests;
 using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
 using Microsoft.CodeAnalysis.MoveToNamespace;
+using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Test.Utilities;
 using Microsoft.CodeAnalysis.Test.Utilities.MoveToNamespace;
 using Microsoft.VisualStudio.Composition;
@@ -1226,5 +1227,67 @@ partial class MyClass
 {
 }",
     expectedSuccess: false);
+
+        [Fact, Trait(Traits.Feature, Traits.Features.MoveToNamespace)]
+        [WorkItem(39234, "https://github.com/dotnet/roslyn/issues/39234")]
+        public async Task TestMultiTargetingProject()
+        {
+            // Create two projects with same project file path and single linked document to simulate a multi-targeting project.
+            var input = @"<Workspace>
+    <Project Language=""C#"" CommonReferences=""true"" AssemblyName=""Proj1"" FilePath=""SharedProj.csproj"">
+        <Document FilePath=""CurrentDocument.cs"">
+namespace A
+{
+    public class Class1
+    {
+    }
+
+    public class Class2[||]
+    {
+    }
+}
+        </Document>
+    </Project>
+    <Project Language=""C#"" CommonReferences=""true"" AssemblyName=""Proj2"" FilePath=""SharedProj.csproj"">
+        <Document IsLinkFile=""true"" LinkAssemblyName=""Proj1"" LinkFilePath=""CurrentDocument.cs""/>
+    </Project>
+</Workspace>";
+
+            var expected =
+@"namespace A
+{
+    public class Class1
+    {
+    }
+}
+
+namespace B
+{
+    public class Class2
+    {
+    }
+}";
+            using var workspace = TestWorkspace.Create(System.Xml.Linq.XElement.Parse(input), exportProvider: ExportProviderFactory.CreateExportProvider());
+
+            // Set the target namespace to "B"
+            var testDocument = workspace.Projects.Single(p => p.Name == "Proj1").Documents.Single();
+            var document = workspace.CurrentSolution.GetDocument(testDocument.Id);
+            var movenamespaceService = document.GetLanguageService<IMoveToNamespaceService>();
+            var moveToNamespaceOptions = new MoveToNamespaceOptionsResult("B");
+            ((TestMoveToNamespaceOptionsService)movenamespaceService.OptionsService).SetOptions(moveToNamespaceOptions);
+
+            var (_, action) = await GetCodeActionsAsync(workspace, default);
+            var operations = await VerifyActionAndGetOperationsAsync(workspace, action, default);
+            var result = ApplyOperationsAndGetSolution(workspace, operations);
+
+            // Make sure both linked documents are changed.
+            foreach (var id in workspace.Documents.Select(d => d.Id))
+            {
+                var changedDocument = result.Item2.GetDocument(id);
+                var changedRoot = await changedDocument.GetSyntaxRootAsync();
+                var actualText = changedRoot.ToFullString();
+                Assert.Equal(expected, actualText);
+            }
+        }
     }
 }

src/EditorFeatures/TestUtilities/MoveToNamespace/AbstractMoveToNamespaceTests.cs

@@ -83,7 +83,6 @@ protected override CodeRefactoringProvider CreateCodeRefactoringProvider(Workspa
                             checkedCodeActions.Add(codeAction);
                         }
                     }
-
                 }
 
                 if (!optionCancelled && !string.IsNullOrEmpty(targetNamespace))

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

@@ -99,11 +99,11 @@ protected override SyntaxList<MemberDeclarationSyntax> GetMemberDeclarationsInCo
         }
 
         /// <summary>
-        /// Try to get a new node to replace given node, which is a reference to a top-level type declared inside the namespce to be changed.
+        /// Try to get a new node to replace given node, which is a reference to a top-level type declared inside the namespace to be changed.
         /// If this reference is the right side of a qualified name, the new node returned would be the entire qualified name. Depends on 
         /// whether <paramref name="newNamespaceParts"/> is provided, the name in the new node might be qualified with this new namespace instead.
         /// </summary>
-        /// <param name="reference">A reference to a type declared inside the namespce to be changed, which is calculated based on results from 
+        /// <param name="reference">A reference to a type declared inside the namespace to be changed, which is calculated based on results from 
         /// `SymbolFinder.FindReferencesAsync`.</param>
         /// <param name="newNamespaceParts">If specified, and the reference is qualified with namespace, the namespace part of original reference 
         /// will be replaced with given namespace in the new node.</param>
@@ -275,7 +275,7 @@ private static CompilationUnitSyntax MoveMembersFromNamespaceToGlobal(Compilatio
             var eofToken = root.EndOfFileToken
                 .WithAdditionalAnnotations(WarningAnnotation);
 
-            // Try to preserve trivia from original namesapce declaration.
+            // Try to preserve trivia from original namespace declaration.
             // If there's any member inside the declaration, we attach them to the 
             // first and last member, otherwise, simply attach all to the EOF token.
             if (members.Count > 0)
@@ -362,6 +362,11 @@ private static CompilationUnitSyntax MoveMembersFromGlobalToNamespace(Compilatio
             {
                 // Otherwise, the span should contain a namespace declaration node, which must be the only one
                 // in the entire syntax spine to enable the change namespace operation.
+                if (!compilationUnit.Span.Contains(span))
+                {
+                    return null;
+                }
+
                 var node = compilationUnit.FindNode(span, getInnermostNodeForTie: true);
 
                 var namespaceDecl = node.AncestorsAndSelf().OfType<NamespaceDeclarationSyntax>().SingleOrDefault();

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

@@ -33,11 +33,11 @@ internal abstract class AbstractChangeNamespaceService : IChangeNamespaceService
 
         /// <summary>
         /// Try to get a new node to replace given node, which is a reference to a top-level type declared inside the 
-        /// namespce to be changed. If this reference is the right side of a qualified name, the new node returned would
+        /// namespace to be changed. If this reference is the right side of a qualified name, the new node returned would
         /// be the entire qualified name. Depends on whether <paramref name="newNamespaceParts"/> is provided, the name 
         /// in the new node might be qualified with this new namespace instead.
         /// </summary>
-        /// <param name="reference">A reference to a type declared inside the namespce to be changed, which is calculated 
+        /// <param name="reference">A reference to a type declared inside the namespace to be changed, which is calculated 
         /// based on results from `SymbolFinder.FindReferencesAsync`.</param>
         /// <param name="newNamespaceParts">If specified, the namespace of original reference will be replaced with given 
         /// namespace in the replacement node.</param>
@@ -164,7 +164,7 @@ await ChangeNamespaceInSingleDocumentAsync(solutionAfterNamespaceChange, documen
 
                 // After changing documents, we still need to remove unnecessary imports related to our change.
                 // We don't try to remove all imports that might become unnecessary/invalid after the namespace change, 
-                // just ones that fully matche the old/new namespace. Because it's hard to get it right and will almost 
+                // just ones that fully match the old/new namespace. Because it's hard to get it right and will almost 
                 // certainly cause perf issue.
                 // For example, if we are changing namespace `Foo.Bar` (which is the only namespace declaration with such name)
                 // to `A.B`, the using of name `Bar` in a different file below would remain untouched, even it's no longer valid:
@@ -287,7 +287,7 @@ protected static bool IsSupportedLinkedDocument(Document document, out Immutable
 
             // TODO: figure out how to properly determine if and how a document is linked using project system.
 
-            // If we found a linked document which is part of a project with differenct project file,
+            // If we found a linked document which is part of a project with different project file,
             // then it's an actual linked file (i.e. not a multi-targeting project). We don't support that for now.
             if (linkedDocumentIds.Any(id =>
                     !PathUtilities.PathsEqual(solution.GetDocument(id).Project.FilePath, document.Project.FilePath)))

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

@@ -36,7 +36,7 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
             {
                 // These code actions try to move file to a new location based on declared namespace
                 // and the default namespace of the project. The new location is a list of folders
-                // determined by the relateive part of the declared namespace compare to the default namespace.
+                // determined by the relative part of the declared namespace compare to the default namespace.
                 // 
                 // For example, if he default namespace is `A.B.C`, file path is 
                 // "[project root dir]\Class1.cs" and declared namespace in the file is
@@ -81,7 +81,7 @@ public override async Task ComputeRefactoringsAsync(CodeRefactoringContext conte
         /// Try to get the node that can be used to trigger the refactoring based on current cursor position. 
         /// </summary>
         /// <returns>
-        /// (1) a node of type <typeparamref name="TNamespaceDeclarationSyntax"/> node, if curosr in the name and it's the 
+        /// (1) a node of type <typeparamref name="TNamespaceDeclarationSyntax"/> node, if cursor in the name and it's the 
         /// only namespace declaration in the document.
         /// (2) a node of type <typeparamref name="TCompilationUnitSyntax"/> node, if the cursor is in the name of first 
         /// declaration in global namespace and there's no namespace declaration in this document.

src/Features/Core/Portable/MoveToNamespace/AbstractMoveToNamespaceService.cs

@@ -9,6 +9,7 @@
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.ChangeNamespace;
 using Microsoft.CodeAnalysis.CodeRefactorings.MoveType;
+using Microsoft.CodeAnalysis.Formatting;
 using Microsoft.CodeAnalysis.Host;
 using Microsoft.CodeAnalysis.LanguageServices;
 using Microsoft.CodeAnalysis.Shared.Extensions;
@@ -93,7 +94,7 @@ protected AbstractMoveToNamespaceService(IMoveToNamespaceOptionsService moveToNa
                 return null;
             }
 
-            // The underlying ChangeNamespace service doesn't support nested namespace decalration.
+            // The underlying ChangeNamespace service doesn't support nested namespace declaration.
             if (GetNamespaceInSpineCount(declarationSyntax) == 1)
             {
                 var changeNamespaceService = document.GetLanguageService<IChangeNamespaceService>();
@@ -252,11 +253,16 @@ private static async Task<ImmutableArray<ISymbol>> GetMemberSymbolsAsync(Documen
                 moveSpan,
                 MoveTypeOperationKind.MoveTypeNamespaceScope,
                 cancellationToken).ConfigureAwait(false);
-
             var modifiedDocument = modifiedSolution.GetDocument(document.Id);
-            var syntaxRoot = await modifiedDocument.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
 
+            // Since MoveTypeService doesn't handle linked files, we need to merge the diff ourselves, 
+            // otherwise, we will end up with multiple linked documents with different content.
+            var mergedSolution = await PropagateChangeToLinkedDocuments(modifiedDocument, cancellationToken).ConfigureAwait(false);
+            var mergedDocument = mergedSolution.GetDocument(document.Id);
+
+            var syntaxRoot = await mergedDocument.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
             var syntaxNode = syntaxRoot.GetAnnotatedNodes(AbstractMoveTypeService.NamespaceScopeMovedAnnotation).SingleOrDefault();
+
             if (syntaxNode == null)
             {
                 // The type might be declared in global namespace
@@ -264,12 +270,27 @@ private static async Task<ImmutableArray<ISymbol>> GetMemberSymbolsAsync(Documen
             }
 
             return await MoveItemsInNamespaceAsync(
-                modifiedDocument,
+                mergedDocument,
                 syntaxNode,
                 targetNamespace,
                 cancellationToken).ConfigureAwait(false);
         }
 
+        private static async Task<Solution> PropagateChangeToLinkedDocuments(Document document, CancellationToken cancellationToken)
+        {
+            // Need to make sure elastic trivia is formatted properly before pushing the text to other documents.
+            var formattedDocument = await Formatter.FormatAsync(document, SyntaxAnnotation.ElasticAnnotation, cancellationToken: cancellationToken).ConfigureAwait(false);
+            var formattedText = await formattedDocument.GetTextAsync(cancellationToken).ConfigureAwait(false);
+            var solution = formattedDocument.Project.Solution;
+
+            foreach (var documentId in formattedDocument.GetLinkedDocumentIds())
+            {
+                solution = solution.WithDocumentText(documentId, formattedText);
+            }
+
+            return solution;
+        }
+
         private static string GetNewSymbolName(ISymbol symbol, string targetNamespace)
         {
             Debug.Assert(symbol != null && !string.IsNullOrEmpty(targetNamespace));

src/Workspaces/CoreTestUtilities/MEF/ExportProviderCache.cs

@@ -275,7 +275,7 @@ private static void RequireForSingleExportProvider(bool condition)
                     //   method attempting to create a default ExportProvider which did not match the one assigned to
                     //   the test.
                     // * A test attempted to perform multiple test sequences in the context of a single test method,
-                    //   rather than break up the test into distict tests for each case.
+                    //   rather than break up the test into distinct tests for each case.
                     // * A test referenced different predefined ExportProvider instances within the context of a test.
                     //   Each test is expected to use the same ExportProvider throughout the test.
                     throw new InvalidOperationException($"Only one {nameof(ExportProvider)} can be created in the context of a single test.");