[PATCH] D133052: [clang] Avoid crash when expanding conversion templates in concepts.

[Yuanfang Chen via Phabricator via cfe-commits]

ychen added a comment.

In D133052#3792121 <https://reviews.llvm.org/D133052#3792121>, @ilya-biryukov wrote:

> It seems wrong to change semantics of initialization when instantiating concept requirements. It implies that semantic checking may behave differently inside requires expressions, this is a red flag.
> Clang has a mechanism <https://github.com/llvm/llvm-project/blob/84d07d021333f7b5716f0444d5c09105557272e0/clang/lib/Sema/SemaOverload.cpp#L7429> to prevent recursion when considering user-defined conversion for copy initialization.
>
> Currently the intention of the Clang code path that handles this case is to:
>
> 1. Deduce `TO` to be `Deferred`
> 2. Try to check `Numeric<Deferred>`,
> 3. Check conversions during overload resolution for `foo(a)`. Go to step 1, <infinite recursion, so we never get to the next step>
> 4. <never reached> Check <https://github.com/llvm/llvm-project/blob/84d07d021333f7b5716f0444d5c09105557272e0/clang/lib/Sema/SemaOverload.cpp#L7429> that conversion operator converts the type to itself and mark the candidate as failed.
>
> If move the check in step 4 to happen before step 3 (even if we need to duplicate the check), we will get the desired semantics.
>
> Does that look reasonable?

I think the direction we sould go is not to keep this "semantics" (I'm not sure it should be regarded as semantics because the standard wording does not describe any language-level behavior, instead, just say the constructor should not be used in certain cases).  Because the bug report and user experience are that this behavior is surprising and no references in the language require it so it is hard to reason about. I guess that's the reason the original implementation describe it as a "workaround". I'd only consider this workaround (i.e, hoist an certain overload resolution rule to avoid recursion) if you're blocked by this and needs a quick fix.

Ideally, I think we should implement this copy elision elsewhere like CodGen to avoid this unexpected semantic (conversion functions become candidate).

> Clang will currently cut this out because the template instantiation depth is too high, whereas GCC will provide a useful diagnostics that says concept satisfaction computation is recursive.
> We probably want a more informative error message too. Probably worth investigating separately from that particular change.

Agreed. That requires either walking or maintaining extra states in the template instantiation stack and detects cycle, which would make this patch unnecessary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133052