[clangd] Support define outline tweak for non cross file edits

[njames93]

Summary

Currently this is limited to just defining method decls out of class in source files.

// Test.cpp
struct A {
  void Foo() {}
};
// Converted to
struct A {
  void Foo();
};
A::Foo() {}

[njames93]

Theres definitely scope to improve this in follow ups.
We could support this for template methods and classes

template <typename T> struct A {
  void [[Foo]]() {}
  template<typename U> void [[Bar]]() {}
};

// Both ranges transformed to
template <typename T> struct A {
  void Foo();
  template<typename U> void Bar();
};

template <typename T>
void A<T>::Foo() {}

template <typename T>
template <typename U>
void A<T>::Bar<U>() {}

In D147808 The restriction only only applying the tweak for non trivial member functions could be removed when making a non cross file edit.

We could also employ a better insertion point logic for these tweaks as we have the full AST for the file where the function is to be inserted.

[njames93]

• Clang format changes
• Update apply logic to not query the FS when its not a cross file edit

[njames93]

Ping?

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

public:
   llvm::StringLiteral kind() const override {
     return CodeAction::REFACTOR_KIND;
   }
   std::string title() const override {
     return "Move function body to out-of-line";
   }
 
   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;
+    bool IsHeader =
+        isHeaderFile(Sel.AST->getSourceManager().getFilename(Sel.Cursor),
+                     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())
       return false;
 
+    // If we are in a source file and the function is a free function, bail out.
+    if (!isa<CXXMethodDecl>(Source) && !IsHeader)
+      return false;
+
+    IsCrossFileEdit = IsHeader;
+
     // Bail out if this is a function template or specialization, as their
     // definitions need to be visible in all including translation units.
+    // FIXME: This can be supported if we are not making a cross file edit.
     if (Source->getDescribedFunctionTemplate())
       return false;
     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.
bool prepare(const Selection &Sel) override {
     // Note that we don't check whether an implementation file exists or not in
     // the prepare, since performing disk IO on each prepare request might be
     // expensive.
     return true;
   }
 
   Expected<Effect> apply(const Selection &Sel) override {
     const SourceManager &SM = Sel.AST->getSourceManager();
-    auto CCFile = getSourceFile(Sel.AST->tuPath(), Sel);
 
-    if (!CCFile)
+    std::optional<Path> OwnedPath;
+    std::unique_ptr<llvm::MemoryBuffer> OwnedBuffer;
+    PathRef Path;
+    StringRef TargetContents;
+    if (IsCrossFileEdit) {
+      OwnedPath = getSourceFile(Sel.AST->tuPath(), Sel);
+
+      if (!OwnedPath)
         return error("Couldn't find a suitable implementation file.");
+      Path = *OwnedPath;
       assert(Sel.FS && "FS Must be set in apply");
-    auto Buffer = Sel.FS->getBufferForFile(*CCFile);
-    // FIXME: Maybe we should consider creating the implementation file if it
-    // doesn't exist?
-    if (!Buffer)
+      if (auto Buffer = Sel.FS->getBufferForFile(Path))
+        OwnedBuffer = std::move(Buffer.get());
+      else
+        // FIXME: Maybe we should consider creating the implementation file if
+        // it doesn't exist?
         return llvm::errorCodeToError(Buffer.getError());
-    auto Contents = Buffer->get()->getBuffer();
-    auto InsertionPoint = getInsertionPoint(
-        Contents, Source->getQualifiedNameAsString(), Sel.AST->getLangOpts());
+      TargetContents = OwnedBuffer->getBuffer();
+    } else {
+      Path = Sel.AST->tuPath();
+      TargetContents = Sel.Code;
+    }
+
+    auto InsertionPoint =
+        getInsertionPoint(TargetContents, Source->getQualifiedNameAsString(),
+                          Sel.AST->getLangOpts());
     if (!InsertionPoint)
       return InsertionPoint.takeError();
 
     auto FuncDef = getFunctionSourceCode(
         Source, InsertionPoint->EnclosingNamespace, Sel.AST->getTokens(),
         Sel.AST->getHeuristicResolver());
     if (!FuncDef)
       return FuncDef.takeError();
 
-    SourceManagerForFile SMFF(*CCFile, Contents);
-    const tooling::Replacement InsertFunctionDef(
-        *CCFile, InsertionPoint->Offset, 0, *FuncDef);
-    auto Effect = Effect::mainFileEdit(
-        SMFF.get(), tooling::Replacements(InsertFunctionDef));
-    if (!Effect)
-      return Effect.takeError();
-
     // FIXME: We should also get rid of inline qualifier.
     const tooling::Replacement DeleteFuncBody(
         Sel.AST->getSourceManager(),
         CharSourceRange::getTokenRange(*toHalfOpenFileRange(
             SM, Sel.AST->getLangOpts(),
             getDeletionRange(Source, Sel.AST->getTokens()))),
         ";");
+    const tooling::Replacement InsertFunctionDef(Path, InsertionPoint->Offset,
+                                                 0, *FuncDef);
+
+    if (IsCrossFileEdit) {
+      SourceManagerForFile SMFF(Path, TargetContents);
+      auto Effect = Effect::mainFileEdit(
+          SMFF.get(), tooling::Replacements(InsertFunctionDef));
+      if (!Effect)
+        return Effect.takeError();
+
       auto HeaderFE = Effect::fileEdit(SM, SM.getMainFileID(),
                                        tooling::Replacements(DeleteFuncBody));
       if (!HeaderFE)
         return HeaderFE.takeError();
 
       Effect->ApplyEdits.try_emplace(HeaderFE->first,
                                      std::move(HeaderFE->second));
       return std::move(*Effect);
     }
+    tooling::Replacements R(DeleteFuncBody);
+    if (auto Err = R.add(InsertFunctionDef)) {
+      return Err;
+    }
+    return Effect::mainFileEdit(SM, R);
+  }
 
 private:
   const FunctionDecl *Source = nullptr;
+  bool IsCrossFileEdit = false;
 };
 
 REGISTER_TWEAK(DefineOutline)
 
 } // namespace
 } // namespace clangd
 } // namespace clang

clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp

 TEST_F(DefineOutlineTest, FailsWithoutSource) {
   FileName = "Test.hpp";
   llvm::StringRef Test = "void fo^o() { return; }";
   llvm::StringRef Expected =
       "fail: Couldn't find a suitable implementation file.";
   EXPECT_EQ(apply(Test), Expected);
 }
 
+TEST_F(DefineOutlineTest, SourceFileAllowed) {
+  FileName = "Test.cpp";
+
+  llvm::StringRef Test = "struct A { A^() {} };\n";
+  llvm::StringRef Expected = "struct A { A() ; };\nA::A() {} ";
+  EXPECT_EQ(Expected, apply(Test));
+
+  Test = "struct A { struct B { B^() {} }; };\n";
+  Expected = "struct A { struct B { B() ; }; };\nA::B::B() {} ";
+  EXPECT_EQ(Expected, apply(Test));
+
+  Test = "struct A { struct B; };\nstruct A::B { B^() {} };\n";
+  Expected = "struct A { struct B; };\nstruct A::B { B() ; };\nA::B::B() {} ";
+  EXPECT_EQ(Expected, apply(Test));
+}
+
 TEST_F(DefineOutlineTest, ApplyTest) {
   llvm::StringMap<std::string> EditedFiles;
   ExtraFiles["Test.cpp"] = "";
   FileName = "Test.hpp";
 
   struct {
     llvm::StringRef Test;
     llvm::StringRef ExpectedHeader;