[clang-tools-extra] 5ab9a85 - [clangd] Reland DefineInline action availability checks
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Sun Oct 27 23:28:34 PDT 2019
Author: Kadir Cetinkaya
Date: 2019-10-28T07:28:21+01:00
New Revision: 5ab9a850f6bde53974798ee285a06335fb788ae5
URL: https://github.com/llvm/llvm-project/commit/5ab9a850f6bde53974798ee285a06335fb788ae5
DIFF: https://github.com/llvm/llvm-project/commit/5ab9a850f6bde53974798ee285a06335fb788ae5.diff
LOG: [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
Added:
clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
Modified:
clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
clang-tools-extra/clangd/unittests/TweakTesting.cpp
clang-tools-extra/clangd/unittests/TweakTesting.h
clang-tools-extra/clangd/unittests/TweakTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt b/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
index f43f132304c9..ddf10a2ca2ba 100644
--- a/clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt
+++ b/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
diff --git a/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp b/clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp
new file mode 100644
index 000000000000..c2806d756983
--- /dev/null
+++ b/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
diff erent 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
diff erent 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
diff --git a/clang-tools-extra/clangd/unittests/TweakTesting.cpp b/clang-tools-extra/clangd/unittests/TweakTesting.cpp
index b67b41368329..c3afce6bae01 100644
--- a/clang-tools-extra/clangd/unittests/TweakTesting.cpp
+++ b/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) {
diff --git a/clang-tools-extra/clangd/unittests/TweakTesting.h b/clang-tools-extra/clangd/unittests/TweakTesting.h
index 1e8cd58849bb..8b9209d943da 100644
--- a/clang-tools-extra/clangd/unittests/TweakTesting.h
+++ b/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 @@ class TweakTest : public ::testing::Test {
Expression,
};
+ // Mapping from file name to contents.
+ llvm::StringMap<std::string> ExtraFiles;
+
protected:
TweakTest(const char *TweakID) : TweakID(TweakID) {}
diff --git a/clang-tools-extra/clangd/unittests/TweakTests.cpp b/clang-tools-extra/clangd/unittests/TweakTests.cpp
index c5caa4b7dc5c..bf8eeaca0a9f 100644
--- a/clang-tools-extra/clangd/unittests/TweakTests.cpp
+++ b/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
More information about the cfe-commits
mailing list