Raku / old-issue-tracker

Tickets from RT
https://github.com/Raku/old-issue-tracker/issues
2 stars 1 forks source link

Create a set of conventions to minimize impact internal changes to user's code #5780

Open p6rt opened 8 years ago

p6rt commented 8 years ago

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

Searchable as RT130020$

p6rt commented 8 years ago

From @ajs

The example​: class Foo is Hash { has $.blah = 0 }; say Foo.new.blah.perl

The output​: rakudo-moar 2ef2fd​: OUTPUT«Any␤»

Expected output​: 0

Reason​: Hash defines its own new which bypasses the default handling in bootstrap

Concerns​: The code above is not obviously wrong to the user, but is guaranteed not to work, and worse, if the base class is unknown (either parameterized or via an interstitial derived class) then this code can't even be determined valid or invalid without following the context.

From IRC​:

[15​:09] \<harmil_wk> m​: class Foo is Hash { has $.blah = 0 }; say Foo.new.blah.perl [15​:09] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«Any␤» [15​:09] \<harmil_wk> I have a feeling I'm doing something I should know better than to do... [15​:11] \ s​: Hash, 'new', \() [15​:11] \ viki, Sauce is at https://github.com/rakudo/rakudo/blob/2ef2fdb/src/core/Map.pm#L12 [15​:15] \ m​: use nqp; class Foo { has $.blah = 0; method new { self.bless​: :42blaz } }.new.blah.say [15​:15] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«0␤» [15​:15] \ m​: use nqp; class Foo { has $.blah = 0; method new { self.bless​: :42blah } }.new.blah.say [15​:15] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«42␤» [15​:15] \ m​: use nqp; class Foo { has $.blah = 0; method new { nqp​::create(self) } }.new.blah.say [15​:15] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«(Any)␤» [15​:15] \<harmil_wk> oh really... [15​:16] \ harmil_wk​: what I'm seeing is nqp​::create(self) doesn't set the defaults and that's what the Map's .new method is doin' [15​:16] \<[Coke]> all the default logic is in the bootstrap file in rakudo, so that makes sense. [15​:16] \<harmil_wk> Yeah, that seems highly counterintuitive. At the very least, a warning (probably error) should result if your defaults are going to be ignored. [15​:17] \ We can't warn about every possible usage without slowing the runtime down to a screetching halt [15​:17] \ m​: use nqp; class Foo { has $.blah; submethod BUILD (​:$!blah = 42) {} }.new(​:72blah).blah.say [15​:17] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«72␤» [15​:18] \ m​: use nqp; class Foo { has $.blah; submethod BUILD (​:$!blah = 42) {} }.new.blah.say [15​:18] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«42␤» [15​:18] \ m​: class Foo is Hash { has $.blah; submethod BUILD (​:$!blah = 42) {} }.new.blah.perl.say [15​:18] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«Any␤» [15​:19] \<[Coke]> is there rakudo only code that demonstrates the issue? or do you have to (as viki did above), "use nqp" and then an nqp opcode? [15​:19] \<harmil_wk> My original code that derived from Hash [15​:19] \<harmil_wk> m​: class Foo is Hash { has $.blah = 0 }; say Foo.new.blah.perl [15​:19] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«Any␤» [15​:19] == wisti [~brianwist@​c-73-109-31-174.hsd1.wa.comcast.net] has quit [Ping timeout​: 265 seconds] [15​:19] \ lizmat​: what's the answer? [15​:19] \ I mean, answer to what [15​:20] \ to your cry for help​: "mother..." :-) [15​:20] \ m​: class Foo is Hash { has $.blah; method new { self.bless​: %_ } }.new.blah.perl.say [15​:20] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«Too many positionals passed; expected 1 argument but got 2␤ in method new at \ line 1␤ in block \ at \ line 1␤␤» [15​:20] \ Ah :) [15​:20] \<harmil_wk> lizmat​: heh [15​:21] \ m​: use nqp; class Foo { has $.blah; method new { self.bless​: |​:42blah, |%_} }.new(​:72blah).blah.say [15​:21] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«72␤» [15​:21] \ m​: use nqp; class Foo { has $.blah; method new { self.bless​: |​:42blah, |%_} }.new.blah.say [15​:21] \<+camelia> rakudo-moar 2ef2fd​: OUTPUT«42␤» [15​:22] \ There. I win. [15​:22] \<harmil_wk> My point is that "class Foo is :​:($x) { has $.blah = 0 }" will only work if $x is a class that doesn't do what Hash does... that feels like a very difficult-to-detect bug from the user's side. [15​:22] \<[Coke]> Yup. I would say that should either work or, if it can't, throw an error asap that it can't work. [15​:23] \<harmil_wk> I'll rakudobug it for the record.

-- Aaron Sherman, M.​: P​: 617-440-4332 Google Talk, Email and Google Plus​: ajs@​ajs.com Toolsmith, developer, gamer and life-long student.

p6rt commented 8 years ago

From @ajs

Here is a more complete and rigorous example of how this creates substantial problems in library code​:

https://gist.github.com/ajs/e3a9fb6caf76f23de0940a676ef7dd2b

p6rt commented 8 years ago

From @zoffixznet

Here's a link to the above IRC conversation, which continued after the quoted part​: https://irclog.perlgeek.de/perl6/2016-11-04#i_13517260

To expand, it's not just Hashes that have this issue; Arrays, SetHash, and Complex also do, and probably many others. And the issue is not just about not setting default values for attributes when instantiating the object, but use of attributes as well.

  \ m​: class Foo does Setty { has %.elems; }   \ rakudo-moar a581bf​: OUTPUT«===SORRY!=== Error while compiling   \␤Attribute '%!elems' already exists in the class 'Foo', but a role also   wishes to compose it␤at \​:1␤»

Above, if we imagine %.elems as some new internal attribute we added to Setty, you can see it now breaks some user's code.

So I think it would be useful to create some sort of convention where we're less likely to impact changes to users' code by making some internal change. For attributes, we can prefix the names with 'rak-' or something short and consistent to mark them as internal. For the .new() issue with Hashes and others, I think lizmat proposed doing something similar to what we do in Date/DateTime classes.

p6rt commented 8 years ago

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

p6rt commented 8 years ago

From @ajs

An update​:

This code shows what low-level classes do not invoke their BUILD submethod when subclassed​:

https://gist.github.com/ajs/c11a00290b52957a5f686c8ad3c60885

p6rt commented 7 years ago

From @zoffixznet

More comments on similar issue​: https://irclog.perlgeek.de/perl6/2017-09-17#i_15176823

Basically, a lot of core constructs aren't workable with user-land subclasses.

p6rt commented 7 years ago

From @skids

On Sun, 17 Sep 2017 08​:44​:47 -0700, cpan@​zoffix.com wrote​:

More comments on similar issue​: https://irclog.perlgeek.de/perl6/2017- 09-17#i_15176823

Basically, a lot of core constructs aren't workable with user-land subclasses.

Note on the performance concerns... if we had a faster-than-a-where-clause type smiley (or other mechanism) that only accepted the exact type, not subclasses, then the core could provide very optimized implementations for core types as separate candidates, and a more generic implementation for subclasses, which would be slower but less likely to be affected by internals churn.

p6rt commented 7 years ago

From @lizmat

On 18 Sep 2017, at 04​:39, Brian S. Julin via RT \perl6\-bugs\-followup@&#8203;perl\.org wrote​:

On Sun, 17 Sep 2017 08​:44​:47 -0700, cpan@​zoffix.com wrote​:

More comments on similar issue​: https://irclog.perlgeek.de/perl6/2017- 09-17#i_15176823

Basically, a lot of core constructs aren't workable with user-land subclasses.

Note on the performance concerns... if we had a faster-than-a-where-clause type smiley (or other mechanism) that only accepted the exact type, not subclasses, then the core could provide very optimized implementations for core types as separate candidates, and a more generic implementation for subclasses, which would be slower but less likely to be affected by internals churn.

Technical note​: that would mean internally using nqp​::eqaddr vs nqp​::istype.

p6rt commented 6 years ago

From @zoffixznet

Spotted another case where there's impact​: whether or not a routine is a multi can have large impact on user's code. For example here​: https://stackoverflow.com/questions/48819031/where-did-my-perl-6-operator-go-after-i-defined-a-more-specific-multi/48827522

As part of conventions, we'd need to also figure out what we'll declare as multies and what as onlies.

p6rt commented 6 years ago

From @lizmat

I propose we change all onlies in the core to multis after the release and see how this breaks things / makes things slower.

On 16 Feb 2018, at 14​:17, Zoffix Znet via RT \perl6\-bugs\-followup@&#8203;perl\.org wrote​:

Spotted another case where there's impact​: whether or not a routine is a multi can have large impact on user's code. For example here​: https://stackoverflow.com/questions/48819031/where-did-my-perl-6-operator-go-after-i-defined-a-more-specific-multi/48827522

As part of conventions, we'd need to also figure out what we'll declare as multies and what as onlies.

p6rt commented 6 years ago

From @zoffixznet

On Fri, 16 Feb 2018 05​:52​:09 -0800, elizabeth wrote​:

I propose we change all onlies in the core to multis after the release and see how this breaks things / makes things slower.

+1. Let's try.

p6rt commented 6 years ago

From @AlexDaniel

On 2018-02-16 09​:01​:57, cpan@​zoffix.com wrote​:

On Fri, 16 Feb 2018 05​:52​:09 -0800, elizabeth wrote​:

I propose we change all onlies in the core to multis after the release and see how this breaks things / makes things slower.

+1. Let's try.

Yeah, sounds good. +1