[concepts] deferred substitution into requirements of class template members not implemented

[marehr]

Bugzilla Link	44833
Version	trunk
OS	Linux
CC	@CaseyCarter,@erichkeane,@friedkeenan,@h-2,@JohelEGP,@AlexeyDmitriev,@PiliLatiesa,@zygoloid,@saarraz,@HighCommander4,@jwakely

Extended Description

Sorry for this verbose and hard to read code, but I wanted to make this configurable.

This code basically has multiple code paths. If REQUIRES_BUG is set, the impl function uses requires to infer if char_is_valid_for is defined or not. If not impl uses SFINAE.

#include <utility>

// #define REQUIRES_BUG     1 - enable the requires bug, 0 - disable it
// #define FUNCTION_DEFINED 1 - define function char_is_valid_for, 0 - do not define char_is_valid_for 

template <int I> struct priority_tag : priority_tag<I - 1> {};
template <> struct priority_tag<0> {};

struct alphabet_base {};

#if FUNCTION_DEFINED
using expect_return_type = bool;
bool char_is_valid_for(char, alphabet_base) { return true; };
#else
using expect_return_type = int;
#endif

namespace seqan3::detail::adl_only {

template <typename...> void char_is_valid_for(...) = delete;

template <typename alph_t>
struct char_is_valid_for_fn
{
    using s_alph_t = alph_t;

#if REQUIRES_BUG
    template <typename t>
    static decltype(auto) impl(priority_tag<1>, t v)
        requires requires { (char_is_valid_for(v, s_alph_t{})); }
    { return (char_is_valid_for(v, s_alph_t{})); }
#else
    template <typename t>
    static auto impl(priority_tag<1>, t v)
        -> std::enable_if_t<true, decltype(char_is_valid_for(v, s_alph_t{}))>
    { return (char_is_valid_for(v, s_alph_t{})); }
#endif

    template <typename t, typename = void>
    static decltype(auto) impl(priority_tag<0>, t v) { return 0; }
};
} // namespace seqan3::detail::adl_only

int main() {
  auto a = seqan3::detail::adl_only::char_is_valid_for_fn<alphabet_base>::impl(priority_tag<1>{}, char{});
  static_assert(std::is_same_v<decltype(a), expect_return_type>);
}

As you can see on https://godbolt.org/z/tyeiJ2 when defining clang++ -std=c++2a -DREQUIRES_BUG=1 -DFUNCTION_DEFINED=0 the requires block should silently see that the requires is not valid (because of a deleted function) and skip to the next definition.

Error Message:

<source>:31:15: error: call to deleted function 'char_is_valid_for'
    { return (char_is_valid_for(v, s_alph_t{})); }
              ^~~~~~~~~~~~~~~~~

<source>:45:75: note: in instantiation of function template specialization 'seqan3::detail::adl_only::char_is_valid_for_fn<alphabet_base>::impl<char>' requested here

  auto a = seqan3::detail::adl_only::char_is_valid_for_fn<alphabet_base>::impl(priority_tag<1>{}, char{});
                                                                          ^

<source>:20:29: note: candidate function [with $0 = <>] has been explicitly deleted
template <typename...> void char_is_valid_for(...) = delete;
                            ^

<source>:45:8: error: variable has incomplete type 'void'
  auto a = seqan3::detail::adl_only::char_is_valid_for_fn<alphabet_base>::impl(priority_tag<1>{}, char{});
       ^

2 errors generated.

Interestingly, if you substitute s_alph_t with alph_t; everything works as expected.

[zygoloid]

The difference between GCC and Clang is due to an open question for C++20 concepts: do we defer substitution into member requires-clauses or not? The current thinking in the committee is that we do not, which is what Clang implements (the older direction was that we would defer such substitution, which is what GCC implements).

You can avoid this problem by using a named concept instead of an inline requires-clause: https://godbolt.org/z/XCjyw_

[zygoloid]

(Using a named concept allows the code to be written a bit more concisely too: https://godbolt.org/z/QsSnXM)

[marehr]

Thank you Richard Smith for the quick response.

So if I understand you correctly an inline requires is currently okay, if I don't use anything declared within the context of the class.

That means

• https://godbolt.org/z/B_hMwk is "undefined behaviour" because I declare s_alph_t within the class and use it in an inline requires
• moving the typedef using s_alph_t = alph_t; into the template parameter is okay, because it is outside of the class (https://godbolt.org/z/yffhHc)
• or using named concepts which are per definition not inside the class (https://godbolt.org/z/aJx4C9)

Do you know what the voting in the last meeting on this was and what the resolution is?

As a user I think of inline requires (if you don't use them in combination with named concepts) as glorified enable_if's with better diagnostic output. Because subsumption can only take place with named concepts, so I personally would prefer if they behave the same.

This code was reduced and was originally part of a macro that made CPO overloads easier, if I have to use named concepts this would defeat the purpose of that macro.

#define SEQAN3_CPO_IMPL(PRIO, TERM) \
template <typename t, typename ...arg_ts> \
static constexpr decltype(auto) impl(seqan3::detail::priority_tag<PRIO> const &, t && v, [[maybe_unused]] arg_ts && ... args) \
    requires requires (seqan3::detail::priority_tag<PRIO> const &, t && v, arg_ts && ... args) \
    { { TERM }; } \
{ return TERM; }

But maybe we have to find a different solution.

On a side note, none of my/our solutions work with msvc.

[zygoloid]

Do you know what the voting in the last meeting on this was and what the resolution is?

The rule that we decided on is this:

• Substitution of template arguments into a class does not substitute into constraints, ever.
• Constrained friends in classes that mention template arguments from outside the friend are "unique", do not redeclare anything else, and must be definitions. (This avoids the need to substitute into their constraints for mangling and declaration matching.)
• Declaration matching for out-of-class explicit specializations, instantiations, and so on, of a member of a class template does perform full substitution into the constraints; substitution failure in the immediate context results in the declaration not matching.

This is closer to GCC's behavior than to Clang's, and in particular means your example is valid and this is a Clang bug.

[CaseyCarter]

+1, stumbled across this implementing std::ranges::view_interface (which is specified to expect "Thou shalt not substitute into requires-clauses before overload resolution!").

[llvmbot]

Is this the same issue that prevents the use of ranges from GCC 10.2.0 libstdc++?

https://godbolt.org/z/f51ses

Which boils down to:

template <typename D>
struct CRTP {
   void call_foo() requires
     requires (D& v) { v.foo();} {
       static_cast<D*>(this)->foo();
   }
};

struct Test: public CRTP<Test> {
   void foo() { }
};

int main()
{
   Test t;
   t.call_foo();
   return 0;
}

<source>:16:7: error: invalid reference to function 'call_foo': constraints not satisfied
   t.call_foo();
     ^
<source>:4:27: note: because 'v.foo()' would be invalid: no member named 'foo' in 'Test'
     requires (D& v) { v.foo();} {
                         ^

https://godbolt.org/z/8jrz9q

[zygoloid]

*** Bug llvm/llvm-bugzilla-archive#47508 has been marked as a duplicate of this bug. ***

[PiliLatiesa]

+1

Having to use a named concept or boolean variable is counter-intuitive.

[friedkeenan]

I believe this is a genuine bug, as the standard seems to expect that the requires clause of a method is only evaluated when the method is used, see std::ranges::view_interface: https://eel.is/c++draft/view.interface

Those requires clauses can't be satisfied with incomplete types.

[erichkeane]

Is this the same issue that prevents the use of ranges from GCC 10.2.0
libstdc++?

https://godbolt.org/z/f51ses

Which boils down to:

template <typename D>
struct CRTP {
   void call_foo() requires
     requires (D& v) { v.foo();} {
       static_cast<D*>(this)->foo();
   }
};

struct Test: public CRTP<Test> {
   void foo() { }
};

int main()
{
   Test t;
   t.call_foo();
   return 0;
}

<source>:16:7: error: invalid reference to function 'call_foo': constraints
not satisfied
   t.call_foo();
     ^
<source>:4:27: note: because 'v.foo()' would be invalid: no member named
'foo' in 'Test'
     requires (D& v) { v.foo();} {
                         ^

https://godbolt.org/z/8jrz9q

Thanks for the reproducer! I've poked around this for a few days, and this example is pretty clarifying. In this case, the constraint fails because we've been evaluating it immediately upon instantiation of the class (in the definition of 'Test''s inheritence), rather than waiting until it is called.

I see that Richard posted about it here:

Substitution of template arguments into a class does not substitute into constraints, ever.

Which I think is likely the solution to the ranges problem as well! Unfortunately, it is a pretty significant architectural change to make this happen as far as I can tell. I'm still looking into it, as compiling ranges is pretty important for us.

[erichkeane]

Do you know what the voting in the last meeting on this was and what the resolution is?

The rule that we decided on is this:

• Substitution of template arguments into a class does not substitute
  into constraints, ever.
• Constrained friends in classes that mention template arguments from
  outside the friend are "unique", do not redeclare anything else, and must be
  definitions. (This avoids the need to substitute into their constraints for
  mangling and declaration matching.)
• Declaration matching for out-of-class explicit specializations,
  instantiations, and so on, of a member of a class template does perform
  full substitution into the constraints; substitution failure in the
  immediate context results in the declaration not matching.

This is closer to GCC's behavior than to Clang's, and in particular means
your example is valid and this is a Clang bug.

Hi Richard-
I'm trying to work through this(at least the 1st bullet at least), but am having trouble finding the appropriate place to do the instantiation work for a called function. I was wondering if you had a link to the wording that made that change? Or some suggestion on how to go about doing those checks?

[jwakely]

+1, stumbled across this implementing std::ranges::view_interface (which is
specified to expect "Thou shalt not substitute into requires-clauses before
overload resolution!").

Is this the same issue that prevents the use of ranges from GCC 10.2.0
libstdc++?

Yes: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102807

[AaronBallman]

*** #49634 has been marked as a duplicate of this bug. ***

[h-2]

Is there any progress on this issue? Does it indeed break all usage of views from libstdc++?

[zygoloid]

mentioned in #46852

[AaronBallman]

mentioned in #49634

[erichkeane]

mentioned in #50208

[erichkeane]

Is there any progress on this issue? Does it indeed break all usage of views from libstdc++?

Sorry @h-2 that this took a while to respond to. My understanding is yes, this breaks a significant portion of the 'ranges' views (and much of the rest of ranges as well). I looked at it for a while and made only a minimum amount of progress, not enough to consider it 'progress'. I can disable the premature instantiation of the concept, but cannot figure out how to properly re-instantiate it when it needs to be done.

I suspect someone who understands the template instantiation engine better than I (like @zygoloid ) would have to either give some extremely good hints, or just do it themselves :(

[h-2]

Thanks for the update! It seems like this is really a showstopper for lots of C++20 goodness, but there is not much I can do to help. If you do get any experimental code, I would of course be willing to test it.

[erichkeane]

As an update... I'm still working on this as my non-emergency-top-priority type thing. I FEEL like I'm making progress, but I've still got 5 tests that crash.

[erichkeane]

I FINALLY got a version together that doesn't assert/crash on any of the lit tests, but I'm sure there are at least one issue left: https://reviews.llvm.org/D119544

I currently have 7 lit test failures, and some seem to be that some template arguments are reversed for some reason. I'll need to take a look at those, but this is at least something that could be 'played with'.

[erichkeane]

FWIW, I was able to get by all of the lit tests today on this patch and tested a handful of ranges examples (including the one from @llvmbot here) https://reviews.llvm.org/D119544.

[erichkeane]

Patch re-committed as yesterday, but I'm not going to be brave enough to mark this 'closed' for a few more days at minimum :)

[erichkeane]

Alright, we made it a week now and only have 2 different regressions reported. One was fixed immediately, the other (#57958) is under review. So I think I'm far enough along to have SOME faith this isn't going to be reverted.

Thanks all for the help with testing, and the reports in the first place!

[dimitry-ishenko]

@erichkeane any chance of this getting back-ported to LLVM/Clang 13?

[erichkeane]

@erichkeane any chance of this getting back-ported to LLVM/Clang 13?

None whatsoever unfortunately. There have been a number of regressions thanks to this (that I've been working through as they come up), and I'm sure there are likely more. This would be too high risk to backport to any of the 13-15 branches.

So if you want to use this, you'll have to update to Clang 16 when it comes out.

[dimitry-ishenko]

Thanks @erichkeane. I didn't think so, but I figured it won't hurt to ask. 🤷🏻‍♂️ 😃

[kelbon]

Alright, we made it a week now and only have 2 different regressions reported. One was fixed immediately, the other (#57958) is under review. So I think I'm far enough along to have SOME faith this isn't going to be reverted.

Thanks all for the help with testing, and the reports in the first place!

It still dont work for = default methods and destructors as i see

https://godbolt.org/z/r77oWoMTr

May be in can be easily fixed (based on previous fix)?

[erichkeane]

Alright, we made it a week now and only have 2 different regressions reported. One was fixed immediately, the other (#57958) is under review. So I think I'm far enough along to have SOME faith this isn't going to be reverted.
Thanks all for the help with testing, and the reports in the first place!

It still dont work for = default methods and destructors as i see

https://godbolt.org/z/r77oWoMTr

May be in can be easily fixed (based on previous fix)?

Interesting, thanks for pointing this out! @royjacobson was poking around default function constraints at one point as well, so he might have some idea.

[erichkeane]

I see that the code that calls this is part of SetEligibleMethods which @royjacobson wrote a while back, and is intending to implement class.mem.special p6. I don't have a good idea what all is going on there, nor if it was supposed to have been 'eliminated' by that in the first place. I'm hoping Roy can comment.

EDIT:
Looking a little closer, it looks like the calls to SetEligibleMethods is happening while dealing with the 'fields' of the base class, which seems too early? So we're failing to 'add' it to the my_iter type, since it isn't valid when instantiating the base. The only thing I can think of is that we should have done the special-member-func calculation 'as referred to' rather than immediately? Does this make sense Roy?

[royjacobson]

The standard is clear that we can't defer constraints checks for destructors, but P0848 is unclear about when, or if, to perform the constraints checks for the other special member functions.

When I implemented P0848 I decided to preemptively evaluating the constraints when we complete a class, and that it's probably OK. Not doing it would've added a non-trivial effort to support deferred computation of canPassInRegs and class [copy-]triviality, which we currently store as flags in RecordDeclBits. AFAIK pretty much everywhere assumes that those properties are fixed once a class is complete, and changing that seemed to me something worthy to avoid, as long as the standard doesn't explicitly prevents us from doing it.

But maybe this is non compliant and we really should defer the triviality calculations.

[erichkeane]

Hmm... that seems worthy of a CWG reflector discussion? Mind starting one?

[zygoloid]

I don't think we should, or reasonably can, defer these calculations in general. This seems to be the same kind of situation as in CWG1344, which described another way in which the set of special member functions could change after the class becomes complete.