Merge pull request #14040 from CyrusNajmabadi/moveTypeCrash

Ensure we dont' try to remove both a parent and child from the tree when moving a type.

Fixes #13969

src/EditorFeatures/CSharpTest/CodeActions/MoveType/MoveTypeTests.MoveToNewFile.cs

@@ -549,6 +549,118 @@ class Class2
             public void InnerMethod() { }
         }
     }
+}";
+            await TestMoveTypeToNewFileAsync(code, codeAfterMove, expectedDocumentName, destinationDocumentText);
+        }
+
+        [WpfFact, Trait(Traits.Feature, Traits.Features.CodeActionsMoveType)]
+        [WorkItem(13969, "https://github.com/dotnet/roslyn/issues/13969")]
+        public async Task MoveTypeInFileWithComplexHierarchy()
+        {
+            var code =
+@"namespace OuterN1.N1
+{
+    namespace InnerN2.N2
+    {
+        class OuterClass1
+        {
+            class InnerClass2
+            {
+            }
+        }
+    }
+
+    namespace InnerN3.N3
+    {
+        class OuterClass2
+        {
+            [||]class InnerClass2 
+            {
+                class InnerClass3
+                {
+                }
+            }
+
+            class InnerClass4
+            {
+            }
+        }
+
+        class OuterClass3
+        {
+        }
+    }
+}
+
+namespace OuterN2.N2
+{
+    namespace InnerN3.N3
+    {
+        class OuterClass5 {
+            class InnerClass6 {
+            }
+        }
+    }
+}
+";
+
+            var codeAfterMove =
+@"namespace OuterN1.N1
+{
+    namespace InnerN2.N2
+    {
+        class OuterClass1
+        {
+            class InnerClass2
+            {
+            }
+        }
+    }
+
+    namespace InnerN3.N3
+    {
+        partial class OuterClass2
+        {
+            class InnerClass4
+            {
+            }
+        }
+
+        class OuterClass3
+        {
+        }
+    }
+}
+
+namespace OuterN2.N2
+{
+    namespace InnerN3.N3
+    {
+        class OuterClass5 {
+            class InnerClass6 {
+            }
+        }
+    }
+}";
+
+            var expectedDocumentName = "InnerClass2.cs";
+
+            var destinationDocumentText =
+@"
+namespace OuterN1.N1
+{
+    namespace InnerN3.N3
+    {
+        partial class OuterClass2
+        {
+            class InnerClass2 
+            {
+                class InnerClass3
+                {
+                }
+            }
+        }
+    }
 }";
             await TestMoveTypeToNewFileAsync(code, codeAfterMove, expectedDocumentName, destinationDocumentText);
         }

src/Features/Core/Portable/CodeRefactorings/MoveType/AbstractMoveTypeService.MoveTypeEditor.cs

@@ -137,7 +137,7 @@ private async Task<Solution> RemoveTypeFromSourceDocumentAsync(Document sourceDo
             /// </summary>
             /// <param name="root">root, of the syntax tree of forked document</param>
             /// <returns>list of syntax nodes, to be removed from the forked copy.</returns>
-            private IEnumerable<SyntaxNode> GetMembersToRemove(SyntaxNode root)
+            private ISet<SyntaxNode> GetMembersToRemove(SyntaxNode root)
             {
                 var spine = new HashSet<SyntaxNode>();
 
@@ -146,31 +146,39 @@ private IEnumerable<SyntaxNode> GetMembersToRemove(SyntaxNode root)
 
                 // get potential namespace, types and members to remove.
                 var removableCandidates = root
-                    .DescendantNodes(n => DescendIntoChildren(n, spine.Contains(n)))
-                    .Where(n => FilterToTopLevelMembers(n, State.TypeNode));
+                    .DescendantNodes(n => spine.Contains(n))
+                    .Where(n => FilterToTopLevelMembers(n, State.TypeNode)).ToSet();
 
                 // diff candidates with items we want to keep.
-                return removableCandidates.Except(spine);
-            }
+                removableCandidates.ExceptWith(spine);
 
-            private static bool DescendIntoChildren(SyntaxNode node, bool shouldDescendIntoType)
-            {
-                // 1. get top level types and namespaces to remove.
-                // 2. descend into types and get members to remove, only if type is part of spine, which means
-                //    we'll be keeping the type declaration but not other members, in the new file.
-                return node is TCompilationUnitSyntax
-                    || node is TNamespaceDeclarationSyntax
-                    || (node is TTypeDeclarationSyntax && shouldDescendIntoType);
+#if DEBUG
+                // None of the nodes we're removing should also have any of their parent
+                // nodes removed.  If that happened we could get a crash by first trying to remove
+                // the parent, then trying to remove the child.
+                foreach (var node in removableCandidates)
+                {
+                    foreach (var ancestor in node.GetAncestors())
+                    {
+                        Debug.Assert(!removableCandidates.Contains(ancestor));
+                    }
+                }
+#endif
+
+                return removableCandidates;
             }
 
             private static bool FilterToTopLevelMembers(SyntaxNode node, SyntaxNode typeNode)
             {
-                // It is a type declaration that is not the node we've moving
-                // or its a container namespace, or a member declaration that is not a type,
-                // thereby ignoring other stuff like statements and identifiers.
-                return node is TTypeDeclarationSyntax
-                    ? !node.Equals(typeNode)
-                    : (node is TNamespaceDeclarationSyntax || node is TMemberDeclarationSyntax);
+                // We never filter out the actual node we're trying to keep around.
+                if (node == typeNode)
+                {
+                    return false;
+                }
+
+                return node is TTypeDeclarationSyntax ||
+                       node is TMemberDeclarationSyntax ||
+                       node is TNamespaceDeclarationSyntax;
             }
 
             /// <summary>