Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 13, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6332862
Code simplification
CyrusNajmabadi Jun 13, 2025
a3cd8e7
Specify single reducer
CyrusNajmabadi Jun 13, 2025
2a9e58b
Simplify
CyrusNajmabadi Jun 13, 2025
76f7f40
Add test
CyrusNajmabadi Jun 13, 2025
4276652
Only add annotations to potential references
CyrusNajmabadi Jun 13, 2025
09585f0
:Extract type
CyrusNajmabadi Jun 13, 2025
38e7dac
Docs
CyrusNajmabadi Jun 13, 2025
fd69b21
move up
CyrusNajmabadi Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 67 additions & 1 deletion ...EditorFeatures/CSharpTest/CodeActions/SyncNamespace/SyncNamespaceTests_ChangeNamespace.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down Expand Up @@ -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;
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

desirable change in behavior. we were unnecessarily changing this here, despite it having nothing to do with the namespaces being changed.

}
}
""";
Expand Down
20 changes: 13 additions & 7 deletions src/Features/CSharp/Portable/CodeRefactorings/SyncNamespace/CSharpChangeNamespaceService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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();
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general idea is htat we go find the locations of interest, and only simplify them with the language's name simplifier (not all the other simplifiers).


protected override async Task<ImmutableArray<(DocumentId, SyntaxNode)>> GetValidContainersFromAllLinkedDocumentsAsync(
Document document,
Expand Down
15 changes: 4 additions & 11 deletions ...arp/Portable/CodeRefactorings/SyncNamespace/CSharpSyncNamespaceCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -58,7 +54,4 @@ public CSharpSyncNamespaceCodeRefactoringProvider()

return null;
}

protected override string EscapeIdentifier(string identifier)
=> identifier.EscapeIdentifier();
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused.

}
122 changes: 82 additions & 40 deletions src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 = ['.'];

Expand Down Expand Up @@ -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();
Expand All @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moved to its own file.

{
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)
{
Expand Down Expand Up @@ -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;
}
}
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Jun 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crux of the change. we only go fixup names that were using one of hte namespace names in either the old namespace name, or the new namespace name.


private static async Task<Document> FixReferencingDocumentAsync(
Expand All @@ -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);

Expand All @@ -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>
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
{
Expand Down
Loading
Loading