[clangd] Allow "move function body out-of-line" in non-header files (#69704)

ckandeler · web-flow · commit 955c2237629a · 2024-06-03T14:48:53.000+02:00
Moving the body of member functions out-of-line makes sense for classes
defined in implementation files too.
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
@@ -179,14 +179,11 @@ deleteTokensWithKind(const syntax::TokenBuffer &TokBuf, tok::TokenKind Kind,
 // looked up in the context containing the function/method.
 // FIXME: Drop attributes in function signature.
 llvm::Expected<std::string>
-getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
+getFunctionSourceCode(const FunctionDecl *FD, const DeclContext *TargetContext,
                       const syntax::TokenBuffer &TokBuf,
                       const HeuristicResolver *Resolver) {
   auto &AST = FD->getASTContext();
   auto &SM = AST.getSourceManager();
-  auto TargetContext = findContextForNS(TargetNamespace, FD->getDeclContext());
-  if (!TargetContext)
-    return error("define outline: couldn't find a context for target");
 
   llvm::Error Errors = llvm::Error::success();
   tooling::Replacements DeclarationCleanups;
@@ -216,7 +213,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
         }
         const NamedDecl *ND = Ref.Targets.front();
         const std::string Qualifier =
-            getQualification(AST, *TargetContext,
+            getQualification(AST, TargetContext,
                              SM.getLocForStartOfFile(SM.getMainFileID()), ND);
         if (auto Err = DeclarationCleanups.add(
                 tooling::Replacement(SM, Ref.NameLoc, 0, Qualifier)))
@@ -232,7 +229,7 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
   if (const auto *Destructor = llvm::dyn_cast<CXXDestructorDecl>(FD)) {
     if (auto Err = DeclarationCleanups.add(tooling::Replacement(
             SM, Destructor->getLocation(), 0,
-            getQualification(AST, *TargetContext,
+            getQualification(AST, TargetContext,
                              SM.getLocForStartOfFile(SM.getMainFileID()),
                              Destructor))))
       Errors = llvm::joinErrors(std::move(Errors), std::move(Err));
@@ -319,29 +316,9 @@ getFunctionSourceCode(const FunctionDecl *FD, llvm::StringRef TargetNamespace,
 }
 
 struct InsertionPoint {
-  std::string EnclosingNamespace;
+  const DeclContext *EnclosingNamespace = nullptr;
   size_t Offset;
 };
-// Returns the most natural insertion point for \p QualifiedName in \p Contents.
-// This currently cares about only the namespace proximity, but in feature it
-// should also try to follow ordering of declarations. For example, if decls
-// come in order `foo, bar, baz` then this function should return some point
-// between foo and baz for inserting bar.
-llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
-                                                 llvm::StringRef QualifiedName,
-                                                 const LangOptions &LangOpts) {
-  auto Region = getEligiblePoints(Contents, QualifiedName, LangOpts);
-
-  assert(!Region.EligiblePoints.empty());
-  // FIXME: This selection can be made smarter by looking at the definition
-  // locations for adjacent decls to Source. Unfortunately pseudo parsing in
-  // getEligibleRegions only knows about namespace begin/end events so we
-  // can't match function start/end positions yet.
-  auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
-  if (!Offset)
-    return Offset.takeError();
-  return InsertionPoint{Region.EnclosingNamespace, *Offset};
-}
 
 // Returns the range that should be deleted from declaration, which always
 // contains function body. In addition to that it might contain constructor
@@ -409,14 +386,9 @@ class DefineOutline : public Tweak {
   }
 
   bool prepare(const Selection &Sel) override {
-    // Bail out if we are not in a header file.
-    // FIXME: We might want to consider moving method definitions below class
-    // definition even if we are inside a source file.
-    if (!isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
-                      Sel.AST->getLangOpts()))
-      return false;
-
+    SameFile = !isHeaderFile(Sel.AST->tuPath(), Sel.AST->getLangOpts());
     Source = getSelectedFunction(Sel.ASTSelection.commonAncestor());
+
     // Bail out if the selection is not a in-line function definition.
     if (!Source || !Source->doesThisDeclarationHaveABody() ||
         Source->isOutOfLine())
@@ -429,19 +401,24 @@ class DefineOutline : public Tweak {
     if (Source->getTemplateSpecializationInfo())
       return false;
 
-    if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) {
-      // Bail out in templated classes, as it is hard to spell the class name,
-      // i.e if the template parameter is unnamed.
-      if (MD->getParent()->isTemplated())
-        return false;
-
-      // The refactoring is meaningless for unnamed classes and definitions
-      // within unnamed namespaces.
-      for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
-        if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
-          if (ND->getDeclName().isEmpty())
-            return false;
-        }
+    auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source);
+    if (!MD) {
+      // Can't outline free-standing functions in the same file.
+      return !SameFile;
+    }
+
+    // Bail out in templated classes, as it is hard to spell the class name,
+    // i.e if the template parameter is unnamed.
+    if (MD->getParent()->isTemplated())
+      return false;
+
+    // The refactoring is meaningless for unnamed classes and namespaces,
+    // unless we're outlining in the same file
+    for (const DeclContext *DC = MD->getParent(); DC; DC = DC->getParent()) {
+      if (auto *ND = llvm::dyn_cast<NamedDecl>(DC)) {
+        if (ND->getDeclName().isEmpty() &&
+            (!SameFile || !llvm::dyn_cast<NamespaceDecl>(ND)))
+          return false;
       }
     }
 
@@ -453,8 +430,8 @@ class DefineOutline : public Tweak {
 
   Expected<Effect> apply(const Selection &Sel) override {
     const SourceManager &SM = Sel.AST->getSourceManager();
-    auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);
-
+    auto CCFile = SameFile ? Sel.AST->tuPath().str()
+                           : getSourceFile(Sel.AST->tuPath(), Sel);
     if (!CCFile)
       return error("Couldn't find a suitable implementation file.");
     assert(Sel.FS && "FS Must be set in apply");
@@ -464,8 +441,7 @@ class DefineOutline : public Tweak {
     if (!Buffer)
       return llvm::errorCodeToError(Buffer.getError());
     auto Contents = Buffer->get()->getBuffer();
-    auto InsertionPoint = getInsertionPoint(
-        Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+    auto InsertionPoint = getInsertionPoint(Contents, Sel);
     if (!InsertionPoint)
       return InsertionPoint.takeError();
 
@@ -499,17 +475,77 @@ class DefineOutline : public Tweak {
       HeaderUpdates = HeaderUpdates.merge(*DelInline);
     }
 
-    auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
-    if (!HeaderFE)
-      return HeaderFE.takeError();
-
-    Effect->ApplyEdits.try_emplace(HeaderFE->first,
-                                   std::move(HeaderFE->second));
+    if (SameFile) {
+      tooling::Replacements &R = Effect->ApplyEdits[*CCFile].Replacements;
+      R = R.merge(HeaderUpdates);
+    } else {
+      auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(), HeaderUpdates);
+      if (!HeaderFE)
+        return HeaderFE.takeError();
+      Effect->ApplyEdits.try_emplace(HeaderFE->first,
+                                     std::move(HeaderFE->second));
+    }
     return std::move(*Effect);
   }
 
+  // Returns the most natural insertion point for \p QualifiedName in \p
+  // Contents. This currently cares about only the namespace proximity, but in
+  // feature it should also try to follow ordering of declarations. For example,
+  // if decls come in order `foo, bar, baz` then this function should return
+  // some point between foo and baz for inserting bar.
+  // FIXME: The selection can be made smarter by looking at the definition
+  // locations for adjacent decls to Source. Unfortunately pseudo parsing in
+  // getEligibleRegions only knows about namespace begin/end events so we
+  // can't match function start/end positions yet.
+  llvm::Expected<InsertionPoint> getInsertionPoint(llvm::StringRef Contents,
+                                                   const Selection &Sel) {
+    // If the definition goes to the same file and there is a namespace,
+    // we should (and, in the case of anonymous namespaces, need to)
+    // put the definition into the original namespace block.
+    if (SameFile) {
+      auto *Klass = Source->getDeclContext()->getOuterLexicalRecordContext();
+      if (!Klass)
+        return error("moving to same file not supported for free functions");
+      const SourceLocation EndLoc = Klass->getBraceRange().getEnd();
+      const auto &TokBuf = Sel.AST->getTokens();
+      auto Tokens = TokBuf.expandedTokens();
+      auto It = llvm::lower_bound(
+          Tokens, EndLoc, [](const syntax::Token &Tok, SourceLocation EndLoc) {
+            return Tok.location() < EndLoc;
+          });
+      while (It != Tokens.end()) {
+        if (It->kind() != tok::semi) {
+          ++It;
+          continue;
+        }
+        unsigned Offset = Sel.AST->getSourceManager()
+                              .getDecomposedLoc(It->endLocation())
+                              .second;
+        return InsertionPoint{Klass->getEnclosingNamespaceContext(), Offset};
+      }
+      return error(
+          "failed to determine insertion location: no end of class found");
+    }
+
+    auto Region = getEligiblePoints(
+        Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+
+    assert(!Region.EligiblePoints.empty());
+    auto Offset = positionToOffset(Contents, Region.EligiblePoints.back());
+    if (!Offset)
+      return Offset.takeError();
+
+    auto TargetContext =
+        findContextForNS(Region.EnclosingNamespace, Source->getDeclContext());
+    if (!TargetContext)
+      return error("define outline: couldn't find a context for target");
+
+    return InsertionPoint{*TargetContext, *Offset};
+  }
+
 private:
   const FunctionDecl *Source = nullptr;
+  bool SameFile = false;
 };
 
 REGISTER_TWEAK(DefineOutline)
diff --git a/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp b/clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp
@@ -19,12 +19,47 @@ TWEAK_TEST(DefineOutline);
 
 TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
   FileName = "Test.cpp";
-  // Not available unless in a header file.
+  // Not available for free function unless in a header file.
   EXPECT_UNAVAILABLE(R"cpp(
     [[void [[f^o^o]]() [[{
       return;
     }]]]])cpp");
 
+  // Available in soure file.
+  EXPECT_AVAILABLE(R"cpp(
+    struct Foo {
+      void f^oo() {}
+    };
+  )cpp");
+
+  // Available within named namespace in source file.
+  EXPECT_AVAILABLE(R"cpp(
+    namespace N {
+      struct Foo {
+        void f^oo() {}
+      };
+    } // namespace N
+  )cpp");
+
+  // Available within anonymous namespace in source file.
+  EXPECT_AVAILABLE(R"cpp(
+    namespace {
+      struct Foo {
+        void f^oo() {}
+      };
+    } // namespace
+  )cpp");
+
+  // Not available for out-of-line method.
+  EXPECT_UNAVAILABLE(R"cpp(
+    class Bar {
+      void baz();
+    };
+
+    [[void [[Bar::[[b^a^z]]]]() [[{
+      return;
+    }]]]])cpp");
+
   FileName = "Test.hpp";
   // Not available unless function name or fully body is selected.
   EXPECT_UNAVAILABLE(R"cpp(
@@ -100,7 +135,7 @@ TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
     };
   )cpp");
 
-  // Not available on definitions within unnamed namespaces
+  // Not available on definitions in header file within unnamed namespaces
   EXPECT_UNAVAILABLE(R"cpp(
     namespace {
       struct Foo {
@@ -349,6 +384,40 @@ TEST_F(DefineOutlineTest, ApplyTest) {
   }
 }
 
+TEST_F(DefineOutlineTest, InCppFile) {
+  FileName = "Test.cpp";
+
+  struct {
+    llvm::StringRef Test;
+    llvm::StringRef ExpectedSource;
+  } Cases[] = {
+      {
+          R"cpp(
+            namespace foo {
+            namespace {
+            struct Foo { void ba^r() {} };
+            struct Bar { void foo(); };
+            void Bar::foo() {}
+            }
+            }
+        )cpp",
+          R"cpp(
+            namespace foo {
+            namespace {
+            struct Foo { void bar() ; };void Foo::bar() {} 
+            struct Bar { void foo(); };
+            void Bar::foo() {}
+            }
+            }
+        )cpp"},
+  };
+
+  for (const auto &Case : Cases) {
+    SCOPED_TRACE(Case.Test);
+    EXPECT_EQ(apply(Case.Test, nullptr), Case.ExpectedSource);
+  }
+}
+
 TEST_F(DefineOutlineTest, HandleMacros) {
   llvm::StringMap<std::string> EditedFiles;
   ExtraFiles["Test.cpp"] = "";
