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

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D134542#3812292 <https://reviews.llvm.org/D134542#3812292>, @erichkeane wrote:

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

Agreed that short-circuiting cases would not hit this issue. I actually meant cases where a supposedly failed constraint has errors (https://clang.godbolt.org/z/5P7fYYvao). But that could be reconsidered in the future if use cases arise. With this patch, we already handle this better than GCC/MSVC.

  constexpr bool True = true;
  constexpr bool False = b;
  template <typename T> concept C = True;
  template <typename T> concept D = False;
  
  template<D T>
  void foo(T) = delete;
  template<C T>
  void foo(T);
  
  void g() {
      foo(1);
  }



================
Comment at: clang/include/clang/Sema/Overload.h:931
 
+    bool ConstraintFailureBecauseCascadingError() const;
+
----------------
erichkeane wrote:
> 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!
Thanks. This looks good to me.


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

https://reviews.llvm.org/D134542