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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D117391#3299483 <https://reviews.llvm.org/D117391#3299483>, @kimgr wrote:

> 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

Agreed. This stuff is about as clear as mud given that the rules change in every version of C++ and no two compilers implement the same constant expression evaluation rules at this point.

> - `A::ret_a` returns a new A, whose constructor does heap allocation; no consteval possible

Agreed, though there are cases where it could be possible: http://eel.is/c++draft/expr.const#5.19.

> - `to_lvalue_ref` attempts to return a reference to a temporary; no consteval possible

Hmm, is that still the case? http://eel.is/c++draft/expr.const#4.7

> 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`.

Assuming that the `to_lvalue_ref(A{})` case should diagnose, then yes, I agree, I think the `ret_a()` portion should as well.



================
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();
 
----------------
kimgr wrote:
> 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 
> Aaron what do you think? Kim can you verify this alternative would also solve the problem without breaking any tests?

I think that's a good idea to explore. I hadn't spotted the `FullExpr` difference when I was looking at it before.


================
Comment at: clang/lib/AST/Expr.cpp:1949-1950
 
     if (E->getCastKind() == CK_ConstructorConversion)
       return cast<CXXConstructExpr>(SubExpr)->getConstructor();
 
----------------
kimgr wrote:
> kimgr wrote:
> > davrec wrote:
> > > I think the errors prove we should fall back to the most conservative possible fix: remove all the other changes and just change these 2 lines to:
> > > ```
> > > if (E->getCastKind() == CK_ConstructorConversion) {
> > >   if (auto *CE = dyn_cast<ConstantExpr>(SubExpr)
> > >     SubExpr = CE->getSubExpr();
> > >   return cast<CXXConstructExpr>(SubExpr)->getConstructor();
> > > }
> > > ```
> > > I'm can't quite remember but I believe that would solve the bug as narrowly as possible.  @kimgr can you give it a try if and see if it avoids the errors?
> > > (If it doesn't, I'm stumped...)
> > Yes, it does. I needed to add the same `ConstantExpr` skipping to the user-defined conversion handling below, but once those two were in place the new unittests succeed and the existing lit tests also.
> > 
> > It does feel a little awkward, though... I wish I had a clearer mental model of how the implicit-skipping is supposed to work. My intuition is telling me `skipImplicitTemporary` should probably disappear and we should use the `IgnoreExpr.h` facilities directly instead, but it's all a little fuzzy to me at this point.
> > 
> > I'll run the full `check-clang` suite overnight with this alternative patch, I have no reason to think there will be problems, but I'll make sure in the morning.
> > 
> > Thanks!
> I can confirm that full `check-clang` also works with the narrower fix. 
> My intuition is telling me skipImplicitTemporary should probably disappear and we should use the IgnoreExpr.h facilities directly instead, but it's all a little fuzzy to me at this point.

This matches the direction I think we should be heading. If we need to add a new facility to `IgnoreExpr.h`, that's also fine, but comments describing how it differs from the existing facilities would be critical.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117391