[include-cleaner] Fix the member-expr-access usage for sugar type.

[hokein]

Summary

Fixes https://github.com/llvm/llvm-project/issues/59533

[sammccall]

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

41

Resolving a template type to only its template name (ignoring template args) seems dubious in general. Foo is an important part of vector<Foo> and a critical part of unique_ptr<Foo>.

Currently we're only using this for the base types of MemberExpr, and here what we really care about is: which decl is responsible for this member? For this, the template (as opposed to args) is a reasonable heuristic. The exceptions I can think of:

• dependent base: template <class T> class Wrapper : public T {}; - this is rare I think
• type aliases: template <class T> using raw_pointer = T*;
• smart pointers (non-exception): unique_ptr<T>: the base of the member expr ends up being T* at least in non-dependent cases, so we're fine

So I'd suggest:

• renaming this function to getMemberProvider(QualType Base) or so
• adding a comment explicitly stating that the template name rather than params generally provide the members

71–72

this comment is confusing: it suggests that reporting a usage preverts inserting a header!
Would be nice to clarify:

• we report a usage so that code returnFoo().bar can keep #include "foo.h"
• reporting the member decl would cause problems (inheritance, aliases)

80

only tangentially related, but should these be implicit references?

I don't think we actually want to insert headers based on them, right? Just allow people to keep the ones that they have inserted that are required for compilation?

This would also make it less severe if our heuristic above goes wrong.

[hokein]

address comments

[hokein]

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

80

I don't think we actually want to insert headers based on them, right?

I think no? We want to insert headers, below is the motivated case

// all.h
#include "foo.h"
#include "declare.h"

// declare.h
class Foo;
Foo& getFoo();

// in main.cc
#include "all.h"
void k() {
   getFoo().member;
}

After the cleanup, we expect: all.h is removed, declare.h, and foo.h are inserted, right? I think this is the case we should care about, it is a simple version of protobuf case.

[sammccall]

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

80

I would expect declare.h to be removed and foo.h to be inserted, and then the user to have to insert Foo.h.

The problem with inserting "foo.h" here is that we'll do it even if declare.h includes foo.h, which is a false positive according to the IWYS principle.

I think this is the case we should care about, it is a simple version of protobuf case.

I think the plan was to have only limited support for forward-declarations (because we can't reason about whether a full declaration is needed), and consider special-casing protos later because we do understand in that case.

[sammccall]

Mostly comment nits and was ready to approve, but I think I found a bug (getAs)

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

46

hmm, isn't getAs<> wrong and dyn_cast right here?

e.g. if we have a UsingType wrapping a TypedefType, we'll return the typedef rather than the using (because we check for typedef first, and getAs<> will unwrap)

50

nit: "A heuristic to" => "A heuristic: "

(otherwise it sounds like the *aim* here that we're approximating is to resolve to only the template name, rather than resolving to the template name being the approximation)

51

use -> using
critial -> critical

52

the template name -> the template

72

will => would (subjunctive makes it clearer this is *not* what we're doing)

cause issues => say what the problems are: "(e.g. force including the base class for inherited members)"

(sorry for unclear previous comment)

[hokein]

address comments.

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

46

oh, right, good catch! Added a test.

I used getAs is to unwrap the outer sugar ElaboratedType for Base, I think we have to do it manually here.

ElaboratedType 0x56313be324a0 'Foo' sugar
`-UsingType 0x56313be32470 'ns::Foo' sugar
  |-UsingShadow 0x56313be32410 'Foo'
  `-RecordType 0x56313be32200 'struct ns::Foo'
    `-CXXRecord 0x56313be32170 'Foo'

80

I would expect declare.h to be removed and foo.h to be inserted, and then the user to have to insert Foo.h.

I think by "foo.h to be inserted" you mean declare.h, right? There is a problem as well, we break the code after the cleanup :( (I'd image this is a common pattern used in LLVM)

Thanks I see the point now. (proto is just a special case rather than doing it in generally). I added a FIXME here, and plan to fix it in a follow-up patch (this is an existing issue).

clang/include/clang/AST/Type.h

2595

with the dyn_cast, we don't need this template specialization for UsingType now. But I think it is useful to keep it (I have spent sometime on debugging out why getAs<Typedef> work but getAs<UsingType> not).

[sammccall]

LG, thanks!

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

45

I think the relative order of unwrapping pointer and elaboratedtype is correct, but I also think we're going to end up adding more ignored sugar here and reasoning about the order is hard.

I think return getMemberProvider(Base->getPointeeType()) (and the same for ElaboratedType) is a more robust pattern that will be easier to extend.

46

Yep, unfortunately I think we have to be explicit about what kind of sugar we want to unwrap vs use.

For written types (i.e. typelocs) we get this for free, as basically want to unwrap if there's another typeloc (lexically) contained. But I don't know if we can easily reuse that machinery.

80

I think by "foo.h to be inserted" you mean declare.h, right

Sorry, yes I meant all.h removed, declare.h inserted. (Not at all what I wrote)

Yes we break the code. The condition for doing so is:
a) the current file is not IWYU-clean to start with
b) the codebase relies on forward-declarations
c) forward-declarations aren't always sufficient

So this is bad, but I'm not sure actually disastrous.

I added a FIXME here, and plan to fix it in a follow-up patch (this is an existing issue).

thanks!

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

 namespace clang::include_cleaner {
 namespace {
 using DeclCallback =
     llvm::function_ref<void(SourceLocation, NamedDecl &, RefType)>;
 
 class ASTWalker : public RecursiveASTVisitor<ASTWalker> {
   DeclCallback Callback;
 
-  bool handleTemplateName(SourceLocation Loc, TemplateName TN) {
-    // For using-templates, only mark the alias.
-    if (auto *USD = TN.getAsUsingShadowDecl()) {
-      report(Loc, USD);
-      return true;
-    }
-    report(Loc, TN.getAsTemplateDecl());
-    return true;
-  }
-
   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();
-    return Type->getAsRecordDecl();
+  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())
+      Base = Base->getPointeeType();
+    if (const auto *TT = Base->getAs<TypedefType>())
+      return TT->getDecl();
+    if (const auto *UT = Base->getAs<UsingType>())
+      return UT->getFoundDecl();
+    // A heuristic to resolve a template type to **only** its template name.
+    // We're only use this method for the base type of MemberExpr, in general
+    // the template name provides the member, and the critial case
+    // `unique_ptr<Foo>` is handled (the base type is a Foo*).
+    //
+    // There are some exceptions that this heuristic could fail (dependent base,
+    // dependent typealias), but we believe they are rare.
+    if (const auto *TST = Base->getAs<TemplateSpecializationType>())
+      return resolveTemplateName(TST->getTemplateName());
+
+    return Base->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).
+    // Reporting a usage of the member decl will cause issues (when the base
+    // type is a type alias or a subclass). Instead, we report a usage of the
+    // base type of the MemberExpr, so that e.g. code `returnFoo().bar` can keep
+    // #include "foo.h" (rather than inserting "bar.h" for the underlying base
+    // type `Bar`).
+    //
     // FIXME: support dependent types, e.g., "std::vector<T>().size()".
     QualType Type = E->getBase()->IgnoreImpCasts()->getType();
-    report(E->getMemberLoc(), resolveType(Type));
+    report(E->getMemberLoc(), getMemberProvider(Type));
     return true;
   }
 
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
     report(E->getLocation(), E->getConstructor(),
            E->getParenOrBraceRange().isValid() ? RefType::Explicit
                                                : RefType::Implicit);
     return true;
public:
 
   bool VisitTypedefTypeLoc(TypedefTypeLoc TTL) {
     report(TTL.getNameLoc(), TTL.getTypedefNameDecl());
     return true;
   }
 
   bool VisitTemplateSpecializationTypeLoc(TemplateSpecializationTypeLoc TL) {
     // FIXME: Handle explicit specializations.
-    return handleTemplateName(TL.getTemplateNameLoc(),
-                              TL.getTypePtr()->getTemplateName());
+    report(TL.getTemplateNameLoc(),
+           resolveTemplateName(TL.getTypePtr()->getTemplateName()));
+    return true;
   }
 
   bool VisitDeducedTemplateSpecializationTypeLoc(
       DeducedTemplateSpecializationTypeLoc TL) {
     // FIXME: Handle specializations.
-    return handleTemplateName(TL.getTemplateNameLoc(),
-                              TL.getTypePtr()->getTemplateName());
+    report(TL.getTemplateNameLoc(),
+           resolveTemplateName(TL.getTypePtr()->getTemplateName()));
+    return true;
   }
 
   bool TraverseTemplateArgumentLoc(const TemplateArgumentLoc &TL) {
     auto &Arg = TL.getArgument();
     // Template-template parameters require special attention, as there's no
     // TemplateNameLoc.
     if (Arg.getKind() == TemplateArgument::Template ||
-        Arg.getKind() == TemplateArgument::TemplateExpansion)
-      return handleTemplateName(TL.getLocation(),
-                                Arg.getAsTemplateOrTemplatePattern());
+        Arg.getKind() == TemplateArgument::TemplateExpansion) {
+      report(TL.getLocation(),
+             resolveTemplateName(Arg.getAsTemplateOrTemplatePattern()));
+      return true;
+    }
     return RecursiveASTVisitor::TraverseTemplateArgumentLoc(TL);
   }
 };
 
 } // namespace
 
 void walkAST(Decl &Root, DeclCallback Callback) {
   ASTWalker(Callback).TraverseDecl(&Root);
 }
 
 } // namespace clang::include_cleaner

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

testWalk(R"cpp(
            "void test(unique_ptr<Foo> &V) { V->^a; }");
   testWalk(R"cpp(
       template <typename T>
       struct $explicit^unique_ptr {
         void release();
       };
       struct Foo {};)cpp",
            "void test(unique_ptr<Foo> &V) { V.^release(); }");
+  // Respect the sugar type (typedef, using-type).
+  testWalk(R"cpp(
+      namespace ns { struct Foo { int a; }; }
+      using $explicit^Bar = ns::Foo;)cpp",
+           "void test(Bar b) { b.^a; }");
+  testWalk(R"cpp(
+      namespace ns { struct Foo { int a; }; }
+      using ns::$explicit^Foo;)cpp",
+           "void test(Foo 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(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;");
 }

clang/include/clang/AST/Type.h

#include "clang/Basic/OpenCLExtensionTypes.def"
   CanQualType getCanonicalTypeUnqualified() const; // in CanonicalType.h
   void dump() const;
   void dump(llvm::raw_ostream &OS, const ASTContext &Context) const;
 };
 
 /// This will check for a TypedefType by removing any existing sugar
 /// until it reaches a TypedefType or a non-sugared type.
 template <> const TypedefType *Type::getAs() const;
+template <> const UsingType *Type::getAs() const;
 
 /// This will check for a TemplateSpecializationType by removing any
 /// existing sugar until it reaches a TemplateSpecializationType or a
 /// non-sugared type.
 template <> const TemplateSpecializationType *Type::getAs() const;
 
 /// This will check for an AttributedType by removing any existing sugar
 /// until it reaches an AttributedType or a non-sugared type.

clang/lib/AST/Type.cpp

#include "clang/AST/TypeNodes.inc"
     }
   }
 }
 
 template <> const TypedefType *Type::getAs() const {
   return getAsSugar<TypedefType>(this);
 }
 
+template <> const UsingType *Type::getAs() const {
+  return getAsSugar<UsingType>(this);
+}
+
 template <> const TemplateSpecializationType *Type::getAs() const {
   return getAsSugar<TemplateSpecializationType>(this);
 }
 
 template <> const AttributedType *Type::getAs() const {
   return getAsSugar<AttributedType>(this);
 }