Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

multi built-ins are not assignable to Callable #1566

Closed
0racle opened this issue Feb 25, 2018 · 12 comments
Closed

multi built-ins are not assignable to Callable #1566

0racle opened this issue Feb 25, 2018 · 12 comments
Labels
dispatching Routine dispatching regression Issue did not exist previously

Comments

@0racle
Copy link
Contributor

0racle commented Feb 25, 2018

The Problem

When assigning a built-in function (eg. max) to a Callable, Rakudo complains that it "expected Callable but got Sub+{is-pure}"

Expected Behavior

my &func = &max;
say func([3, 5, 2]);    # OUTPUT: 5

# OR

sub foo(&func, |c) {
    return func(|c);
}

say foo(&max, [3, 5, 2]);    # OUTPUT: 5

Actual Behavior

Since max and other built-ins were turned into multi's (eg. 55bc053) trying to do either of the above yields the following runtime error

Type check failed in assignment to &func;
expected Callable but got Sub+{is-pure} (Sub+{is-pure}.new)
@0racle
Copy link
Contributor Author

0racle commented Feb 25, 2018

I created my own pure multi sub in a module and exported it and it did not suffer from the same issue. Despite identifying as a (Sub+{is-pure}) I could still assign to Callable vars just fine.

unit module Mux;

proto sub mux(|) is pure is export {*} 
multi sub mux(+args, :&by!) { args.max(&by) }
multi sub mux(+args)        { args.max      } 

... elsewhere

use lib 'lib';
use Mux;

say &mux.WHAT;    # OUTPUT: (Sub+{is-pure})
my &func = &mux;
say func([3, 5, 2]);    # OUTPUT: 5

@AlexDaniel AlexDaniel added the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Feb 25, 2018
@AlexDaniel
Copy link
Contributor

Blocker label added to make sure we do something about this before the release.

@zoffixznet
Copy link
Contributor

The title of the Issue is inaccurate, you can assign core multies:

<Zoffix___> m: my &foo = &say
<camelia> rakudo-moar 2a8b17ae9: ( no output )
<Zoffix___> m: my &foo = &say; say +&say.candidates
<camelia> rakudo-moar 2a8b17ae9: OUTPUT: «5␤»

The issue looks to be the same as RT#128905. Seems 55bc053 made &max/&min/&minmax join the ranks of affected routines.

@zoffixznet
Copy link
Contributor

zoffixznet commented Feb 26, 2018

Found that composing affected types fixes the issue:

<Zoffix_> m: &min.^compose; my &f = &min;
<camelia> rakudo-moar 838782b7d: ( no output )

and Perl6::Metamodel::Mixins.mixin (that traits, like is pure use) has this comment in it:

        # XXX Workaround for mixing in to non-composed types; when this takes
        # place (a bunch during CORE.setting) the mixin is missing bits. This
        # has long been a problem, and needs a real solution 

So I guess the work-around doesn't work-around perfectly.

@lizmat
Copy link
Contributor

lizmat commented Mar 2, 2018

FWIW, I believe the underlying issue (aka golf) is:

use nqp; say nqp::istype(&max,Callable)   # 0 instead of 1

@lizmat
Copy link
Contributor

lizmat commented Mar 2, 2018

I tried fixing this by moving Routine.pm to before traits.pm so that Routine would be fully composed. But this turned out to be a rabbit-hole that will take some real re-organizing the setting for that to work, as it would also need Attribute.pm before Routine.pm and that is really a can of worms.

@zoffixznet zoffixznet assigned zoffixznet and unassigned zoffixznet Mar 7, 2018
@zoffixznet
Copy link
Contributor

Started on a promising endeavor of moving all subs into a place past all Callable types in the setting: 3506395576

...but we have traits on a bunch of methods too and dealing with those is trickier. I started adding .^compose through methods we used traits on, BUT, the types that INHERITED those methods would need that done too. So now I can only think of going through ALL methods of all types declared by the src/core/traited-routines.pm6 point in the setting and .^composeing all the ones that !~~ Callable False.

I've no real idea of what .^compose does. Is doing that stuff for methods that fail Callable typecheck by that point too much?

@AlexDaniel AlexDaniel added the regression Issue did not exist previously label Mar 7, 2018
zoffixznet added a commit that referenced this issue Mar 16, 2018
Removes blocker status from R#1566
#1566

The actual bug is that Callable role gets mixed in into routines
before it's composed, and when it is composed, the routines end up
not "doing" `Callable` role, even though they do. There are many more
routines suffering this issue, but these three regressed since last
release and we don't have the time to fix the primary bug before the
release, so in this fudge goes.
zoffixznet added a commit to Raku/roast that referenced this issue Mar 16, 2018
@zoffixznet
Copy link
Contributor

zoffixznet commented Mar 16, 2018

We didn't have the time to fix this before the release. Since conditions that cause the bug are known (mixing in a role into Callables early enough in the setting before Callable is composed), what I done is looked through commits since last release and found all of the routines that acquired this bug since last release. These appear to be only &min, &max, and &minmax.

For those three, I added a workaround and tests covering them, so this ticket is no longer a release blocker, but the bug still needs to be fixed and more thoroughly tested.

@zoffixznet zoffixznet removed the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Mar 16, 2018
@AlexDaniel AlexDaniel added the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Apr 17, 2018
@zoffixznet
Copy link
Contributor

zoffixznet commented Apr 17, 2018

Heh. Well, time flew by, eh? Reading the logs, &lc and likely a bunch more are newly-affected this release.

@0racle
Copy link
Contributor Author

0racle commented Apr 17, 2018

Have found some more fallout from the multi-fication of subs.

> say dir.sort(&lc)
Cannot resolve caller lc(IO::Path, IO::Path); none of these signatures match:
    (Cool $s)

Additional context

bisectable • perlawhirl, bisect log: https://gist.github.com/f1dd78f91bf09798c9b9f5eb4cce8b3b
bisectable • perlawhirl, (2018-02-21) https://github.com/rakudo/rakudo/commit/cdb45fa5a65f06b543db7939fc51af7b598e0bfb

It seems the proto sig (|) causes 2 IO::Path values to be passed, through despite the fact that IO::Path ~~ Cool.

Current workaround is to force a single value, eg. sort(*.lc), sort({ lc($_ }).

A possible quick-fix might be to introduce a another multi, ie.

multi sub lc(Cool $a, Cool $b) {
    $a.lc cmp $b.lc
}

@zoffixznet
Copy link
Contributor

Ah, that's an entirely different issue from OP. I filed it here: #1739

@zoffixznet zoffixznet removed the BLOCKER Preventing the next release of rakudo, or just needing attention before the release label Apr 20, 2018
@lucasbuchala lucasbuchala added the dispatching Routine dispatching label Mar 15, 2019
@Kaiepi
Copy link
Contributor

Kaiepi commented Jul 28, 2020

Fixed with 7f2ae26.

@Kaiepi Kaiepi closed this as completed Jul 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dispatching Routine dispatching regression Issue did not exist previously
Projects
None yet
Development

No branches or pull requests

6 participants