Author: Sam McCall Date: 2021-06-11T14:29:45+02:00 New Revision: 6aca6032c5b62b5d26999da5f55779a1b08ec6a2
URL: https://github.com/llvm/llvm-project/commit/6aca6032c5b62b5d26999da5f55779a1b08ec6a2 DIFF: https://github.com/llvm/llvm-project/commit/6aca6032c5b62b5d26999da5f55779a1b08ec6a2.diff LOG: [AST] Include the TranslationUnitDecl when traversing with TraversalScope Given `int foo, bar;`, TraverseAST reveals this tree: TranslationUnitDecl - foo - bar Before this patch, with the TraversalScope set to {foo}, TraverseAST yields: foo After this patch it yields: TranslationUnitDecl - foo Also, TraverseDecl(TranslationUnitDecl) now respects the traversal scope. --- The main effect of this today is that clang-tidy checks that match the translationUnitDecl(), either in order to traverse it or check parentage, should work. Differential Revision: https://reviews.llvm.org/D104071 Added: Modified: clang-tools-extra/clangd/DumpAST.cpp clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp clang-tools-extra/clangd/unittests/TestTU.cpp clang/include/clang/AST/ASTContext.h clang/include/clang/AST/RecursiveASTVisitor.h clang/unittests/AST/ASTContextParentMapTest.cpp clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clangd/DumpAST.cpp b/clang-tools-extra/clangd/DumpAST.cpp index 30b90d2e0b548..36d61aca70fa6 100644 --- a/clang-tools-extra/clangd/DumpAST.cpp +++ b/clang-tools-extra/clangd/DumpAST.cpp @@ -337,12 +337,8 @@ class DumpVisitor : public RecursiveASTVisitor<DumpVisitor> { // Generally, these are nodes with position information (TypeLoc, not Type). bool TraverseDecl(Decl *D) { - return !D || isInjectedClassName(D) || traverseNode("declaration", D, [&] { - if (isa<TranslationUnitDecl>(D)) - Base::TraverseAST(const_cast<ASTContext &>(Ctx)); - else - Base::TraverseDecl(D); - }); + return !D || isInjectedClassName(D) || + traverseNode("declaration", D, [&] { Base::TraverseDecl(D); }); } bool TraverseTypeLoc(TypeLoc TL) { return !TL || traverseNode("type", TL, [&] { Base::TraverseTypeLoc(TL); }); diff --git a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp index d6a57efeeef13..a8c937a951dfc 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp @@ -82,7 +82,8 @@ class UsingFinder : public RecursiveASTVisitor<UsingFinder> { // There is no need to go deeper into nodes that do not enclose selection, // since "using" there will not affect selection, nor would it make a good // insertion point. - if (Node->getDeclContext()->Encloses(SelectionDeclContext)) { + if (!Node->getDeclContext() || + Node->getDeclContext()->Encloses(SelectionDeclContext)) { return RecursiveASTVisitor<UsingFinder>::TraverseDecl(Node); } return true; diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 4aa9cb7d66105..87f3c8737d464 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -248,13 +248,23 @@ TEST(DiagnosticsTest, ClangTidy) { return SQUARE($macroarg[[++]]y); return $doubled[[sizeof]](sizeof(int)); } + + // misc-no-recursion uses a custom traversal from the TUDecl + void foo(); + void $bar[[bar]]() { + foo(); + } + void $foo[[foo]]() { + bar(); + } )cpp"); auto TU = TestTU::withCode(Test.code()); TU.HeaderFilename = "assert.h"; // Suppress "not found" error. TU.ClangTidyProvider = addTidyChecks("bugprone-sizeof-expression," "bugprone-macro-repeated-side-effects," "modernize-deprecated-headers," - "modernize-use-trailing-return-type"); + "modernize-use-trailing-return-type," + "misc-no-recursion"); EXPECT_THAT( *TU.build().getDiagnostics(), UnorderedElementsAre( @@ -283,8 +293,12 @@ TEST(DiagnosticsTest, ClangTidy) { DiagSource(Diag::ClangTidy), DiagName("modernize-use-trailing-return-type"), // Verify that we don't have "[check-name]" suffix in the message. - WithFix(FixMessage( - "use a trailing return type for this function"))))); + WithFix( + FixMessage("use a trailing return type for this function"))), + Diag(Test.range("foo"), + "function 'foo' is within a recursive call chain"), + Diag(Test.range("bar"), + "function 'bar' is within a recursive call chain"))); } TEST(DiagnosticsTest, ClangTidyEOF) { diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp index 335993a18d6f0..cb258a5ccb549 100644 --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -195,6 +195,19 @@ const Symbol &findSymbol(const SymbolSlab &Slab, llvm::StringRef QName) { return *Result; } +// RAII scoped class to disable TraversalScope for a ParsedAST. +class TraverseHeadersToo { + ASTContext &Ctx; + std::vector<Decl *> ScopeToRestore; + +public: + TraverseHeadersToo(ParsedAST &AST) + : Ctx(AST.getASTContext()), ScopeToRestore(Ctx.getTraversalScope()) { + Ctx.setTraversalScope({Ctx.getTranslationUnitDecl()}); + } + ~TraverseHeadersToo() { Ctx.setTraversalScope(std::move(ScopeToRestore)); } +}; + const NamedDecl &findDecl(ParsedAST &AST, llvm::StringRef QName) { auto &Ctx = AST.getASTContext(); auto LookupDecl = [&Ctx](const DeclContext &Scope, @@ -217,6 +230,7 @@ const NamedDecl &findDecl(ParsedAST &AST, llvm::StringRef QName) { const NamedDecl &findDecl(ParsedAST &AST, std::function<bool(const NamedDecl &)> Filter) { + TraverseHeadersToo Too(AST); struct Visitor : RecursiveASTVisitor<Visitor> { decltype(Filter) F; llvm::SmallVector<const NamedDecl *, 1> Decls; diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index f103ec6deb1f0..5032f3194a9b7 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -635,11 +635,22 @@ class ASTContext : public RefCountedBase<ASTContext> { ParentMapContext &getParentMapContext(); // A traversal scope limits the parts of the AST visible to certain analyses. - // RecursiveASTVisitor::TraverseAST will only visit reachable nodes, and + // RecursiveASTVisitor only visits specified children of TranslationUnitDecl. // getParents() will only observe reachable parent edges. // - // The scope is defined by a set of "top-level" declarations. - // Initially, it is the entire TU: {getTranslationUnitDecl()}. + // The scope is defined by a set of "top-level" declarations which will be + // visible under the TranslationUnitDecl. + // Initially, it is the entire TU, represented by {getTranslationUnitDecl()}. + // + // After setTraversalScope({foo, bar}), the exposed AST looks like: + // TranslationUnitDecl + // - foo + // - ... + // - bar + // - ... + // All other siblings of foo and bar are pruned from the tree. + // (However they are still accessible via TranslationUnitDecl->decls()) + // // Changing the scope clears the parent cache, which is expensive to rebuild. std::vector<Decl *> getTraversalScope() const { return TraversalScope; } void setTraversalScope(const std::vector<Decl *> &); diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h index a29559e5184ab..9bfa5b9c23260 100644 --- a/clang/include/clang/AST/RecursiveASTVisitor.h +++ b/clang/include/clang/AST/RecursiveASTVisitor.h @@ -192,14 +192,12 @@ template <typename Derived> class RecursiveASTVisitor { /// Return whether this visitor should traverse post-order. bool shouldTraversePostOrder() const { return false; } - /// Recursively visits an entire AST, starting from the top-level Decls - /// in the AST traversal scope (by default, the TranslationUnitDecl). + /// Recursively visits an entire AST, starting from the TranslationUnitDecl. /// \returns false if visitation was terminated early. bool TraverseAST(ASTContext &AST) { - for (Decl *D : AST.getTraversalScope()) - if (!getDerived().TraverseDecl(D)) - return false; - return true; + // Currently just an alias for TraverseDecl(TUDecl), but kept in case + // we change the implementation again. + return getDerived().TraverseDecl(AST.getTranslationUnitDecl()); } /// Recursively visit a statement or expression, by @@ -1495,12 +1493,24 @@ DEF_TRAVERSE_DECL(StaticAssertDecl, { TRY_TO(TraverseStmt(D->getMessage())); }) -DEF_TRAVERSE_DECL( - TranslationUnitDecl, - {// Code in an unnamed namespace shows up automatically in - // decls_begin()/decls_end(). Thus we don't need to recurse on - // D->getAnonymousNamespace(). - }) +DEF_TRAVERSE_DECL(TranslationUnitDecl, { + // Code in an unnamed namespace shows up automatically in + // decls_begin()/decls_end(). Thus we don't need to recurse on + // D->getAnonymousNamespace(). + + // If the traversal scope is set, then consider them to be the children of + // the TUDecl, rather than traversing (and loading?) all top-level decls. + auto Scope = D->getASTContext().getTraversalScope(); + bool HasLimitedScope = + Scope.size() != 1 || !isa<TranslationUnitDecl>(Scope.front()); + if (HasLimitedScope) { + ShouldVisitChildren = false; // we'll do that here instead + for (auto *Child : Scope) { + if (!canIgnoreChildDeclWhileTraversingDeclContext(Child)) + TRY_TO(TraverseDecl(Child)); + } + } +}) DEF_TRAVERSE_DECL(PragmaCommentDecl, {}) diff --git a/clang/unittests/AST/ASTContextParentMapTest.cpp b/clang/unittests/AST/ASTContextParentMapTest.cpp index 855d9700b559b..4d11ef0b796a5 100644 --- a/clang/unittests/AST/ASTContextParentMapTest.cpp +++ b/clang/unittests/AST/ASTContextParentMapTest.cpp @@ -81,27 +81,31 @@ TEST(GetParents, ReturnsMultipleParentsInTemplateInstantiations) { } TEST(GetParents, RespectsTraversalScope) { - auto AST = - tooling::buildASTFromCode("struct foo { int bar; };", "foo.cpp", - std::make_shared<PCHContainerOperations>()); + auto AST = tooling::buildASTFromCode( + "struct foo { int bar; }; struct baz{};", "foo.cpp", + std::make_shared<PCHContainerOperations>()); auto &Ctx = AST->getASTContext(); auto &TU = *Ctx.getTranslationUnitDecl(); auto &Foo = *TU.lookup(&Ctx.Idents.get("foo")).front(); auto &Bar = *cast<DeclContext>(Foo).lookup(&Ctx.Idents.get("bar")).front(); + auto &Baz = *TU.lookup(&Ctx.Idents.get("baz")).front(); // Initially, scope is the whole TU. EXPECT_THAT(Ctx.getParents(Bar), ElementsAre(DynTypedNode::create(Foo))); EXPECT_THAT(Ctx.getParents(Foo), ElementsAre(DynTypedNode::create(TU))); + EXPECT_THAT(Ctx.getParents(Baz), ElementsAre(DynTypedNode::create(TU))); // Restrict the scope, now some parents are gone. Ctx.setTraversalScope({&Foo}); EXPECT_THAT(Ctx.getParents(Bar), ElementsAre(DynTypedNode::create(Foo))); - EXPECT_THAT(Ctx.getParents(Foo), ElementsAre()); + EXPECT_THAT(Ctx.getParents(Foo), ElementsAre(DynTypedNode::create(TU))); + EXPECT_THAT(Ctx.getParents(Baz), ElementsAre()); // Reset the scope, we get back the original results. Ctx.setTraversalScope({&TU}); EXPECT_THAT(Ctx.getParents(Bar), ElementsAre(DynTypedNode::create(Foo))); EXPECT_THAT(Ctx.getParents(Foo), ElementsAre(DynTypedNode::create(TU))); + EXPECT_THAT(Ctx.getParents(Baz), ElementsAre(DynTypedNode::create(TU))); } TEST(GetParents, ImplicitLambdaNodes) { diff --git a/clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp b/clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp index c05be7f2e31f6..9e71f9554e595 100644 --- a/clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp +++ b/clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp @@ -16,6 +16,12 @@ class Visitor : public ExpectedLocationVisitor<Visitor, clang::TestVisitor> { public: Visitor(ASTContext *Context) { this->Context = Context; } + bool VisitTranslationUnitDecl(TranslationUnitDecl *D) { + auto &SM = D->getParentASTContext().getSourceManager(); + Match("TU", SM.getLocForStartOfFile(SM.getMainFileID())); + return true; + } + bool VisitNamedDecl(NamedDecl *D) { if (!D->isImplicit()) Match(D->getName(), D->getLocation()); @@ -41,6 +47,7 @@ struct foo { Ctx.setTraversalScope({&Bar}); Visitor V(&Ctx); + V.ExpectMatch("TU", 1, 1); V.DisallowMatch("foo", 2, 8); V.ExpectMatch("bar", 3, 10); V.ExpectMatch("baz", 4, 12); _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits