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
Comments
From @zoffixznetThis code shows the bug: zoffix@leliana:~$ perl6 -e 'm: multi foo ($) {"right" }; multi And if we turn off the optimizer, we get the right candidate called zoffix@leliana:~$ perl6 --optimize=off -e 'm: multi foo ($) |
From @dogbert17On Wed, 05 Oct 2016 14:23:57 -0700, cpan@zoffix.com wrote:
It would seem that this bug was fixed with rakudo/rakudo@f8b3469 |
The RT System itself - Status changed from 'new' to 'open' |
From @skidsOn Sat, 05 Aug 2017 06:28:34 -0700, jan-olof.hendig@bredband.net wrote:
Tests added with roast commit 63181b3c9. So resolving. |
@skids - Status changed from 'open' to 'resolved' |
From @usev6On Thu, 21 Sep 2017 16:27:09 -0700, bri@abrij.org wrote:
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@db077336c5 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 |
@usev6 - Status changed from 'resolved' to 'open' |
From @usev6This 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 ( '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
From @usev6This 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 ( '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. |
From @usev6I 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. |
It looks like I actually committed the patch the fixes the remaining problem for the JVM backend, but didn't close this issue. Doing that now.
|
Migrated from rt.perl.org#129812 (status was 'open')
Searchable as RT129812$
The text was updated successfully, but these errors were encountered: