Address feedback

src/Workspaces/Core/Portable/Editing/SyntaxEditor.cs

@@ -49,16 +49,6 @@ private SyntaxNode ApplyTrackingToNewNode(SyntaxNode node)
             return node.TrackNodes(node.DescendantNodesAndSelf());
         }
 
-        private IEnumerable<SyntaxNode> ApplyTrackingToNewNodesIfRequired(IEnumerable<SyntaxNode> nodes, bool trackNodes)
-        {
-            if (!trackNodes)
-            {
-                return nodes;
-            }
-
-            return ApplyTrackingToNewNodes(nodes);
-        }
-
         private IEnumerable<SyntaxNode> ApplyTrackingToNewNodes(IEnumerable<SyntaxNode> nodes)
         {
             foreach (var node in nodes)
@@ -130,27 +120,17 @@ public void RemoveNode(SyntaxNode node, SyntaxRemoveOptions options)
         /// <param name="computeReplacement">A function that computes a replacement node. 
         /// The node passed into the compute function includes changes from prior edits. It will not appear as a descendant of the original root.</param>
         public void ReplaceNode(SyntaxNode node, Func<SyntaxNode, SyntaxGenerator, SyntaxNode> computeReplacement)
-            => ReplaceNode(node, computeReplacement, trackNewNode: false);
-
-        /// <summary>
-        /// Replace the specified node with a node produced by the function.
-        /// </summary>
-        /// <param name="node">The node to replace that already exists in the tree.</param>
-        /// <param name="computeReplacement">A function that computes a replacement node. 
-        /// The node passed into the compute function includes changes from prior edits. It will not appear as a descendant of the original root.</param>
-        /// <param name="trackNewNode">Indicates if the replacement node should be tracked for future edits.</param>
-        internal void ReplaceNode(SyntaxNode node, Func<SyntaxNode, SyntaxGenerator, SyntaxNode> computeReplacement, bool trackNewNode = false)
         {
             CheckNodeInOriginalTreeOrTracked(node);
-            _allowEditsOnLazilyCreatedTrackedNewNodes |= trackNewNode;
-            _changes.Add(new ReplaceChange(node, computeReplacement, ApplyTrackingToNewNode, trackNewNode));
+            _allowEditsOnLazilyCreatedTrackedNewNodes = true;
+            _changes.Add(new ReplaceChange(node, computeReplacement, ApplyTrackingToNewNode));
         }
 
-        internal void ReplaceNode<TArgument>(SyntaxNode node, Func<SyntaxNode, SyntaxGenerator, TArgument, SyntaxNode> computeReplacement, TArgument argument, bool trackNewNode = false)
+        internal void ReplaceNode<TArgument>(SyntaxNode node, Func<SyntaxNode, SyntaxGenerator, TArgument, SyntaxNode> computeReplacement, TArgument argument)
         {
             CheckNodeInOriginalTreeOrTracked(node);
-            _allowEditsOnLazilyCreatedTrackedNewNodes |= trackNewNode;
-            _changes.Add(new ReplaceChange<TArgument>(node, computeReplacement, argument, ApplyTrackingToNewNode, trackNewNode));
+            _allowEditsOnLazilyCreatedTrackedNewNodes = true;
+            _changes.Add(new ReplaceChange<TArgument>(node, computeReplacement, argument, ApplyTrackingToNewNode));
         }
 
         /// <summary>
@@ -159,15 +139,6 @@ internal void ReplaceNode<TArgument>(SyntaxNode node, Func<SyntaxNode, SyntaxGen
         /// <param name="node">The node to replace that already exists in the tree.</param>
         /// <param name="newNode">The new node that will be placed into the tree in the existing node's location.</param>
         public void ReplaceNode(SyntaxNode node, SyntaxNode newNode)
-            => ReplaceNode(node, newNode, trackNewNode: false);
-
-        /// <summary>
-        /// Replace the specified node with a different node.
-        /// </summary>
-        /// <param name="node">The node to replace that already exists in the tree.</param>
-        /// <param name="newNode">The new node that will be placed into the tree in the existing node's location.</param>
-        /// <param name="trackNewNode">Flag to indicate if the new node should be tracked for further edits.</param>
-        internal void ReplaceNode(SyntaxNode node, SyntaxNode newNode, bool trackNewNode)
         {
             CheckNodeInOriginalTreeOrTracked(node);
             if (node == newNode)
@@ -175,12 +146,8 @@ internal void ReplaceNode(SyntaxNode node, SyntaxNode newNode, bool trackNewNode
                 return;
             }
 
-            if (trackNewNode)
-            {
-                newNode = ApplyTrackingToNewNode(newNode);
-            }
-
-            _changes.Add(new ReplaceChange(node, (n, g) => newNode, ApplyTrackingToNewNode, trackNewNode));
+            newNode = ApplyTrackingToNewNode(newNode);
+            _changes.Add(new ReplaceChange(node, (n, g) => newNode, ApplyTrackingToNewNode));
         }
 
         /// <summary>
@@ -189,18 +156,9 @@ internal void ReplaceNode(SyntaxNode node, SyntaxNode newNode, bool trackNewNode
         /// <param name="node">The node already existing in the tree that the new nodes will be placed before. This must be a node this is contained within a syntax list.</param>
         /// <param name="newNodes">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.</param>
         public void InsertBefore(SyntaxNode node, IEnumerable<SyntaxNode> newNodes)
-            => InsertBefore(node, newNodes, trackNewNodes: false);
-
-        /// <summary>
-        /// Insert the new nodes before the specified node already existing in the tree.
-        /// </summary>
-        /// <param name="node">The node already existing in the tree that the new nodes will be placed before. This must be a node this is contained within a syntax list.</param>
-        /// <param name="newNodes">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.</param>
-        /// <param name="trackNewNodes">Flag to indicate if the new nodes should be tracked for further edits.</param>
-        internal void InsertBefore(SyntaxNode node, IEnumerable<SyntaxNode> newNodes, bool trackNewNodes)
         {
             CheckNodeInOriginalTreeOrTracked(node);
-            newNodes = ApplyTrackingToNewNodesIfRequired(newNodes, trackNewNodes);
+            newNodes = ApplyTrackingToNewNodes(newNodes);
             _changes.Add(new InsertChange(node, newNodes, isBefore: true));
         }
 
@@ -210,16 +168,7 @@ internal void InsertBefore(SyntaxNode node, IEnumerable<SyntaxNode> newNodes, bo
         /// <param name="node">The node already existing in the tree that the new nodes will be placed before. This must be a node this is contained within a syntax list.</param>
         /// <param name="newNode">The node to place before the existing node. This node must be of a compatible type to be placed in the same list containing the existing node.</param>
         public void InsertBefore(SyntaxNode node, SyntaxNode newNode)
-            => InsertBefore(node, newNode, trackNewNode: false);
-
-        /// <summary>
-        /// Insert the new node before the specified node already existing in the tree.
-        /// </summary>
-        /// <param name="node">The node already existing in the tree that the new nodes will be placed before. This must be a node this is contained within a syntax list.</param>
-        /// <param name="newNode">The node to place before the existing node. This node must be of a compatible type to be placed in the same list containing the existing node.</param>
-        /// <param name="trackNewNode">Flag to indicate if the new node should be tracked for further edits.</param>
-        internal void InsertBefore(SyntaxNode node, SyntaxNode newNode, bool trackNewNode)
-            => this.InsertBefore(node, new[] { newNode }, trackNewNode);
+            => InsertBefore(node, new[] { newNode });
 
         /// <summary>
         /// Insert the new nodes after the specified node already existing in the tree.
@@ -227,18 +176,9 @@ internal void InsertBefore(SyntaxNode node, SyntaxNode newNode, bool trackNewNod
         /// <param name="node">The node already existing in the tree that the new nodes will be placed after. This must be a node this is contained within a syntax list.</param>
         /// <param name="newNodes">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.</param>
         public void InsertAfter(SyntaxNode node, IEnumerable<SyntaxNode> newNodes)
-            => InsertAfter(node, newNodes, trackNewNodes: false);
-
-        /// <summary>
-        /// Insert the new nodes after the specified node already existing in the tree.
-        /// </summary>
-        /// <param name="node">The node already existing in the tree that the new nodes will be placed after. This must be a node this is contained within a syntax list.</param>
-        /// <param name="newNodes">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.</param>
-        /// <param name="trackNewNodes">Flag to indicate if the new nodes should be tracked for further edits.</param>
-        internal void InsertAfter(SyntaxNode node, IEnumerable<SyntaxNode> newNodes, bool trackNewNodes)
         {
             CheckNodeInOriginalTreeOrTracked(node);
-            newNodes = ApplyTrackingToNewNodesIfRequired(newNodes, trackNewNodes);
+            newNodes = ApplyTrackingToNewNodes(newNodes);
             _changes.Add(new InsertChange(node, newNodes, isBefore: false));
         }
 
@@ -248,30 +188,32 @@ internal void InsertAfter(SyntaxNode node, IEnumerable<SyntaxNode> newNodes, boo
         /// <param name="node">The node already existing in the tree that the new nodes will be placed after. This must be a node this is contained within a syntax list.</param>
         /// <param name="newNode">The node to place after the existing node. This node must be of a compatible type to be placed in the same list containing the existing node.</param>
         public void InsertAfter(SyntaxNode node, SyntaxNode newNode)
-            => this.InsertAfter(node, newNode, trackNewNode: false);
-
-        /// <summary>
-        /// Insert the new node after the specified node already existing in the tree.
-        /// </summary>
-        /// <param name="node">The node already existing in the tree that the new nodes will be placed after. This must be a node this is contained within a syntax list.</param>
-        /// <param name="newNode">The node to place after the existing node. This node must be of a compatible type to be placed in the same list containing the existing node.</param>
-        /// <param name="trackNewNode">Flag to indicate if the new node should be tracked for further edits.</param>
-        internal void InsertAfter(SyntaxNode node, SyntaxNode newNode, bool trackNewNode)
-            => InsertAfter(node, new[] { newNode }, trackNewNodes: trackNewNode);
-
-        private void ApplyTrackingToNodesToInsert(params SyntaxNode[] nodes)
-        {
-
-        }
+            => this.InsertAfter(node, new[] { newNode });
 
         private void CheckNodeInOriginalTreeOrTracked(SyntaxNode node)
         {
-            if (!OriginalRoot.Contains(node) &&
-                !_allowEditsOnLazilyCreatedTrackedNewNodes &&
-                (_lazyTrackedNewNodesOpt == null || !_lazyTrackedNewNodesOpt.Contains(node)))
+            if (OriginalRoot.Contains(node))
+            {
+                // Node is contained in the original tree.
+                return;
+            }
+
+            if (_allowEditsOnLazilyCreatedTrackedNewNodes)
             {
-                throw new ArgumentException(WorkspacesResources.The_node_is_not_part_of_the_tree, nameof(node));
+                // 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));
         }
 
         private abstract class Change
@@ -319,25 +261,22 @@ private class ReplaceChange : Change
         {
             private readonly Func<SyntaxNode, SyntaxGenerator, SyntaxNode> _modifier;
             private readonly Func<SyntaxNode, SyntaxNode> _applyTrackingToNewNode;
-            private readonly bool _trackNewNode;
 
             public ReplaceChange(
                 SyntaxNode node,
                 Func<SyntaxNode, SyntaxGenerator, SyntaxNode> modifier,
-                Func<SyntaxNode, SyntaxNode> applyTrackingToNewNode,
-                bool trackNewNode)
+                Func<SyntaxNode, SyntaxNode> applyTrackingToNewNode)
                 : base(node)
             {
                 _modifier = modifier;
                 _applyTrackingToNewNode = applyTrackingToNewNode;
-                _trackNewNode = trackNewNode;
             }
 
             public override SyntaxNode Apply(SyntaxNode root, SyntaxGenerator generator)
             {
                 var current = root.GetCurrentNode(this.Node);
                 var newNode = _modifier(current, generator);
-                newNode = _trackNewNode ? _applyTrackingToNewNode(newNode) : newNode;
+                newNode = _applyTrackingToNewNode(newNode);
                 return generator.ReplaceNode(root, current, newNode);
             }
         }
@@ -347,27 +286,24 @@ private class ReplaceChange<TArgument> : Change
             private readonly Func<SyntaxNode, SyntaxGenerator, TArgument, SyntaxNode> _modifier;
             private readonly TArgument _argument;
             private readonly Func<SyntaxNode, SyntaxNode> _applyTrackingToNewNode;
-            private readonly bool _trackNewNode;
 
             public ReplaceChange(
                 SyntaxNode node,
                 Func<SyntaxNode, SyntaxGenerator, TArgument, SyntaxNode> modifier,
                 TArgument argument,
-                Func<SyntaxNode, SyntaxNode> applyTrackingToNewNode,
-                bool trackNewNode)
+                Func<SyntaxNode, SyntaxNode> applyTrackingToNewNode)
                 : base(node)
             {
                 _modifier = modifier;
                 _argument = argument;
                 _applyTrackingToNewNode = applyTrackingToNewNode;
-                _trackNewNode = trackNewNode;
             }
 
             public override SyntaxNode Apply(SyntaxNode root, SyntaxGenerator generator)
             {
                 var current = root.GetCurrentNode(this.Node);
                 var newNode = _modifier(current, generator, _argument);
-                newNode = _trackNewNode ? _applyTrackingToNewNode(newNode) : newNode;
+                newNode = _applyTrackingToNewNode(newNode);
                 return generator.ReplaceNode(root, current, newNode);
             }
         }

src/Workspaces/CoreTest/Editting/SyntaxEditorTests.cs

@@ -143,15 +143,13 @@ public async Task TestReplaceWithTracking()
             // ReplaceNode overload #1
             await TestReplaceWithTrackingCoreAsync((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) =>
             {
-                editor.ReplaceNode(node, newNode, trackNewNode: true);
+                editor.ReplaceNode(node, newNode);
             });
 
             // ReplaceNode overload #2
             await TestReplaceWithTrackingCoreAsync((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) =>
             {
-                editor.ReplaceNode(node,
-                    computeReplacement: (originalNode, generator) => newNode,
-                    trackNewNode: true);
+                editor.ReplaceNode(node, computeReplacement: (originalNode, generator) => newNode);
             });
 
             // ReplaceNode overload #3
@@ -159,8 +157,7 @@ public async Task TestReplaceWithTracking()
             {
                 editor.ReplaceNode(node,
                      computeReplacement: (originalNode, generator, argument) => newNode,
-                     argument: (object)null,
-                     trackNewNode: true);
+                     argument: (object)null);
             });
         }
 
@@ -218,13 +215,13 @@ public async Task TestInsertAfterWithTracking()
             // InsertAfter overload #1
             await TestInterAfterWithTrackingCoreAsync((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) =>
             {
-                editor.InsertAfter(node, newNode, trackNewNode: true);
+                editor.InsertAfter(node, newNode);
             });
 
             // InsertAfter overload #2
             await TestInterAfterWithTrackingCoreAsync((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) =>
             {
-                editor.InsertAfter(node, new[] { newNode }, trackNewNodes: true);
+                editor.InsertAfter(node, new[] { newNode });
             });
         }
 
@@ -274,13 +271,13 @@ public async Task TestInsertBeforeWithTracking()
             // InsertBefore overload #1
             await TestInterBeforeWithTrackingCoreAsync((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) =>
             {
-                editor.InsertBefore(node, newNode, trackNewNode: true);
+                editor.InsertBefore(node, newNode);
             });
 
             // InsertBefore overload #2
             await TestInterBeforeWithTrackingCoreAsync((SyntaxNode node, SyntaxNode newNode, SyntaxEditor editor) =>
             {
-                editor.InsertBefore(node, new[] { newNode }, trackNewNodes: true);
+                editor.InsertBefore(node, new[] { newNode });
             });
         }