diff --git a/src/Features/Core/Portable/PullMemberUp/MembersPuller.cs b/src/Features/Core/Portable/PullMemberUp/MembersPuller.cs index 3853040c7dc74..3e711bbaf64c8 100644 --- a/src/Features/Core/Portable/PullMemberUp/MembersPuller.cs +++ b/src/Features/Core/Portable/PullMemberUp/MembersPuller.cs @@ -32,7 +32,8 @@ internal static class MembersPuller /// Annotation used to mark imports that we move over, so that we can remove these imports if they are unnecessary /// (and so we don't remove any other unnecessary imports) /// - private static readonly SyntaxAnnotation s_annotation = new("PullMemberRemovableImport"); + private static readonly SyntaxAnnotation s_removableImportAnnotation = new("PullMemberRemovableImport"); + private static readonly SyntaxAnnotation s_destinationNodeAnnotation = new("DestinationNode"); public static CodeAction TryComputeCodeAction( Document document, @@ -312,7 +313,9 @@ private static async Task PullMembersIntoClassAsync( var options = await destinationEditor.OriginalDocument.GetCleanCodeGenerationOptionsAsync(fallbackOptions, cancellationToken).ConfigureAwait(false); var info = codeGenerationService.GetInfo(context, options.GenerationOptions, destinationEditor.OriginalDocument.Project.ParseOptions); - var newDestination = codeGenerationService.AddMembers(destinationSyntaxNode, pullUpMembersSymbols, info, cancellationToken); + var newDestination = codeGenerationService + .AddMembers(destinationSyntaxNode, pullUpMembersSymbols, info, cancellationToken) + .WithAdditionalAnnotations(s_destinationNodeAnnotation); using var _ = PooledHashSet.GetInstance(out var sourceImports); var syntaxFacts = destinationEditor.OriginalDocument.GetRequiredLanguageService(); @@ -329,7 +332,7 @@ private static async Task PullMembersIntoClassAsync( sourceImports.Add( destinationEditor.Generator.NamespaceImportDeclaration( resultNamespace.ToDisplayString(SymbolDisplayFormats.NameFormat)) - .WithAdditionalAnnotations(s_annotation)); + .WithAdditionalAnnotations(s_removableImportAnnotation)); } foreach (var syntax in symbolToDeclarations[analysisResult.Member]) @@ -342,7 +345,7 @@ private static async Task PullMembersIntoClassAsync( .Select(import => import .WithoutLeadingTrivia() .WithTrailingTrivia(originalMemberEditor.Generator.ElasticCarriageReturnLineFeed) - .WithAdditionalAnnotations(s_annotation))); + .WithAdditionalAnnotations(s_removableImportAnnotation))); if (!analysisResult.MakeMemberDeclarationAbstract || analysisResult.Member.IsAbstract) { @@ -378,7 +381,7 @@ private static async Task PullMembersIntoClassAsync( destinationEditor.ReplaceNode(destinationEditor.OriginalRoot, (node, generator) => addImportsService.AddImports( destinationEditor.SemanticModel.Compilation, node, - node.GetCurrentNode(newDestination), + node.GetAnnotatedNodes(s_destinationNodeAnnotation).FirstOrDefault(), sourceImports, generator, options.CleanupOptions.AddImportOptions, @@ -387,12 +390,12 @@ private static async Task PullMembersIntoClassAsync( var removeImportsService = destinationEditor.OriginalDocument.GetRequiredLanguageService(); var destinationDocument = await removeImportsService.RemoveUnnecessaryImportsAsync( destinationEditor.GetChangedDocument(), - node => node.HasAnnotation(s_annotation), + node => node.HasAnnotation(s_removableImportAnnotation), options.CleanupOptions.FormattingOptions, cancellationToken).ConfigureAwait(false); // Format whitespace trivia within the import statements we pull up - destinationDocument = await Formatter.FormatAsync(destinationDocument, s_annotation, options.CleanupOptions.FormattingOptions, cancellationToken).ConfigureAwait(false); + destinationDocument = await Formatter.FormatAsync(destinationDocument, s_removableImportAnnotation, options.CleanupOptions.FormattingOptions, cancellationToken).ConfigureAwait(false); var destinationRoot = AddLeadingTriviaBeforeFirstMember( await destinationDocument.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false), diff --git a/src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs b/src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs index 4697aac77f44b..1fd3be19768c8 100644 --- a/src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs +++ b/src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics.CodeAnalysis; using System.Linq; using Microsoft.CodeAnalysis.Host; using Roslyn.Utilities; @@ -12,14 +11,52 @@ namespace Microsoft.CodeAnalysis.Editing { /// - /// An editor for making changes to a syntax tree. + /// An editor for making changes to a syntax tree. The editor works by giving a list of changes to perform to a + /// particular tree in order. Changes are given a they will apply to in the + /// original tree the editor is created for. The semantics of application are as follows: + /// + /// + /// + /// The original root provided is used as the 'current' root for all operations. This 'current' root will + /// continually be updated, becoming the new 'current' root. The original root is never changed. + /// + /// + /// Each change has its given tracked, using a , producing a + /// 'current' root that tracks all of them. This allows that same node to be found after prior changes are applied + /// which mutate the tree. + /// + /// + /// Each change is then applied in order it was added to the editor. + /// + /// + /// A change first attempts to find its in the 'current' root. If that node cannot be + /// found, the operation will fail with an . + /// + /// + /// The particular change will run on that node, removing, replacing, or inserting around it according to the + /// change. If the change is passed a delegate as its 'compute' argument, it will be given the found in the current root. The 'current' root will then be updated by replacing the current + /// node with the new computed node. + /// + /// + /// The 'current' root is then returned. + /// + /// /// + /// + /// The above editing strategy makes it an error for a client of the editor to add a change that updates a parent + /// node and then adds a change that updates a child node (unless the parent change is certain to contain the + /// child), and attempting this will throw at runtime. If a client ever needs to update both a child and a parent, + /// it should add the child change first, and then the parent change. And the parent change should pass an + /// appropriate 'compute' callback so it will see the results of the child change. + /// If a client wants to make a replacement, then find the value put into + /// the tree, that can be done by adding a dedicated annotation to that node and then looking it back up in the + /// 'current' node passed to a 'compute' callback. + /// public class SyntaxEditor { private readonly SyntaxGenerator _generator; private readonly List _changes = new(); - private bool _allowEditsOnLazilyCreatedTrackedNewNodes; - private HashSet? _lazyTrackedNewNodesOpt; /// /// Creates a new instance. @@ -53,32 +90,6 @@ internal SyntaxEditor(SyntaxNode root, SyntaxGenerator generator) _generator = generator; } - [return: NotNullIfNotNull(nameof(node))] - private SyntaxNode? ApplyTrackingToNewNode(SyntaxNode? node) - { - if (node == null) - { - return null; - } - - _lazyTrackedNewNodesOpt ??= new HashSet(); - foreach (var descendant in node.DescendantNodesAndSelf()) - { - _lazyTrackedNewNodesOpt.Add(descendant); - } - - return node.TrackNodes(node.DescendantNodesAndSelf()); - } - - private IEnumerable ApplyTrackingToNewNodes(IEnumerable nodes) - { - foreach (var node in nodes) - { - var result = ApplyTrackingToNewNode(node); - yield return result; - } - } - /// /// The that was specified when the was constructed. /// @@ -99,9 +110,7 @@ public SyntaxNode GetChangedRoot() var newRoot = OriginalRoot.TrackNodes(nodes); foreach (var change in _changes) - { newRoot = change.Apply(newRoot, _generator); - } return newRoot; } @@ -111,7 +120,7 @@ public SyntaxNode GetChangedRoot() /// public void TrackNode(SyntaxNode node) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); _changes.Add(new NoChange(node)); } @@ -129,7 +138,7 @@ public void RemoveNode(SyntaxNode node) /// Options that affect how node removal works. public void RemoveNode(SyntaxNode node, SyntaxRemoveOptions options) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); _changes.Add(new RemoveChange(node, options)); } @@ -141,38 +150,29 @@ public void RemoveNode(SyntaxNode node, SyntaxRemoveOptions options) /// The node passed into the compute function includes changes from prior edits. It will not appear as a descendant of the original root. public void ReplaceNode(SyntaxNode node, Func computeReplacement) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); if (computeReplacement == null) - { throw new ArgumentNullException(nameof(computeReplacement)); - } - _allowEditsOnLazilyCreatedTrackedNewNodes = true; - _changes.Add(new ReplaceChange(node, computeReplacement, this)); + _changes.Add(new ReplaceChange(node, computeReplacement)); } internal void ReplaceNode(SyntaxNode node, Func> computeReplacement) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); if (computeReplacement == null) - { throw new ArgumentNullException(nameof(computeReplacement)); - } - _allowEditsOnLazilyCreatedTrackedNewNodes = true; - _changes.Add(new ReplaceWithCollectionChange(node, computeReplacement, this)); + _changes.Add(new ReplaceWithCollectionChange(node, computeReplacement)); } internal void ReplaceNode(SyntaxNode node, Func computeReplacement, TArgument argument) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); if (computeReplacement == null) - { throw new ArgumentNullException(nameof(computeReplacement)); - } - _allowEditsOnLazilyCreatedTrackedNewNodes = true; - _changes.Add(new ReplaceChange(node, computeReplacement, argument, this)); + _changes.Add(new ReplaceChange(node, computeReplacement, argument)); } /// @@ -182,14 +182,11 @@ internal void ReplaceNode(SyntaxNode node, FuncThe new node that will be placed into the tree in the existing node's location. public void ReplaceNode(SyntaxNode node, SyntaxNode newNode) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); if (node == newNode) - { return; - } - newNode = ApplyTrackingToNewNode(newNode); - _changes.Add(new ReplaceChange(node, (n, g) => newNode, this)); + _changes.Add(new ReplaceChange(node, (n, g) => newNode)); } /// @@ -199,13 +196,10 @@ public void ReplaceNode(SyntaxNode node, SyntaxNode newNode) /// The nodes to place before the existing node. These nodes must be of a compatible type to be placed in the same list containing the existing node. public void InsertBefore(SyntaxNode node, IEnumerable newNodes) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); if (newNodes == null) - { throw new ArgumentNullException(nameof(newNodes)); - } - newNodes = ApplyTrackingToNewNodes(newNodes); _changes.Add(new InsertChange(node, newNodes, isBefore: true)); } @@ -224,13 +218,10 @@ public void InsertBefore(SyntaxNode node, SyntaxNode newNode) /// The nodes to place after the existing node. These nodes must be of a compatible type to be placed in the same list containing the existing node. public void InsertAfter(SyntaxNode node, IEnumerable newNodes) { - CheckNodeInOriginalTreeOrTracked(node); + CheckNodeInOriginalTree(node); if (newNodes == null) - { throw new ArgumentNullException(nameof(newNodes)); - } - newNodes = ApplyTrackingToNewNodes(newNodes); _changes.Add(new InsertChange(node, newNodes, isBefore: false)); } @@ -242,33 +233,13 @@ public void InsertAfter(SyntaxNode node, IEnumerable newNodes) public void InsertAfter(SyntaxNode node, SyntaxNode newNode) => this.InsertAfter(node, new[] { newNode }); - private void CheckNodeInOriginalTreeOrTracked(SyntaxNode node) + private void CheckNodeInOriginalTree(SyntaxNode node) { if (node == null) - { throw new ArgumentNullException(nameof(node)); - } if (OriginalRoot.Contains(node)) - { - // Node is contained in the original tree. - return; - } - - if (_allowEditsOnLazilyCreatedTrackedNewNodes) - { - // This could be a node that is handed to us lazily from one of the prior edits - // which support lazy replacement nodes, we conservatively avoid throwing here. - // If this was indeed an unsupported node, syntax editor will throw an exception later - // when attempting to compute changed root. return; - } - - if (_lazyTrackedNewNodesOpt?.Contains(node) == true) - { - // Node is one of the new nodes, which is already tracked and supported. - return; - } throw new ArgumentException(WorkspacesResources.The_node_is_not_part_of_the_tree, nameof(node)); } @@ -284,9 +255,7 @@ public SyntaxNode Apply(SyntaxNode root, SyntaxGenerator generator) { var currentNode = root.GetCurrentNode(OriginalNode); if (currentNode is null) - { Contract.Fail($"GetCurrentNode returned null with the following node: {OriginalNode}"); - } return Apply(root, currentNode, generator); } @@ -325,78 +294,53 @@ protected override SyntaxNode Apply(SyntaxNode root, SyntaxNode currentNode, Syn private sealed class ReplaceChange : Change { private readonly Func _modifier; - private readonly SyntaxEditor _editor; public ReplaceChange( SyntaxNode node, - Func modifier, - SyntaxEditor editor) + Func modifier) : base(node) { Contract.ThrowIfNull(node); _modifier = modifier; - _editor = editor; } protected override SyntaxNode Apply(SyntaxNode root, SyntaxNode currentNode, SyntaxGenerator generator) - { - var newNode = _modifier(currentNode, generator); - newNode = _editor.ApplyTrackingToNewNode(newNode); - return ValidateNewRoot(generator.ReplaceNode(root, currentNode, newNode)); - } + => ValidateNewRoot(generator.ReplaceNode(root, currentNode, _modifier(currentNode, generator))); } private sealed class ReplaceWithCollectionChange : Change { private readonly Func> _modifier; - private readonly SyntaxEditor _editor; public ReplaceWithCollectionChange( SyntaxNode node, - Func> modifier, - SyntaxEditor editor) + Func> modifier) : base(node) { _modifier = modifier; - _editor = editor; } protected override SyntaxNode Apply(SyntaxNode root, SyntaxNode currentNode, SyntaxGenerator generator) - { - var newNodes = _modifier(currentNode, generator).ToList(); - for (var i = 0; i < newNodes.Count; i++) - { - newNodes[i] = _editor.ApplyTrackingToNewNode(newNodes[i]); - } - - return SyntaxGenerator.ReplaceNode(root, currentNode, newNodes); - } + => SyntaxGenerator.ReplaceNode(root, currentNode, _modifier(currentNode, generator)); } private sealed class ReplaceChange : Change { private readonly Func _modifier; private readonly TArgument _argument; - private readonly SyntaxEditor _editor; public ReplaceChange( SyntaxNode node, Func modifier, - TArgument argument, - SyntaxEditor editor) + TArgument argument) : base(node) { _modifier = modifier; _argument = argument; - _editor = editor; } protected override SyntaxNode Apply(SyntaxNode root, SyntaxNode currentNode, SyntaxGenerator generator) - { - var newNode = _modifier(currentNode, generator, _argument); - newNode = _editor.ApplyTrackingToNewNode(newNode); - return ValidateNewRoot(generator.ReplaceNode(root, currentNode, newNode)); - } + => ValidateNewRoot(generator.ReplaceNode(root, currentNode, _modifier(currentNode, generator, _argument))); } private sealed class InsertChange : Change diff --git a/src/Workspaces/CoreTest/Editing/SyntaxEditorTests.cs b/src/Workspaces/CoreTest/Editing/SyntaxEditorTests.cs index 5bd9f0f36ca95..fdf4dd139a425 100644 --- a/src/Workspaces/CoreTest/Editing/SyntaxEditorTests.cs +++ b/src/Workspaces/CoreTest/Editing/SyntaxEditorTests.cs @@ -144,294 +144,6 @@ public class C }"); } - [Fact] - public void TestReplaceWithTracking() - { - // ReplaceNode overload #1 - TestReplaceWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.ReplaceNode(node, newNode); - }); - - // ReplaceNode overload #2 - TestReplaceWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.ReplaceNode(node, computeReplacement: (originalNode, generator) => newNode); - }); - - // ReplaceNode overload #3 - TestReplaceWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.ReplaceNode(node, - computeReplacement: (originalNode, generator, argument) => newNode, - argument: (object)null); - }); - } - - private void TestReplaceWithTrackingCore(Action replaceNodeWithTracking) - { - var code = @" -public class C -{ - public int X; -}"; - - var cu = SyntaxFactory.ParseCompilationUnit(code); - var cls = cu.Members[0]; - - var editor = GetEditor(cu); - var fieldX = editor.Generator.GetMembers(cls)[0]; - var newFieldY = editor.Generator.FieldDeclaration("Y", editor.Generator.TypeExpression(SpecialType.System_String), Accessibility.Public); - replaceNodeWithTracking(fieldX, newFieldY, editor); - - var newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public string Y; -}"); - var newFieldYType = newFieldY.DescendantNodes().Single(n => n.ToString() == "string"); - var newType = editor.Generator.TypeExpression(SpecialType.System_Char); - replaceNodeWithTracking(newFieldYType, newType, editor); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public char Y; -}"); - - editor.RemoveNode(newFieldY); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ -}"); - } - - [Fact] - public void TestReplaceWithTracking_02() - { - var code = @" -public class C -{ - public int X; - public string X2; - public char X3; -}"; - - var cu = SyntaxFactory.ParseCompilationUnit(code); - - var editor = GetEditor(cu); - - var cls = cu.Members[0]; - var fieldX = editor.Generator.GetMembers(cls)[0]; - var fieldX2 = editor.Generator.GetMembers(cls)[1]; - var fieldX3 = editor.Generator.GetMembers(cls)[2]; - - var newFieldY = editor.Generator.FieldDeclaration("Y", editor.Generator.TypeExpression(SpecialType.System_String), Accessibility.Public); - editor.ReplaceNode(fieldX, newFieldY); - - var newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public string Y; - public string X2; - public char X3; -}"); - var newFieldYType = newFieldY.DescendantNodes().Single(n => n.ToString() == "string"); - var newType = editor.Generator.TypeExpression(SpecialType.System_Char); - editor.ReplaceNode(newFieldYType, newType); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public char Y; - public string X2; - public char X3; -}"); - - var newFieldY2 = editor.Generator.FieldDeclaration("Y2", editor.Generator.TypeExpression(SpecialType.System_Boolean), Accessibility.Private); - editor.ReplaceNode(fieldX2, newFieldY2); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public char Y; - private bool Y2; - public char X3; -}"); - - var newFieldZ = editor.Generator.FieldDeclaration("Z", editor.Generator.TypeExpression(SpecialType.System_Boolean), Accessibility.Public); - editor.ReplaceNode(newFieldY, newFieldZ); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public bool Z; - private bool Y2; - public char X3; -}"); - - var originalFieldX3Type = fieldX3.DescendantNodes().Single(n => n.ToString() == "char"); - newType = editor.Generator.TypeExpression(SpecialType.System_Boolean); - editor.ReplaceNode(originalFieldX3Type, newType); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public bool Z; - private bool Y2; - public bool X3; -}"); - - editor.RemoveNode(newFieldY2); - editor.RemoveNode(fieldX3); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public bool Z; -}"); - } - - [Fact] - public void TestInsertAfterWithTracking() - { - // InsertAfter overload #1 - TestInsertAfterWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.InsertAfter(node, newNode); - }); - - // InsertAfter overload #2 - TestInsertAfterWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.InsertAfter(node, new[] { newNode }); - }); - } - - private void TestInsertAfterWithTrackingCore(Action insertAfterWithTracking) - { - var code = @" -public class C -{ - public int X; -}"; - - var cu = SyntaxFactory.ParseCompilationUnit(code); - var cls = cu.Members[0]; - - var editor = GetEditor(cu); - var fieldX = editor.Generator.GetMembers(cls)[0]; - var newFieldY = editor.Generator.FieldDeclaration("Y", editor.Generator.TypeExpression(SpecialType.System_String), Accessibility.Public); - insertAfterWithTracking(fieldX, newFieldY, editor); - - var newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public int X; - public string Y; -}"); - - var newFieldZ = editor.Generator.FieldDeclaration("Z", editor.Generator.TypeExpression(SpecialType.System_String), Accessibility.Public); - editor.ReplaceNode(newFieldY, newFieldZ); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public int X; - public string Z; -}"); - } - - [Fact] - public void TestInsertBeforeWithTracking() - { - // InsertBefore overload #1 - TestInsertBeforeWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.InsertBefore(node, newNode); - }); - - // InsertBefore overload #2 - TestInsertBeforeWithTrackingCore((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) => - { - editor.InsertBefore(node, new[] { newNode }); - }); - } - - private void TestInsertBeforeWithTrackingCore(Action insertBeforeWithTracking) - { - var code = @" -public class C -{ - public int X; -}"; - - var cu = SyntaxFactory.ParseCompilationUnit(code); - var cls = cu.Members[0]; - - var editor = GetEditor(cu); - var fieldX = editor.Generator.GetMembers(cls)[0]; - var newFieldY = editor.Generator.FieldDeclaration("Y", editor.Generator.TypeExpression(SpecialType.System_String), Accessibility.Public); - insertBeforeWithTracking(fieldX, newFieldY, editor); - - var newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public string Y; - public int X; -}"); - - var newFieldZ = editor.Generator.FieldDeclaration("Z", editor.Generator.TypeExpression(SpecialType.System_String), Accessibility.Public); - editor.ReplaceNode(newFieldY, newFieldZ); - - newRoot = editor.GetChangedRoot(); - VerifySyntax( - newRoot, - @" -public class C -{ - public string Z; - public int X; -}"); - } - [Fact] public void TestTrackNode() {