[include-cleaner] Handle base class member access from derived class.

[VitaNuo]

Fix formatting.

[hokein]

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

65

This is not safe, we can get a nullptr if this type is not a normal RecordType.

I think there are more cases we need to handle:

1. pointer type (Derived *)
2. reference type (Derived &)
3. a dependent type

For the 3), some code like std::vector<T>().size(), it is tricky -- because we can't get any decl from a dependent type, we need some heuristics (in clangd we have some bits), we don't need to do that in this patch.

In the code implementation, we should add a dedicated handleType(QualType) helper to handle all these cases, so VisitMemberExpr, VisitCXXDependentScopeMemberExpr callbacks can just dispatch the type to this helper.

[VitaNuo]

Handle pointer and reference types.

[VitaNuo]

Refactor QualType to RecordDecl logic into a separate function.

[kadircet]

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

62–66

comment doesn't really say why. maybe include something along the lines as we want to mark usage of most specific type (e.g. derived class, when using members from the base).

[VitaNuo]

Add more comments. Add unimplemented VisitCXXDependentScopeMemberExpr method with a FIXME comment.

[VitaNuo]

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

65

Ok, added pointer types. Reference types seem to be working already. Added a FIXME for dependent members.

[hokein]

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

68

nit: just report(E->getMemberLoc(), getDeclFromTye(Type));.

71

Be more specific about the comment, something like // FIMXE: support the dependent type case like "std::vector<T>().size();".

I will move this FIXME to VisitMemberExpr, and remove this function.

75

• I'd use NamedDecl* as a return type (which is used in report)
• move this method to private, it is only used internally inside class, no need to make it public
• what do you think about naming it resolveType?

76

nit: remove the surrounding {}

79

nit: just return Type->getAsRecordDecl();.

[VitaNuo]

Address review comments, mostly style issues.

[hokein]

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

62–66

I'd rephrase something like -- 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).

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

184

can you add more test cases (the AST node is a bit different among the following cases) to make sure our code handle all of them?

• Derived foo(); foo().^a;
• Derived& foo(); foo().^a;
• Derived().^a;

[VitaNuo]

Address review comments: change comment, add more test cases.

[hokein]

Thanks, looks good.

Add a Fix <github issue link> in the commit message.

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

61

while reading it again, I realize that there is another case (operator overloading) we probably want to handle it as well. It is a more tricky case, no need to worry about it in this patch.

[VitaNuo]

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

61

Thank you. If you'd like me to take care of it, please file an issue with an example that needs to be taken care of :)

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

public:
   ASTWalker(DeclCallback Callback, bool IsHeader)
       : Callback(Callback), IsHeader(IsHeader) {}
 
   bool VisitDeclRefExpr(DeclRefExpr *DRE) {
     report(DRE->getLocation(), DRE->getFoundDecl());
     return true;
   }
 
   bool VisitMemberExpr(MemberExpr *E) {
-    report(E->getMemberLoc(), E->getFoundDecl().getDecl());
+    // Instead of the FieldDecl for MemberExpr, we report the Decl of
+    // the corresponding record.
+    Expr *BE = E->getBase()->IgnoreImpCasts();
+    RecordDecl *RD = BE->getType()->getAsRecordDecl();
+    report(E->getMemberLoc(), RD);
     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, TemplateNames) {
   testWalk("template<typename T> struct $explicit^S { S(T); };", "^S s(42);");
   // Should we mark the specialization instead?
   testWalk(
       "template<typename> struct $explicit^S {}; template <> struct S<int> {};",
       "^S<int> s;");
 }
 
 TEST(WalkAST, MemberExprs) {
-  testWalk("struct S { void $explicit^foo(); };", "void foo() { S{}.^foo(); }");
+  testWalk("struct $explicit^S { void foo(); };", "void foo() { S{}.^foo(); }");
   testWalk(
-      "struct S { void foo(); }; struct X : S { using S::$explicit^foo; };",
+      "struct S { void foo(); }; struct $explicit^X : S { using S::foo; };",
       "void foo() { X{}.^foo(); }");
+  testWalk("struct Base { int a; }; struct $explicit^Derived : public Base {};",
+           "void fun(Derived d) { d.^a; }");
 }
 
 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() {}");