Wording for `execution::scheduler` and `execution::schedule` seems to be cyclic

[cjdb]

template<class S>
 concept scheduler =
   copy_constructible<remove_cvref_t<S>> &&
   equality_comparable<remove_cvref_t<S>> &&
   requires(S&& s) {
     execution::schedule((S&&)s);
   };

From execuition.schedulers.

If S does not satisfy execution::scheduler, execution::schedule is ill-formed

From execution.senders.schedule/p2. Since execution::schedule is a CPO, it's not possible for this requirement to be satisfied, since we can't forward declare concepts and we can't define an object ahead of the type's definition.

[villevoutilainen]

That's not necessarily cyclic. Consider https://wandbox.org/permlink/7FfVl7pabnb51L6w In that example, 'okay' satisfies scheduler, 'junk' does not. execution::schedule() is ill-formed for 'junk', and well-formed for 'okay'.

Whether that's a good way to non-constrain execution::schedule() is another question, but it isn't cyclic. :P

[cjdb]

Hmm... yes, I see what you mean (it leaves a sour taste in my mouth!).

Does this inhibit schedule from being constexpr though?

[villevoutilainen]

Does this inhibit schedule from being constexpr though?

No. I can easily modify that example to forward-declare schedule() as a constexpr function and define it as such, too.

I do think there's an actual issue here. The scheduler concept is vague, because by the liberal reading that led to the example I wrote, the requirement that invoking schedule() is a valid expression for a scheduler doesn't mean much at the scheduler concept level. The concept doesn't check the instantiation of a schedule() function template, so unless schedule() SFINAEs away for a non-scheduler, the concept doesn't really verify that your scheduler is in fact a scheduler.

We might need to actually lift the validity of the tag_invoke invocation into the constraints of schedule().

[miscco]

Another issue is that it does not verify that the returned result is actually a sender. The "working" example is actually not really useful

[ericniebler]

Yes and I would like a schedulers sender to be required to be a typed sender of void.

[miscco]

That's not necessarily cyclic. Consider https://wandbox.org/permlink/7FfVl7pabnb51L6w In that example, 'okay' satisfies scheduler, 'junk' does not. execution::schedule() is ill-formed for 'junk', and well-formed for 'okay'.

Unfortunately, that is not correct. The issue at hand is that schedule is not a template but a CPO. So you would have to do something like this:

    namespace _Schedule {
        struct _Cpo;
    }

    template <class _Sender>
    concept scheduler = copy_constructible<remove_cvref_t<_Sender>> //
                     && equality_comparable<remove_cvref_t<_Sender>> //
                     && is_nothrow_copy_constructible_v<remove_cvref_t<_Sender>> //
                     && is_nothrow_destructible_v<remove_cvref_t<_Sender>> //
                     && noexcept(_STD declval<_Sender>() == _STD declval<_Sender>()) //
                     && requires(_Sender&& __s) {
        {_Schedule::_Cpo{}(_STD forward<_Sender>(__s))};
    };

But that still does not compile as _Schedule::_Cpo is not complete yet.

I think the only possibility is to create a _Scheduler_impl sub concept which carries everything except the requires expression and constrain the CPO with _Scheduler_impl

    template <class _Sender>
    concept _Scheduler_impl= copy_constructible<remove_cvref_t<_Sender>> //
                     && equality_comparable<remove_cvref_t<_Sender>> //
                     && is_nothrow_copy_constructible_v<remove_cvref_t<_Sender>> //
                     && is_nothrow_destructible_v<remove_cvref_t<_Sender>> //
                     && noexcept(_STD declval<_Sender>() == _STD declval<_Sender>());

    namespace _Schedule {
        struct _Cpo {
            template <_Scheduler_impl _Scheduler>
                requires tag_invocable<_Cpo, _Scheduler>
            constexpr auto operator()(_Scheduler _Sched) const
                noexcept(noexcept(tag_invoke(*this, _STD declval<_Scheduler>())))
                    -> decltype(tag_invoke(*this, _STD declval<_Scheduler>())) {
                return tag_invoke(*this, _STD forward<_Scheduler>(_Sched));
            }
        };
    } // namespace _Schedule
    using schedule_t = _Schedule::_Cpo;

    inline namespace _Cpos {
        inline constexpr _Schedule::_Cpo schedule;
    } // namespace _Cpos

    template <class _Sender>
    concept scheduler = _Scheduler_impl<_Sender> && requires(_Sender&& __s) {
        {execution::schedule(_STD forward<_Sender>(__s))};
    };

Note that this is effectively equivalent because of the additional constraint on the CPO

[villevoutilainen]

That's not necessarily cyclic. Consider https://wandbox.org/permlink/7FfVl7pabnb51L6w In that example, 'okay' satisfies scheduler, 'junk' does not. execution::schedule() is ill-formed for 'junk', and well-formed for 'okay'.

Unfortunately, that is not correct. The issue at hand is that schedule is not a template but a CPO.

Sure, fine, https://wandbox.org/permlink/Gaio6txruXqhqzjl

[miscco]

I think that is still not correct, as the function call operator needs to be constrained with scheduler

[villevoutilainen]

I think that is still not correct, as the function call operator needs to be constrained with scheduler

Says what? That's what makes this non-cyclic, the proposal says that invoking schedule() is ill-formed for a non-scheduler, but it doesn't say that schedule(non_scheduler) fails to overload-resolve.

[miscco]

Yes, it is a solution, even if it is a terrible one.

My 2 cents are that even if it is cyclic, the best solution is to split the concept up as shown above. That way we keep everything properly constrainted, which is a much better QoI.

Note that both solutions require about the same lines of code and both are similarly complex. Given equal implementation complexity I would always prefer the better constrained version

[villevoutilainen]

Sure, fully agreed, as suggested in https://github.com/brycelelbach/wg21_p2300_std_execution/issues/227#issuecomment-954495697

..I mean, I don't know what it means to "split the concept". We can't do that, we need to give users a single concept to work on, not multiple fine-grained ones. In general, a scheduler is a scheduler if it has a tag_invoke for schedule(), and that spits out a sender, plus the comparable and copyable things, so maybe we should just have that as the constraints of the scheduler concept, and then it doesn't need to mention execution::schedule. That also "splits the concept".

[miscco]

I meant that snipped of code here

    template <class _Sender>
    concept _Scheduler_impl= copy_constructible<remove_cvref_t<_Sender>> //
                     && equality_comparable<remove_cvref_t<_Sender>> //
                     && is_nothrow_copy_constructible_v<remove_cvref_t<_Sender>> //
                     && is_nothrow_destructible_v<remove_cvref_t<_Sender>> //
                     && noexcept(_STD declval<_Sender>() == _STD declval<_Sender>());

    namespace _Schedule {
        struct _Cpo {
            template <_Scheduler_impl _Scheduler>
                requires tag_invocable<_Cpo, _Scheduler>
            constexpr auto operator()(_Scheduler _Sched) const
                noexcept(noexcept(tag_invoke(*this, _STD declval<_Scheduler>())))
                    -> decltype(tag_invoke(*this, _STD declval<_Scheduler>())) {
                return tag_invoke(*this, _STD forward<_Scheduler>(_Sched));
            }
        };
    } // namespace _Schedule
    using schedule_t = _Schedule::_Cpo;

    inline namespace _Cpos {
        inline constexpr _Schedule::_Cpo schedule;
    } // namespace _Cpos

    template <class _Sender>
    concept scheduler = _Scheduler_impl<_Sender> && requires(_Sender&& __s) {
        {execution::schedule(_STD forward<_Sender>(__s))};
    };

We can split the scheduler concept into two parts, one with the type constraints (_Scheduler_impl) and one that checks whether the CPO is valid.

In the CPO we only need to additionally check tag_invocable<_Cpo, _Scheduler>. With that additional constraint the CPO is fully constrained and we can then use it for a user accesible scheduler

And to be sure I meant. Leave the specification as is and only implement it like that

[villevoutilainen]

Sure, and I'm suggesting that we don't need all that dance. We can just make the scheduler concept state its requirements in terms of tag_invoke, and not in terms of execution::schedule(), and the cycle goes away.

[miscco]

Sure, and I'm suggesting that we don't need all that dance. We can just make the scheduler concept state its requirements in terms of tag_invoke, and not in terms of execution::schedule(), and the cycle goes away.

I was bitten by that in my recent attempts at implementing this. It is kind of fine for execution::schedule as that does not constraint tag_invoke any further, but it falls apart the moment there are further constraints on the call to tag_invoke

For example in execution::start we have the same cyclic dependency between operation_state and execution::start. Now the issue is that execution::start requires an lvalue reference, as for an rvalue the passed operation_state would go away and we would work with dangling references.

Now imagine a user wrote a tag_invoke that accepts rvalues. In that case tag_invocable<execution::start_t, Faulty> will be true but invocable<execution::start_t, Faulty> will be false.

My test case is the following:

namespace operation_state {
    enum class IsDestructible : bool { Yes, No };
    enum class HasStart { LValue, RValue, No };

    template <IsDestructible Destructible = IsDestructible::Yes, HasStart Start = HasStart::LValue>
    struct tester {
        tester() = default;
        ~tester() noexcept(Destructible == IsDestructible::Yes){};

        friend constexpr auto tag_invoke(execution::start_t, tester&) noexcept requires(Start == HasStart::LValue) {
            return true;
        }

        friend constexpr auto tag_invoke(execution::start_t, tester&&) noexcept requires(Start == HasStart::RValue) {
            return true;
        }
    };

    static_assert(execution::operation_state<tester<IsDestructible::Yes>>);
    static_assert(invocable<execution::start_t, tester<IsDestructible::Yes>&>);

    // Needs to be destructible
    static_assert(!execution::operation_state<tester<IsDestructible::No>>);
    static_assert(!invocable<execution::start_t, tester<IsDestructible::No>>);

    // Should be an object
    static_assert(!execution::operation_state<tester<IsDestructible::Yes>&>);

    // Must have `start` operation
    static_assert(!execution::operation_state<tester<IsDestructible::Yes, HasStart::No>>);
    static_assert(!invocable<execution::start_t, tester<IsDestructible::Yes, HasStart::No>>);

    // Must take lvalue argument for start even if tag_invocable for rvalue
    static_assert(!execution::operation_state<tester<IsDestructible::Yes, HasStart::RValue>>);
    static_assert(!invocable<execution::start_t, tester<IsDestructible::Yes, HasStart::RValue>>);
    static_assert(tag_invocable<execution::start_t, tester<IsDestructible::Yes, HasStart::RValue>>); // That would be a bug
} // namespace operation_state

[ericniebler]

I believe #234 fixed this issue. Closing.