Do not make unnecessarily simplification changes in sync-namespace. (#78969)

Author: CyrusNajmabadi

src/EditorFeatures/CSharpTest/CodeActions/SyncNamespace/SyncNamespaceTests_ChangeNamespace.cs

@@ -1448,6 +1448,72 @@ class Class1
         await TestChangeNamespaceAsync(code, expectedSourceOriginal);
     }
 
+    [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/34707")]
+    public async Task ChangeFromGlobalNamespace_DoNotSimplifyUnrelatedCode()
+    {
+        var defaultNamespace = "A";
+        var (folder, filePath) = CreateDocumentFilePath(["B", "C"], "File1.cs");
+        var documentPath2 = CreateDocumentFilePath([], "File2.cs");
+        var code =
+            $$"""
+            <Workspace>
+                <Project Language="C#" AssemblyName="Assembly1" FilePath="{{ProjectFilePath}}" RootNamespace="{{defaultNamespace}}" CommonReferences="true">
+                    <Document Folders="{{folder}}" FilePath="{{filePath}}"> 
+            class [||]Class1 
+            { 
+                private A.Class2 c2;
+                private A.B.Class3 c3;
+                private A.B.C.Class4 c4;
+
+                void M1()
+                {
+                    int i = 0;
+                    // This cast should not be touched.
+                    int j = (int)i;
+                }
+            }</Document>
+
+            <Document Folders="{{documentPath2.folder}}" FilePath="{{documentPath2.filePath}}"> 
+            namespace A
+            {
+                class Class2{}
+
+                namespace B
+                {
+                    class Class3 {}
+
+                    namespace C
+                    {
+                        class Class4 {}    
+                    }
+                }
+            }</Document>
+                </Project>
+            </Workspace>
+            """;
+
+        var expectedSourceOriginal =
+            """
+            namespace A.B.C
+            {
+                class Class1
+                {
+                    private Class2 c2;
+                    private Class3 c3;
+                    private Class4 c4;
+            
+                    void M1()
+                    {
+                        int i = 0;
+                        // This cast should not be touched.
+                        int j = (int)i;
+                    }
+                }
+            }
+            """;
+        await TestChangeNamespaceAsync(code, expectedSourceOriginal);
+    }
+
     [Fact]
     public async Task ChangeFromGlobalNamespace_ChangeUsingsInMultipleContainers()
     {
@@ -2169,7 +2235,7 @@ namespace {{defaultNamespace}}
             {
                 public static class Extensions
                 {
-                    public static bool Foo(this string s) => true;
+                    public static bool Foo(this String s) => true;
                 }
             }
             """;

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

@@ -13,11 +13,13 @@
 using Microsoft.CodeAnalysis.ChangeNamespace;
 using Microsoft.CodeAnalysis.CSharp.CodeGeneration;
 using Microsoft.CodeAnalysis.CSharp.Extensions;
+using Microsoft.CodeAnalysis.CSharp.Simplification;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Host.Mef;
 using Microsoft.CodeAnalysis.LanguageService;
 using Microsoft.CodeAnalysis.PooledObjects;
 using Microsoft.CodeAnalysis.Shared.Extensions;
+using Microsoft.CodeAnalysis.Simplification;
 using Microsoft.CodeAnalysis.Text;
 using Roslyn.Utilities;
 
@@ -27,14 +29,18 @@ namespace Microsoft.CodeAnalysis.CSharp.ChangeNamespace;
 using static SyntaxFactory;
 
 [ExportLanguageService(typeof(IChangeNamespaceService), LanguageNames.CSharp), Shared]
-internal sealed class CSharpChangeNamespaceService :
-    AbstractChangeNamespaceService<BaseNamespaceDeclarationSyntax, CompilationUnitSyntax, MemberDeclarationSyntax>
+[method: ImportingConstructor]
+[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
+internal sealed class CSharpChangeNamespaceService() :
+    AbstractChangeNamespaceService<
+        CompilationUnitSyntax,
+        MemberDeclarationSyntax,
+        BaseNamespaceDeclarationSyntax,
+        NameSyntax,
+        SimpleNameSyntax,
+        QualifiedCrefSyntax>
 {
-    [ImportingConstructor]
-    [Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
-    public CSharpChangeNamespaceService()
-    {
-    }
+    public override AbstractReducer NameReducer { get; } = new CSharpNameReducer();
 
     protected override async Task<ImmutableArray<(DocumentId, SyntaxNode)>> GetValidContainersFromAllLinkedDocumentsAsync(
         Document document,

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

@@ -17,15 +17,11 @@
 namespace Microsoft.CodeAnalysis.CSharp.CodeRefactorings.SyncNamespace;
 
 [ExportCodeRefactoringProvider(LanguageNames.CSharp, Name = PredefinedCodeRefactoringProviderNames.SyncNamespace), Shared]
-internal sealed class CSharpSyncNamespaceCodeRefactoringProvider
-    : AbstractSyncNamespaceCodeRefactoringProvider<BaseNamespaceDeclarationSyntax, CompilationUnitSyntax, MemberDeclarationSyntax>
+[method: ImportingConstructor]
+[method: SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
+internal sealed class CSharpSyncNamespaceCodeRefactoringProvider()
+        : AbstractSyncNamespaceCodeRefactoringProvider<BaseNamespaceDeclarationSyntax, CompilationUnitSyntax, MemberDeclarationSyntax>
 {
-    [ImportingConstructor]
-    [SuppressMessage("RoslynDiagnosticsReliability", "RS0033:Importing constructor should be [Obsolete]", Justification = "Used in test code: https://github.com/dotnet/roslyn/issues/42814")]
-    public CSharpSyncNamespaceCodeRefactoringProvider()
-    {
-    }
-
     protected override async Task<SyntaxNode?> TryGetApplicableInvocationNodeAsync(Document document, TextSpan span, CancellationToken cancellationToken)
     {
         if (!span.IsEmpty)
@@ -58,7 +54,4 @@ public CSharpSyncNamespaceCodeRefactoringProvider()
 
         return null;
     }
-
-    protected override string EscapeIdentifier(string identifier)
-        => identifier.EscapeIdentifier();
 }

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

@@ -34,14 +34,16 @@ namespace Microsoft.CodeAnalysis.ChangeNamespace;
 /// <summary>
 /// This intermediate class is used to hide method `TryGetReplacementReferenceSyntax` from <see cref="IChangeNamespaceService" />.
 /// </summary>
-internal abstract class AbstractChangeNamespaceService : IChangeNamespaceService
+internal abstract partial class AbstractChangeNamespaceService : IChangeNamespaceService
 {
     public abstract Task<bool> CanChangeNamespaceAsync(Document document, SyntaxNode container, CancellationToken cancellationToken);
 
     public abstract Task<Solution> ChangeNamespaceAsync(Document document, SyntaxNode container, string targetNamespace, CancellationToken cancellationToken);
 
     public abstract Task<Solution?> TryChangeTopLevelNamespacesAsync(Document document, string targetNamespace, CancellationToken cancellationToken);
 
+    public abstract AbstractReducer NameReducer { get; }
+
     /// <summary>
     /// 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
@@ -57,11 +59,20 @@ internal abstract class AbstractChangeNamespaceService : IChangeNamespaceService
     public abstract bool TryGetReplacementReferenceSyntax(SyntaxNode reference, ImmutableArray<string> newNamespaceParts, ISyntaxFactsService syntaxFacts, [NotNullWhen(returnValue: true)] out SyntaxNode? old, [NotNullWhen(returnValue: true)] out SyntaxNode? @new);
 }
 
-internal abstract class AbstractChangeNamespaceService<TNamespaceDeclarationSyntax, TCompilationUnitSyntax, TMemberDeclarationSyntax>
+internal abstract partial class AbstractChangeNamespaceService<
+    TCompilationUnitSyntax,
+    TMemberDeclarationSyntax,
+    TNamespaceDeclarationSyntax,
+    TNameSyntax,
+    TSimpleNameSyntax,
+    TCrefSyntax>
     : AbstractChangeNamespaceService
-    where TNamespaceDeclarationSyntax : SyntaxNode
     where TCompilationUnitSyntax : SyntaxNode
     where TMemberDeclarationSyntax : SyntaxNode
+    where TNamespaceDeclarationSyntax : TMemberDeclarationSyntax
+    where TNameSyntax : SyntaxNode
+    where TSimpleNameSyntax : TNameSyntax
+    where TCrefSyntax : SyntaxNode
 {
     private static readonly char[] s_dotSeparator = ['.'];
 
@@ -125,9 +136,7 @@ public override async Task<bool> CanChangeNamespaceAsync(Document document, Synt
         var originalNamespaceDeclarations = await GetTopLevelNamespacesAsync(document, cancellationToken).ConfigureAwait(false);
 
         if (originalNamespaceDeclarations.Length == 0)
-        {
             return null;
-        }
 
         var semanticModel = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
         var originalNamespaceName = semanticModel.GetRequiredDeclaredSymbol(originalNamespaceDeclarations.First(), cancellationToken).ToDisplayString();
@@ -139,12 +148,10 @@ public override async Task<bool> CanChangeNamespaceAsync(Document document, Synt
         for (var i = 0; i < originalNamespaceDeclarations.Length; i++)
         {
             var namespaceName = semanticModel.GetRequiredDeclaredSymbol(originalNamespaceDeclarations[i], cancellationToken).ToDisplayString();
+
+            // Skip all namespaces that didn't match the original namespace name that we were syncing. 
             if (namespaceName != originalNamespaceName)
-            {
-                // Skip all namespaces that didn't match the original namespace name that 
-                // we were syncing. 
                 continue;
-            }
 
             syntaxRoot = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
 
@@ -194,16 +201,12 @@ public override async Task<Solution> ChangeNamespaceAsync(
 
         var containersFromAllDocuments = await GetValidContainersFromAllLinkedDocumentsAsync(document, container, cancellationToken).ConfigureAwait(false);
         if (containersFromAllDocuments.IsDefault)
-        {
             return solution;
-        }
 
         // No action required if declared namespace already matches target.
         var declaredNamespace = GetDeclaredNamespace(container);
         if (syntaxFacts.StringComparer.Equals(targetNamespace, declaredNamespace))
-        {
             return solution;
-        }
 
         // Annotate the container nodes so we can still find and modify them after syntax tree has changed.
         var annotatedSolution = await AnnotateContainersAsync(solution, containersFromAllDocuments, cancellationToken).ConfigureAwait(false);
@@ -222,9 +225,8 @@ public override async Task<Solution> ChangeNamespaceAsync(
 
         foreach (var documentId in documentIds)
         {
-            var (newSolution, refDocumentIds) =
-                await ChangeNamespaceInSingleDocumentAsync(solutionAfterNamespaceChange, documentId, declaredNamespace, targetNamespace, cancellationToken)
-                    .ConfigureAwait(false);
+            var (newSolution, refDocumentIds) = await ChangeNamespaceInSingleDocumentAsync(
+                solutionAfterNamespaceChange, documentId, declaredNamespace, targetNamespace, cancellationToken).ConfigureAwait(false);
             solutionAfterNamespaceChange = newSolution;
             referenceDocuments.AddRange(refDocumentIds);
         }
@@ -463,8 +465,8 @@ private static SyntaxNode CreateImport(SyntaxGenerator syntaxGenerator, string n
             }
         }
 
-        var documentWithNewNamespace = await FixDeclarationDocumentAsync(document, refLocationsInCurrentDocument, oldNamespace, newNamespace, cancellationToken)
-            .ConfigureAwait(false);
+        var documentWithNewNamespace = await FixDeclarationDocumentAsync(
+            document, refLocationsInCurrentDocument, oldNamespace, newNamespace, cancellationToken).ConfigureAwait(false);
         var solutionWithChangedNamespace = documentWithNewNamespace.Project.Solution;
 
         var refLocationsInSolution = refLocationsInOtherDocuments
@@ -501,15 +503,6 @@ private static SyntaxNode CreateImport(SyntaxGenerator syntaxGenerator, string n
         return (solutionWithFixedReferences, refLocationGroups.SelectAsArray(g => g.Key));
     }
 
-    private readonly struct LocationForAffectedSymbol(ReferenceLocation location, bool isReferenceToExtensionMethod)
-    {
-        public ReferenceLocation ReferenceLocation { get; } = location;
-
-        public bool IsReferenceToExtensionMethod { get; } = isReferenceToExtensionMethod;
-
-        public Document Document => ReferenceLocation.Document;
-    }
-
     private static async Task<ImmutableArray<LocationForAffectedSymbol>> FindReferenceLocationsForSymbolAsync(
         Document document, ISymbol symbol, CancellationToken cancellationToken)
     {
@@ -631,9 +624,49 @@ private async Task<Document> FixDeclarationDocumentAsync(
         var services = documentWithAddedImports.Project.Solution.Services;
         root = Formatter.Format(root, Formatter.Annotation, services, documentOptions.FormattingOptions, cancellationToken);
 
-        root = root.WithAdditionalAnnotations(Simplifier.Annotation);
+        using var _ = PooledHashSet<string>.GetInstance(out var allNamespaceNameParts);
+        allNamespaceNameParts.AddRange(oldNamespaceParts);
+        allNamespaceNameParts.AddRange(newNamespaceParts);
+
+        var syntaxFacts = document.GetRequiredLanguageService<ISyntaxFactsService>();
+        root = AddSimplifierAnnotationToPotentialReferences(syntaxFacts, root, allNamespaceNameParts);
+
         var formattedDocument = documentWithAddedImports.WithSyntaxRoot(root);
-        return await Simplifier.ReduceAsync(formattedDocument, documentOptions.SimplifierOptions, cancellationToken).ConfigureAwait(false);
+        return await SimplifyTypeNamesAsync(formattedDocument, documentOptions, cancellationToken).ConfigureAwait(false);
+    }
+
+    private static SyntaxNode AddSimplifierAnnotationToPotentialReferences(
+        ISyntaxFactsService syntaxFacts, SyntaxNode root, HashSet<string> allNamespaceNameParts)
+    {
+        // Find all identifiers in this tree that use at least one of the namespace names of either the old or new
+        // namespace.  Mark those as needing potential complexification/simplification to preserve meaning.
+        //
+        // Note: we could go further here and actually bind these nodes to make sure they are actually references
+        // to one of the namespaces in question.  But that doesn't seem super necessary as the chance that these names
+        // are actually to something else *and* they would reduce without issue seems very low.  This can be revisited
+        // if we get feedback on this.
+
+        using var _ = PooledHashSet<SyntaxNode>.GetInstance(out var namesToUpdate);
+        foreach (var descendent in root.DescendantNodes(descendIntoTrivia: true))
+        {
+            if (descendent is TSimpleNameSyntax simpleName &&
+                allNamespaceNameParts.Contains(syntaxFacts.GetIdentifierOfSimpleName(simpleName).ValueText))
+            {
+                namesToUpdate.Add(GetHighestNameOrCref(simpleName));
+            }
+        }
+
+        return root.ReplaceNodes(
+            namesToUpdate,
+            (_, current) => current.WithAdditionalAnnotations(Simplifier.Annotation));
+
+        static SyntaxNode GetHighestNameOrCref(TNameSyntax name)
+        {
+            while (name.Parent is TNameSyntax parentName)
+                name = parentName;
+
+            return name.Parent is TCrefSyntax ? name.Parent : name;
+        }
     }
 
     private static async Task<Document> FixReferencingDocumentAsync(
@@ -651,9 +684,8 @@ private static async Task<Document> FixReferencingDocumentAsync(
 
         var newNamespaceParts = GetNamespaceParts(newNamespace);
 
-        var (documentWithRefFixed, containers) =
-            await FixReferencesAsync(document, changeNamespaceService, addImportService, refLocations, newNamespaceParts, cancellationToken)
-                .ConfigureAwait(false);
+        var (documentWithRefFixed, containers) = await FixReferencesAsync(
+            document, changeNamespaceService, addImportService, refLocations, newNamespaceParts, cancellationToken).ConfigureAwait(false);
 
         var documentOptions = await document.GetCodeCleanupOptionsAsync(cancellationToken).ConfigureAwait(false);
 
@@ -666,10 +698,24 @@ await FixReferencesAsync(document, changeNamespaceService, addImportService, ref
             cancellationToken).ConfigureAwait(false);
 
         // Need to invoke formatter explicitly since we are doing the diff merge ourselves.
-        var formattedDocument = await Formatter.FormatAsync(documentWithAdditionalImports, Formatter.Annotation, documentOptions.FormattingOptions, cancellationToken)
-            .ConfigureAwait(false);
+        var formattedDocument = await Formatter.FormatAsync(
+            documentWithAdditionalImports, Formatter.Annotation, documentOptions.FormattingOptions, cancellationToken).ConfigureAwait(false);
 
-        return await Simplifier.ReduceAsync(formattedDocument, documentOptions.SimplifierOptions, cancellationToken).ConfigureAwait(false);
+        return await SimplifyTypeNamesAsync(formattedDocument, documentOptions, cancellationToken).ConfigureAwait(false);
+    }
+
+    private static async Task<Document> SimplifyTypeNamesAsync(
+        Document document, CodeCleanupOptions documentOptions, CancellationToken cancellationToken)
+    {
+        var changeNamespaceService = document.GetRequiredLanguageService<IChangeNamespaceService>();
+        var text = await document.GetTextAsync(cancellationToken).ConfigureAwait(false);
+        var service = document.GetRequiredLanguageService<ISimplificationService>();
+        return await service.ReduceAsync(
+            document,
+            [new TextSpan(0, text.Length)],
+            documentOptions.SimplifierOptions,
+            [changeNamespaceService.NameReducer],
+            cancellationToken).ConfigureAwait(false);
     }
 
     /// <summary>
@@ -708,9 +754,7 @@ await FixReferencesAsync(document, changeNamespaceService, addImportService, ref
             // it will be handled properly because it is one of the reference to the type symbol. Otherwise, we don't
             // attempt to make a potential fix, and user might end up with errors as a result.                    
             if (refLoc.ReferenceLocation.Alias != null)
-            {
                 continue;
-            }
 
             // Other documents in the solution might have changed after we calculated those ReferenceLocation, 
             // so we can't trust anything to be still up-to-date except their spans.
@@ -743,9 +787,7 @@ await FixReferencesAsync(document, changeNamespaceService, addImportService, ref
         }
 
         foreach (var container in containers)
-        {
             editor.TrackNode(container);
-        }
 
         var fixedDocument = editor.GetChangedDocument();
         root = await fixedDocument.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);
@@ -858,7 +900,7 @@ private SyntaxNodeSpanStartComparer()
         {
         }
 
-        public static SyntaxNodeSpanStartComparer Instance { get; } = new SyntaxNodeSpanStartComparer();
+        public static SyntaxNodeSpanStartComparer Instance { get; } = new();
 
         public int Compare(SyntaxNode? x, SyntaxNode? y)
         {