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

`is rw` candidates get called even if a non-rw argument is passed #5728

Open
p6rt opened this issue Oct 5, 2016 · 10 comments
Open

`is rw` candidates get called even if a non-rw argument is passed #5728

p6rt opened this issue Oct 5, 2016 · 10 comments

Comments

@p6rt
Copy link

@p6rt p6rt commented Oct 5, 2016

Migrated from rt.perl.org#129812 (status was 'open')

Searchable as RT129812$

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Oct 5, 2016

From @zoffixznet

This code shows the bug​:

  zoffix@​leliana​:~$ perl6 -e 'm​: multi foo ($) {"right" }; multi
foo ($ is rw) {"wrong"}; say foo "42"'
  wrong

And if we turn off the optimizer, we get the right candidate called
(same would happen if we add more complex sub bodies, so possibly the
sub gets inlined)​:

  zoffix@​leliana​:~$ perl6 --optimize=off -e 'm​: multi foo ($)
{"right" }; multi foo ($ is rw) {"wrong"}; say foo "42"'
  right

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Aug 5, 2017

From @dogbert17

On Wed, 05 Oct 2016 14​:23​:57 -0700, cpan@​zoffix.com wrote​:

This code shows the bug​:

 zoffix@​leliana​:\~$ perl6 \-e 'm​: multi foo \($\) \{"right" \}; multi  

foo ($ is rw) {"wrong"}; say foo "42"'
wrong

And if we turn off the optimizer, we get the right candidate called
(same would happen if we add more complex sub bodies, so possibly the
sub gets inlined)​:

 zoffix@​leliana​:\~$ perl6 \-\-optimize=off \-e 'm​: multi foo \($\)  

{"right" }; multi foo ($ is rw) {"wrong"}; say foo "42"'
right

It would seem that this bug was fixed with rakudo/rakudo@f8b3469
Test(s) needed

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Aug 5, 2017

The RT System itself - Status changed from 'new' to 'open'

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Sep 21, 2017

From @skids

On Sat, 05 Aug 2017 06​:28​:34 -0700, jan-olof.hendig@​bredband.net wrote​:

On Wed, 05 Oct 2016 14​:23​:57 -0700, cpan@​zoffix.com wrote​:

This code shows the bug​:

zoffix@​leliana​:~$ perl6 -e 'm​: multi foo ($) {"right" }; multi
foo ($ is rw) {"wrong"}; say foo "42"'
wrong

And if we turn off the optimizer, we get the right candidate called
(same would happen if we add more complex sub bodies, so possibly the
sub gets inlined)​:

zoffix@​leliana​:~$ perl6 --optimize=off -e 'm​: multi foo ($)
{"right" }; multi foo ($ is rw) {"wrong"}; say foo "42"'
right

It would seem that this bug was fixed with
rakudo/rakudo@f8b3469
Test(s) needed

Tests added with roast commit 63181b3c9. So resolving.

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Sep 21, 2017

@skids - Status changed from 'open' to 'resolved'

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Nov 10, 2017

From @usev6

On Thu, 21 Sep 2017 16​:27​:09 -0700, bri@​abrij.org wrote​:

On Sat, 05 Aug 2017 06​:28​:34 -0700, jan-olof.hendig@​bredband.net
wrote​:

On Wed, 05 Oct 2016 14​:23​:57 -0700, cpan@​zoffix.com wrote​:

This code shows the bug​:

zoffix@​leliana​:~$ perl6 -e 'm​: multi foo ($) {"right" }; multi
foo ($ is rw) {"wrong"}; say foo "42"'
wrong

And if we turn off the optimizer, we get the right candidate called
(same would happen if we add more complex sub bodies, so possibly
the
sub gets inlined)​:

zoffix@​leliana​:~$ perl6 --optimize=off -e 'm​: multi foo ($)
{"right" }; multi foo ($ is rw) {"wrong"}; say foo "42"'
right

It would seem that this bug was fixed with
rakudo/rakudo@f8b3469
Test(s) needed

Tests added with roast commit 63181b3c9. So resolving.

This bug is still present on the JVM backend (and the test code is passing with optimize=1). I'm going to reopen this ticket and tag it with [JVM].

I tried to find the source of the bug and I think it is in 'analyze_dispatch' in src/Perl6/Metamodel/BOOTSTRAP.nqp. We only check for literals passing to rw parameters if the argument passed is native. I added a similar check for the case with a non-native argument and the problem disappeared​: usev6/rakudo@db07733

I'm not sure if that's the right way to fix the problem -- especially given that some tests that expect code like '1++' to fail with X​::Multi​::NoMatch are failing now. The new exception is X​::TypeCheck​::Argument+{X​::Comp} -- which makes some sense as well.

I'd appreciate some feedback on this topic.

Thanks

Christian

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Nov 10, 2017

@usev6 - Status changed from 'resolved' to 'open'

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Oct 13, 2019

From @usev6

This is still a problem on the JVM backend. I tried a second time to find the underlying problem and arrived at the same conclusion​: There seems to be something wrong in 'analyze_dispatch'. When running the given code

  multi foo ($) {"right" }; multi foo ($ is rw) {"wrong"}; say foo "42"

'analyze_dispatch' returns the second sub with $MD_CT_DECIDED. This (wrong) result is taken for real on the JVM backend, whereas on MoarVM it isn't really used in this case -- cmp. https://github.com/rakudo/rakudo/blob/4df02facd0/src/Perl6/Optimizer.nqp#L3109-L3113

I didn't grasp all the details, but the problem might be related to the fact that 'sort_dispatchees_internal' returns an array with five results​: [2nd_sub, Mu, 1st_sub, Mu, Mu]. The second sub comes first, because it is narrower than the first sub. The Mu at index 1 seems to indicate the end of a tied group. This leads to 'analyze_dispatch' looking at the second sub first, not detecting a problem there (due to the missing check for a literal being passed in) and returning this sub after seeing the Mu.

I still think my patch from 2017 makes sense. With this patch, 'analyze_dispatch' rejects the second sub, notices that it didn't analyze all candidates and returns $MD_CT_NOT_SURE. But it would be even better if 'analyze_dispatch' dispatches to the first sub with $MD_CT_DECIDED.

1 similar comment
@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Oct 13, 2019

From @usev6

This is still a problem on the JVM backend. I tried a second time to find the underlying problem and arrived at the same conclusion​: There seems to be something wrong in 'analyze_dispatch'. When running the given code

  multi foo ($) {"right" }; multi foo ($ is rw) {"wrong"}; say foo "42"

'analyze_dispatch' returns the second sub with $MD_CT_DECIDED. This (wrong) result is taken for real on the JVM backend, whereas on MoarVM it isn't really used in this case -- cmp. https://github.com/rakudo/rakudo/blob/4df02facd0/src/Perl6/Optimizer.nqp#L3109-L3113

I didn't grasp all the details, but the problem might be related to the fact that 'sort_dispatchees_internal' returns an array with five results​: [2nd_sub, Mu, 1st_sub, Mu, Mu]. The second sub comes first, because it is narrower than the first sub. The Mu at index 1 seems to indicate the end of a tied group. This leads to 'analyze_dispatch' looking at the second sub first, not detecting a problem there (due to the missing check for a literal being passed in) and returning this sub after seeing the Mu.

I still think my patch from 2017 makes sense. With this patch, 'analyze_dispatch' rejects the second sub, notices that it didn't analyze all candidates and returns $MD_CT_NOT_SURE. But it would be even better if 'analyze_dispatch' dispatches to the first sub with $MD_CT_DECIDED.

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Oct 13, 2019

From @usev6

I forgot to add​: 'find_best_dispatchee' does a better job and recognizes that the second sub won't work with a literal​: https://github.com/rakudo/rakudo/blob/4df02facd0/src/Perl6/bootstrap.c/BOOTSTRAP.nqp#L2665-L2668

This code is executed on MoarVM -- but on the JVM we depend on the compile time analysis being correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.