[PATCH] D146465: [clang] Fix 2 bugs with parenthesized aggregate initialization

Tue Mar 28 15:40:18 PDT 2023

ayzhao marked 2 inline comments as done.
ayzhao added inline comments.

================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:1582-1596
+  MultiExprArg ExprsToPass;
+  if (Exprs.size() == 1 && isa<CXXParenListInitExpr>(Exprs[0])) {
+    // C++20 [expr.static.cast]p4:
+    //   An expression E can be explicitly converted to a type T...if T is an
+    //   aggregate type ([dcl.init.aggr]) having a first element x and there is
+    //   an implicit conversion sequence from E to the type of x
+    //
----------------
rsmith wrote:
> ayzhao wrote:
> > rsmith wrote:
> > > The input to this function should be a syntactic initializer such as a `ParenListExpr`, not an already-type-checked semantic initializer such as a `CXXParenListInitExpr`. The right place to do this unwrapping is in `TreeTransform::TransformInitializer`, where we unwrap various other kinds of semantic initializer.
> > >  The right place to do this unwrapping is in `TreeTransform::TransformInitializer`, where we unwrap various other kinds of semantic initializer.
> > 
> > So the `CXXParenListInitExpr` for `S` never gets passed to `TreeTransform::TransformInitializer` - rather, `TransformCXXParenListInitExpr(...)` calls `TransformExprs(...)` on its subexpressions. If the subexpression happens to be a `CXXConstructExpr`, then `TransformCXXConstructExpr` will call `TransformInitializer(...)`, which is [why we saw the CXXConstructExpr getting deleted](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L13030-L13036)
> > 
> > Interestingly enough, if we initialize `S` with a braced init-list (i.e. `S{(V&&) Vec}`), we never end up calling `TransformCXXConstructExpr(...)` since [we revert to the syntatic form.](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L11584-L11585).
> > 
> > > The input to this function should be a syntactic initializer such as a `ParenListExpr`, not an already-type-checked semantic initializer such as a `CXXParenListInitExpr`.
> > 
> > We can probably do this by returning a `ParenListExpr` in `TransformCXXParenListInitExpr(...)`. This does cause some problems though. If we use a `ParenListExpr`, how would we know when to bail out in line 1551 (`!isa<InitListExpr>(Exprs[0]) && !isa<CXXParenListInitExpr>(Exprs[0])`)? Surely, not every instance of a `ParenListExpr` would be caused by a `CXXParenListInitExpr`, and if this were the case, that would be a pretty brittle assumption. Furthermore, we never create a `ParenListExpr` for a corresponding `CXXParenListInitExpr`.
> > 
> > As an alternative, we could probably do something similar to `InitListExpr`, which has separate semantic and syntactic forms. Would that work?
> > 
> > Also, in general, are the `TransformFooExpr(...)` functions supposed to return semantic or syntactic results? I seem to see a mix of both...
> OK, I've looked at this a bit more. It looks to me like there are problems in two places:
> 
> 1) [TreeTransform::RebuildCXXParenListInitExpr](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L3889). When transforming the subexpressions of a `CXXParenListInitExpr`, we call `TransformExprs` passing `IsCall=true`, which means we call `TransformInitializer` on each of them, which reverts them to syntactic form. So we need to redo the semantic analysis for forming a `CXXParenListInitExpr` here, and can't just create a new `CXXParenListInitExpr` node directly. Probably the easiest way to handle that would be to produce a `ParenListExpr` here instead.
> 
> 2) [Expr::getSubExprAsWritten](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/AST/Expr.cpp#L1978) ([reached via here](https://github.com/llvm/llvm-project/blob/4c483a046d2ff29ec2fd5bad6305f97424a2b880/clang/lib/Sema/TreeTransform.h#L12070)) is not properly handling `CXXParenListExpr` -- it should be stepping over it to the single argument of the expression, which was the as-written subexpression.
> 
> But looking more into (2), it seems like there's a representational oddity here, too. We use a `CXXFunctionalCastExpr` wrapping a `CXXParenListExpr` to model both the case of `Aggregate(a)` (which is a functional cast, equivalent by definition to `(Aggregate)a`) and the case of `Aggregate(a, b)` (which is //not// a functional cast). I don't think that's necessarily a problem -- we also incorrectly use `CXXFunctionalCastExpr` wrapping an `InitListExpr` for `Aggregate{a, b}` -- but it means that `getSubExprAsWritten` will need to be careful to only unwrap in the case where the parenthesized aggregate initialization expression had exactly one element, and it's not ideal from a source fidelity perspective.
> 
> In the long term, I think we should split `CXXTemporaryObjectExpr` into two -- an outer wrapper representing an explicit type construction expression of the form `T(...)` or `T{...}` (excluding the case `T(a)` which is a functional cast expression) and an inner `CXXConstructExpr` representing the actual constructor call. And then we can represent parenthesized aggregate initialization as either a `CXXTemporaryObjectExpr` wrapping a `CXXParenListInitExpr` (when the number of arguments is not equal to 1) or a `CXXFunctionalCastExpr` wrapping a `CXXParenListInitExpr` (when the number of arguments is equal to 1) -- and similarly we can represent the case of `(Aggregate)a` as a `CastExpr` wrapping a `CXXParenListInitExpr`. But for now I think we can leave the representation as-is and make `TreeTranform` handle it.
> 
> (Incidentally, the name `CXXParenListInitExpr` seems confusing here -- there aren't necessarily any parentheses involved, and the name really ought to mention aggregate initialization. `CXXAggregateInitExpr` would make more sense to me.)
> 
> > If we use a `ParenListExpr`, how would we know when to bail out in line 1551
> 
> We should never see a `ParenListExpr` there (that would represent `T((a, b))`, not `T(a, b)`, which would result in a comma expression not a `ParenListExpr`). Instead, the caller of `BuildCXXTypeConstructExpr` should convert the `ParenListExpr` into a list of expressions passed as `Exprs`, and pass the locations of the parentheses as `LParenOrBraceLoc` and `RParenOrBraceLoc`.
> 
> In particular, I think it would make sense for `TransformCXXFunctionalCastExpr` to detect when it's in this special case where the thing it's transforming isn't a functional cast expression, and call a different `Rebuild...` overload that takes a list of expressions instead of a single expression. (It would make some sense to call `RebuildCXXTemporaryObjectExpr` in this case.)
> 
> > Also, in general, are the `TransformFooExpr(...)` functions supposed to return semantic or syntactic results? I seem to see a mix of both...
> 
> The intent is that `TreeTransform` on an expression removes all implicit nodes -- implicit conversions, semantic forms for initializers, and so on -- and returns the node to how it would have been before it was given to `Sema` as a subexpression. That means that the `Sema` functionality for building an expression with a given set of arguments doesn't need to handle the case where they've already been implicitly converted in some way, which gives us consistent behavior between the initial parse and template instantiation.
> 
> That means that when we see an expression being used as an initializer, we want to strip off all the purely semantic pieces and revert it back to a syntactic form -- that happens when transforming the initializer of a variable, an argument to a function, etc -- and also when we transform an expression that performed some kinds of conversions on its operands, we need to redo those conversions when transforming it.
Patch has been updated per this comment. PTAL.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146465