Fix 9906: extend unaryFun and binaryFun to recognize function objects

[quickfur]

Structs or classes with static opCall should be recognized as valid
function objects.

Fixes: https://issues.dlang.org/show_bug.cgi?id=9906

[quickfur]

N.B. please don't merge yet, as a bug/quirk with is() causes non-static opCalls to be recognized as well, which may be problematic in some cases. Need help to consider the possible pitfalls of this before merging.

[DmitryOlshansky]

what's the problem with non-static opCall? It just means that fun is a function-like object, and should be called directly.

[bearophile]

I'd like map/filter to accept arrays and associative arrays, and not just functions:

void main() {
    import std.algorithm: map;
    auto keys = [1, 2, 1, 1, 1];
    auto a = [0, 10, 20];
    auto r1 = map!(k => a[k])(keys);
    auto r2 = map!a(keys);
    auto aa = [1: 10, 2: 20];
    auto r3 = map!(k => aa[k])(keys);
    auto r4 = map!aa(keys);
}

(This is a common idiom in Clojure. Arrays and associative arrays can be seen as functions defined by an enumeration of input-outputs.)

[quickfur]

@DmitryOlshansky The problem is that if you have, say, a class C with (non-static) method opCall, then unaryFun!C will be a valid type. Which isn't a real problem, except a current compiler quirk where is(unaryFun!C(0)) (assuming opCall takes an int) will return true, but if you actually try to call unaryFun!C(0) from code, you'll get a compile error saying that you need a base object to call C.opCall. So current Phobos functions that use unaryFun in signature contraints may get instantiated where they shouldn't be because the user wrongly passed the class/struct type instead of the required class/struct instance. What will likely happen in this case is that you'll end up with an inscrutable compile error from deep inside Phobos.

[quickfur]

@bearophile That looks like another enhancement request.

[quickfur]

Hmm, actually, it may not be that hard to implement after all:

...
else static if (is(typeof(fun) : V[K], V, K))
{
    V unaryFUn(K key) { return fun[key]; }
}
else static if (is(typeof(fun) : T[], T))
{
    T unaryFun(size_t i) { return fun[i]; }
}
...

[bearophile]

@quickfur: >That looks like another enhancement request.

Yes, it's part of Issue 8715, but when the amount of added code is small enough I sometimes suggest to bundle enhancements.

[quickfur]

Added unaryFun!AA as function mapping keys to values. Gotta run now, will add array support shortly.

[quickfur]

Added unaryFun!array as function mapping size_t to array elements.

[bearophile]

@quickfur: good, with this change I'll shorten/simplify several spots in my codebase.

Perhaps unaryFun/binaryFun ddocs or some std.algorithm documentation examples should show the new possibilities.

[quickfur]

Updated ddocs. Migrated to ddoc'd unittests.

[bearophile]

Looks nice. In the meantime I have opened another small ER: https://issues.dlang.org/show_bug.cgi?id=13546 It's related but it's sufficiently different that I think it's better to not bundle it with this patch.

[monarchdodra]

The "goal" of unaryfun was originally only to allow string lambdas, and to forward everything else. When the passed "fun" was a not a string, the unaryFun is designed to be a no-op. The idea is that if you aren't using string lambdas, then you shouldn't need to use unaryFun.

In fact, I think I can think of at least 1 "new" function in std.algorithm that does not provide support for string lambdas, and as such, does not use unaryFun

It seems like this change is trying to bake new functionality into unaryFun.

What I mean is that if this passes:

void foo(alias fun, T)(T t)
{
    static assert(!is(typeof(fun) : string));
    unaryFun!fun(t)
}

Then this better damn well pass too:

void foo(alias fun, T)(T t)
{
    static assert(!is(typeof(fun) : string));
    fun(t)
}

So I'm OK for the static opCall thing, but against the opIndex support.

Also, I'm not actually convinced yet that array=>function is a good idea in and out of itself...

[DmitryOlshansky]

Also, I'm not actually convinced yet that array=>function is a good idea in and out of itself...

Absolutely awesome thing for lookup tables, but I agreed it better be a separate entity. Agreed on other points.

[bearophile]

@monarchdodra:

It seems like this change is trying to bake new functionality into unaryFun

Yes, because it's a very compact and general way to introduce new capabilities into all algorithms. Doing the same changes to every algorithm is a lot more work.

Also, I'm not actually convinced yet that array=>function is a good idea in and out of itself...

Take a look at Clojure code. We should make D more flexible reducing the unnecessary friction between parts. Arrays are a mapping, so this allows you to abstract away the irrelevant detail of the difference between a function and an array.

[bearophile]

@DmitryOlshansky: >but I agreed it better be a separate entity

What do you mean? We're already able to write a lambda like "k => aa[k]", the point of this change is to shorten the code and make it more clean, hiding an unnecessary difference between arrays and functions.

[monarchdodra]

Arrays are a mapping

Arrays are objects you can get the length of, slice, append to, pop etc. I don't really agree that it's unaryFun's job to, by default, decide that () => []. In particular, it makes arrays (and their associative brother) 1st class in the sense that RA ranges (or hash tables) will not benefit from this, even though they are just "mappings".

So even though you'd have map!myArray(someRange), you'd still have to do map!(x=>myRange[x])(someRange).

And I definitively disagree that unary fun should be implicitly indexing RA ranges... especially since those could have (static) opIndex to boot.

However, I think providing an functor-like adaptor is a good compromise.

[JakobOvrum]

I'm against adding support for arrays at this level of abstraction, we should stick to the principle of least surprise. I'd much rather see indices.map!(i => range[i]) than indices.map!range in user code - it's not that much longer to type but is much more readable and requires no prior knowledge of special handling hidden away in std.functional.

The situation is bad enough as it is with string lambdas, let's not make algorithm predicates even more "special".

[bearophile]

@JakobOvrum: >we should stick to the principle of least surprise. I'd much rather see indices.map!(i => range[i]) than indices.map!range in user code - it's not that much longer to type but is much more readable and requires no prior knowledge of special handling hidden away in std.functional.

I think the use of arrays as callables is sufficiently natural and avoids some noise:

uint[] sieve(in uint limit) nothrow @safe {
    if (limit < 2)
        return [];
    auto composite = new bool[limit];

    foreach (immutable uint n; 2 .. cast(uint)(limit ^^ 0.5) + 1)
        if (!composite[n])
            for (uint k = n * n; k < limit; k += n)
                composite[k] = true;

    return iota(2, limit).filter!(not!composite).array;
}

I'm against adding support for arrays at this level of abstraction,

I agree that in Clojure (and Scala too?) the situation is a little different, because arrays are seen as callables at all abstraction levels.

[mihails-strasuns]

I am with @JakobOvrum here. Such special case for arrays is very surprising unless you have aready encountered it before.

[quickfur]

Alright, let's leave the AA/array extensions out of this PR for now. Let's just fix the opCall issue first, then we can discuss the merits of adding the AA/array extensions (or not).

[quickfur]

ping @Dicebot

[mihails-strasuns]

Are objects with no-static opCall supposed to be supported too? If yes, need to add relevant test case.
Other than that LGTM

[quickfur]

Hmm. Apparently the current code doesn't work with non-static opCall. :-( I'm not sure I understand why... the compiler complains Error: need 'this' for 'opCall' of type 'bool(int n)' but I'm passing the function object (not just its type) to unaryFun. Could this be the alias issue that needs the template hack to work around it?

[quickfur]

Heh, actually, this problem is what I originally predicted in my second comment from the top. :) Function objects used to work fine, but this fix broke them because the new static if is too broad; it should only pick up static opCalls but instead also picks up non-static ones. I'll have to add another clause to the static if to weed out non-static opCalls. Nice catch!

[quickfur]

Alright, should be fixed now.

[mihails-strasuns]

Thanks, LGTM

[mihails-strasuns]

Auto-merge toggled on

[quickfur]

Thanks!