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

Deep-cloned array loses its original content on .unshift in Rakudo #3084

Closed
p6rt opened this issue Mar 20, 2013 · 10 comments
Closed

Deep-cloned array loses its original content on .unshift in Rakudo #3084

p6rt opened this issue Mar 20, 2013 · 10 comments

Comments

@p6rt
Copy link

p6rt commented Mar 20, 2013

Migrated from rt.perl.org#117235 (status was 'resolved')

Searchable as RT117235$

@p6rt
Copy link
Author

p6rt commented Mar 20, 2013

From @masak

<masak> just found my heisenbug :)
<masak> yeah, this is a weird one.
<diakopter> run it?
<masak> rn​: my $c = [[1]].map({ [ @​$_ ] }); $c.push( 42 ); say $c.perl
<p6eval> rakudo 9be4cc, niecza v24-35-g5c06e28​: OUTPUT«([1], 42).list.item␤»
<masak> rn​: my $c = [[1]].map({ [ @​$_ ] }); $c.unshift( 42 ); say $c.perl
<p6eval> rakudo 9be4cc​: OUTPUT«(42,).list.item␤»
<p6eval> ..niecza v24-35-g5c06e28​: OUTPUT«(42, [1]).list.item␤»
* masak submits rakudobug
<masak> rn​: my $c = [[1], [2], [3]].map({ [ @​$_ ] }); $c.unshift( 42
); say $c.perl
<p6eval> niecza v24-35-g5c06e28​: OUTPUT«(42, [1], [2], [3]).list.item␤»
<p6eval> ..rakudo 9be4cc​: OUTPUT«(42,).list.item␤»

Niecza is right throughout, of course. The .unshift method adds
elements; it doesn't remove them.

@p6rt
Copy link
Author

p6rt commented Mar 20, 2013

From @pmichaud

On Wed, Mar 20, 2013 at 12​:41​:41PM -0700, Carl Mäsak wrote​:

# New Ticket Created by "Carl Mäsak"
# Please include the string​: [perl #​117235]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=117235 >

<masak> just found my heisenbug :)
<masak> yeah, this is a weird one.
<diakopter> run it?
<masak> rn​: my $c = [[1]].map({ [ @​$_ ] }); $c.push( 42 ); say $c.perl
<p6eval> rakudo 9be4cc, niecza v24-35-g5c06e28​: OUTPUT«([1], 42).list.item␤»
<masak> rn​: my $c = [[1]].map({ [ @​$_ ] }); $c.unshift( 42 ); say $c.perl
<p6eval> rakudo 9be4cc​: OUTPUT«(42,).list.item␤»
<p6eval> ..niecza v24-35-g5c06e28​: OUTPUT«(42, [1]).list.item␤»
* masak submits rakudobug
<masak> rn​: my $c = [[1], [2], [3]].map({ [ @​$_ ] }); $c.unshift( 42
); say $c.perl
<p6eval> niecza v24-35-g5c06e28​: OUTPUT«(42, [1], [2], [3]).list.item␤»
<p6eval> ..rakudo 9be4cc​: OUTPUT«(42,).list.item␤»

Niecza is right throughout, of course. The .unshift method adds
elements; it doesn't remove them.

The problem is apparently with sink context, not .unshift. I have a temporary
fix in mind, but can't get to it until Wednesday night or Thursday night.

20​:26 <pmichaud> I wonder if the problem is related to sink context.
20​:26 <pmichaud> r​: my $c = [[1],[2],[3]].map( { $_ } ); $c.unshift(42).say
20​:26 <p6eval> rakudo 9be4cc​: OUTPUT«42 1 2 3␤»
20​:26 <pmichaud> $c.unshift(42); is being evaluated in sink context.
20​:26 <moritz> that might be it
20​:27 <pmichaud> r​: my $c = [[1],[2],[3]].map( { $_ } ); $c.unshift(42).sink; say $c.perl;
20​:27 <p6eval> rakudo 9be4cc​: OUTPUT«(42,).list.item␤»
20​:27 <pmichaud> there ya go.
20​:27 <moritz> niecza doesn't implement sink context
20​:27 <masak> huh.
20​:28 <pmichaud> r​: my $c = [[1],[2],[3]].map( { $_ } ); $c.sink; say $c.perl;
20​:28 <p6eval> rakudo 9be4cc​: OUTPUT«().list.item␤»
20​:29 <moritz> pmichaud​: c4083c42f2e63c97254b7a215a858913c4ff4a44 is the relevant merge commit
20​:29 <pmichaud> golfed.

Pm

@p6rt
Copy link
Author

p6rt commented Mar 20, 2013

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

@p6rt
Copy link
Author

p6rt commented Mar 21, 2013

From @moritz

On 03/20/2013 10​:37 PM, Patrick R. Michaud wrote​:

On Wed, Mar 20, 2013 at 12​:41​:41PM -0700, Carl Mäsak wrote​:

# New Ticket Created by "Carl Mäsak"
# Please include the string​: [perl #​117235]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl6/Ticket/Display.html?id=117235 >

<masak> just found my heisenbug :)
<masak> yeah, this is a weird one.
<diakopter> run it?
<masak> rn​: my $c = [[1]].map({ [ @​$_ ] }); $c.push( 42 ); say $c.perl
<p6eval> rakudo 9be4cc, niecza v24-35-g5c06e28​: OUTPUT«([1], 42).list.item␤»
<masak> rn​: my $c = [[1]].map({ [ @​$_ ] }); $c.unshift( 42 ); say $c.perl
<p6eval> rakudo 9be4cc​: OUTPUT«(42,).list.item␤»
<p6eval> ..niecza v24-35-g5c06e28​: OUTPUT«(42, [1]).list.item␤»
* masak submits rakudobug
<masak> rn​: my $c = [[1], [2], [3]].map({ [ @​$_ ] }); $c.unshift( 42
); say $c.perl
<p6eval> niecza v24-35-g5c06e28​: OUTPUT«(42, [1], [2], [3]).list.item␤»
<p6eval> ..rakudo 9be4cc​: OUTPUT«(42,).list.item␤»

Niecza is right throughout, of course. The .unshift method adds
elements; it doesn't remove them.

The problem is apparently with sink context, not .unshift. I have a temporary
fix in mind, but can't get to it until Wednesday night or Thursday night.

20​:26 <pmichaud> I wonder if the problem is related to sink context.
20​:26 <pmichaud> r​: my $c = [[1],[2],[3]].map( { $_ } ); $c.unshift(42).say
20​:26 <p6eval> rakudo 9be4cc​: OUTPUT«42 1 2 3␤»
20​:26 <pmichaud> $c.unshift(42); is being evaluated in sink context.
20​:26 <moritz> that might be it
20​:27 <pmichaud> r​: my $c = [[1],[2],[3]].map( { $_ } ); $c.unshift(42).sink; say $c.perl;
20​:27 <p6eval> rakudo 9be4cc​: OUTPUT«(42,).list.item␤»
20​:27 <pmichaud> there ya go.
20​:27 <moritz> niecza doesn't implement sink context
20​:27 <masak> huh.
20​:28 <pmichaud> r​: my $c = [[1],[2],[3]].map( { $_ } ); $c.sink; say $c.perl;
20​:28 <p6eval> rakudo 9be4cc​: OUTPUT«().list.item␤»
20​:29 <moritz> pmichaud​: c4083c42f2e63c97254b7a215a858913c4ff4a44 is the relevant merge commit
20​:29 <pmichaud> golfed.

This patch works in this particular case​:

Inline Patch
diff --git a/src/core/List.pm b/src/core/List.pm
index 84f7eff..964d6f4 100644
--- a/src/core/List.pm
+++ b/src/core/List.pm
@@ -379,8 +379,8 @@ my class List does Positional {
          $val;
      }

-    method sink() {
-        self.gimme(*, :sink);
+    method sink(\SELF:) {
+        SELF.gimme(*, :sink) unless nqp::iscont(SELF);
          Nil;
      }
  }


But of course it's not a real fix. Should I apply it?

Cheers,
Moritz

@p6rt
Copy link
Author

p6rt commented Mar 21, 2013

From @pmichaud

On Thu, Mar 21, 2013 at 08​:49​:42AM +0100, Moritz Lenz wrote​:

- method sink() {
- self.gimme(*, :sink);
+ method sink(\SELF​:) {
+ SELF.gimme(*, :sink) unless nqp​::iscont(SELF);
Nil;
}
}

But of course it's not a real fix. Should I apply it?

I think I prefer

  method sink(\SELF​:) {
  SELF.gimme(*) unless nqp​::iscont(SELF);
  Nil;
  }

because I'm not entirely certain that :sink is safe in
all cases (and it may go away anyway).

Pm

@p6rt
Copy link
Author

p6rt commented Mar 21, 2013

From @moritz

On 03/21/2013 01​:40 PM, Patrick R. Michaud wrote​:

On Thu, Mar 21, 2013 at 08​:49​:42AM +0100, Moritz Lenz wrote​:

- method sink() {
- self.gimme(*, :sink);
+ method sink(\SELF​:) {
+ SELF.gimme(*, :sink) unless nqp​::iscont(SELF);
Nil;
}
}

But of course it's not a real fix. Should I apply it?

I think I prefer

method sink\(\\SELF&#8203;:\) \{
    SELF\.gimme\(\*\) unless nqp&#8203;::iscont\(SELF\);
    Nil;
\}

because I'm not entirely certain that :sink is safe in
all cases (and it may go away anyway).

Not using :sink isn't safe either​:

Test Summary Report


t/spec/S04-statements/next.rakudo (Wstat​: 0 Tests​:
12 Failed​: 2)
  Failed tests​: 8-9
t/spec/S12-introspection/walk.t (Wstat​: 0 Tests​:
11 Failed​: 2)
  Failed tests​: 6-7

They go away when keeping the :sink.

Cheers,
Moritz

@p6rt
Copy link
Author

p6rt commented Mar 21, 2013

From @pmichaud

On Thu, Mar 21, 2013 at 08​:14​:08PM +0100, Moritz Lenz wrote​:

On 03/21/2013 01​:40 PM, Patrick R. Michaud wrote​:

On Thu, Mar 21, 2013 at 08​:49​:42AM +0100, Moritz Lenz wrote​:

- method sink() {
- self.gimme(*, :sink);
+ method sink(\SELF​:) {
+ SELF.gimme(*, :sink) unless nqp​::iscont(SELF);
Nil;
}
}

This doesn't seem to be sufficient; as $c.unshift(47) still
fails on my system. The List coming back from .unshift is
currently decontainerized.

In the meantime, I've added a test for this ticket as
S04-statements/sink.t . It's not included in Rakudo's spectests
yet.

Pm

@p6rt
Copy link
Author

p6rt commented Aug 30, 2015

From @usev6

On Thu Mar 21 15​:43​:19 2013, pmichaud wrote​:

[...]
In the meantime, I've added a test for this ticket as
S04-statements/sink.t . It's not included in Rakudo's spectests
yet.

This test passes now on the 'glr' branch (and was unfudged with commit Raku/roast@38af820522).

I'm closing this ticket as 'resolved'.

1 similar comment
@p6rt
Copy link
Author

p6rt commented Aug 30, 2015

From @usev6

On Thu Mar 21 15​:43​:19 2013, pmichaud wrote​:

[...]
In the meantime, I've added a test for this ticket as
S04-statements/sink.t . It's not included in Rakudo's spectests
yet.

This test passes now on the 'glr' branch (and was unfudged with commit Raku/roast@38af820522).

I'm closing this ticket as 'resolved'.

@p6rt
Copy link
Author

p6rt commented Aug 30, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant