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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

LG, thanks!



================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:45
+    if (Base->isPointerType())
+      Base = Base->getPointeeType();
+    // Unwrap the sugar ElaboratedType.
----------------
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.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:46
+      Base = Base->getPointeeType();
+    if (const auto *TT = Base->getAs<TypedefType>())
+      return TT->getDecl();
----------------
hokein wrote:
> sammccall wrote:
> > 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)
> 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'
> ```
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.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:70
     QualType Type = E->getBase()->IgnoreImpCasts()->getType();
     report(E->getMemberLoc(), resolveType(Type));
     return true;
----------------
hokein wrote:
> sammccall wrote:
> > hokein wrote:
> > > sammccall wrote:
> > > > 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.
> > > > 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.
> > 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.
> > 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).
> 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!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140095/new/

https://reviews.llvm.org/D140095