[Clangd] ExtractFunction: Don't extract body of enclosing function.

[SureYeaah]

Summary

This patch disable extraction of the body of the enclosing function.
void f() [[{}]]

Extracting this CompoundStmt would leave the enclosing function without
a body.

[kadircet]

LGTM, thanks

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

234

I believe it is not possible to have an EnclosingFunc without a body at this point. Let's make it an assertion instead.

[SureYeaah]

Added assert and removed fixme

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

 // between the attributes and the EnclosingFunction.
 llvm::Optional<SourceRange>
 computeEnclosingFuncRange(const FunctionDecl *EnclosingFunction,
                           const SourceManager &SM,
                           const LangOptions &LangOpts) {
   return toHalfOpenFileRange(SM, LangOpts, EnclosingFunction->getSourceRange());
 }
 
+// returns true if Child can be a single RootStmt being extracted from
+// EnclosingFunc.
+bool validSingleChild(const Node *Child, const FunctionDecl *EnclosingFunc) {
+  // Don't extract expressions.
+  // FIXME: We should extract expressions that are "statements" i.e. not
+  // subexpressions
+  if (Child->ASTNode.get<Expr>())
+    return false;
+  // Extracting the body of EnclosingFunc would remove it's definition.
+  assert(EnclosingFunc->hasBody() &&
+         "We should always be extracting from a function body.");
+  if (Child->ASTNode.get<Stmt>() == EnclosingFunc->getBody())
+    return false;
+  return true;
+}
+
 // FIXME: Check we're not extracting from the initializer/condition of a control
 // flow structure.
-// FIXME: Check that we don't extract the compound statement of the
-// enclosingFunction.
 llvm::Optional<ExtractionZone> findExtractionZone(const Node *CommonAnc,
                                                   const SourceManager &SM,
                                                   const LangOptions &LangOpts) {
   ExtractionZone ExtZone;
   ExtZone.Parent = getParentOfRootStmts(CommonAnc);
   if (!ExtZone.Parent || ExtZone.Parent->Children.empty())
     return llvm::None;
-  // Don't extract expressions.
-  // FIXME: We should extract expressions that are "statements" i.e. not
-  // subexpressions
-  if (ExtZone.Parent->Children.size() == 1 &&
-      ExtZone.getLastRootStmt()->ASTNode.get<Expr>())
-    return llvm::None;
   ExtZone.EnclosingFunction = findEnclosingFunction(ExtZone.Parent);
   if (!ExtZone.EnclosingFunction)
     return llvm::None;
+  // When there is a single RootStmt, we must check if it's valid for
+  // extraction.
+  if (ExtZone.Parent->Children.size() == 1 &&
+      !validSingleChild(ExtZone.getLastRootStmt(), ExtZone.EnclosingFunction))
+    return llvm::None;
   if (auto FuncRange =
           computeEnclosingFuncRange(ExtZone.EnclosingFunction, SM, LangOpts))
     ExtZone.EnclosingFuncRange = *FuncRange;
   if (auto ZoneRange = findZoneRange(ExtZone.Parent, SM, LangOpts))
     ExtZone.ZoneRange = *ZoneRange;
   if (ExtZone.EnclosingFuncRange.isInvalid() || ExtZone.ZoneRange.isInvalid())
     return llvm::None;
   ExtZone.generateRootStmts();

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

void f(const int c) {
   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");
+  // Don't extract if we select the entire function body (CompoundStmt).
+  std::string CompoundFailInput = R"cpp(
+    void f() [[{
+      int a;
+    }]]
+  )cpp";
+  EXPECT_EQ(apply(CompoundFailInput), "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"));