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

Custom .new method is not reflected in standard .perl #2448

Open
lizmat opened this issue Oct 31, 2018 · 13 comments

Comments

Projects
None yet
4 participants
@lizmat
Copy link
Contributor

commented Oct 31, 2018

class Point {
    has $.x;
    has $.y;
    method new($x,$y) {
        self.bless(:$x,:$y)
    }
}
say Point.new(42,66).perl  # Point.new(x => 42, y => 66)

The standard .perl should probably look at the signature of the .new method, then look for positional parameters and see if the names match with any attribute names. So that:

say Point.new(42,66).perl  # Point.new(42,66)
@zoffixznet

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

signature of the .new method ... and see if the names match with any attribute names.

That sounds extremely fragile. Even with a basic signature, consider:

    method new($x,$y) {
        self.bless: :x($x+42),:y($x*100-$y)
    }

I think adc9683 that makes default .perl create a stub is a great solution to the OP problem, unfortunately, it creates at least two more:

  1. &infix:<eqv> uses .perl and now it gives True for any two instances of a class that has custom .new. Filed as #2449
  2. Mu.gist uses .perl and now it gives fairly useless information, while in the past it gave you a snapshot of attribute values, even if the .new call the information was presented in was bogus. Personally, I use dd instead of say for debugging, so it's mildly annoying to have that debugging aid now effectively broken, but if .gist shows the same information too, that means to dump the guts of a custom class you now have to implement your own .perl and presumably list all the used attributes in that implementation, even though you don't care about serializing that object or it could even be impossible.
@zoffixznet

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

Some more discussion in https://colabti.org/irclogger/irclogger_log/perl6-dev?date=2018-10-31#l107


Zoffix | Maybe dump all attributes like before, but stick a stub
                    before them if there is a custom .new
Zoffix | So it'd be Foo.new(…, a => 1, b => 2, c => 3, d => 4)

+timotimo │ can we somehow get a better error message than just "stub code executed"?
        … would take an argument, so a word or two there could be nice



jnthn | I  think the default `.perl` for objects should assume the default  construction
           interface, and if you make a custom `new` and you care  about round-tripping
           then it's on you to write a custom `method perl` to  go with it
jnthn | Alternatively, dump with `.bless` instead of `.new` :)
@lizmat

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Part 2 of this should fix this. Working on that now

@jnthn

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

As a follow-up to my earlier suggestion about bless, I think maybe best is:

  • If there's no custom new then do what we already were doing
  • If not, then make it use bless instead of new

That still stands a decent chance of round-tripping when the constructor was simply used to make positionals into nameds.

@zoffixznet

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

That still stands a decent chance of round-tripping when the constructor was simply used to make positionals into nameds.

(as discussed in https://colabti.org/irclogger/irclogger_log/perl6-dev?date=2018-10-31#l276 )

That doesn't cover all the cases though, and in the remaining cases you get bizarre errors and only-partially initialized objects, causing potentially bug-at-a-distance problems in user's code.

IMO the stub saying custom new is in use + params dump (Point.new((... "custom .new but no custom .perl"), x => 42, y => 66)) is the best option. I rather the code dies right away and tells me to implement a .perl instead of it having it maybe do the right thing and a chance to fail in unpredictable ways.

Even looking at core code, Failure.new(...).perl.EVAL already crashes with a strange error and we have a gazillion iterators that do !SET-SELF(...) on private attributes (which won't show up in .blessed .perl) inside .new.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I will look at making it work right for custom new if the number of parameters in .new matches the public attributes. Any diversion from that will cause an execution error telling to create your own .perl method. Is that a plan?

@lizmat lizmat closed this Oct 31, 2018

@lizmat lizmat reopened this Oct 31, 2018

@zoffixznet

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

I will look at making it work right for custom new if the number of parameters in .new matches the public attributes.

🤷 I don't get the point of trying to wildly guess at what .new does and hope for the best that your guess works right and doesn't cause a crucial setup step to be skipped, causing someone to loose all their data.

The relationship between number of parameters of .new and number of public attributes is tenuous at best. There will be a ticket about it in the future. May as well avoid it now.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

Now I'm confused. So what is the problem with using "bless" then if there is a custom "new" ?

@jnthn

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Any diversion from that will cause an execution error

Note that a large number of uses of .perl - probably the majority - are for debug output, not for data structure serialization. An error is too harsh for that case.

@lizmat

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

So just doing the best we can, and then a ... so it will bomb when actually EVALled ?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

BTW, even if there is no custom "new" method, then we cannot be sure it will roundtrip. For instance, if we have a method TWEAK:

$ 6 'class A { has $.a; method TWEAK() { $!a = !$!a } }; say A.new(:a).perl'
A.new(a => Bool::False)
$ 6 'class A { has $.a; method TWEAK() { $!a = !$!a } }; say A.new(:a).perl.EVAL.perl'
A.new(a => Bool::True)
@jnthn

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

So just doing the best we can, and then a ... so it will bomb when actually EVALled ?

If we must, but even if there is a default new, there's still no promise that feeding the current attribute values back in will succeed in constructing a new object.

Consider an object where you're allowed to pass in starting values for an attribute that fall within a certain range, but then over the object's lifetime they might go outside of that range. The .perl would produce something that could never be constructed. Or a BUILD that takes something of one name and splurges it over many attributes of different names. In fact, I'd argue that this problem is more common than the custom new one. I struggle to think of a module I wrote with a custom new, but I can think of code I worked on today with more interesting BUILD/TWEAK stuff going on.

So where do we draw the line?

@lizmat

This comment has been minimized.

Copy link
Contributor Author

commented Oct 31, 2018

I've reverted what changes I made with c04d5b7 . This ticket should probably stay open.

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.