[Clangd] Ensure children are always RootStmt in ExtractFunction (Fixes #153)

[SureYeaah]

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.

[SureYeaah]

Added llmv_unreachable

[SureYeaah]

Moved Unittest

[kadircet]

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

110

can you also update the comment to mention

returns null if any child of the parent is not a root statement

111

can we restructure this into:

if (!CommonAnc) return nullptr;
const Node *Parent = nullptr;
switch(...) {
...Fill In Parent...
default:
llvm_unreachable(..)
}
return llvm::all_of(..)

[SureYeaah]

Address review comments

clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp

if (N->Selected == SelectionTree::Unselected && !N->ASTNode.get<DeclStmt>())
     return false;
   return true;
 }
 
 // Returns the (unselected) parent of all RootStmts given the commonAncestor.
 // 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
 // outside the selection range, ends outside as well.
 const Node *getParentOfRootStmts(const Node *CommonAnc) {
   if (!CommonAnc)
     return nullptr;
+  const Node* Parent = CommonAnc;
   switch (CommonAnc->Selected) {
-  case SelectionTree::Selection::Unselected:
-    // Ensure all Children are RootStmts.
-    return llvm::all_of(CommonAnc->Children, isRootStmt) ? CommonAnc : nullptr;
     case SelectionTree::Selection::Partial:
       // Treat Partially selected VarDecl as completely selected since
       // SelectionTree doesn't always select VarDecls correctly.
       // FIXME: Remove this after D66872 is upstream)
       if (!CommonAnc->ASTNode.get<VarDecl>())
         return nullptr;
       LLVM_FALLTHROUGH;
     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;
+      LLVM_FALLTHROUGH;
+    case SelectionTree::Selection::Unselected:
+      // Ensure all Children are RootStmts.
+      return llvm::all_of(Parent->Children, isRootStmt) ? Parent : nullptr;
   }
   llvm_unreachable("Unhandled SelectionTree::Selection enum");
 }
 
 // The ExtractionZone class forms a view of the code wrt Zone.
 struct ExtractionZone {
   // Parent of RootStatements being extracted.
   const Node *Parent = nullptr;

clang-tools-extra/clangd/unittests/TweakTests.cpp

TEST_F(ExtractFunctionTest, FunctionTest) {
   EXPECT_THAT(apply("int [[x = 0]];"), HasSubstr("extracted"));
   // FIXME: ExtractFunction should be unavailable inside loop construct
   // initalizer/condition.
   EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
   // Don't extract because needs hoisting.
   EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail"));
   // Don't extract return
   EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail"));
-  
 }
 
 TEST_F(ExtractFunctionTest, FileTest) {
   // Check all parameters are in order
   std::string ParameterCheckInput = R"cpp(
 struct Foo {
   int x;
 };
void f(const int c) {
 
   // FIXME: This should be extractable after selectionTree works correctly for
   // macros (currently it doesn't select anything for the following case)
   std::string MacroFailInput = R"cpp(
     #define F(BODY) void f() { BODY }
     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) {
   Context = Function;
   // We should be able to extract break/continue with a parent loop/switch.
   EXPECT_THAT(apply(" [[for(;;) if(1) break;]] "), HasSubstr("extracted"));
   EXPECT_THAT(apply(" for(;;) [[while(1) break;]] "), HasSubstr("extracted"));
   EXPECT_THAT(apply(" [[switch(1) { break; }]]"), HasSubstr("extracted"));