Fix issue 22701: Remove is(typeof()) callable check in std.typecons.apply.

[FeepingCreature]

To quote my bug report:

As I said in my DConf talk, __traits(compiles) is Satan, the Great Deceiver. The presence of is(typeof()) (__traits(compiles by any other name) in the definition of apply needlessly turns a clear, readable error in the passed lambda into an incomprehensible template error that requires advanced language knowledge or blind guessing to untangle. And there's no point, because there's only one apply function! It should just be removed.

(Let's see if this passes the testsuite!)

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	22701	enhancement	std.typecons.apply needlessly checks if the predicate is callable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#8364"

[FeepingCreature]

Errors seem spurious.

[RazvanN7]

@FeepingCreature please rebase. That will fix the testing pipeline.

[RazvanN7]

Could you provide an example where the error message is improved with this patch? I tested this (adapted from the public unittest):

import std.typecons;

void main()
{
    Nullable!int sample;   
    Nullable!float f;
    sample = 3;
    
    f = sample.apply!sample;  
}

And I get the following results:

1. Before patch:

onlineapp.d(9): Error: template `onlineapp.main.apply!(sample).apply` cannot deduce function from argument types `!()(Nullable!int)`
/dlang/dmd/linux/bin64/../../src/phobos/std/typecons.d(4038):        Candidate is: `apply(T)(auto ref T t)`
  with `T = Nullable!int`
  must satisfy the following constraint:
`       is(typeof(unaryFun!fun(T.init.get)))`

2. After patch:

phobos/std/typecons.d(4047): Error: struct `Nullable` does not overload ()
phobos/std/typecons.d(4068): Error: struct `Nullable` does not overload ()
test.d(9): Error: template instance `test.main.apply!(Nullable!int)` error instantiating

Note how this patch moves the failure in the innards of the apply function.

[FeepingCreature]

Sure, for example:

import std.typecons;
void main() {
    Nullable!int foo;
    auto twice = foo.apply!(a => a ~ a);
}

Without patch:

test.d(4): Error: none of the overloads of template `test.main.apply!((a) => a ~ a).apply` are callable using argument types `!()(Nullable!int)`
dmd/generated/linux/release/64/../../../../../phobos/std/typecons.d(4044):        Candidate is: `apply(T)(auto ref T t)`
  with `T = Nullable!int`
  must satisfy the following constraint:
`       is(typeof(unaryFun!fun(T.init.get)))`

With patch:

test.d(4): Error: incompatible types for `(a) ~ (a)`: both operands are of type `int`
test.d(4):        instantiated from here: `apply!(Nullable!int)`

That's where you need the magic knowledge to replace the apply with apply!((const int a) => a ~ a), or to use some magic DMD switch that spams the console with thousands of errors, of which hopefully the last one is the actually relevant one. IMO this is hostile to any newcomer. You shouldn't need special incantations to get good errors. You should just get good errors.

edit: Rebased.

edit: Really, the template condition should just check "you are passing an in principle callable thing." It shouldn't check if it can actually be called; more urgently, if it does check that, it shouldn't then silently eat the reason why it couldn't be called. Etc __traits(compiles) delendam est.

[RazvanN7]

I understand that the current error message is not ideal, however:

1. It does specify which constraint you are not respecting, therefore giving a hint on what is wrong.
2. For whatever combination of wrong arguments you will not be presented with failing code in the innards of apply.

This patch, however, opens the door to issuing errors from inside the implementation of apply and I personally
don't see how the new error message that you pasted is better. It just says that int is incompatible for ~ however
it does not give you any indication on how to fix this because it is entirely omitting the fact that you are working with a Nullable.
Imagine that you are working with voldemort types that you are passing to apply, something along the lines of:

auto v = someFunctionReturningNullableOfInt();
v.apply!(a => a ~ a);    // error: incompatible type int for ~; ok what do I do now? 

This, in my opinion, is more confusing then presenting a condition which has failed. With the current status you can simply copy that condition and run it in a separate file and see why it failed, plus all the actual types are included in the error message.

[FeepingCreature]

it is entirely omitting the fact that you are working with a Nullable.

test.d(4): instantiated from here: apply!(Nullable!int)

In my experience with Nullable, and Phobos' predicate functions in general, more than 90% of the time the issue is a semantic error inside the lambda. The issue is practically never an error in apply itself. And if it was, that would be the sort of issue that you can apply inside knowledge to solving. Semantic errors in the lambda are the thing that's most plausible to turn up in user code, which is exactly the target group where vague, content-free hints like is(typeof(unaryFun!fun(T.init.get))) (which relies on two expert-level D interna, is(typeof()) and unaryFun, to understand - and requires that the user manually compile the lambda in their head to figure out what's wrong, as it gives no information other than "it's wrong") are at their most harmful.

tl;dr: might as well

test.d(4): (a) => a ~ a: error 

(And yes, that's obvious, but now assume it's a much bigger range expression.)

[RazvanN7]

I am willing concede in front of real life experience with Nullable since I don't use it in my day to day programming routine. However, I would like another heavy user of Nullable to weight in on this. @burner do you have any opinions on this?

[FeepingCreature]

May be a good thing to take to the forum too? Since it's really a problem with all of std.algorithm. (Ie. if I can find the time and people agree, I'd love to remove the is(typeof())s from everything that takes a lambda. But that's the sort of problem I wouldn't want to embark on unless I had some confidence the reviewers agreed that this was worthwhile.)

[burner]

Only, that I have not used apply that much. Like once or twice. But in general I'm not a fan of template constraint in general.
As there is no overloading for apply pulling the constraint into a static assert inside of the function and then generating a nice(r) error would be "better".

[moon-chilled]

I will say: I recently made a very similar change at fork for the exact same reason: error messages were incomprehensible without, useful with. (That said I have not used apply. But seeing as feep did, ran into the same issue, this seems quite reasonable.)

[RazvanN7]

@FeepingCreature this

Only, that I have not used apply that much. Like once or twice. But in general I'm not a fan of template constraint in general.
As there is no overloading for apply pulling the constraint into a static assert inside of the function and then generating a nice(r) error would be "better".

Would be ideal.

[FeepingCreature]

Not sure how to do that? The error that I want is inside the lambda. If I static assert something in apply, wouldn't that error suppress the error in the lambda?

In serialized I just used pragma(msg) to add context to an error, but since there's no way to capture error messages from __traits(compiles), the message still needs to come from a regular compile attempt at the lambda.

[burner]

I would compare type of the parameter of the passed function with the T of the Nullable.

When you get a void != int you likely know where to look.

[FeepingCreature]

Well, with a lambda you don't have a passed type. I guess I should add a std.functional template that checks everything that can be statically checked, without trying to instantiate the argument if it's a template, as a companion to unaryFun? Then that can replace is(typeof()) in the incondition?

edit: I've been playing around with this for the past half hour. I don't see any way to do it right. DMD just does not give enough control over the actual error message that will be generated.

Closest I have so far is

    auto apply(T)(auto ref T t)
    if (isInstanceOf!(Nullable, T) && (
        is(typeof(unaryFun!fun(T.init.get)))
        || __traits(isTemplate, unaryFun!fun)))

But that prints multiple versions of the in-lambda error, and I don't see any way to avoid that without way more static if than I'm comfortable with.

edit: It seems to work if I add some more nesting:

    auto apply(T)(auto ref T t)
    if (isInstanceOf!(Nullable, T) && (
        is(typeof(unaryFun!fun(T.init.get)))
        || __traits(isTemplate, unaryFun!fun)))
    {
        auto call() { return unaryFun!fun(t.get); }
        alias FunType = typeof(call);

As far as I can tell this does the right thing ~ everywhere, but it's very hacky and I hate it.

[RazvanN7]

Ok, let's just merge as is and we can improve on it in the future if needed.