[clang-tools-extra] r373471 - [Clangd] Ensure children are always RootStmt in ExtractFunction (Fixes #153)

[Shaurya Gupta via cfe-commits]

Author: sureyeaah
Date: Wed Oct  2 06:51:06 2019
New Revision: 373471

URL: http://llvm.org/viewvc/llvm-project?rev=373471&view=rev
Log:
[Clangd] Ensure children are always RootStmt in ExtractFunction (Fixes #153)

Summary:
We weren't always checking if children are RootStmts in ExtractFunction.

For `void f([[int a]]);`, the ParmVarDecl appeared as a RootStmt since
we didn't perform the check and ended up being casted to a (null) Stmt.

Reviewers: sammccall, kadircet

Subscribers: kristof.beyls, ilya-biryukov, MaskRay, jkorous, arphaman, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D68182

Modified:
    clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
    clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp

Modified: clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp?rev=373471&r1=373470&r2=373471&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp (original)
+++ clang-tools-extra/trunk/clangd/refactor/tweaks/ExtractFunction.cpp Wed Oct  2 06:51:06 2019
@@ -104,6 +104,11 @@ bool isRootStmt(const Node *N) {
 }
 
 // Returns the (unselected) parent of all RootStmts given the commonAncestor.
+// Returns null if:
+// 1. any node is partially selected
+// 2. If all completely selected nodes don't have the same common parent
+// 3. Any child of Parent isn't a RootStmt.
+// Returns null if any child is not a RootStmt.
 // We only support extraction of RootStmts since it allows us to extract without
 // having to change the selection range. Also, this means that any scope that
 // begins in selection range, ends in selection range and any scope that begins
@@ -111,25 +116,30 @@ bool isRootStmt(const Node *N) {
 const Node *getParentOfRootStmts(const Node *CommonAnc) {
   if (!CommonAnc)
     return nullptr;
+  const Node *Parent = nullptr;
   switch (CommonAnc->Selected) {
   case SelectionTree::Selection::Unselected:
     // Typicaly a block, with the { and } unselected, could also be ForStmt etc
     // Ensure all Children are RootStmts.
-    return llvm::all_of(CommonAnc->Children, isRootStmt) ? CommonAnc : nullptr;
+    Parent = CommonAnc;
+    break;
   case SelectionTree::Selection::Partial:
     // Only a fully-selected single statement can be selected.
     return nullptr;
   case SelectionTree::Selection::Complete:
     // If the Common Ancestor is completely selected, then it's a root statement
     // and its parent will be unselected.
-    const Node *Parent = CommonAnc->Parent;
+    Parent = CommonAnc->Parent;
     // If parent is a DeclStmt, even though it's unselected, we consider it a
     // root statement and return its parent. This is done because the VarDecls
     // claim the entire selection range of the Declaration and DeclStmt is
     // always unselected.
-    return Parent->ASTNode.get<DeclStmt>() ? Parent->Parent : Parent;
+    if (Parent->ASTNode.get<DeclStmt>())
+      Parent = Parent->Parent;
+    break;
   }
-  llvm_unreachable("Unhandled SelectionTree::Selection enum");
+  // Ensure all Children are RootStmts.
+  return llvm::all_of(Parent->Children, isRootStmt) ? Parent : nullptr;
 }
 
 // The ExtractionZone class forms a view of the code wrt Zone.

Modified: clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp?rev=373471&r1=373470&r2=373471&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/TweakTests.cpp Wed Oct  2 06:51:06 2019
@@ -629,6 +629,9 @@ void f(const int c) {
     F ([[int x = 0;]])
   )cpp";
   EXPECT_EQ(apply(MacroFailInput), "unavailable");
+
+  // Shouldn't crash.
+  EXPECT_EQ(apply("void f([[int a]]);"), "unavailable");
 }
 
 TEST_F(ExtractFunctionTest, ControlFlow) {