[PATCH] D130308: [clang] extend getCommonSugaredType to merge sugar nodes

[Matheus Izvekov via Phabricator via cfe-commits]

mizvekov marked 5 inline comments as done.
mizvekov added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:12218
+        CTN,
+        NExpX && NExpY ? Optional<unsigned>(std::min(*NExpX, *NExpY)) : None);
+  }
----------------
davrec wrote:
> I'm not clear on how `NExpX` could not equal `NExpY` - could you add a test which demonstrates this case?
It may not be needed in this patch, and it might in fact be related to a bug which I already solved in the main resugaring patch. I will double check later.


================
Comment at: clang/lib/AST/ASTContext.cpp:12767
+        Ctx.getQualifiedType(Underlying),
+        ::getCommonDecl(EX->getOwnedTagDecl(), EY->getOwnedTagDecl()));
+  }
----------------
davrec wrote:
> mizvekov wrote:
> > davrec wrote:
> > > mizvekov wrote:
> > > > davrec wrote:
> > > > > This last argument should probably be omitted/nullptr passed, since
> > > > > # We probably won't encounter any owned tag decls in the types passed to this function;
> > > > > # Even if we did, it would not necessarily be true that they both have non-null owned tag decls, and I don't see any nullptr checks in getCommonDecl; and
> > > > > # Even if we checked for nullptr there, and say passed the same argument to X and Y so EX==EY, and that had an owned tag decl, it is not clear to me it would be appropriate to construct a new type with the same owned tag decl as another type.
> > > > 1. We can see, a one liner like this will do: `int e = true ? (struct S*)0 : (struct S*)0;`
> > > > 
> > > > Though normally in an example like the above, you would only see the owned decl on X, because when we get to Y the name has already been introduced into the scope.
> > > > 
> > > > I have an idea for a test case, but it's a bit convoluted:
> > > > 
> > > > This also introduces an OwnedTagDecl: `(struct S<T>*)0`.
> > > > 
> > > > So with resugaring, if we resugar T, it might be possible to construct this situatiation. Given if it's appropriate to keep the OwnedTagDecl when resugaring, of course, which goes to 3)
> > > > 
> > > > 2. This is handled by `declaresSameEntity`, if either or both decls are null, we say they are not the same decl.
> > > > 
> > > > 3. That I am not sure. I see that this OwnedTagDecl is only used by the type printer, and it replaces printing the rest of the type by printing the OwnedDecl instead. So why do you think it would not be appropriate that the rebuilt type keeps this?
> > > (Re #3) Because I think the sense ownership is respected in how ownedTagDecls are presently handled: at most one ElaboratedType owns a particular TagDecl, *and* that type does not seem to be reused elsewhere in the AST.  (This is relied on in https://reviews.llvm.org/D131685, which was on my mind when I looked at this.)
> > > 
> > > E.g. consider this expansion of your example:
> > > ```
> > > auto e = true ? (struct S*)0 : (true ? (struct S*)0 : (struct S*)0);
> > > ```
> > > The first ElaboratedType has an ownedTagDecl; the second is a distinct ElaboratedType without an ownedTagDecl, and the third equals the second.  
> > > 
> > > Now in practice what this means is that `getCommonDecl` when used on binary expressions of this sort will always be nullptr, so no harm done.  But suppose it is called with X=Y=some ET with an ownedTagDecl (which is the only time I believe we could see a non null getCommonDecl result here).  Should the type that is returned have that same owned tag decl?  I think that would violate the sense of the original type "owning" that decl that seems to be respected elsewhere in the AST.
> > > 
> > > Re type printing, I believe that ownedTagDecls only affects printing of defined TagDecls like `struct S { int i; } x;` (which is what I was mostly thinking re ownedTagDecls - I don't think we will see those here); for undefined ones like `struct S x;`, printing should be unaffected whether there is an ownedTagDecl or not.
> > > 
> > With type deduction this can happen:
> > 
> > ```
> > auto x = (struct S*)0; // it will also appear in the type of x
> > using t = decltype(x); // and now also in t
> > ```
> > 
> > With your second example, it can also happen:
> > 
> > ```
> > struct S { int i; } x;
> > int e = true ? &x : (struct S*)0;
> > ```
> > 
> > In those cases, the type node that owns the decl will be the same object, but that is only because of uniquing.
> > 
> > It may be possible that two different objects end up in this situation, if for example they come from different modules that got merged.
> > With type deduction this can happen:
> > ```
> > auto x = (struct S*)0; // it will also appear in the type of x
> > using t = decltype(x); // and now also in t
> > ```
> 
> My local clang is out of date - the type of `x` for me is just an AutoType wrapping a PointerType to a RecordType.  In light of the addition of the ElaboratedType there, it seems fine to be consistent here with however that case handles ownedTagDecls, but FWIW I do not think that deduced ElaboratedType should have an ownedTagDecl either.  Only the original type should be the owner - not any type deduced from it.
> 
> This is a minor issue with few if any observable effects for now, but should be kept in mind if issues arise later: the fact the ownership of ownedTagDecls is respected was the only factor that made https://discourse.llvm.org/t/ast-parent-of-class-in-a-friend-declaration/64275 easily solvable.
> 
> > With your second example, it can also happen:
> > 
> > ```
> > struct S { int i; } x;
> > int e = true ? &x : (struct S*)0;
> > ```
> 
> The DeclRefExpr `x` indeed reuses the ElaboratedType used in the VarDecl `x`, with its ownedTagDecl and all.  That seems fair, a special case.  For all other type deductions I would mildly object to including an ownedTagDecl.
> 
I think that assumption might work on ElaboratedTypeLocs instead. I think only one per TU should appear in the AST.
Resugaring might make us rebuild such TypeLocs, but the non-resugared one should be unreachable from the AST.


================
Comment at: clang/lib/AST/ASTContext.cpp:12872
   }
 
   SplitQualType SX = X.split(), SY = Y.split();
----------------
davrec wrote:
> It would be very helpful to incorporate your description and the description from D111283 as comments in this function.  E.g. something like the following ...
Thanks! I also added some extras in there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130308