[clangd] ExtractFunction: disable on regions that sometimes, but not always return.

[adamcz]

Summary

apply() will fail in those cases, so it's better to detect it in
prepare() already and hide code action from the user.

This was especially annoying on code bases that use a lot of
RETURN_IF_ERROR-like macros.

[usaxena95]

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

711

s/ReturnStms/ReturnStmts

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

613

super super nit: !x instead of x.

[usaxena95]

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

724

nit: s/auto/Stmt

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

613

Oops this is fine. Ignore this comment.

[adamcz]

addressed review comments

[adamcz]

Thanks!

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

 }
 
 tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc,
                                               const SourceManager &SM) {
   std::string FunctionDef = ExtractedFunc.renderDefinition(SM);
   return tooling::Replacement(SM, ExtractedFunc.InsertionPoint, 0, FunctionDef);
 }
 
+// Returns true if ExtZone contains any ReturnStms.
+bool hasReturnStmt(const ExtractionZone &ExtZone) {
+  class ReturnStmtVisitor
+      : public clang::RecursiveASTVisitor<ReturnStmtVisitor> {
+  public:
+    bool VisitReturnStmt(ReturnStmt *Return) {
+      Found = true;
+      return false; // We found the answer, abort the scan.
+    }
+    bool Found = false;
+  };
+
+  ReturnStmtVisitor V;
+  for (const auto *RootStmt : ExtZone.RootStmts) {
+    V.TraverseStmt(const_cast<Stmt *>(RootStmt));
+    if (V.Found)
+      break;
+  }
+  return V.Found;
+}
+
 bool ExtractFunction::prepare(const Selection &Inputs) {
   const LangOptions &LangOpts = Inputs.AST->getLangOpts();
   if (!LangOpts.CPlusPlus)
     return false;
   const Node *CommonAnc = Inputs.ASTSelection.commonAncestor();
   const SourceManager &SM = Inputs.AST->getSourceManager();
   auto MaybeExtZone = findExtractionZone(CommonAnc, SM, LangOpts);
+  if (!MaybeExtZone ||
+      (hasReturnStmt(*MaybeExtZone) && !alwaysReturns(*MaybeExtZone)))
+    return false;
+
   // FIXME: Get rid of this check once we support hoisting.
-  if (!MaybeExtZone || MaybeExtZone->requiresHoisting(SM))
+  if (MaybeExtZone->requiresHoisting(SM))
     return false;
 
   ExtZone = std::move(*MaybeExtZone);
   return true;
 }
 
 Expected<Tweak::Effect> ExtractFunction::apply(const Selection &Inputs) {
   const SourceManager &SM = Inputs.AST->getSourceManager();

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

TEST_F(ExtractFunctionTest, FunctionTest) {
   // lead to break being included in the extraction zone.
   EXPECT_THAT(apply("for(;;) { [[int x;]]break; }"), HasSubstr("extracted"));
   // FIXME: ExtractFunction should be unavailable inside loop construct
   // initializer/condition.
   EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("extracted"));
   // Extract certain return
   EXPECT_THAT(apply(" if(true) [[{ return; }]] "), HasSubstr("extracted"));
   // Don't extract uncertain return
-  EXPECT_THAT(apply(" if(true) [[if (false) return;]] "), StartsWith("fail"));
+  EXPECT_THAT(apply(" if(true) [[if (false) return;]] "),
+              StartsWith("unavailable"));
+  EXPECT_THAT(
+      apply("#define RETURN_IF_ERROR(x) if (x) return\nRETU^RN_IF_ERROR(4);"),
+      StartsWith("unavailable"));
 
   FileName = "a.c";
   EXPECT_THAT(apply(" for([[int i = 0;]];);"), HasSubstr("unavailable"));
 }
 
 TEST_F(ExtractFunctionTest, FileTest) {
   // Check all parameters are in order
   std::string ParameterCheckInput = R"cpp(