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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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

> 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);
>   }

Thanks for the quick review!  Yeah, that case is a tough one, we don't really have a good way of determining whether that was supposed to fail, or be the best match.  GCC does the option of just treating it as failed, so it'll have the problem that I mentioned above.  No idea what MSVC is doing though.... thats frightening.


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

https://reviews.llvm.org/D134542