[inplace_function] Const-callability means thread-safety

[Quuxplusone]

Right now, inplace_function permits

auto lam = [i=0]() mutable { return ++i; };
const stdext::inplace_function<int()> f(lam);
f();

That is, if I have a reference to a const inplace_function, I can call its operator(), which can mutate its internal state, even though I've promised not to modify it!

I believe we ought to follow the lead of Folly::Function, fu2::function, etc., and split inplace_function into two kinds of specialization:

• inplace_function<R(As...)> is constructible from either const-callable or non-const-callable lambdas, and provides only operator() (non-const)

• inplace_function<R(As...) const> is constructible only from const-callable lambdas, and provides only operator() const

In this paradigm, you would not be permitted to write

int one(const inplace_function<int(int)>& f) {
    return f(1) + f(2);
}
int two() {
    auto mutlambda = [i = 0](int j) mutable { return i += j; };
    return one(mutlambda);
}

The call to f(1) would fail to compile because inplace_function<int(int)>::operator()(int) is not const-qualified. Instead, you would have to write either

int one(const inplace_function<int(int) const>& f) {
    return f(1) + f(2);
}
int two() {
    int i = 0;
    auto nonmutlambda = [&i](int j) { return i += j; };
    return one(nonmutlambda);
}

or

int one(const inplace_function<int(int) const>& f) {
    return f(1) + f(2);
}
int two() {
    auto mutlambda = [i = 0](int j) mutable { return i += j; };
    return one(std::ref(mutlambda));
}

or

int one(inplace_function<int(int)> f) {
    return f(1) + f(2);
}
int two() {
    auto mutlambda = [i = 0](int j) mutable { return i += j; };
    return one(mutlambda);
}

depending on the semantics you want.

[Voultapher]

Const-callability means thread-safety
This title seems misleading and dangerous, not following const correctness quickly means thread hostile behavior, the inverse is not automatically true.

For once we have the chance to choose the right default in const and make mutability opt in.
Having the double const in this might be surprising for many readers and not intuitive, as this 'abuses' the otherwise useless const marked return in free function signatures, int one(const inplace_function<int(int) const>& f) {. Also how does this work for actually const member functions signatures. This would be a setting a precedence in the standard library, this would add yet another point of teaching where we'll have to say usually this means X, but here it means Y. In this case, usually const return for free functions is discouraged and doesn't do much, but for the standard library type erased box it defines interior mutability.

I'd much rather go with mut_inplace_function that is not only much easier to spot in code review, imo it's also more intuitive, and most importantly it makes const the default like newer C++ features, eg lambdas do, and makes mutability an explicit opt in.

[Quuxplusone]

@Voultapher You make some good points. But I continue to think that the biggest advantage of inplace_function<int(int) const> is compatibility with other libraries' idioms — "I believe we ought to follow the lead of folly::Function, fu2::function, etc..." — to which I can now add @SuperV1234's std::function_ref proposal and @sempuki's std::unique_function proposal (but not llvm::function_ref or folly::FunctionRef or gdb::function_view).

Having the double const in this might be surprising for many readers and not intuitive, as this 'abuses' the otherwise useless const marked return in free function signatures, int one(const inplace_function<int(int) const>& f)...

True, if you want to pass an inplace_function by const reference you'd have to double the const. However, in my usage of similar function types, this has never jumped out at me before. I think it's because inplace_function is not usually a parameter type (as opposed to, say, function_ref). The typical usage of an inplace_function would be as a class member:

struct NaiveCallbackNode {
    char *name_;
    stdext::inplace_function<int(int)> cb_;
};

struct ConstSafeCallbackNode {
    const char *name_;  // I don't modify the chars pointed to by name_
    stdext::inplace_function<int(int) const> cb_;  // I don't modify the functor wrapped by cb_
};

struct CopyOnlyCallbackNode {
    const char *const name_;  // this is too much const
    const stdext::inplace_function<int(int) const> cb_;  // this is too much const
};

Also how does this work for actually const member functions signatures?

inplace_function etc. are never parameterized by member function signatures. inplace_function<int (T::*)() const> is not a thing. So we don't have to worry about that, or I'm misunderstanding your question.

In this case, usually const return for free functions is discouraged and doesn't do much, but for the standard library type erased box it defines interior mutability.

The discouraged "return by const value" looks like this: inplace_function<const int(int)>
The discouraged "take by const value" looks like this: inplace_function<int(const int)>
The encouraged "don't modify the object" const looks like this: inplace_function<int(int) const>; and that's the one we're abusing in this case.

[Voultapher]

But I continue to think that the biggest advantage of inplace_function<int(int) const> is compatibility with other libraries' idioms

While this is nice to have, there is neither a full convergance of interfaces nor should it be difficult to string replace existing signatures, so it shouldn't overrule all other arguments.

I think it's because inplace_function is not usually a parameter type (as opposed to, say, function_ref). The typical usage of an inplace_function would be as a class member:

Commonly owning a type erased closure is the main usage, then again, looking through github it seems the vast majority of users 'misuse' std::function as function parameter. Which shows there is a big education gap, unlikely to ever fully close and should motivate us to build abstractions that are not only 'easy to use correctly' but also 'hard to use incorrectly'.
Which ties into the last point, yes after looking again more carefully at the function signatures it's true, I miss qualified the const. Shouldn't that be a warning for us, we 'experts' with several years of C++ experience got this wrong at least once. While it's essentially impossible to create something useful that cannot be used incorrectly, we can definitely make it harder to use incorrectly. By making const the default and expressing interior mutability as a different name, we get a win win. Much easier to teach, no need to fully understand all positions const can be found it and it's implications. Also searching online should be easier with an explicit name eg 'mut_inplace_function' vs 'inplace_function not const', same goes for discoverability, looking through the header or module synopsis on cppreference for example, figuring out the existence of 2 distinct types, over reading the fine print of the handful inplace_function constructors.

In the past C++ has repeatedly made the mistake of making things unnecessarily expert friendly in an attempt at satisfying an incompatible unweighted soup of requirements. Let's please not repeat that.

[sempuki]

While I'm sympathetic to the fact

    const some_functional_wrapper<void(size_t*)> f(free_function);
    f(&called); // compile error

looks bad, this is the price you pay for type erasure (and simplicity -- we could arguably remember we swallowed a free function those don't have state to mutate).

The question really is: what is the use case for a const version of your wrapper?
a) you want thread safety
b) you want cheap view (ie. const some_functional_wrapper&)

But what does it mean to "view" a function -- this gets right back to whether calling a function should be able to mutate it's state. Which should take precedence if they conflict? At the end of the day

    const some_functional_wrapper<void(size_t*) const> f(free_function);

is not an "abuse" of the type system for free functions, because the template signature is a constraint on the wrapper class -- it's promising it won't call anything that would mutate its state, and free_function conforms to that contract.

If the worst thing about your API is F<void()> has a meaning, F<void()const> has a meaning, const F<void() const> has a meaning, but const F<void()> doesn't have a meaning -- then you're doing pretty well in life. Keep in mind LEWG has seen this design and approves.