From 0f9a0009774af4132fda0568fa265d32424de786 Mon Sep 17 00:00:00 2001 From: Gen Lu Date: Thu, 29 Nov 2018 16:14:28 -0800 Subject: [PATCH] Some minor changes --- .../CSharpSyncNamespaceTestsBase.cs | 1 - .../CSharpChangeNamespaceService.cs | 142 ++++++------ .../AbstractChangeNamespaceService.cs | 219 +++++++++--------- .../SyncNamespace/IChangeNamespaceService.cs | 8 +- .../VisualBasicChangeNamespaceService.vb | 6 +- 5 files changed, 186 insertions(+), 190 deletions(-) diff --git a/src/EditorFeatures/CSharpTest/CodeActions/SyncNamespace/CSharpSyncNamespaceTestsBase.cs b/src/EditorFeatures/CSharpTest/CodeActions/SyncNamespace/CSharpSyncNamespaceTestsBase.cs index d6737c3b9b7..4c8287c7aa7 100644 --- a/src/EditorFeatures/CSharpTest/CodeActions/SyncNamespace/CSharpSyncNamespaceTestsBase.cs +++ b/src/EditorFeatures/CSharpTest/CodeActions/SyncNamespace/CSharpSyncNamespaceTestsBase.cs @@ -91,7 +91,6 @@ protected async Task TestMoveFileToMatchNamespace(string initialMarkup, List GetMemberDeclarationsInCo } /// - /// Try to change the namespace declaration based on the follwing rules: + /// Try to change the namespace declaration based on the following rules: /// - if neither declared nor target namespace are "" (i.e. global namespace), /// then we try to change the name of the namespace. /// - if declared namespace is "", then we try to move all types declared @@ -206,11 +206,12 @@ protected override SyntaxList GetMemberDeclarationsInCo ImmutableArray targetNamespaceParts) { Debug.Assert(!declaredNamespaceParts.IsDefault && !targetNamespaceParts.IsDefault); - var container = root.GetAnnotatedNodes(s_containerAnnotation[0]).Single(); + var container = root.GetAnnotatedNodes(ContainerAnnotation[0]).Single(); // Move everything from global namespace to a namespace declaration if (container is CompilationUnitSyntax compilationUnit) { + Debug.Assert(!compilationUnit.Members.Any(m => m is NamespaceDeclarationSyntax)); Debug.Assert(IsGlobalNamespace(declaredNamespaceParts)); var targetNamespaceDecl = SyntaxFactory.NamespaceDeclaration( @@ -220,7 +221,7 @@ protected override SyntaxList GetMemberDeclarationsInCo usings: default, members: compilationUnit.Members); return compilationUnit.WithMembers(new SyntaxList(targetNamespaceDecl)) - .WithoutAnnotations(s_containerAnnotation); + .WithoutAnnotations(ContainerAnnotation); } if (container is NamespaceDeclarationSyntax namespaceDecl) @@ -278,12 +279,78 @@ protected override SyntaxList GetMemberDeclarationsInCo namespaceDecl.WithName( CreateNameSyntax(targetNamespaceParts, aliasQualifier: null, targetNamespaceParts.Length - 1) .WithTriviaFrom(namespaceDecl.Name).WithAdditionalAnnotations(WarningAnnotation)) - .WithoutAnnotations(s_containerAnnotation)); + .WithoutAnnotations(ContainerAnnotation)); } throw ExceptionUtilities.Unreachable; } + /// + /// For the node specified by to be applicable container, it must be a namespace + /// declaration or a compilation unit, contain no partial declarations and meet the following additional + /// requirements: + /// + /// - If a namespace declaration: + /// 1. It doesn't contain or is nested in other namespace declarations + /// 2. The name of the namespace is valid (i.e. no errors) + /// + /// - If a compilation unit (i.e. is empty), there must be no namespace declaration + /// inside (i.e. all members are declared in global namespace) + /// + protected override async Task TryGetApplicableContainerFromSpanAsync(Document document, TextSpan span, CancellationToken cancellationToken) + { + var compilationUnit = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) as CompilationUnitSyntax; + SyntaxNode container = null; + + // Empty span means that user wants to move all types declared in the document to a new namespace. + // This action is only supported when everything in the document is declared in global namespace, + // which we use the number of namespace declaration nodes to decide. + if (span.IsEmpty) + { + var hasNamespaceDecl = compilationUnit.DescendantNodes(IsCompilationUnitOrNamespaceDeclaration) + .OfType().Any(); + + if (hasNamespaceDecl) + { + return null; + } + + container = compilationUnit; + } + else + { + // 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. + var node = compilationUnit.FindNode(span, getInnermostNodeForTie: true); + + var namespaceDecl = node.AncestorsAndSelf().OfType().SingleOrDefault(); + if (namespaceDecl?.Name.GetDiagnostics().All(diag => diag.DefaultSeverity != DiagnosticSeverity.Error) != true) + { + return null; + } + + if (node.DescendantNodes(IsCompilationUnitOrNamespaceDeclaration).OfType().Any()) + { + return null; + } + + container = namespaceDecl; + } + + var containsPartial = + await ContainsPartialTypeWithMultipleDeclarationsAsync(document, container, cancellationToken).ConfigureAwait(false); + + if (containsPartial) + { + return null; + } + + return container; + + bool IsCompilationUnitOrNamespaceDeclaration(SyntaxNode n) + => n is CompilationUnitSyntax || n is NamespaceDeclarationSyntax; + } + private static bool IsGlobalNamespace(ImmutableArray parts) => parts.Length == 1 && parts[0].Length == 0; @@ -326,7 +393,7 @@ private NameSyntax CreateNameSyntax(ImmutableArray namespaceParts, strin /// Leading trivia of the node and trivia around opening brace, as well as /// trivia around closing brace are concatenated together respectively. /// - private static (ImmutableArray openingTrivia, ImmutableArray closingTrivia) + private static (ImmutableArray openingTrivia, ImmutableArray closingTrivia) GetOpeningAndClosingTriviaOfNamespaceDeclaration(NamespaceDeclarationSyntax namespaceDeclaration) { var openingBuilder = ArrayBuilder.GetInstance(); @@ -340,70 +407,5 @@ private static (ImmutableArray openingTrivia, ImmutableArray TryGetApplicableContainerFromSpanAsync(Document document, TextSpan span, CancellationToken cancellationToken) - { - var container = await TryGetApplicableContainerWorkerAsync(document, span, cancellationToken); - if (container != null) - { - var containsPartial = await ContainsPartialTypeWithMultipleDeclarationsAsync(document, container, cancellationToken) - .ConfigureAwait(false); - - if (!containsPartial) - { - return container; - } - } - - return default; - } - - /// - /// To be applicable, the node specified by must be a namespace declaration or - /// a compilation unit, and meet the following requirements: - /// - /// - If a namespace declaration: - /// 1. It doesn't contain or is nested in other namespace declarations - /// 2. The name of the namespace is valid (i.e. no errors) - /// - /// - If a compilation unit, there must be no namespace declaration and all types in the document are - /// declared in global namespace - /// - private static async Task TryGetApplicableContainerWorkerAsync(Document document, TextSpan span, CancellationToken cancellationToken) - { - var compilationUnit = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false) as CompilationUnitSyntax; - - // Empty span means that user wants to move all types declared in the document to a new namespace. - // This action is only supported when everything in the document is declared in global namespace, - // which we use the number of namespace declaration nodes as the indication. - if (span.IsEmpty) - { - var hasNamespaceDecl = compilationUnit.DescendantNodes(IsCompilationUnitOrNamespaceDeclaration) - .OfType().Any(); - - return hasNamespaceDecl ? null : compilationUnit; - } - - // 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. - var node = compilationUnit.FindNode(span, getInnermostNodeForTie: true); - - var namespaceDecl = node.AncestorsAndSelf().OfType().SingleOrDefault(); - if (namespaceDecl?.Name.GetDiagnostics().All(diag => diag.DefaultSeverity != DiagnosticSeverity.Error) != true) - { - return null; - } - - if (node.DescendantNodes(IsCompilationUnitOrNamespaceDeclaration) - .OfType().Any()) - { - return null; - } - - return namespaceDecl; - - bool IsCompilationUnitOrNamespaceDeclaration(SyntaxNode n) - => n is CompilationUnitSyntax || n is NamespaceDeclarationSyntax; - } } } diff --git a/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs b/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs index 2834e4d8d3f..8c21f9a1e74 100644 --- a/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs +++ b/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/AbstractChangeNamespaceService.cs @@ -25,9 +25,9 @@ namespace Microsoft.CodeAnalysis.ChangeNamespace /// internal abstract class AbstractChangeNamespaceService : IChangeNamespaceService { - public abstract Task CanChangeNamespaceAsync(Document document, SyntaxNode node, CancellationToken cancellationToken); + public abstract Task CanChangeNamespaceAsync(Document document, SyntaxNode container, CancellationToken cancellationToken); - public abstract Task ChangeNamespaceAsync(Document document, SyntaxNode node, string targetNamespace, CancellationToken cancellationToken); + public abstract Task ChangeNamespaceAsync(Document document, SyntaxNode container, string targetNamespace, CancellationToken cancellationToken); /// /// Try to get a new node to replace given node, which is a reference to a top-level type declared inside the @@ -53,9 +53,9 @@ internal abstract class AbstractChangeNamespaceService - /// The annotation used to mark applicable container in each document to be fixed. + /// The annotation used to track applicable container in each document to be fixed. /// - protected static SyntaxAnnotation[] s_containerAnnotation { get; } = new[] { new SyntaxAnnotation() }; + protected static SyntaxAnnotation[] ContainerAnnotation { get; } = new[] { new SyntaxAnnotation() }; protected static SyntaxAnnotation WarningAnnotation { get; } = CodeActions.WarningAnnotation.Create( @@ -68,23 +68,116 @@ internal abstract class AbstractChangeNamespaceService TryGetApplicableContainerFromSpanAsync(Document document, TextSpan span, CancellationToken cancellationToken); - protected abstract string GetDeclaredNamespace(SyntaxNode node); + protected abstract string GetDeclaredNamespace(SyntaxNode container); - protected abstract Task> CanChangeNamespaceWorkerAsync(Document document, SyntaxNode node, CancellationToken cancellationToken); + protected abstract Task> CanChangeNamespaceWorkerAsync(Document document, SyntaxNode container, CancellationToken cancellationToken); - public override async Task CanChangeNamespaceAsync(Document document, SyntaxNode node, CancellationToken cancellationToken) + public override async Task CanChangeNamespaceAsync(Document document, SyntaxNode container, CancellationToken cancellationToken) { - var applicableContainers = await CanChangeNamespaceWorkerAsync(document, node, cancellationToken).ConfigureAwait(false); + var applicableContainers = await CanChangeNamespaceWorkerAsync(document, container, cancellationToken).ConfigureAwait(false); return !applicableContainers.IsDefault; } + public override async Task ChangeNamespaceAsync( + Document document, + SyntaxNode container, + string targetNamespace, + CancellationToken cancellationToken) + { + var solution = document.Project.Solution; + + // Make sure given namespace name is valid, "" means global namespace. + var syntaxFacts = document.GetLanguageService(); + if (targetNamespace == null + || (targetNamespace.Length > 0 && !targetNamespace.Split(s_dotSeparator).All(syntaxFacts.IsValidIdentifier))) + { + return solution; + } + + var containersFromAllDocuments = await CanChangeNamespaceWorkerAsync(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; + } + + var annotatedSolution = await AnnotateContainersAsync(solution, containersFromAllDocuments, cancellationToken).ConfigureAwait(false); + + // Here's the entire process for changing namespace: + // 1. Change the namespace declaration, fix references and add imports that might be necessary. + // 2. Explicitly merge the diff to get a new solution. + // 3. Remove added imports that are unnecessary. + // 4. Do another explicit diff merge based on last merged solution. + // + // The reason for doing explicit diff merge twice is so merging after remove unnecessary imports can be correctly handled. + + var ids = containersFromAllDocuments.SelectAsArray(pair => pair.id); + var solutionAfterNamespaceChange = annotatedSolution; + var referenceDocuments = PooledHashSet.GetInstance(); + + try + { + foreach (var id in ids) + { + var (newSolution, refDocumentIds) = + await ChangeNamespaceInSingleDocumentAsync(solutionAfterNamespaceChange, id, declaredNamespace, targetNamespace, cancellationToken) + .ConfigureAwait(false); + solutionAfterNamespaceChange = newSolution; + referenceDocuments.AddRange(refDocumentIds); + } + + var solutionAfterFirstMerge = await MergeDiffAsync(solution, solutionAfterNamespaceChange, cancellationToken).ConfigureAwait(false); + + // 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 + // 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: + // + // namespace Foo + // { + // using Bar; + // ~~~~~~~~~ + // } + // + // Also, because we may have added different imports to document that triggered the refactoring + // and the documents that reference affected types declared in changed namespace, we try to remove + // unnecessary imports separately. + + var solutionAfterImportsRemoved = await RemoveUnnecessaryImportsAsync( + solutionAfterFirstMerge, + containersFromAllDocuments.SelectAsArray(pair => pair.id), + CreateAllContainingNamespaces(declaredNamespace), + cancellationToken).ConfigureAwait(false); + + solutionAfterImportsRemoved = await RemoveUnnecessaryImportsAsync( + solutionAfterImportsRemoved, + referenceDocuments.ToImmutableArray(), + ImmutableArray.Create(declaredNamespace, targetNamespace), + cancellationToken).ConfigureAwait(false); + + return await MergeDiffAsync(solutionAfterFirstMerge, solutionAfterImportsRemoved, cancellationToken).ConfigureAwait(false); + } + finally + { + referenceDocuments.Free(); + } + } + protected async Task> TryGetApplicableContainersFromAllDocumentsAsync( Solution solution, ImmutableArray ids, TextSpan span, CancellationToken cancellationToken) { - // If the node selected doesn't meet the requirement for changing namespace in any of the documents + // If the node specified by span doesn't meet the requirement to be an applicable container in any of the documents // (See `TryGetApplicableContainerFromSpanAsync`), or we are getting different namespace declarations among // those documents, then we know we can't make a proper code change. We will return null and the check // will return false. We use span of namespace declaration found in each document to decide if they are identical. @@ -125,13 +218,16 @@ public override async Task CanChangeNamespaceAsync(Document document, Synt } } + /// + /// Mark container nodes with our annotation so we can keep track of them across syntax modifications. + /// protected async Task AnnotateContainersAsync(Solution solution, ImmutableArray<(DocumentId, SyntaxNode)> containers, CancellationToken cancellationToken) { var solutionEditor = new SolutionEditor(solution); foreach (var (id, container) in containers) { var documentEditor = await solutionEditor.GetDocumentEditorAsync(id, cancellationToken).ConfigureAwait(false); - documentEditor.ReplaceNode(container, container.WithAdditionalAnnotations(s_containerAnnotation)); + documentEditor.ReplaceNode(container, container.WithAdditionalAnnotations(ContainerAnnotation)); } return solutionEditor.GetChangedSolution(); } @@ -178,107 +274,6 @@ protected static bool IsSupportedLinkedDocument(Document document, out Immutable return true; } - /// - /// This code action tries to change the name of the namespace declaration to - /// match the folder hierarchy of the document. The new namespace is constructed - /// by concatenating the default namespace of the project and all the folders in - /// the file path up to the project root. - /// - /// For example, if he default namespace is `A.B.C`, file path is - /// "[project root dir]\D\E\F\Class1.cs" and declared namespace in the file is - /// `Foo.Bar.Baz`, then this action will change the namespace declaration - /// to `A.B.C.D.E.F`. - /// - /// Note that it also handles the case where the target namespace or declared namespace - /// is global namespace, i.e. default namespace is "" and the file is located at project - /// root directory, and no namespace declaration in the document, respectively. - /// - public override async Task ChangeNamespaceAsync( - Document document, - SyntaxNode container, - string targetNamespace, - CancellationToken cancellationToken) - { - var solution = document.Project.Solution; - - var syntaxFacts = document.GetLanguageService(); - if (targetNamespace == null - || (targetNamespace.Length > 0 && !targetNamespace.Split(s_dotSeparator).All(syntaxFacts.IsValidIdentifier))) - { - return solution; - } - - var containersFromAllDocuments = await CanChangeNamespaceWorkerAsync(document, container, cancellationToken).ConfigureAwait(false); - if (containersFromAllDocuments.IsDefault) - { - return solution; - } - - var annotatedSolution = await AnnotateContainersAsync(solution, containersFromAllDocuments, cancellationToken).ConfigureAwait(false); - - // Here's the entire process for changing namespace: - // 1. Change the namespace declaration, fix references and add imports that might be necessary. - // 2. Explicitly merge the diff to get a new solution. - // 3. Remove added imports that are unnecessary. - // 4. Do another explicit diff merge based on last merged solution. - // - // The reason for doing explicit diff merge twice is so merging after remove unnecessary imports can be correctly handled. - - var ids = containersFromAllDocuments.SelectAsArray(pair => pair.id); - var declaredNamespace = GetDeclaredNamespace(container); - var solutionAfterNamespaceChange = annotatedSolution; - var referenceDocuments = PooledHashSet.GetInstance(); - - try - { - foreach (var id in ids) - { - var (newSolution, refDocumentIds) = - await ChangeNamespaceInSingleDocumentAsync(solutionAfterNamespaceChange, id, declaredNamespace, targetNamespace, cancellationToken) - .ConfigureAwait(false); - solutionAfterNamespaceChange = newSolution; - referenceDocuments.AddRange(refDocumentIds); - } - - var solutionAfterFirstMerge = await MergeDiffAsync(solution, solutionAfterNamespaceChange, cancellationToken).ConfigureAwait(false); - - // 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 - // 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: - // - // namespace Foo - // { - // using Bar; - // ~~~~~~~~~ - // } - // - // Also, because we may have added different imports to document that triggered the refactoring - // and the documents that reference affected types declared in changed namespace, we try to remove - // unnecessary imports separately. - - var solutionAfterImportsRemoved = await RemoveUnnecessaryImportsAsync( - solutionAfterFirstMerge, - containersFromAllDocuments.SelectAsArray(pair => pair.id), - CreateAllContainingNamespaces(declaredNamespace), - cancellationToken).ConfigureAwait(false); - - solutionAfterImportsRemoved = await RemoveUnnecessaryImportsAsync( - solutionAfterImportsRemoved, - referenceDocuments.ToImmutableArray(), - ImmutableArray.Create(declaredNamespace, targetNamespace), - cancellationToken).ConfigureAwait(false); - - return await MergeDiffAsync(solutionAfterFirstMerge, solutionAfterImportsRemoved, cancellationToken).ConfigureAwait(false); - } - finally - { - referenceDocuments.Free(); - } - } - private async Task> GetDeclaredSymbolsInContainerAsync( Document document, SyntaxNode container, @@ -350,7 +345,7 @@ private static SyntaxNode CreateImport(SyntaxGenerator syntaxGenerator, string n { var document = solution.GetDocument(id); var root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); - var container = root.GetAnnotatedNodes(s_containerAnnotation[0]).Single(); + var container = root.GetAnnotatedNodes(ContainerAnnotation[0]).Single(); // Get types declared in the changing namespace, because we need to fix all references to them, // e.g. change the namespace for qualified name, add imports to proper containers, etc. diff --git a/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs b/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs index a35ea6efac3..9e602694185 100644 --- a/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs +++ b/src/Features/Core/Portable/CodeRefactorings/SyncNamespace/IChangeNamespaceService.cs @@ -11,17 +11,17 @@ internal interface IChangeNamespaceService : ILanguageService /// /// Determine whether we can change the namespace for given in the document. /// Linked documents are not supported, except for a regular document in a multi-targeting project, - /// where the container node must be consisten among all linked documents. + /// where the container node must be consistent among all linked documents. /// Here's the additional requirements on to use this service: /// /// - If is a namespace declaration node: - /// 1. doesn't contain or is nested in other namespace declarations + /// 1. Doesn't contain or is nested in other namespace declarations /// 2. The name of the namespace is valid (i.e. no errors) /// 3. No partial type declared in the namespace. Otherwise its multiple declaration will /// end up in different namespace. /// /// - If is a compilation unit node: - /// 1. it must contain no namespace declaration + /// 1. It must contain no namespace declaration /// 2. No partial type declared in the document. Otherwise its multiple declaration will /// end up in different namespace. /// @@ -33,7 +33,7 @@ internal interface IChangeNamespaceService : ILanguageService /// Change namespace for given to the name specified by . /// Everything declared in the will be moved to the new namespace. /// Change will only be made if returns and - /// is a valid name for namespace. + /// is a valid name for namespace (we use "" to specify global namespace). /// Task ChangeNamespaceAsync(Document document, SyntaxNode container, string targetNamespace, CancellationToken cancellationToken); } diff --git a/src/Features/VisualBasic/Portable/CodeRefactorings/SyncNamespace/VisualBasicChangeNamespaceService.vb b/src/Features/VisualBasic/Portable/CodeRefactorings/SyncNamespace/VisualBasicChangeNamespaceService.vb index 388864c13ad..aba8d706619 100644 --- a/src/Features/VisualBasic/Portable/CodeRefactorings/SyncNamespace/VisualBasicChangeNamespaceService.vb +++ b/src/Features/VisualBasic/Portable/CodeRefactorings/SyncNamespace/VisualBasicChangeNamespaceService.vb @@ -49,7 +49,7 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ChangeNamespace End Function ' This is only reachable when called from a VB service, which is not implemented yet. - Protected Overrides Function GetMemberDeclarationsInContainer(compilationUnitOrNamespaceDecl As SyntaxNode) As SyntaxList(Of StatementSyntax) + Protected Overrides Function GetMemberDeclarationsInContainer(container As SyntaxNode) As SyntaxList(Of StatementSyntax) Throw ExceptionUtilities.Unreachable End Function @@ -59,11 +59,11 @@ Namespace Microsoft.CodeAnalysis.VisualBasic.ChangeNamespace End Function ' This is only reachable when called from a VB service, which is not implemented yet. - Protected Overrides Function GetDeclaredNamespace(node As SyntaxNode) As String + Protected Overrides Function GetDeclaredNamespace(container As SyntaxNode) As String Throw ExceptionUtilities.Unreachable End Function - Private Function CreateNameSyntax(namespaceParts As ImmutableArray(Of String), index As Integer) As NameSyntax + Private Shared Function CreateNameSyntax(namespaceParts As ImmutableArray(Of String), index As Integer) As NameSyntax Dim part = namespaceParts(index).EscapeIdentifier() Dim namePiece = SyntaxFactory.IdentifierName(part) -- GitLab