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

Module Crane is failing tests on HEAD #2400

Closed
AlexDaniel opened this Issue Oct 19, 2018 · 11 comments

Comments

Projects
None yet
3 participants
@AlexDaniel
Copy link
Member

AlexDaniel commented Oct 19, 2018

Crane used to install fine on 2018.09 but on HEAD it is failing some tests. My tool bisected it to the js branch merge but that info is totally not helpful.

Crane – Fail, Bisected: a09536f, …

===> Testing: Crane:ver<0.1.0>:auth<atweiden>
Type check failed for return value; expected Hash[Any:D,List:D] but got $(my Any:D %{List:D} ...
  in method flatten at /home/alex/.zef/store/crane.git/77270457082ff654f91ab13ad52e359ea6c68752/lib/Crane.pm6 (Crane) line 189
  in block <unit> at t/methods/flatten.t line 16

===> Testing [FAIL]: Crane:ver<0.1.0>:auth<atweiden>
Aborting due to test failure: Crane:ver<0.1.0>:auth<atweiden> (use --force-test to override)
@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 19, 2018

Golfed:

$ cat Bar.pm6 
my $ = Hash[Any:D,List:D]

$ cat Foo.pm6 
use Bar;
our sub foo(--> Hash[Any:D,List:D]) { my Any:D %tree{List:D} }

$ perl6 -I. -MFoo -e 'dd foo'
Type check failed for return value; expected Hash[Any:D,List:D] but got $(my Any:D %{List:D})
  in sub foo at /tmp/tmp.GR21mJFoZH/Foo.pm6 (Foo) line 2
  in block <unit> at -e line 1

Which looks to be a brother of this bug: #2345 (comment)

zoffixznet added a commit to perl6/roast that referenced this issue Oct 19, 2018

zoffixznet added a commit to perl6/roast that referenced this issue Oct 19, 2018

@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 19, 2018

@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 22, 2018

Which looks to be a brother of this bug: #2345 (comment)

Although, reverting 4b60df8 helped #2345, but does not help this bug.

@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 22, 2018

Golfed:

And turns out the golfed version was broken since at least 2016.01.

@jnthn

This comment has been minimized.

Copy link
Member

jnthn commented Oct 23, 2018

Could well be related to parametric intern handling or some such. I'll not be able to look for a couple more days, but this file and this bit of the deserialization handling are probably reasonable places to poke in a bit of debug logging to see what's happening.

@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 23, 2018

Some more findings:

:D is not needed for the bug. Changing the type used to Hash[Mu] in all three places still has the bug. However, changing it to Iterable[Mu] avoids the bug. The same with Array[Mu] (has bug) vs. Positional[Mu] (no bug). So... something to do with using non-role things with parametarization?

@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 23, 2018

I give up. What I don't get is how this bug managed to just show up in Crane, when the golfed version existed since Christmas.

I tried to figure out why it likes Positional[Int] but not Array[Int]. The only thing I can think of is the Array version has Positional[TValue] in its .^roles. That Positional is parameterized with TValue type, so maybe that's what it doesn't like? This type capture becoming some sort of a weird type and not serializing right?

m: Array[Int].^roles.say
+camelia │ rakudo-moar 64b3dbd79: OUTPUT: «((Array::TypedArray[Int]) (Positional[TValue]) (Positional) (Iterable))␤»

m: dd Array[Int].^roles[1].of
+camelia │ rakudo-moar 64b3dbd79: OUTPUT: «(TValue without .perl method)␤»
@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 23, 2018

Also, noticed this change in how the module crashes, which was effected by fd5d5bd according to committable, yet reverting that commit doesn't revert back to old behaviour.

Also, I don't see Crane::Add used as a type constraint anywhere in the code, suggesting that was coming from something else.


¦«fd5d5bd»:
1..2
Type check failed for return value; expected Hash[Any:D,List:D] but got $(my Any:D %{List:D} ...
  in method flatten at /home/bisectable/git/whateverable/sandbox/crane/lib/Crane.pm6 (Crane) line 189
  in block <unit> at EVAL_0 line 16
  in block <unit> at /tmp/AjV9cHx5Rb line 1

 «exit code = 1»
¦«fd5d5bdfb97930cfae1f9b2572d2ab6eea7ca92f~1»:
1..2
Type check failed in assignment to %tree; expected Crane::Add but got Int (1)
  in method flatten at /home/bisectable/git/whateverable/sandbox/crane/lib/Crane.pm6 (Crane) line 189
  in block <unit> at EVAL_0 line 16
  in block <unit> at /tmp/AjV9cHx5Rb line 1

«exit code = 1»
@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 23, 2018

More info. This is comparing golfed code that uses Array[Mu] (broken) vs. Positional[Mu] (working) as types.

When resolve_param_interns is called, the reader->root.num_param_interns is 1 in working version, but in broken one it's 0, so that loop never runs and never finds anything.


Just to repeat the code I'm running:

$ cat Bar.pm6 
my $a = Array[Mu];

$ cat Foo.pm6 
use Bar;
our sub foo(--> Array[Mu]) { Array[Mu] }

$ ./perl6 -I. -e 'use Foo; foo'
Type check failed for return value; expected Array[Mu] but got Array[Mu]
...

Yet changing that to Positional[Mu] does not crash:

$ cat Bar.pm6 
my $a = Positional[Mu];

$ cat Foo.pm6 
use Bar;
our sub foo(--> Positional[Mu]) { Positional[Mu] }

$ ./perl6 -I. -e 'use Foo; foo'
$
@jnthn

This comment has been minimized.

Copy link
Member

jnthn commented Oct 26, 2018

Oddly enough, it seems it's not an interning bug, or at least not a very direct one. The two Array[Mu] come out as precisely the same object, and this patch:

diff --git a/src/vm/moar/spesh-plugins.nqp b/src/vm/moar/spesh-plugins.nqp
index 74ac15b..8d778bb 100644
--- a/src/vm/moar/spesh-plugins.nqp
+++ b/src/vm/moar/spesh-plugins.nqp
@@ -177,7 +177,7 @@ sub identity($obj) { $obj }
     }
     sub check_type($type, $orig_type) {
         -> $ret {
-            nqp::istype($ret, $type) || nqp::istype($ret, Nil)
+            $ret =:= $type || nqp::istype($ret, $type) || nqp::istype($ret, Nil)
                 ?? $ret
                 !! return_error($ret, $orig_type)
         }

Works around the issue. Next us is trying to work out why nqp::istype gives the wrong result here.

jnthn added a commit that referenced this issue Oct 26, 2018

Don't unconditionally add parameterizations to SC
A parameterization of a type may not create a new type, but instead
return an already produced type. Thus, it may already belong to the
SC of another compilation unit. Placing it into ours results in a
duplicate of the object - in this case a type object - existing after
deserialization.

This was the cause of issue #2400: the type check failed because the
duplicate of the type object was quite rightly not in the MRO of the
meta-object.

The fix is simply to only add it if it's not already in an SC. If it
is, we need do nothing more.
@zoffixznet

This comment has been minimized.

Copy link
Contributor

zoffixznet commented Oct 26, 2018

Test unfudged and I verified Crane's tests now pass.

@zoffixznet zoffixznet closed this Oct 26, 2018

ugexe added a commit to perl6/roast that referenced this issue Dec 15, 2018

ugexe added a commit to perl6/roast that referenced this issue Dec 15, 2018

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