kadircet updated this revision to Diff 218936. kadircet marked 4 inline comments as done. kadircet added a comment.
- Rebase and address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65433/new/ https://reviews.llvm.org/D65433 Files: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp clang-tools-extra/clangd/unittests/TweakTesting.cpp clang-tools-extra/clangd/unittests/TweakTesting.h clang-tools-extra/clangd/unittests/TweakTests.cpp
Index: clang-tools-extra/clangd/unittests/TweakTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TweakTests.cpp +++ clang-tools-extra/clangd/unittests/TweakTests.cpp @@ -22,8 +22,10 @@ #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" #include "clang/Rewrite/Core/Rewriter.h" +#include "clang/StaticAnalyzer/Core/AnalyzerOptions.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 +35,8 @@ #include "gmock/gmock.h" #include "gtest/gtest.h" #include <cassert> +#include <string> +#include <vector> using ::testing::AllOf; using ::testing::HasSubstr; @@ -554,7 +558,6 @@ EXPECT_THAT(apply(" [[int a = 5;]] a++; "), StartsWith("fail")); // Don't extract return EXPECT_THAT(apply(" if(true) [[return;]] "), StartsWith("fail")); - } TEST_F(ExtractFunctionTest, FileTest) { @@ -648,6 +651,108 @@ EXPECT_THAT(apply(" for(;;) { [[while(1) break; break;]] }"), StartsWith("fail")); } + +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( + #include "a.h" + 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( + #include "a.h" + void fo^o(int baz) { + int x = 10; + bar(); + })cpp"); + + // FIXME: Move declaration below bar to make it visible. + 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"); +} + } // namespace } // namespace clangd } // namespace clang Index: clang-tools-extra/clangd/unittests/TweakTesting.h =================================================================== --- clang-tools-extra/clangd/unittests/TweakTesting.h +++ 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 { @@ -33,8 +35,6 @@ // EXPECT_UNAVAILABLE("auto ^X^ = ^foo<int>();"); // } class TweakTest : public ::testing::Test { - const char *TweakID; - public: // Inputs are wrapped in file boilerplate before attempting to apply a tweak. // Context describes the type of boilerplate. @@ -47,6 +47,8 @@ Expression, }; + llvm::StringMap<std::string> ExtraFiles; + protected: TweakTest(const char *TweakID) : TweakID(TweakID) {} @@ -76,6 +78,8 @@ // Returns a matcher that accepts marked code snippets where the tweak is // available at the marked range. ::testing::Matcher<llvm::StringRef> isAvailable() const; + + const char *TweakID; }; #define TWEAK_TEST(TweakID) \ Index: clang-tools-extra/clangd/unittests/TweakTesting.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TweakTesting.cpp +++ clang-tools-extra/clangd/unittests/TweakTesting.cpp @@ -59,7 +59,7 @@ 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 @@ 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); @@ -111,7 +112,8 @@ } ::testing::Matcher<llvm::StringRef> TweakTest::isAvailable() const { - return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header); + return TweakIsAvailable(llvm::StringRef(TweakID), Context, Header, + ExtraFiles); } std::vector<std::string> TweakTest::expandCases(llvm::StringRef MarkedCode) { Index: clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/refactor/tweaks/DefineInline.cpp @@ -0,0 +1,238 @@ +//===--- 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 "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; +} + +// Runs clang indexing api to get a list of declarations referenced inside +// function decl. Skips local symbols. +llvm::DenseSet<const Decl *> getNonLocalDeclRefs(const FunctionDecl *FD, + ParsedAST &AST) { + class DeclRecorder : public index::IndexDataConsumer { + public: + bool handleDeclOccurence(const Decl *D, index::SymbolRoleSet Roles, + ArrayRef<index::SymbolRelation> Relations, + SourceLocation Loc, ASTNodeInfo ASTNode) override { + // We only care about referenced decls. + if (!(Roles & static_cast<unsigned int>(index::SymbolRole::Reference))) + return true; + + // Skip method and field refs, since they are accessed through a VarDecl, + // and that is the relavant Decl. + if (llvm::isa<CXXMethodDecl>(D) || llvm::isa<FieldDecl>(D)) + return true; + + // Store the original decl, e.g. specialization rather than templated + // decl. + DeclRefs.insert(ASTNode.OrigD); + return true; + } + + llvm::DenseSet<const Decl *> DeclRefs; + }; + DeclRecorder Recorder; + index::IndexingOptions Opts; + Opts.SystemSymbolFilter = + index::IndexingOptions::SystemSymbolFilterKind::None; + + index::indexTopLevelDecls(AST.getASTContext(), AST.getPreprocessor(), {FD}, + Recorder, std::move(Opts)); + return Recorder.DeclRefs; +} + +// Checks the decls mentioned in Source are visible in the context of Target. +// Achives that by performing lookups in Target's DeclContext. +// Unfortunately these results need to be post-filtered since the AST we get is +// for the fully parsed TU. Therefore might contain decls that were not visible +// while parsing Target. +// Post filtering is currently done by filtering decls defined after Target, +// unless they are declared in the same class. +bool checkDeclsAreVisible(const llvm::DenseSet<const Decl *> &DeclRefs, + const FunctionDecl *Target, const SourceManager &SM) { + const DeclContext *TargetContext = Target->getDeclContext(); + if (!TargetContext) + return false; + + // 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 *D : DeclRefs) { + if (D == Target) + continue; + + // FIXME: Allow declarations from different files with include insertion. + if (!SM.isWrittenInSameFile(D->getLocation(), Target->getLocation())) + return false; + // Since we've got the full translation unit in the AST, make sure + // declaration of Result comes before Target, or decl and target are in the + // same class. + if (!SM.isBeforeInTranslationUnit(D->getLocation(), + Target->getLocation())) { + if (!Class) + return false; + // Or they are in the same class. + 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 the previous +// declaration instead of the first one, since it will be template decl itself +// rather then declaration of specialization. +const FunctionDecl *choseCanonical(const FunctionDecl *FD) { + if (FD->isFunctionTemplateSpecialization()) { + // For specializations CanonicalDecl is the TemplatedDecl, which is not the + // target we want to inline into. Instead we use the previous declaration. + return FD->getPreviousDecl(); + } + return FD->getCanonicalDecl(); +} + +/// Moves definition of a function 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 "Inline function definition"; } + + // 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; + + Target = choseCanonical(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. + auto DeclRefs = getNonLocalDeclRefs(Source, Sel.AST); + if (!checkDeclsAreVisible(getNonLocalDeclRefs(Source, Sel.AST), 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 Index: clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt +++ clang-tools-extra/clangd/refactor/tweaks/CMakeLists.txt @@ -14,6 +14,7 @@ add_clang_library(clangDaemonTweaks OBJECT AnnotateHighlightings.cpp DumpAST.cpp + DefineInline.cpp ExpandAutoType.cpp ExpandMacro.cpp ExtractFunction.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits