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

[Erich Keane via Phabricator via cfe-commits]

erichkeane marked 2 inline comments as done.
erichkeane 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.

WOOPS!  I was wrong about 'still crashes' here.  I mis-applied one of your suggestions and it crashed for another reason!  When properly done, this stops that crash, and the Class-template-partial-specializations are just invalid at that point.  I think we should still do the same thing as far as failing-lookup for the same reasons, so I'll do that separately.


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

https://reviews.llvm.org/D134542