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

`but role { ... }` interacts strangely with outer lexicals, and doesn't warn about it #6139

Open
p6rt opened this issue Mar 9, 2017 · 9 comments
Labels
LTA

Comments

@p6rt
Copy link

@p6rt p6rt commented Mar 9, 2017

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

Searchable as RT130965$

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Mar 9, 2017

From @smls

  sub a ($a) {
  return True but role {
  method foo { $a }
  }
  }

  my $x = a 42;
  say $x.foo; # 42

  my $y = a "Hello";
  say $y.foo; # Hello
  say $x.foo; # Hello

I have a suspicion that this is not a bug but rather a consequence of
classes/roles not being proper closures.

But the current behavior is rather LTA in cases like this.
Could this possibly be made to emit a warning?

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Mar 9, 2017

From @lizmat

Yes, this is because classes / roles are not proper closures. And yes, I’ve been bitten by this as well.

To me, making them proper closures, feels like the correct solution to me. But I’ll settle for a warning / exceptione :-)

On 9 Mar 2017, at 16​:35, Sam S. (via RT) <perl6-bugs-followup@​perl.org> wrote​:

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

sub a ($a) {
return True but role {
method foo { $a }
}
}

my $x = a 42;
say $x.foo; # 42

my $y = a "Hello";
say $y.foo; # Hello
say $x.foo; # Hello

I have a suspicion that this is not a bug but rather a consequence of
classes/roles not being proper closures.

But the current behavior is rather LTA in cases like this.
Could this possibly be made to emit a warning?

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Mar 9, 2017

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

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Mar 9, 2017

From @jnthn

On Thu, 09 Mar 2017 07​:39​:39 -0800, elizabeth wrote​:

Yes, this is because classes / roles are not proper closures. And
yes, I’ve been bitten by this as well.

It boils down to roles and classes being compile-time constructs, while closures are decidedly runtime ones. You can force a role to be re-evaluated each time by making it parametric, and passing the state as a role parameter. Note, however, that roles are interned based on the object identity of their positional parameters. This means the pattern, used on values, has a high chance of causing a memory "leak".

Note that the `but RoleName($value)` syntax that mixes in the role and, if it has a single attribute, assigns $value to it, is there to make cases like this a bit more convenient.

To me, making them proper closures, feels like the correct solution to
me.
For this to happen we'd need​:

* To clone the methods, attribute initialization closures, etc.
* To therefore have a cloned method table, Attribute objects, etc. to point to these clones
* Which implies a cloned meta-object
* Which in turn forces a new type table (which raises a bunch of questions about what .WHAT evaluates to)
* And the new type, formed per closed over set of values, will then miss on every single bit of our dynamic optimization infrastructure that keys on type (method caches, multi caches, specializations, etc.)

Declaring lexical classes and roles isn't an entirely unusual practice; without some careful analysis of when we need to do the cloning, we'd end up giving programs using this pattern a huge performance regression. All to give other programs a feature that will be hard to optimize, and so will then get a (deserved) reputation for being slow and thus probably not used all that much anyway.

But I’ll settle for a warning / exceptione :-)

I think these options are worth consideration. I can't think of a false positive off hand.

/jnthn

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Mar 16, 2017

From @timo

On 09/03/17 20​:38, jnthn@​jnthn.net via RT wrote​:

On Thu, 09 Mar 2017 07​:39​:39 -0800, elizabeth wrote​:

But I’ll settle for a warning / exceptione :-)
I think these options are worth consideration. I can't think of a false positive off hand.

class Test { method tester { say "testest" } }

# warning​: you're not allowed to close over variable &say!

is there a sane heuristic for this? "does it have a & sigil" isn't sane,
IMO, and neither is "does it come from the core setting?".

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Aug 28, 2017

From j.david.lowe@apple.com

This short program behaves dies on my system, apparently because the mixed-in attribute is somehow "leaking" between the two threads (???)​:

```
#!/usr/bin/env perl6

await((^2).map​: {
  start {
  for (^10) -> $i {
  my $x = Str.new but role { has $.test = $i };
  die "$i != " ~ $x.test unless $x.test == $i;
  }
  }
});
```

produces output like​:

```
$ ./bug
Tried to get the result of a broken Promise
  in block <unit> at ./bug line 3

Original exception​:
  1 != 2
  in block at ./bug line 7

$ ./bug
Tried to get the result of a broken Promise
  in block <unit> at ./bug line 3

Original exception​:
  3 != 4
  in block at ./bug line 7
```

It's possible that what I'm doing in this code isn't *intended* to be thread-safe, but if that's the case, I don't understand why (e.g. looking at the code, the two threads appear to be completely isolated.)

More information​:

```
$ perl6 --version
This is Rakudo version 2017.07 built on MoarVM version 2017.07
implementing Perl 6.c.
```

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Aug 29, 2017

From @smls

@​David

I think this is not a thread safety issue, but rather a result of `role`s (intentionally) not being closures.

I.e. a duplicate of this RT​: This is a duplicate of https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130965

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Aug 29, 2017

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

@p6rt

This comment has been minimized.

Copy link
Author

@p6rt p6rt commented Aug 30, 2017

From j.david.lowe@apple.com

Agreed. In that case, the threads are just racing to observe a mutating value while it's in the "expected" state.

I will add my voice to the chorus calling for either making roles into closures, or producing a compile-time warning or error. It's tempting to use roles to "monkey patch", and would be good to be warned if it isn't a safe thing to do.

I'm fine with closing this as a duplicate.

On Aug 29, 2017, at 7​:32 AM, Sam S. via RT <perl6-bugs-followup@​perl.org> wrote​:

@​David

I think this is not a thread safety issue, but rather a result of `role`s (intentionally) not being closures.

I.e. a duplicate of this RT​: This is a duplicate of https://rt-archive.perl.org/perl6/Ticket/Display.html?id=130965

@p6rt p6rt added the LTA label Jan 5, 2020
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.