From 6aca6032c5b62b5d26999da5f55779a1b08ec6a2 Mon Sep 17 00:00:00 2001 From: Sam McCall Date: Fri, 11 Jun 2021 00:16:14 +0200 Subject: [PATCH] [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 --- clang-tools-extra/clangd/DumpAST.cpp | 8 ++--- .../clangd/refactor/tweaks/AddUsing.cpp | 3 +- .../clangd/unittests/DiagnosticsTests.cpp | 20 +++++++++++-- clang-tools-extra/clangd/unittests/TestTU.cpp | 14 +++++++++ clang/include/clang/AST/ASTContext.h | 17 +++++++++-- clang/include/clang/AST/RecursiveASTVisitor.h | 34 ++++++++++++++-------- clang/unittests/AST/ASTContextParentMapTest.cpp | 12 +++++--- .../RecursiveASTVisitorTests/TraversalScope.cpp | 7 +++++ 8 files changed, 86 insertions(+), 29 deletions(-) diff --git a/clang-tools-extra/clangd/DumpAST.cpp b/clang-tools-extra/clangd/DumpAST.cpp index 30b90d2..36d61aca 100644 --- a/clang-tools-extra/clangd/DumpAST.cpp +++ b/clang-tools-extra/clangd/DumpAST.cpp @@ -337,12 +337,8 @@ public: // Generally, these are nodes with position information (TypeLoc, not Type). bool TraverseDecl(Decl *D) { - return !D || isInjectedClassName(D) || traverseNode("declaration", D, [&] { - if (isa(D)) - Base::TraverseAST(const_cast(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 d6a57ef..a8c937a 100644 --- a/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp +++ b/clang-tools-extra/clangd/refactor/tweaks/AddUsing.cpp @@ -82,7 +82,8 @@ public: // 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::TraverseDecl(Node); } return true; diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 4aa9cb7..87f3c87 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 335993a..cb258a5 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 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 Filter) { + TraverseHeadersToo Too(AST); struct Visitor : RecursiveASTVisitor { decltype(Filter) F; llvm::SmallVector Decls; diff --git a/clang/include/clang/AST/ASTContext.h b/clang/include/clang/AST/ASTContext.h index f103ec6..5032f319 100644 --- a/clang/include/clang/AST/ASTContext.h +++ b/clang/include/clang/AST/ASTContext.h @@ -635,11 +635,22 @@ public: 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 getTraversalScope() const { return TraversalScope; } void setTraversalScope(const std::vector &); diff --git a/clang/include/clang/AST/RecursiveASTVisitor.h b/clang/include/clang/AST/RecursiveASTVisitor.h index a29559e..9bfa5b9 100644 --- a/clang/include/clang/AST/RecursiveASTVisitor.h +++ b/clang/include/clang/AST/RecursiveASTVisitor.h @@ -192,14 +192,12 @@ public: /// 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(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 855d970..4d11ef0 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()); + auto AST = tooling::buildASTFromCode( + "struct foo { int bar; }; struct baz{};", "foo.cpp", + std::make_shared()); auto &Ctx = AST->getASTContext(); auto &TU = *Ctx.getTranslationUnitDecl(); auto &Foo = *TU.lookup(&Ctx.Idents.get("foo")).front(); auto &Bar = *cast(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 c05be7f..9e71f95 100644 --- a/clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp +++ b/clang/unittests/Tooling/RecursiveASTVisitorTests/TraversalScope.cpp @@ -16,6 +16,12 @@ class Visitor : public ExpectedLocationVisitor { 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); -- 2.7.4