[clangd] Reland DefineInline action availability checks

Summary:
Introduces DefineInline action and initial version of availability
checks.

Reviewers: sammccall, ilya-biryukov, hokein

Reviewed By: hokein

Subscribers: thakis, usaxena95, mgorny, ilya-biryukov, MaskRay, jkorous, arphaman, cfe-commits

Tags: #clang

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

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

@@ -14,6 +14,7 @@ set(LLVM_LINK_COMPONENTS
 add_clang_library(clangDaemonTweaks OBJECT
   AnnotateHighlightings.cpp
   DumpAST.cpp
+  DefineInline.cpp
   ExpandAutoType.cpp
   ExpandMacro.cpp
   ExtractFunction.cpp

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

@@ -0,0 +1,214 @@
+//===--- DefineInline.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 "AST.h"
+#include "Logger.h"
+#include "Selection.h"
+#include "SourceCode.h"
+#include "XRefs.h"
+#include "refactor/Tweak.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/ASTTypeTraits.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/DeclBase.h"
+#include "clang/AST/DeclCXX.h"
+#include "clang/AST/DeclTemplate.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/ExprCXX.h"
+#include "clang/AST/NestedNameSpecifier.h"
+#include "clang/AST/PrettyPrinter.h"
+#include "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/Stmt.h"
+#include "clang/AST/TemplateBase.h"
+#include "clang/AST/Type.h"
+#include "clang/AST/TypeLoc.h"
+#include "clang/Basic/LangOptions.h"
+#include "clang/Basic/SourceLocation.h"
+#include "clang/Basic/SourceManager.h"
+#include "clang/Basic/TokenKinds.h"
+#include "clang/Index/IndexDataConsumer.h"
+#include "clang/Index/IndexSymbol.h"
+#include "clang/Index/IndexingAction.h"
+#include "clang/Lex/Lexer.h"
+#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/Token.h"
+#include "clang/Sema/Lookup.h"
+#include "clang/Sema/Sema.h"
+#include "clang/Tooling/Core/Replacement.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/DenseSet.h"
+#include "llvm/ADT/Optional.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/Support/Casting.h"
+#include "llvm/Support/Error.h"
+#include "llvm/Support/Signals.h"
+#include "llvm/Support/raw_ostream.h"
+#include <cstddef>
+#include <set>
+#include <string>
+#include <unordered_map>
+#include <vector>
+
+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.
+const FunctionDecl *getSelectedFunction(const SelectionTree::Node *SelNode) {
+  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;
+}
+
+// Checks the decls mentioned in Source are visible in the context of Target.
+// Achives that by checking declarations occur before target location in
+// translation unit or declared in the same class.
+bool checkDeclsAreVisible(const llvm::DenseSet<const Decl *> &DeclRefs,
+                          const FunctionDecl *Target, const SourceManager &SM) {
+  SourceLocation TargetLoc = Target->getLocation();
+  // To be used in visibility check below, decls in a class are visible
+  // independent of order.
+  const RecordDecl *Class = nullptr;
+  if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(Target))
+    Class = MD->getParent();
+
+  for (const auto *DR : DeclRefs) {
+    // Use canonical decl, since having one decl before target is enough.
+    const Decl *D = DR->getCanonicalDecl();
+    if (D == Target)
+      continue;
+    SourceLocation DeclLoc = D->getLocation();
+
+    // FIXME: Allow declarations from different files with include insertion.
+    if (!SM.isWrittenInSameFile(DeclLoc, TargetLoc))
+      return false;
+
+    // If declaration is before target, then it is visible.
+    if (SM.isBeforeInTranslationUnit(DeclLoc, TargetLoc))
+      continue;
+
+    // Otherwise they need to be in same class
+    if (!Class)
+      return false;
+    const RecordDecl *Parent = nullptr;
+    if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(D))
+      Parent = MD->getParent();
+    else if (const auto *FD = llvm::dyn_cast<FieldDecl>(D))
+      Parent = FD->getParent();
+    if (Parent != Class)
+      return false;
+  }
+  return true;
+}
+
+// Returns the canonical declaration for the given FunctionDecl. This will
+// usually be the first declaration in current translation unit with the
+// exception of template specialization.
+// For those we return first declaration different than the canonical one.
+// Because canonical declaration points to template decl instead of
+// specialization.
+const FunctionDecl *findTarget(const FunctionDecl *FD) {
+  auto CanonDecl = FD->getCanonicalDecl();
+  if (!FD->isFunctionTemplateSpecialization())
+    return CanonDecl;
+  // For specializations CanonicalDecl is the TemplatedDecl, which is not the
+  // target we want to inline into. Instead we traverse previous decls to find
+  // the first forward decl for this specialization.
+  auto PrevDecl = FD;
+  while (PrevDecl->getPreviousDecl() != CanonDecl) {
+    PrevDecl = PrevDecl->getPreviousDecl();
+    assert(PrevDecl && "Found specialization without template decl");
+  }
+  return PrevDecl;
+}
+
+/// Moves definition of a function/method to its declaration location.
+/// Before:
+/// a.h:
+///   void foo();
+///
+/// a.cc:
+///   void foo() { return; }
+///
+/// ------------------------
+/// After:
+/// a.h:
+///   void foo() { return; }
+///
+/// a.cc:
+///
+class DefineInline : public Tweak {
+public:
+  const char *id() const override final;
+
+  Intent intent() const override { return Intent::Refactor; }
+  std::string title() const override {
+    return "Move function body to declaration";
+  }
+  bool hidden() const override { return true; }
+
+  // Returns true when selection is on a function definition that does not
+  // make use of any internal symbols.
+  bool prepare(const Selection &Sel) override {
+    const SelectionTree::Node *SelNode = Sel.ASTSelection.commonAncestor();
+    if (!SelNode)
+      return false;
+    Source = getSelectedFunction(SelNode);
+    if (!Source || !Source->isThisDeclarationADefinition())
+      return false;
+    // Only the last level of template parameter locations are not kept in AST,
+    // so if we are inlining a method that is in a templated class, there is no
+    // way to verify template parameter names. Therefore we bail out.
+    if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(Source)) {
+      if (MD->getParent()->isTemplated())
+        return false;
+    }
+
+    Target = findTarget(Source);
+    if (Target == Source) {
+      // The only declaration is Source. No other declaration to move function
+      // body.
+      // FIXME: If we are in an implementation file, figure out a suitable
+      // location to put declaration. Possibly using other declarations in the
+      // AST.
+      return false;
+    }
+
+    // Check if the decls referenced in function body are visible in the
+    // declaration location.
+    if (!checkDeclsAreVisible(getNonLocalDeclRefs(Sel.AST, Source), Target,
+                              Sel.AST.getSourceManager()))
+      return false;
+
+    return true;
+  }
+
+  Expected<Effect> apply(const Selection &Sel) override {
+    return llvm::createStringError(llvm::inconvertibleErrorCode(),
+                                   "Not implemented yet");
+  }
+
+private:
+  const FunctionDecl *Source = nullptr;
+  const FunctionDecl *Target = nullptr;
+};
+
+REGISTER_TWEAK(DefineInline);
+
+} // namespace
+} // namespace clangd
+} // namespace clang

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

@@ -60,7 +60,7 @@ std::pair<unsigned, unsigned> rangeOrPoint(const Annotations &A) {
           cantFail(positionToOffset(A.code(), SelectionRng.end))};
 }
 
-MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs,
+MATCHER_P5(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs, ExtraFiles,
            (TweakID + (negation ? " is unavailable" : " is available")).str()) {
   std::string WrappedCode = wrap(Ctx, arg);
   Annotations Input(WrappedCode);
@@ -69,6 +69,7 @@ MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraArgs,
   TU.HeaderCode = Header;
   TU.Code = Input.code();
   TU.ExtraArgs = ExtraArgs;
+  TU.AdditionalFiles = std::move(ExtraFiles);
   ParsedAST AST = TU.build();
   Tweak::Selection S(AST, Selection.first, Selection.second);
   auto PrepareResult = prepareTweak(TweakID, S);
@@ -115,7 +116,8 @@ std::string TweakTest::apply(llvm::StringRef MarkedCode) const {
 }
 
 ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const {
-  return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraArgs);
+  return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, ExtraArgs,
+                          ExtraFiles);
 }
 
 std::vector<std::string> TweakTest::expandCases(llvm::StringRef MarkedCode) {

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

@@ -10,8 +10,10 @@
 #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TWEAKTESTING_H
 
 #include "TestTU.h"
-#include "gtest/gtest.h"
+#include "llvm/ADT/StringMap.h"
 #include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <string>
 
 namespace clang {
 namespace clangd {
@@ -47,6 +49,9 @@ public:
     Expression,
   };
 
+  // Mapping from file name to contents.
+  llvm::StringMap<std::string> ExtraFiles;
+
 protected:
   TweakTest(const char *TweakID) : TweakID(TweakID) {}
 

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

@@ -24,6 +24,7 @@
 #include "clang/Rewrite/Core/Rewriter.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/IntrusiveRefCntPtr.h"
+#include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Error.h"
 #include "llvm/Support/MemoryBuffer.h"
@@ -33,6 +34,8 @@
 #include "gmock/gmock.h"
 #include "gtest/gtest.h"
 #include <cassert>
+#include <string>
+#include <vector>
 
 using ::testing::AllOf;
 using ::testing::HasSubstr;
@@ -880,6 +883,173 @@ TEST_F(RemoveUsingNamespaceTest, All) {
     EXPECT_EQ(C.second, apply(C.first)) << C.first;
 }
 
+TWEAK_TEST(DefineInline);
+TEST_F(DefineInlineTest, TriggersOnFunctionDecl) {
+  // Basic check for function body and signature.
+  EXPECT_AVAILABLE(R"cpp(
+  class Bar {
+    void baz();
+  };
+
+  [[void [[Bar::[[b^a^z]]]]() [[{
+    return;
+  }]]]]
+
+  void foo();
+  [[void [[f^o^o]]() [[{
+    return;
+  }]]]]
+  )cpp");
+
+  EXPECT_UNAVAILABLE(R"cpp(
+  // Not a definition
+  vo^i[[d^ ^f]]^oo();
+
+  [[vo^id ]]foo[[()]] {[[
+    [[(void)(5+3);
+    return;]]
+  }]]
+  )cpp");
+}
+
+TEST_F(DefineInlineTest, NoForwardDecl) {
+  Header = "void bar();";
+  EXPECT_UNAVAILABLE(R"cpp(
+  void bar() {
+    return;
+  }
+  // FIXME: Generate a decl in the header.
+  void fo^o() {
+    return;
+  })cpp");
+}
+
+TEST_F(DefineInlineTest, ReferencedDecls) {
+  EXPECT_AVAILABLE(R"cpp(
+    void bar();
+    void foo(int test);
+
+    void fo^o(int baz) {
+      int x = 10;
+      bar();
+    })cpp");
+
+  // Internal symbol usage.
+  Header = "void foo(int test);";
+  EXPECT_UNAVAILABLE(R"cpp(
+    void bar();
+    void fo^o(int baz) {
+      int x = 10;
+      bar();
+    })cpp");
+
+  // Becomes available after making symbol visible.
+  Header = "void bar();" + Header;
+  EXPECT_AVAILABLE(R"cpp(
+    void fo^o(int baz) {
+      int x = 10;
+      bar();
+    })cpp");
+
+  // FIXME: Move declaration below bar to make it visible.
+  Header.clear();
+  EXPECT_UNAVAILABLE(R"cpp(
+    void foo();
+    void bar();
+
+    void fo^o() {
+      bar();
+    })cpp");
+
+  // Order doesn't matter within a class.
+  EXPECT_AVAILABLE(R"cpp(
+    class Bar {
+      void foo();
+      void bar();
+    };
+
+    void Bar::fo^o() {
+      bar();
+    })cpp");
+
+  // FIXME: Perform include insertion to make symbol visible.
+  ExtraFiles["a.h"] = "void bar();";
+  Header = "void foo(int test);";
+  EXPECT_UNAVAILABLE(R"cpp(
+    #include "a.h"
+    void fo^o(int baz) {
+      int x = 10;
+      bar();
+    })cpp");
+}
+
+TEST_F(DefineInlineTest, TemplateSpec) {
+  EXPECT_UNAVAILABLE(R"cpp(
+    template <typename T> void foo();
+    template<> void foo<char>();
+
+    template<> void f^oo<int>() {
+    })cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+    template <typename T> void foo();
+
+    template<> void f^oo<int>() {
+    })cpp");
+  EXPECT_UNAVAILABLE(R"cpp(
+    template <typename T> struct Foo { void foo(); };
+
+    template <typename T> void Foo<T>::f^oo() {
+    })cpp");
+  EXPECT_AVAILABLE(R"cpp(
+    template <typename T> void foo();
+    void bar();
+    template <> void foo<int>();
+
+    template<> void f^oo<int>() {
+      bar();
+    })cpp");
+}
+
+TEST_F(DefineInlineTest, CheckForCanonDecl) {
+  EXPECT_UNAVAILABLE(R"cpp(
+    void foo();
+
+    void bar() {}
+    void f^oo() {
+      // This bar normally refers to the definition just above, but it is not
+      // visible from the forward declaration of foo.
+      bar();
+    })cpp");
+  // Make it available with a forward decl.
+  EXPECT_AVAILABLE(R"cpp(
+    void bar();
+    void foo();
+
+    void bar() {}
+    void f^oo() {
+      bar();
+    })cpp");
+}
+
+TEST_F(DefineInlineTest, UsingShadowDecls) {
+  // Template body is not parsed until instantiation time on windows, which
+  // results in arbitrary failures as function body becomes NULL.
+  ExtraArgs.push_back("-fno-delayed-template-parsing");
+  EXPECT_UNAVAILABLE(R"cpp(
+  namespace ns1 { void foo(int); }
+  namespace ns2 { void foo(int*); }
+  template <typename T>
+  void bar();
+
+  using ns1::foo;
+  using ns2::foo;
+
+  template <typename T>
+  void b^ar() {
+    foo(T());
+  })cpp");
+}
+
 } // namespace
 } // namespace clangd
 } // namespace clang