[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

92

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

104

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

106

You can simplify it in a single if statement:

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

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

260

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

104

Sure.

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

260

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

37

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

38

nit: remove the {} around the if statement

91

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

247

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).

249

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

251

And this case as well

[hokein]

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

36–37

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

37

Seems to be obsolete with the above patch.

38

Seems to be obsolete with the above patch.

[VitaNuo]

Remove unusued include.

[hokein]

Thanks!

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

class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
   DeclCallback Callback;
 
   void report(SourceLocation Loc, NamedDecl *ND,
               RefType RT = RefType::Explicit) {
     if (!ND || Loc.isInvalid())
       return;
     Callback(Loc, *cast<NamedDecl>(ND->getCanonicalDecl()), RT);
   }
 
   NamedDecl *resolveTemplateName(TemplateName TN) {
     // For using-templates, only mark the alias.
     if (auto *USD = TN.getAsUsingShadowDecl())
       return USD;
     return TN.getAsTemplateDecl();
   }
   NamedDecl *getMemberProvider(QualType Base) {
     if (Base->isPointerType())
       return getMemberProvider(Base->getPointeeType());
     // Unwrap the sugar ElaboratedType.
bool VisitMemberExpr(MemberExpr *E) {
     // "bar.h" for the underlying base type `Bar`).
     //
     // FIXME: support dependent types, e.g., "std::vector<T>().size()".
     QualType Type = E->getBase()->IgnoreImpCasts()->getType();
     // FIXME: this should report as implicit reference.
     report(E->getMemberLoc(), getMemberProvider(Type));
     return true;
   }
+  bool VisitCXXDependentScopeMemberExpr(CXXDependentScopeMemberExpr *E) {
+    report(E->getMemberLoc(), getMemberProvider(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();
       auto IsUsed = TD->isUsed() || TD->isReferenced();
       report(UD->getLocation(), TD,
              IsUsed ? RefType::Explicit : RefType::Ambiguous);
     }
     return true;

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

testWalk(R"cpp(
       namespace ns2 { using Bar = ns::Foo; }
       using ns2::$explicit^Bar;
       )cpp",
            "void test(Bar b) { b.^a; }");
   testWalk(R"cpp(
       namespace ns { template<typename> struct Foo { int a; }; }
       using ns::$explicit^Foo;)cpp",
            "void k(Foo<int> b) { b.^a; }");
+  // Test the dependent-type case (CXXDependentScopeMemberExpr)
+  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(); }");
 }
 
 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() {}");
   testWalk("void foo() {}", "void ^foo();");
 
   // Unresolved calls marks all the overloads.
   testWalk("void $ambiguous^foo(int); void $ambiguous^foo(char);",
            "template <typename T> void bar() { ^foo(T{}); }");
 }
 
 TEST(WalkAST, Enums) {
   testWalk("enum E { $explicit^A = 42, B = 43 };", "int e = ^A;");
   testWalk("enum class $explicit^E : int;", "enum class ^E : int {};");
   testWalk("enum class E : int {};", "enum class ^E : int ;");
 }
 
 } // namespace
 } // namespace clang::include_cleaner