[include-cleaner] Handle dependent type members in AST

[VitaNuo]

Summary

Handles dependent type members in AST.

[VitaNuo]

Implement dependent type handling for template specialization types.

[VitaNuo]

Remove extra import.

[VitaNuo]

Adjust implementation to handle pointer types.

[VitaNuo]

Add handling of nested types.

[VitaNuo]

Add more test cases.

[VitaNuo]

Add more test cases.

[hokein]

clang-tools-extra/include-cleaner/lib/WalkAST.cpp

75

Rebase the current patch. Move the implementation in the resolveType method, then this method will just pass the BaseType to resolveType.

87

if we just aim to support the vector<T>().size() case, we only need this part of code right?

89

You can simplify it in a single if statement:

if (const TemplateSpecializationType *TS = Type->getAs<TemplateSpecializationType>())

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

220

Thanks for coming up these cases. IMO a more reasonable behavior would be that the target symbol of the reported reference is Nested rather than the outer class Base, but implementing it would require some dependent name lookup in the Base (in clangd we have a similar thing HeuristicResolver, but it is sophisticated).

I think it is find to ignore these cases (these cases are more tricky, and less interesting to us) right now, this would simplify the implementation.

Note that the existing behavior of accessing a member expr from a dependent type is that we don't report any reference on d.^method(), so the original "include base header via a base-member-access from a derived-class object" issue doesn't exist. We aim to improve this behavior for some critical cases.

The critical case we want to support is the vector<T>().size(), if other cases come up in the furture, we could revisit it.

[VitaNuo]

Address review comments.

[VitaNuo]

clang-tools-extra/include-cleaner/lib/WalkAST.cpp

87

Sure.

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

220

Thank you for the extended explanation!

[hokein]

thanks, looks mostly good, I think we can simplify the unittest further.

clang-tools-extra/include-cleaner/lib/WalkAST.cpp

51

nit: we can use const auto* here as the type is explicitly spelled in the getAs<XXX>

52

nit: remove the {} around the if statement

74

nit: I will remove this blank line, to make VisitMemberExpr and VisitCXXDependentScopeMemberExpr group together as they both achieve the same goal.

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

207

I think keeping the above 3 test cases is enough, I would probably add them to the exiting MemberExprs test (with a comment saying this is dependent-type case).

209

I think we can remove this test, this test duplicates the first test

211

And this case as well

[hokein]

clang-tools-extra/include-cleaner/lib/WalkAST.cpp

48

please rebase the patch, this API is changed in https://github.com/llvm/llvm-project/commit/0ab57bdc9745bfc8147831c09ed05073f87e7040, and the TemplateSpecializationType is handled already (to fix the sugar type issue).

[VitaNuo]

Address review comments.

[VitaNuo]

Thanks for the comments!

clang-tools-extra/include-cleaner/lib/WalkAST.cpp

51

Seems to be obsolete with the above patch.

52

Seems to be obsolete with the above patch.

[VitaNuo]

Remove unusued include.

[hokein]

Thanks!

clang-tools-extra/include-cleaner/lib/WalkAST.cpp

 //===--- WalkAST.cpp - Find declaration references in the AST -------------===//
 //
 // 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 "AnalysisInternal.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/Expr.h"
 #include "clang/AST/ExprCXX.h"
+#include "clang/AST/NestedNameSpecifier.h"
 #include "clang/AST/RecursiveASTVisitor.h"
 #include "clang/AST/TemplateName.h"
 #include "clang/AST/Type.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceLocation.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/Support/Casting.h"
 
class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
 
   void report(SourceLocation Loc, NamedDecl *ND,
               RefType RT = RefType::Explicit) {
     if (!ND || Loc.isInvalid())
       return;
     Callback(Loc, *cast<NamedDecl>(ND->getCanonicalDecl()), RT);
   }
 
   NamedDecl *resolveType(QualType Type) {
     if (Type->isPointerType())
       Type = Type->getPointeeType();
+    if (const TemplateSpecializationType *TST =
+            Type->getAs<TemplateSpecializationType>()) {
+      return TST->getTemplateName().getAsTemplateDecl();
+    }
     return Type->getAsRecordDecl();
   }
 
 public:
   ASTWalker(DeclCallback Callback) : Callback(Callback) {}
 
   bool VisitDeclRefExpr(DeclRefExpr *DRE) {
     report(DRE->getLocation(), DRE->getFoundDecl());
     return true;
   }
 
   bool VisitMemberExpr(MemberExpr *E) {
     // A member expr implies a usage of the class type
     // (e.g., to prevent inserting a header of base class when using base
     // members from a derived object).
-    // FIXME: support dependent types, e.g., "std::vector<T>().size()".
     QualType Type = E->getBase()->IgnoreImpCasts()->getType();
     report(E->getMemberLoc(), resolveType(Type));
     return true;
   }
 
+  bool VisitCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr *E) {
+    report(E->getMemberLoc(), resolveType(E->getBaseType()));
+    return true;
+  }
+
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     report(E->getLocation(), E->getConstructor(),
            E->getParenOrBraceRange().isValid() ? RefType::Explicit
                                                : RefType::Implicit);
     return true;
   }
 
   bool VisitOverloadExpr(OverloadExpr *E) {
     // Since we can't prove which overloads are used, report all of them.
     llvm::for_each(E->decls(), [this, E](NamedDecl *D) {
       report(E->getNameLoc(), D, RefType::Ambiguous);
     });
     return true;
   }
 
   bool VisitUsingDecl(UsingDecl *UD) {
     for (const auto *Shadow : UD->shadows()) {
       auto *TD = Shadow->getTargetDecl();

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp

TEST(WalkAST, MemberExprs) {
   testWalk("struct Base { int a; }; struct $explicit^Derived : public Base {};",
            "void fun() { Derived().^a; }");
   testWalk("struct Base { int a; }; struct $explicit^Derived : public Base {};",
            "Derived foo(); void fun() { foo().^a; }");
   testWalk("struct Base { int a; }; struct $explicit^Derived : public Base {};",
            "Derived& foo(); void fun() { foo().^a; }");
 }
 
+TEST(WalkAST, CXXDependentScopeMemberExprs) {
+  testWalk("template<typename T> struct $explicit^Base { void method(); };",
+           "template<typename T> void k(Base<T> t) { t.^method(); }");
+  testWalk("template<typename T> struct $explicit^Base { void method(); };",
+           "template<typename T> void k(Base<T>& t) { t.^method(); }");
+  testWalk("template<typename T> struct $explicit^Base { void method(); };",
+           "template<typename T> void k(Base<T>* t) { t->^method(); }");
+  testWalk("template<typename T> struct $explicit^Base { void method(); };",
+           "template<typename T> void k() { Base<T> t; t.^method(); }");
+  testWalk("template<typename T> struct $explicit^Base { void method(); };",
+           "template<typename T> void k() { Base<T>* t; t->^method(); }");
+  testWalk("template<typename T> struct $explicit^Base { void method(); };",
+           "template<typename T> void k() { Base<T>().^method(); }");
+  testWalk("template<typename T> struct Base { void method(); }; "
+           "template<typename T> struct $explicit^Derived: Base<T> {};",
+           "template<typename T> void k(Derived<T> t) { t.^method(); }");
+}
+
 TEST(WalkAST, ConstructExprs) {
   testWalk("struct $implicit^S {};", "S ^t;");
   testWalk("struct S { $implicit^S(); };", "S ^t;");
   testWalk("struct S { $explicit^S(int); };", "S ^t(42);");
   testWalk("struct S { $implicit^S(int); };", "S t = ^42;");
 }
 
 TEST(WalkAST, Functions) {
   // Definition uses declaration, not the other way around.
   testWalk("void $explicit^foo();", "void ^foo() {}");