[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

[Kim Gräsman via Phabricator via cfe-commits]

kimgr added a comment.

I have now convinced myself that including `FullExpr` in `skipImplicitTemporary` gives an improvement in `consteval` diagnostics. But I'm still not sure why. Motivating example, derived from cxx2a-consteval.cpp:

  struct A {
    int *p = new int(42);
    consteval A ret_a() const {
        return A{};
    }
  };
  
  consteval const A &to_lvalue_ref(const A &&a) {
    return a;
  }
  
  void test() {
    constexpr A a{nullptr};
  
    // error: call to consteval function 'A::ret_a' is not a constant expression
    // { (void)a.ret_a(); }
  
    // error: call to consteval function 'to_lvalue_ref' is not a constant expression
    // { (void)to_lvalue_ref(A{}); }
  
    // error: call to consteval function 'to_lvalue_ref' is not a constant expression
    // but should probably also raise
    // error: call to consteval function 'A::ret_a' is not a constant expression
    { (void)to_lvalue_ref(a.ret_a()); }
  }

It's interesting to experiment with these one by one

- `A::ret_a` returns a new A, whose constructor does heap allocation; no consteval possible
- `to_lvalue_ref` attempts to return a reference to a temporary; no consteval possible

Composing the two as in the last example, it seems to me, should print both diagnostics. Mainline Clang doesn't, but changing `skipImplicitTemporary` to also skip `FullExpr`s does (also allowing removal of all `IgnoreImplicit` from `getSubExprAsWritten` and `getConversionFunction`).

It seems intuitively right to me. I'm just a little peeved that I can't figure out the connection between the diagnostic emission and `skipImplicitTemporary`.



================
Comment at: clang/lib/AST/Expr.cpp:1946-1947
   for (const CastExpr *E = this; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) {
     SubExpr = skipImplicitTemporary(E->getSubExpr());
+    SubExpr = SubExpr->IgnoreImplicit();
 
----------------
davrec wrote:
> aaron.ballman wrote:
> > `IgnoreImplicit()` calls `IgnoreImplicitSingleStep()` eventually, and that call does the same work as `skipImplicitTemporary()`, so I think the end result here should be the same.
> As I look at this a second time, I just realized...calling IgnoreImplicit here mean that the loop only ever runs one iteration, since IgnoreImplicit presumably skips over ImplicitCastExprs.  While I liked how Kim did revision initially because it seemed to handle the constructor conversions similarly to their handling of getSubExprAsWritten() above, now I think something different is needed here.
> 
> Proposed alternative:
> Right now skipIimplicitTemporary does what IgnoreImplicit does *except* skip over a) ImplicitCastExprs and b) FullExprs (= ConstantExprs or ExprWithCleanups).
> 
> Kim has identified that we need to skip over at least ConstantExprs at least in this case (i.e. the most conservative possible fix would be to skip over ConstantExprs just before the cast in line 1950).
> 
> But perhaps the better solution, to forestall future bugs, is to skip over FullExprs in skipImplicitTemporary, so that it skips over everything IgnoreImplicit does except ImplicitCastExprs.  (And, some documentation should be added to `skipImplicitTemporary` to that effect, to aid future maintenance.)
> 
> I can't see offhand how the other uses of skipImplicitTemporary would be negatively affected by additionally skipping over FullExprs.
> 
> Aaron what do you think?  Kim can you verify this alternative would also solve the problem without breaking any tests?
> 
@aaron.ballman Just removing the `skipImplicitTemporary` line is probably not going to work, since that's the first time `SubExpr` is assigned a 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391