[clangd] Define out-of-line initial apply logic

[kadircet]

Summary

Initial implementation for apply logic, replaces function body with a
semicolon in source location and copies the full function definition into target
location.

Will handle qualification of return type and function name in following patches
to keep the changes small.

[kadircet]

• Rebase

[hokein]

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

62

the function name doesn't reflect what it does, it doesn't move the function, it just returns the source code of the function, I'd call it some name like getFunctionSourceCode.

151

The FileName is not always absolute, getCorrespondingHeaderOrSource is expecting an absolute file path input.

153

could we use a better name, Source in the context here has two different meaning: 1) the corresponding .cc file 2) the source of moving function declaration (we called it Source);

Maybe call it CCFile?

191

maybe just const tooling::Replacement InsertFunctionDef(Contents, *InsertionOffset, 0, *FunDef);, which would save us a InsertLoc.

200

nit: I think CharSourceRange::getCharRange(Source->getBody()->getSourceRange()) is clearer.

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

1882

thinking more about this, if this function is inline, we will leave an unnecessary inline keyword after running the code action. No need to address it in the patch, just keep in mind.

can we add more tests? e.g. template functions.

[kadircet]

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

1882

added a fixme for inline stuff.

[kadircet]

• Address comments
• Handle template parameters when copying function and add tests

[hokein]

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

57

Could you confirm the code handle template specializations as well? I think getDescribedFunctionTemplate will return null when FD is a specialization, and we still miss the template list.

75

I think we could simplify the code like:

const auto* TargetFD = FD->getDescribedFunctionTemplate() ? FD->getDescribedFunctionTemplate(): FD;
auto CharRange = toHaleOpenFileRange(.., FD->getSourceRange());

152

should we use getCanonicalPath in the SourceCode.h?

179

nit: maybe put the code for calculating the insertion point to a separate function.

[kadircet]

• Address comments

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

57

added a unittest

[kadircet]

• Get rid of raw string literals inside macros

[hokein]

looks good.

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

53

this function seems trivial, I'd inline them into call sides for both define inline and outline.

[kadircet]

• Address comments
• Better handling for macros and tests

[merge_guards_bot]

Build result: FAILURE - Could not check out parent git hash "6a2d56e54fa4a2009787a605607b0df7fe16dd98". It was not found in the repository. Did you configure the "Parent Revision" in Phabricator properly? Trying to apply the patch to the master branch instead...

ERROR: arc patch failed with error code 1. Check build log for details.
Log files: console-log.txt, CMakeCache.txt

[thakis]

ApplyTest fails on Windows; probably the usual delayed template parsing thing: http://45.33.8.238/win/3368/step_7.txt

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

 //===--- DefineOutline.cpp ---------------------------------------*- C++-*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "HeaderSourceSwitch.h"
 #include "Path.h"
 #include "Selection.h"
 #include "SourceCode.h"
 #include "refactor/Tweak.h"
 #include "clang/AST/ASTTypeTraits.h"
 #include "clang/AST/Decl.h"
+#include "clang/AST/DeclTemplate.h"
 #include "clang/AST/Stmt.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Driver/Types.h"
+#include "clang/Format/Format.h"
+#include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/Optional.h"
-#include "llvm/Support/Path.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Error.h"
+#include <cstddef>
 
 namespace clang {
 namespace clangd {
 namespace {
 
 // Deduces the FunctionDecl from a selection. Requires either the function body
 // or the function decl to be selected. Returns null if none of the above
 // criteria is met.
 // FIXME: This is shared with define inline, move them to a common header once
 // we have a place for such.
 const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
   if (!SelNode)
     return nullptr;
   const ast_type_traits::DynTypedNode &AstNode = SelNode->ASTNode;
   if (const FunctionDecl *FD = AstNode.get<FunctionDecl>())
     return FD;
   if (AstNode.get<CompoundStmt>() &&
       SelNode->Selected == SelectionTree::Complete) {
     if (const SelectionTree::Node *P = SelNode->Parent)
       return P->ASTNode.get<FunctionDecl>();
   }
   return nullptr;
 }
 
+llvm::Optional<Path> getSourceFile(llvm::StringRef FileName,
+                                   const Tweak::Selection &Sel) {
+  if (auto Source = getCorrespondingHeaderOrSource(
+          FileName,
+          &Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem()))
+    return *Source;
+  return getCorrespondingHeaderOrSource(FileName, Sel.AST, Sel.Index);
+}
+
+// Creates a modified version of function definition that can be inserted at a
+// different location. Contains both function signature and body.
+llvm::Optional<llvm::StringRef> getFunctionSourceCode(const FunctionDecl *FD) {
+  auto &SM = FD->getASTContext().getSourceManager();
+  auto CharRange = toHalfOpenFileRange(SM, FD->getASTContext().getLangOpts(),
+                                       FD->getSourceRange());
+  if (!CharRange)
+    return llvm::None;
+  // Include template parameter list.
+  if (auto *FTD = FD->getDescribedFunctionTemplate())
+    CharRange->setBegin(FTD->getBeginLoc());
+
+  // FIXME: Qualify return type.
+  // FIXME: Qualify function name depending on the target context.
+  return toSourceCode(SM, *CharRange);
+}
+
+// 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<size_t> getInsertionOffset(llvm::StringRef Contents,
+                                          llvm::StringRef QualifiedName,
+                                          const format::FormatStyle &Style) {
+  auto Region = getEligiblePoints(Contents, QualifiedName, Style);
+
+  assert(!Region.EligiblePoints.empty());
+  // FIXME: This selection can be made smarter by looking at the definition
+  // locations for adjacent decls to Source. Unfortunately psudeo parsing in
+  // getEligibleRegions only knows about namespace begin/end events so we
+  // can't match function start/end positions yet.
+  auto InsertionPoint = Region.EligiblePoints.back();
+  return positionToOffset(Contents, InsertionPoint);
+}
+
 /// Moves definition of a function/method to an appropriate implementation file.
 ///
 /// Before:
 /// a.h
 ///   void foo() { return; }
 /// a.cc
 ///   #include "a.h"
 ///
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 {
-    return llvm::createStringError(llvm::inconvertibleErrorCode(),
-                                   "Not implemented yet");
+    const SourceManager &SM = Sel.AST.getSourceManager();
+    auto MainFileName =
+        getCanonicalPath(SM.getFileEntryForID(SM.getMainFileID()), SM);
+    if (!MainFileName)
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Couldn't get absolute path for mainfile.");
+
+    auto CCFile = getSourceFile(*MainFileName, Sel);
+    if (!CCFile)
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Couldn't find a suitable implementation file.");
+
+    auto &FS =
+        Sel.AST.getSourceManager().getFileManager().getVirtualFileSystem();
+    auto Buffer = FS.getBufferForFile(*CCFile);
+    // FIXME: Maybe we should consider creating the implementation file if it
+    // doesn't exist?
+    if (!Buffer)
+      return llvm::createStringError(Buffer.getError(),
+                                     Buffer.getError().message());
+    auto Contents = Buffer->get()->getBuffer();
+    auto InsertionOffset =
+        getInsertionOffset(Contents, Source->getQualifiedNameAsString(),
+                           getFormatStyleForFile(*CCFile, Contents, &FS));
+    if (!InsertionOffset)
+      return InsertionOffset.takeError();
+
+    auto FuncDef = getFunctionSourceCode(Source);
+    if (!FuncDef)
+      return llvm::createStringError(
+          llvm::inconvertibleErrorCode(),
+          "Couldn't get full source for function definition.");
+
+    SourceManagerForFile SMFF(*CCFile, Contents);
+    const tooling::Replacement InsertFunctionDef(*CCFile, *InsertionOffset, 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.getASTContext().getLangOpts(),
+                                 Source->getBody()->getSourceRange())),
+        ";");
+    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);
   }
 
 private:
   const FunctionDecl *Source = nullptr;
 };
 
 REGISTER_TWEAK(DefineOutline);
 
 } // namespace
 } // namespace clangd
 } // namespace clang

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

for (auto &It : Result->ApplyEdits) {
     llvm::StringRef Unwrapped = unwrap(Context, *NewText);
     if (It.first() == testPath(TU.Filename))
       EditedMainFile = Unwrapped;
     else {
       if (!EditedFiles)
         ADD_FAILURE() << "There were changes to additional files, but client "
                          "provided a nullptr for EditedFiles.";
       else
-        EditedFiles->try_emplace(It.first(), Unwrapped.str());
+        EditedFiles->insert_or_assign(It.first(), Unwrapped.str());
     }
   }
   return EditedMainFile;
 }
 
 ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraArgs,
                           ExtraFiles, Index.get(), FileName);

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

TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
 
   // Not available within templated classes, as it is hard to spell class name
   // out-of-line in such cases.
   EXPECT_UNAVAILABLE(R"cpp(
     template <typename> struct Foo { void fo^o(){} };
     })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, ApplyTest) {
+  llvm::StringMap<std::string> EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+    llvm::StringRef Test;
+    llvm::StringRef ExpectedHeader;
+    llvm::StringRef ExpectedSource;
+  } Cases[] = {
+      // Simple check
+      {
+          "void fo^o() { return; }",
+          "void foo() ;",
+          "void foo() { return; }",
+      },
+      // Templated function.
+      {
+          "template <typename T> void fo^o(T, T x) { return; }",
+          "template <typename T> void foo(T, T x) ;",
+          "template <typename T> void foo(T, T x) { return; }",
+      },
+      {
+          "template <typename> void fo^o() { return; }",
+          "template <typename> void foo() ;",
+          "template <typename> void foo() { return; }",
+      },
+      // Template specialization.
+      {
+          R"cpp(
+            template <typename> void foo();
+            template <> void fo^o<int>() { return; })cpp",
+          R"cpp(
+            template <typename> void foo();
+            template <> void foo<int>() ;)cpp",
+          "template <> void foo<int>() { return; }",
+      },
+  };
+  for (const auto &Case : Cases) {
+    SCOPED_TRACE(Case.Test);
+    EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+    EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+                                 testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
+TEST_F(DefineOutlineTest, HandleMacros) {
+  llvm::StringMap<std::string> EditedFiles;
+  ExtraFiles["Test.cpp"] = "";
+  FileName = "Test.hpp";
+
+  struct {
+    llvm::StringRef Test;
+    llvm::StringRef ExpectedHeader;
+    llvm::StringRef ExpectedSource;
+  } Cases[] = {
+      {R"cpp(
+          #define BODY { return; }
+          void f^oo()BODY)cpp",
+       R"cpp(
+          #define BODY { return; }
+          void foo();)cpp",
+       "void foo()BODY"},
+
+      {R"cpp(
+          #define BODY return;
+          void f^oo(){BODY})cpp",
+       R"cpp(
+          #define BODY return;
+          void foo();)cpp",
+       "void foo(){BODY}"},
+
+      {R"cpp(
+          #define TARGET void foo()
+          [[TARGET]]{ return; })cpp",
+       R"cpp(
+          #define TARGET void foo()
+          TARGET;)cpp",
+       "TARGET{ return; }"},
+
+      {R"cpp(
+          #define TARGET foo
+          void [[TARGET]](){ return; })cpp",
+       R"cpp(
+          #define TARGET foo
+          void TARGET();)cpp",
+       "void TARGET(){ return; }"},
+  };
+  for (const auto &Case : Cases) {
+    SCOPED_TRACE(Case.Test);
+    EXPECT_EQ(apply(Case.Test, &EditedFiles), Case.ExpectedHeader);
+    EXPECT_THAT(EditedFiles, testing::ElementsAre(FileWithContents(
+                                 testPath("Test.cpp"), Case.ExpectedSource)));
+  }
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang