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

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

Open
p6rt opened this issue Nov 4, 2016 · 12 comments
Open
Labels

Comments

@p6rt
Copy link

@p6rt p6rt commented Nov 4, 2016

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

Searchable as RT130020$

@p6rt
Copy link
Author

@p6rt p6rt commented Nov 4, 2016

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] <viki> s​: Hash, 'new', \()
[15​:11] <SourceBaby> viki, Sauce is at
https://github.com/rakudo/rakudo/blob/2ef2fdb/src/core/Map.pm#L12
[15​:15] <viki> 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] <viki> 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] <viki> 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] <viki> 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] <viki> We can't warn about every possible usage without slowing the
runtime down to a screetching halt
[15​:17] <viki> 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] <viki> m​: use nqp; class Foo { has $.blah; submethod BUILD (​:$!blah
= 42) {} }.new.blah.say
[15​:18] <+camelia> rakudo-moar 2ef2fd​: OUTPUT«42␤»
[15​:18] <viki> 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] <viki> lizmat​: what's the answer?
[15​:19] <viki> I mean, answer to what
[15​:20] <lizmat> to your cry for help​: "mother..." :-)
[15​:20] <viki> 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 <tmp> line 1␤ in block
<unit> at <tmp> line 1␤␤»
[15​:20] <viki> Ah :)
[15​:20] <harmil_wk> lizmat​: heh
[15​:21] <viki> 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] <viki> 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] <viki> 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
Copy link
Author

@p6rt p6rt commented Nov 7, 2016

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
Copy link
Author

@p6rt p6rt commented Nov 8, 2016

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.

  <ZoffixW> m​: class Foo does Setty { has %.elems; }
  <camelia> rakudo-moar a581bf​: OUTPUT«===SORRY!=== Error while compiling
  <tmp>␤Attribute '%!elems' already exists in the class 'Foo', but a role also
  wishes to compose it␤at <tmp>​: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
Copy link
Author

@p6rt p6rt commented Nov 8, 2016

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

@p6rt
Copy link
Author

@p6rt p6rt commented Nov 9, 2016

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
Copy link
Author

@p6rt p6rt commented Sep 17, 2017

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
Copy link
Author

@p6rt p6rt commented Sep 18, 2017

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
Copy link
Author

@p6rt p6rt commented Sep 18, 2017

From @lizmat

On 18 Sep 2017, at 04​:39, Brian S. Julin via RT <perl6-bugs-followup@​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
Copy link
Author

@p6rt p6rt commented Feb 16, 2018

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
Copy link
Author

@p6rt p6rt commented Feb 16, 2018

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@​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
Copy link
Author

@p6rt p6rt commented Feb 16, 2018

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
Copy link
Author

@p6rt p6rt commented Feb 16, 2018

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

@p6rt p6rt added at_larry RFC labels Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.