Skip Menu |

This queue is for tickets about the Moose CPAN distribution.

Report information
The Basics
Id: 125416
Status: open
Priority: 0/
Queue: Moose

People
Owner: Nobody in particular
Requestors: olaf [...] wundersolutions.com
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: Moose does not initialize attributes passed to the constructor before others
This can lead to some confusing situations if an attribute which is not explicitly set to lazy has a default or builder which relies on an attribute which has been set via the constructor. For example: #!/usr/bin/perl package Foo; use Moose; has set_me => ( is => 'ro', isa => 'Str', required => 1, ); has read_me => ( is => 'ro', isa => 'Str', default => sub { my $self = shift; $self->was_set( $self->set_me ); return $self->set_me; }, #lazy => 1, ); has was_set => ( is => 'rw', isa => 'Str', ); 1; package main; use strict; use warnings; use feature qw( say ); my $foo = Foo->new( set_me => 'donuts' ); say $foo->set_me; say $foo->read_me; say $foo->was_set; For the above script, as long as the lazy is commented out, this script will randomly execute to completion or die, depending on the order of the attribute initialization, which is also random. If this is something which is fixable, I do have some $work time set aside to address it. I'd just need some pointers on how to go about it. Thanks, Olaf
On 2018-05-28 08:27:52, OALDERS wrote: Show quoted text
> This can lead to some confusing situations if an attribute which is > not explicitly set to lazy has a default or builder which relies on an > attribute which has been set via the constructor. For example: > > #!/usr/bin/perl > > package Foo; > > use Moose; > > has set_me => ( > is => 'ro', > isa => 'Str', > required => 1, > ); > > has read_me => ( > is => 'ro', > isa => 'Str', > default => sub { > my $self = shift; > $self->was_set( $self->set_me ); > return $self->set_me; > }, > > #lazy => 1, > ); > > has was_set => ( > is => 'rw', > isa => 'Str', > ); > > 1; > > package main; > > use strict; > use warnings; > use feature qw( say ); > > my $foo = Foo->new( set_me => 'donuts' ); > > say $foo->set_me; > say $foo->read_me; > say $foo->was_set; > > > For the above script, as long as the lazy is commented out, this > script will randomly execute to completion or die, depending on the > order of the attribute initialization, which is also random. > > If this is something which is fixable, I do have some $work time set > aside to address it. I'd just need some pointers on how to go about > it.
This is definitely fixable, but it might break some MX modules to fix it. Right now what we do is simply iterate over all the class's attributes in the constructor. If the init_arg for the attr is in the passed params, we initialize it from there, otherwise we use a default if it has one. The fix is to change the constructor iterate over the passed param's keys first, looking up attribute based on the key, _then_ iterating over the remaining attributes. This is a little more complicated than what we do now, but not too difficult. However, it requires changing code that MX modules might override. Specifically, you want to take a look at Class::MOP::Class->_inline_slot_initializers and some of the code around it. I think to do this properly you'd need to replace this method entirely. This also needs to be handled in the non-inlined case, which starts with Moose::Meta::Class->new_object. I think it'd be best to prototype this as an MX module. The easiest way to do this for inlining is to override _inline_new_object and simply replace the call to `$self->_inline_slot_initializers` with something new that does this the way I described above.
On Mon May 28 11:32:13 2018, DROLSKY wrote: Show quoted text
> > This is definitely fixable, but it might break some MX modules to fix > it. > > Right now what we do is simply iterate over all the class's attributes > in the constructor. If the init_arg for the attr is in the passed > params, we initialize it from there, otherwise we use a default if it > has one. > > The fix is to change the constructor iterate over the passed param's > keys first, looking up attribute based on the key, _then_ iterating > over the remaining attributes. > > This is a little more complicated than what we do now, but not too > difficult. However, it requires changing code that MX modules might > override. > > Specifically, you want to take a look at Class::MOP::Class-
> >_inline_slot_initializers and some of the code around it. I think to
> do this properly you'd need to replace this method entirely. > > This also needs to be handled in the non-inlined case, which starts > with Moose::Meta::Class->new_object. > > I think it'd be best to prototype this as an MX module. The easiest > way to do this for inlining is to override _inline_new_object and > simply replace the call to `$self->_inline_slot_initializers` with > something new that does this the way I described above.
Great. Thanks! I'll start off with an MX module and see how far I get.
On Mon May 28 11:51:37 2018, OALDERS wrote: Show quoted text
> > Great. Thanks! I'll start off with an MX module and see how far I get.
What do you think about this approach? https://github.com/oalders/moosex-orderedconstruction It's not what we had discussed in above, but it (mostly) works so far. Figured I'd check in before we spend a lot more time on it. Also, suggestions for a better name are quite welcome. https://github.com/oalders/moosex-orderedconstruction
On 2018-06-14 09:44:48, OALDERS wrote: Show quoted text
> On Mon May 28 11:51:37 2018, OALDERS wrote:
> > > > Great. Thanks! I'll start off with an MX module and see how far I > > get.
> > What do you think about this approach? > https://github.com/oalders/moosex-orderedconstruction It's not what > we had discussed in above, but it (mostly) works so far. Figured I'd > check in before we spend a lot more time on it. Also, suggestions for > a better name are quite welcome. > > https://github.com/oalders/moosex-orderedconstruction
It seems weird to me to change the value of lazy for an attribute like you're doing. It also feels weird that you need to apply this in a role just to make it work properly with a class that is using this. The latter is particularly problematic if you're consuming a role that you don't control, for example something from a CPAN distro. I think keeping all of the behavior changes confined to the metaclass would be better. The implementation is a little trickier, but the behavior would be simpler to describe and less error prone to use.