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

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.

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

diff --git a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
index 18fe7bf..8eec428 100644 (file)
--- a/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
+++ b/clang-tools-extra/clangd/refactor/tweaks/ExtractFunction.cpp
@@ -708,6 +708,27 @@ tooling::Replacement createFunctionDefinition(const NewFunction &ExtractedFunc,
   return tooling::Replacement(SM, ExtractedFunc.InsertionPoint, 0, FunctionDef);
 }
 
+// Returns true if ExtZone contains any ReturnStmts.
+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 Stmt *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)
@@ -715,8 +736,12 @@ bool ExtractFunction::prepare(const Selection &Inputs) {
   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);

diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index 07f061b..85edd92 100644 (file)
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TweakTests.cpp
@@ -607,7 +607,11 @@ TEST_F(ExtractFunctionTest, FunctionTest) {
   // 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"));