[PATCH] D115187: [clangd] Expose CoawaitExpr's operand in the AST

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D115187#3500098 <https://reviews.llvm.org/D115187#3500098>, @nridge wrote:

> I think the issue is related to this loop <https://searchfox.org/llvm/rev/d5d498f9baae218c56dc3a3582ef0083f795f088/clang/lib/Sema/SemaChecking.cpp#14088> in `AnalyzeImplicitConversions()`, which iterates over `Expr::children()`, and adds each child to a list of expressions to be checked for implicit conversions.
>
> `CoroutineSuspendExpr` now has the operand as an extra child, and an implicit conversion in the operand gets diagnosed both when processing the operand, and the common-expr.
>
> I'm guessing this will need an explicit carve-out in `AnalyzeImplicitConversions`. There is already some special handling of other expressions types there, including at least one <https://searchfox.org/llvm/rev/d5d498f9baae218c56dc3a3582ef0083f795f088/clang/lib/Sema/SemaChecking.cpp#13997> whose purpose is to avoid duplicate diagnostics.

Yeah, I agree.
The general issue here is that storing alternate representations that share subexpressions can cause duplicated traversal. I guess having RecursiveASTVisitor do the right thing will cover most cases.

PseudoObjectExpr has this pattern (for objc property access), InitListExpr kind of does this (syntactic vs semantic forms). For both, I don't fully understand the mechanism, and it seems to require hacks in lots of places :-(
FWIW PseudoObjectExpr avoids duplicated diagnostics here by wrapping all its semantic/derived subexpressions inside `OpaqueValueExpr`, the special case you found... I don't know what the implications of trying to reuse that mechanism would be. The explicit carve-out seems simpler.



================
Comment at: clang/lib/Sema/TreeTransform.h:1476
                                 bool IsImplicit) {
-    return getSema().BuildResolvedCoawaitExpr(CoawaitLoc, Result, IsImplicit);
+    // This function rebuilds a coawait-expr given its operator.
+    // For an explicit coawait-expr, the rebuild involves the full set
----------------
nridge wrote:
> sammccall wrote:
> > This is essentially inlining BuildUnresolvedCoawaitExpr, with the await_transform logic dropped, right? I wonder how that compares to adding a `bool Transform` to that function.
> > 
> > Such a param would make everything less direct, but it also seems possible that the two copies could unexpectedly diverge either now (e.g. could the "placeholder type" logic from BuildUnresolved be relevant here?) or more likely in the future.
> > 
> > Up to you, I don't feel strongly here, it just seems a little surprising.
> > This is essentially inlining BuildUnresolvedCoawaitExpr, with the await_transform logic dropped, right?
> 
> Not exactly. Rather, it's attempting to replicate the logic [here](https://searchfox.org/llvm/rev/d5d498f9baae218c56dc3a3582ef0083f795f088/clang/lib/Sema/SemaCoroutine.cpp#734-738), which is where implicit CoawaitExpr's are built in the first place.
> 
> (The only reason I didn't factor out those few lines into a function of their own is that here we already have the `UnresolvedLookupExpr` for the `operator coawait`, whereas there it's calling a helper that builds the lookup-expr followed by calling `Sema::BuildOperatorCoawaitCall()`.)
> 
> So, precisely because `BuildUnresolvedCoawaitExpr` does a few extra things like the "placeholder type" logic that the code we're trying to replicate doesn't, I think it's safer not to try and reuse that here.
Thank you, that makes a lot of sense.
Could you add to the comment something like:
// This mirrors how the implicit CoAwaitExpr is originally created in Sema::ActOnCoroutineBodyStart?


================
Comment at: clang/test/AST/coroutine-locals-cleanup-exp-namespace.cpp:88
 // CHECK-NEXT:          CoawaitExpr
-// CHECK-NEXT:            MaterializeTemporaryExpr {{.*}} 'Task::Awaiter':'Task::Awaiter'
+// CHECK-NEXT:            CXXBindTemporaryExpr {{.*}} 'Task' (CXXTemporary {{.*}})
+// CHECK:                 MaterializeTemporaryExpr {{.*}} 'Task::Awaiter':'Task::Awaiter'
----------------
nridge wrote:
> sammccall wrote:
> > nit: I think it'd be nice to include the type here for consistency
> A full line of example output here is:
> 
> ```
> CXXBindTemporaryExpr 0x1898248 <col:14, col:18> 'Task' (CXXTemporary 0x1898248)
> ```
> 
> not sure what type you have in mind besides `'Task'`, which is already there.
I'm not sure what I meant either, sorry! Looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115187