[AST] Traverse attributes inside DEF_TRAVERSE_DECL macro

[ilya-biryukov]

Summary

Instead of traversing inside the TraverseDecl() function.
Previously the attributes were traversed after Travese(Some)Decl
returns.

Logically attributes are properties of particular Decls and should be
traversed alongside other "child" nodes.

None of the tests relied on this behavior, hopefully this is an indication
that the change is relatively safe.

This change started with a discussion on cfe-dev, for details see:
https://lists.llvm.org/pipermail/cfe-dev/2019-July/062899.html

[ilya-biryukov]

• Visit attributes before visiting the Decl

[ilya-biryukov]

Wanted to write unit tests, but couldn't find any that check traversal order. I'm aware of Tooling/RecursiveASTVisitorTests, but they mostly check that implicit nodes are visited.
Do we have other tests for traversals?

[gribozavr]

Looks like a good idea to me.

Regarding tests, I couldn't find existing tests that check order either. Seems like you'd need to make some minimal infrastructure for that.

[ilya-biryukov]

• Add a test

[ilya-biryukov]

• Reformat

[gribozavr]

clang/unittests/AST/RecursiveASTVisitorTest.cpp

107 ↗

(On Diff #212592)

Please add a newline.

[ilya-biryukov]

• Add the missing newline

cfe/trunk/include/clang/AST/RecursiveASTVisitor.h

 #define ABSTRACT_DECL(DECL)
 #define DECL(CLASS, BASE)                                                      \
   case Decl::CLASS:                                                            \
     if (!getDerived().Traverse##CLASS##Decl(static_cast<CLASS##Decl *>(D)))    \
       return false;                                                            \
     break;
 #include "clang/AST/DeclNodes.inc"
   }
-
-  // Visit any attributes attached to this declaration.
-  for (auto *I : D->attrs()) {
-    if (!getDerived().TraverseAttr(I))
-      return false;
-  }
   return true;
 }
 
 #undef DISPATCH
 
 template <typename Derived>
 bool RecursiveASTVisitor<Derived>::TraverseNestedNameSpecifier(
     NestedNameSpecifier *NNS) {
#define DEF_TRAVERSE_DECL(DECL, CODE)                                          \
   bool RecursiveASTVisitor<Derived>::Traverse##DECL(DECL *D) {                 \
     bool ShouldVisitChildren = true;                                           \
     bool ReturnValue = true;                                                   \
     if (!getDerived().shouldTraversePostOrder())                               \
       TRY_TO(WalkUpFrom##DECL(D));                                             \
     { CODE; }                                                                  \
     if (ReturnValue && ShouldVisitChildren)                                    \
       TRY_TO(TraverseDeclContextHelper(dyn_cast<DeclContext>(D)));             \
+    if (ReturnValue) {                                                         \
+      /* Visit any attributes attached to this declaration. */                 \
+      for (auto *I : D->attrs())                                               \
+        TRY_TO(getDerived().TraverseAttr(I));                                  \
+    }                                                                          \
     if (ReturnValue && getDerived().shouldTraversePostOrder())                 \
       TRY_TO(WalkUpFrom##DECL(D));                                             \
     return ReturnValue;                                                        \
   }
 
 DEF_TRAVERSE_DECL(AccessSpecDecl, {})
 
 DEF_TRAVERSE_DECL(BlockDecl, {

cfe/trunk/unittests/AST/CMakeLists.txt

add_clang_unittest(ASTTests
   DataCollectionTest.cpp
   DeclPrinterTest.cpp
   DeclTest.cpp
   EvaluateAsRValueTest.cpp
   ExternalASTSourceTest.cpp
   Language.cpp
   NamedDeclPrinterTest.cpp
   OMPStructuredBlockTest.cpp
+  RecursiveASTVisitorTest.cpp
   SourceLocationTest.cpp
   StmtPrinterTest.cpp
   StructuralEquivalenceTest.cpp
   )
 
 clang_target_link_libraries(ASTTests
   PRIVATE
   clangAST
   clangASTMatchers
   clangBasic
   clangFrontend
   clangSerialization
   clangTooling
   )

cfe/trunk/unittests/AST/RecursiveASTVisitorTest.cpp

+//===- unittest/AST/RecursiveASTVisitorTest.cpp ---------------------------===//
+//
+// 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 "clang/AST/RecursiveASTVisitor.h"
+#include "clang/AST/ASTConsumer.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Attr.h"
+#include "clang/Frontend/FrontendAction.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/FunctionExtras.h"
+#include "llvm/ADT/STLExtras.h"
+#include "gmock/gmock-generated-matchers.h"
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+#include <cassert>
+
+using namespace clang;
+using ::testing::ElementsAre;
+
+namespace {
+class ProcessASTAction : public clang::ASTFrontendAction {
+public:
+  ProcessASTAction(llvm::unique_function<void(clang::ASTContext &)> Process)
+      : Process(std::move(Process)) {
+    assert(this->Process);
+  }
+
+  std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
+                                                 StringRef InFile) {
+    class Consumer : public ASTConsumer {
+    public:
+      Consumer(llvm::function_ref<void(ASTContext &CTx)> Process)
+          : Process(Process) {}
+
+      void HandleTranslationUnit(ASTContext &Ctx) override { Process(Ctx); }
+
+    private:
+      llvm::function_ref<void(ASTContext &CTx)> Process;
+    };
+
+    return llvm::make_unique<Consumer>(Process);
+  }
+
+private:
+  llvm::unique_function<void(clang::ASTContext &)> Process;
+};
+
+enum class VisitEvent {
+  StartTraverseFunction,
+  EndTraverseFunction,
+  StartTraverseAttr,
+  EndTraverseAttr
+};
+
+class CollectInterestingEvents
+    : public RecursiveASTVisitor<CollectInterestingEvents> {
+public:
+  bool TraverseFunctionDecl(FunctionDecl *D) {
+    Events.push_back(VisitEvent::StartTraverseFunction);
+    bool Ret = RecursiveASTVisitor::TraverseFunctionDecl(D);
+    Events.push_back(VisitEvent::EndTraverseFunction);
+
+    return Ret;
+  }
+
+  bool TraverseAttr(Attr *A) {
+    Events.push_back(VisitEvent::StartTraverseAttr);
+    bool Ret = RecursiveASTVisitor::TraverseAttr(A);
+    Events.push_back(VisitEvent::EndTraverseAttr);
+
+    return Ret;
+  }
+
+  std::vector<VisitEvent> takeEvents() && { return std::move(Events); }
+
+private:
+  std::vector<VisitEvent> Events;
+};
+
+std::vector<VisitEvent> collectEvents(llvm::StringRef Code) {
+  CollectInterestingEvents Visitor;
+  clang::tooling::runToolOnCode(
+      new ProcessASTAction(
+          [&](clang::ASTContext &Ctx) { Visitor.TraverseAST(Ctx); }),
+      Code);
+  return std::move(Visitor).takeEvents();
+}
+} // namespace
+
+TEST(RecursiveASTVisitorTest, AttributesInsideDecls) {
+  /// Check attributes are traversed inside TraverseFunctionDecl.
+  llvm::StringRef Code = R"cpp(
+__attribute__((annotate("something"))) int foo() { return 10; }
+  )cpp";
+
+  EXPECT_THAT(collectEvents(Code),
+              ElementsAre(VisitEvent::StartTraverseFunction,
+                          VisitEvent::StartTraverseAttr,
+                          VisitEvent::EndTraverseAttr,
+                          VisitEvent::EndTraverseFunction));
+}