[clangd] Define out-of-line availability checks

[kadircet]

Summary

Initial availability checks for performing define out-of-line code
action, which is a refactoring that will help users move function/method
definitions from headers to implementation files.

Proposed implementation only checks whether we have an interesting selection,
namely function name or full function definition/body.

[hokein]

the patch looks mostly good. would be interesting to see apply implementation.

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

30

nit: looks like we also a similar version in DefineInline? would be nice if we could share the implementation. I don't have a good idea where to put it, maybe add a FIXME?

45

I'm not sure this is useful in general: saying in .cc we have a single definition void foo() {}, after running the code action, we will end up with void foo(); void foo() {}.

If we want to do this, I think we may make it only available for inline class methods.

76

The bail-out here seems violating the above comment /// If current file is already an implementation file, tries to move the definition out-of-line. Basically, now we only allow moving the function definition to a .cc file.

79

I think we can use AST.getASTContext().getLangOpts().IsHeaderFile; to detect the header file rather than relying on the filename extension.

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

1839

could you add a testcase where the method is inline? like

class Bar {
  void baz2() { return; }
}

[kadircet]

• Address comments

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

30

I also had the same concern, but left it here since couldn't find a suitable please.
adding a fixme.

45

yes this was implying methods in my head, but forgot to spell it out. updating comment.

76

this was actually representing the state I wanted to arrive, but you are right it looks confusing, getting rid of the mention in the class documention.

[hokein]

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

1812

nit: please explicitly spell out the FileName, even the default value is TestTU.cpp.

1835

Is this needed? It is unclear to me why we need it.

[hokein]

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

78

I think doesThisDeclarationHaveABody is probably a better choice here? isThisDeclarationADefinition returns true for defaulted methods like void Ctor() = default;, we'd need to disable the tweak for this case, could you add a test case for this?

[hokein]

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

1843

Sorry for not spotting it earlier.

Moving out-of-line methods is different than general functions, void Bar::bar(); is illegal in C++ (C++ requires the out-of-line member declaration must be a definition). I think for this case, we could 1) delete the original method definition 2) disable the tweak. I slightly prefer 2) as out-of-line member definitions are rare in header files. WDYT?

[kadircet]

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

1835

yeah not anymore, I was checking for existence of implementation file in the first version. but moved it from prepare to apply, forgot to update the test.

1843

oopsy thanks for spotting this. yeah I agree with 2.

[kadircet]

• Address comments

[hokein]

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

1836

nit: metohds => methods.

[hokein]

still lgtm.

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

73

nit: we have the isHeaderFile helper in the SourceCode.h, please use it (it is more precise).

[kadircet]

• Address comments
• Bail out on templated classes

[merge_guards_bot]

Build result: pass - 60292 tests passed, 0 failed and 732 were skipped.

Log files: console-log.txt, CMakeCache.txt

clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt

 # constructors that register individual tweaks in a global registry.
 # To enable these tweaks in executables or shared libraries, add
 # $<TARGET_OBJECTS:obj.clangDaemonTweaks> to a list of sources, see
 # clangd/tool/CMakeLists.txt for an example.
 add_clang_library(clangDaemonTweaks OBJECT
   AnnotateHighlightings.cpp
   DumpAST.cpp
   DefineInline.cpp
+  DefineOutline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp
   ExtractVariable.cpp
   RawStringLiteral.cpp
   RemoveUsingNamespace.cpp
   SwapIfBranches.cpp
 

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/Stmt.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/Support/Path.h"
+
+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;
+}
+
+/// Moves definition of a function/method to an appropriate implementation file.
+///
+/// Before:
+/// a.h
+///   void foo() { return; }
+/// a.cc
+///   #include "a.h"
+///
+/// ----------------
+///
+/// After:
+/// a.h
+///   void foo();
+/// a.cc
+///   #include "a.h"
+///   void foo() { return; }
+class DefineOutline : public Tweak {
+public:
+  const char *id() const override;
+
+  bool hidden() const override { return true; }
+  Intent intent() const override { return Intent::Refactor; }
+  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.getASTContext().getLangOpts()))
+      return false;
+
+    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;
+
+    // Bail out in templated classes, as it is hard to spell the class name, i.e
+    // if the template parameter is unnamed.
+    if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) {
+      if (MD->getParent()->isTemplated())
+        return false;
+    }
+
+    // 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");
+  }
+
+private:
+  const FunctionDecl *Source = nullptr;
+};
+
+REGISTER_TWEAK(DefineOutline);
+
+} // namespace
+} // namespace clangd
+} // namespace clang

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

 //===--- TweakTesting.h - Test helpers for refactoring actions ---*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
 
 #include "TestTU.h"
 #include "index/Index.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <memory>
 #include <string>
 
 namespace clang {
 namespace clangd {
 

 protected:
   TweakTest(const char *TweakID) : TweakID(TweakID) {}
 
   // Contents of a header file to be implicitly included.
   // This typically contains declarations that will be used for a set of related
   // testcases.
   std::string Header;
 
+  llvm::StringRef FileName = "TestTU.cpp";
+
   // Extra flags passed to the compilation in apply().
   std::vector<const char *> ExtraArgs;
 
   // Context in which snippets of code should be placed to run tweaks.
   CodeContext Context = File;
 
   // Index to be passed into Tweak::Selection.
   std::unique_ptr<const SymbolIndex> Index = nullptr;

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

if (A.points().size() != 0) {
     SelectionRng = Range{A.point(), A.point()};
   } else {
     SelectionRng = A.range();
   }
   return {cantFail(positionToOffset(A.code(), SelectionRng.start)),
           cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P6(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
+MATCHER_P7(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles, Index,
+           FileName,
            (TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
   Tweak::Selection S(Index, AST, Selection.first, Selection.second);
   auto PrepareResult = prepareTweak(TweakID, S);
   if (PrepareResult)
     return true;
   llvm::consumeError(PrepareResult.takeError());
   return false;
 }
 
 } // namespace
 
 std::string TweakTest::apply(llvm::StringRef MarkedCode,
                              llvm::StringMap<std::string> *EditedFiles) const {
   std::string WrappedCode = wrap(Context, MarkedCode);
   Annotations Input(WrappedCode);
   auto Selection = rangeOrPoint(Input);
 
   TestTU TU;
+  TU.Filename = FileName;
   TU.HeaderCode = Header;
   TU.AdditionalFiles = std::move(ExtraFiles);
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
   ParsedAST AST = TU.build();
   Tweak::Selection S(Index.get(), AST, Selection.first, Selection.second);
 
   auto T = prepareTweak(TweakID, S);
else {
         EditedFiles->try_emplace(It.first(), Unwrapped.str());
     }
   }
   return EditedMainFile;
 }
 
 ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
   return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraArgs,
-                          ExtraFiles, Index.get());
+                          ExtraFiles, Index.get(), FileName);
 }
 
 std::vector<std::string> TweakTest::expandCases(llvm::StringRef MarkedCode) {
   Annotations Test(MarkedCode);
   llvm::StringRef Code = Test.code();
   std::vector<std::string> Cases;
   for (const auto& Point : Test.points()) {
     size_t Offset = llvm::cantFail(positionToOffset(Code, Point));

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

llvm::StringRef Expected = R"cpp(
     }
 
     using namespace b;
     using namespace c;
     )cpp";
   EXPECT_EQ(apply(Test), Expected) << Test;
 }
 
+TWEAK_TEST(DefineOutline);
+TEST_F(DefineOutlineTest, TriggersOnFunctionDecl) {
+  FileName = "Test.cpp";
+  // Not available unless in a header file.
+  EXPECT_UNAVAILABLE(R"cpp(
+    [[void [[f^o^o]]() [[{
+      return;
+    }]]]])cpp");
+
+  FileName = "Test.hpp";
+  // Not available unless function name or fully body is selected.
+  EXPECT_UNAVAILABLE(R"cpp(
+    // Not a definition
+    vo^i[[d^ ^f]]^oo();
+
+    [[vo^id ]]foo[[()]] {[[
+      [[(void)(5+3);
+      return;]]
+    }]])cpp");
+
+  // Available even if there are no implementation files.
+  EXPECT_AVAILABLE(R"cpp(
+    [[void [[f^o^o]]() [[{
+      return;
+    }]]]])cpp");
+
+  // Not available for out-of-line methods.
+  EXPECT_UNAVAILABLE(R"cpp(
+    class Bar {
+      void baz();
+    };
+
+    [[void [[Bar::[[b^a^z]]]]() [[{
+      return;
+    }]]]])cpp");
+
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+    class Bar {
+      [[void [[f^o^o]]() [[{ return; }]]]]
+    };
+
+    void foo();
+    [[void [[f^o^o]]() [[{
+      return;
+    }]]]])cpp");
+
+  // Not available on defaulted/deleted members.
+  EXPECT_UNAVAILABLE(R"cpp(
+    class Foo {
+      Fo^o() = default;
+      F^oo(const Foo&) = delete;
+    };)cpp");
+
+  // 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");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang