[PATCH] D134542: [Concepts] Recover properly from a RecoveryExpr in a concept

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 3 inline comments as done.
erichkeane added a subscriber: tahonermann.
erichkeane added a comment.

In D134542#3812211 <https://reviews.llvm.org/D134542#3812211>, @ychen wrote:

> The patch looks good. Two high-level questions:
>
> - Does the similar thing happen for class templates? Like a constraint expr of a partial specialization has an error. Maybe we don't assert in this case?

WE currently crash in that case as well: https://godbolt.org/z/MGMqz1x59 .  This patch still crashes in that case, and we should fix that in a similar way.  I'll put it on my list of things to do soon!  I don't want to do it in the same patch, simply because the type resolution parts are going to be completely different, and would likely just double the size of this patch.

> - I suppose the constraint error does not always affect the overload resolution results. When it does not, an alternative would be to assume the constraint is a satisfaction failure and the compilation continues. Do you see any value in this approach? Personally, I could go either way. Basically a trade-off between pessimistic and optimistic.

In cases where the constraint error does not affect overload resolution (like with short-circuiting), this patch makes no changes, and will continue without it.  ONLY when a constraint that references a RecoveryExpr in some way is used will we 'quit' overload resolution.

I ALSO considered just marking as a failure, and continuing, but @tahonermann made a point in a private chat that the result is that we'll end up getting wacky follow-up errors.  Consider something like:

  template<typename T> concept HasFoo = /*Test for has foo*/;
     template<typename T> concept HasBarAlternative = /*test for has bar, but with a typo!*/;
  
     template<typename T> requires HasFoo<T>
     void do_thing(T &t) {
       t.Foo();
       t.Bar();
     }
     template<typename T> requires HasFoo<T> && HasBarAlternative<T>
     void do_thing(T&t) {
       t.Foo();
       t.BarAlternative();
     }

The result of just marking `HasBarAlternative' as not satisfied, is the 1st `do_thing` will be called.  THEN you'd get an error on instantiating because of the lack of `Bar`.  This seems like a worse behavior to me, and results in just nonsense-errors/not useful errors most of the time.



================
Comment at: clang/include/clang/Sema/Overload.h:931
 
+    bool ConstraintFailureBecauseCascadingError() const;
+
----------------
ychen wrote:
> How about `ConstraintExprHasError`? `Cascading` sounds like more details than useful.
Yeah, my name is awful here... I'm not sure `ConstraintExprHasError` is correc tin this case (since this is an OverloadCandidate), so the question is "Does this candidate fail because this is a constraint that contains an error".  I'll try to come up with something better.

Feel free to help bikeshed/workshop something better!


================
Comment at: clang/lib/Sema/SemaOverload.cpp:10123-10127
+  if (DeductionFailure.Result != Sema::TDK_ConstraintsNotSatisfied)
+    return false;
+
+  return static_cast<CNSInfo *>(DeductionFailure.Data)
+      ->Satisfaction.ContainsErrors;
----------------
ychen wrote:
> nit: might be easier to read
yep! I like it.  I tend to do early exit when I can, particularly on large functions, but this ended up being pretty small in the final version of this patch.


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

https://reviews.llvm.org/D134542