From 74d39a42f109c06684b950fa4126c07119a210a1 Mon Sep 17 00:00:00 2001 From: Kadir Cetinkaya Date: Thu, 5 Sep 2019 14:10:43 +0200 Subject: [PATCH] [clangd] DefineInline action availability checks Summary: Introduces DefineInline action and initial version of availability checks. Reviewers: sammccall, ilya-biryukov, hokein Tags: #clang Differential Revision: https://reviews.llvm.org/D65433 --- .../clangd/refactor/tweaks/CMakeLists.txt | 1 + .../clangd/refactor/tweaks/DefineInline.cpp | 214 ++++++++++++++++++ .../clangd/unittests/TweakTesting.cpp | 6 +- .../clangd/unittests/TweakTesting.h | 7 +- .../clangd/unittests/TweakTests.cpp | 167 ++++++++++++++ 5 files changed, 392 insertions(+), 3 deletions(-) create mode 100644 clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp 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 +#include +#include +#include +#include + +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()) + return FD; + if (AstNode.get() && + SelNode->Selected == SelectionTree::Complete) { + if (const SelectionTree::Node *P = SelNode->Parent) + return P->ASTNode.get(); + } + 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 &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(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(D)) + Parent = MD->getParent(); + else if (const auto *FD = llvm::dyn_cast(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(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 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 63ecd7b4d4d5..b7a48a8e9979 100644 --- a/clang-tools-extra/clangd/unittests/TweakTesting.cpp +++ b/clang-tools-extra/clangd/unittests/TweakTesting.cpp @@ -59,7 +59,7 @@ std::pair rangeOrPoint(const Annotations &A) { cantFail(positionToOffset(A.code(), SelectionRng.end))}; } -MATCHER_P3(TweakIsAvailable, TweakID, Ctx, Header, +MATCHER_P4(TweakIsAvailable, TweakID, Ctx, Header, ExtraFiles, (TweakID + (negation ? " is unavailable" : " is available")).str()) { std::string WrappedCode = wrap(Ctx, arg); Annotations Input(WrappedCode); @@ -67,6 +67,7 @@ MATCHER_P3(TweakIsAvailable, TweakID, Ctx, Header, TestTU TU; TU.HeaderCode = Header; TU.Code = Input.code(); + TU.AdditionalFiles = std::move(ExtraFiles); ParsedAST AST = TU.build(); Tweak::Selection S(AST, Selection.first, Selection.second); auto PrepareResult = prepareTweak(TweakID, S); @@ -113,7 +114,8 @@ std::string TweakTest::apply(llvm::StringRef MarkedCode) const { } ::testing::Matcher TweakTest::isAvailable() const { - return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header); + return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, + ExtraFiles); } std::vector 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 namespace clang { namespace clangd { @@ -47,6 +49,9 @@ public: Expression, }; + // Mapping from file name to contents. + llvm::StringMap 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..d36fd2f34bc5 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 +#include +#include using ::testing::AllOf; using ::testing::HasSubstr; @@ -880,6 +883,170 @@ 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 void foo(); + template<> void foo(); + + template<> void f^oo() { + })cpp"); + EXPECT_UNAVAILABLE(R"cpp( + template void foo(); + + template<> void f^oo() { + })cpp"); + EXPECT_UNAVAILABLE(R"cpp( + template struct Foo { void foo(); }; + + template void Foo::f^oo() { + })cpp"); + EXPECT_AVAILABLE(R"cpp( + template void foo(); + void bar(); + template <> void foo(); + + template<> void f^oo() { + 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) { + EXPECT_UNAVAILABLE(R"cpp( + namespace ns1 { void foo(int); } + namespace ns2 { void foo(int*); } + template + void bar(); + + using ns1::foo; + using ns2::foo; + + template + void b^ar() { + foo(T()); + })cpp"); +} + } // namespace } // namespace clangd } // namespace clang