Bug #89239 for Moo: extends()ing a class with() a role that uses 'around new' is broken under some common circumstances

Thu Oct 03 15:57:12 2013 dmuey [...] cpan.org - Ticket created

Subject:

extends()ing a class with() a role that uses 'around new' is broken under some common circumstances

Howdy Matt, Hope you are doing well. The title is a bit hard to parse but its a weird problem :) The attached modules are as simple a demonstration as I could get, each pair is named after the combination of extends/has/with they contain. each pair of commands shows how each behaves: 1) this works as expected (i.e. around 'new' happens) [dmuey@multivac lib]$ perl -MMainNoHas -e 'MainNoHas->new' Debug: I am in the around->new [dmuey@multivac lib]$ perl -MExtendsMainNoHas -e 'ExtendsMainNoHas->new' Debug: I am in the around->new [dmuey@multivac lib]$ 2) This works when using the class directly, but is fatal (i.e. deep recursion) when it is extended: [dmuey@multivac lib]$ perl -MMainHasBeforeWith -e 'MainHasBeforeWith->new' Debug: I am in the around->new [dmuey@multivac lib]$ perl -MExtendsMainHasBeforeWith -e 'ExtendsMainHasBeforeWith->new' Debug: I am in the around->new … Debug: I am in the around->new Deep recursion on subroutine "ExtendsMainHasBeforeWith::new" at (eval 16) line 9. [dmuey@multivac lib]$ 3) this fails in that the around new is never even called: [dmuey@multivac lib]$ perl -MMainHasAfterWith -e 'MainHasAfterWith->new' [dmuey@multivac lib]$ perl -MExtendsMainHasAfterWith -e 'ExtendsMainHasAfterWith->new' [dmuey@multivac lib]$ 4) If you add a role to #1's with() that has a has() then it behaves like #3. This hold true on: perl 5.16.0 Moo: 1.000007 Role::Tiny 1.002004 perl 5.18.1 Moo: 1.003001 Role::Tiny: 1.003002 perl 5.8.8 Moo: 1.003001 Role::Tiny: 1.003002 Let me know if you need anything else, thanks!

Subject:

extends-with-around-new-bug.tar.gz

Download extends-with-around-new-bug.tar.gz
application/x-gzip 772b

Message body not shown because it is not plain text.

Thu Oct 03 16:12:55 2013 haarg [...] haarg.org - Correspondence added

You shouldn't be using around new. If you need to adjust the way parameters are accepted in new, use BUILDARGS. It may be good to provide some protection against someone doing this. I have a branch in progress (non-moo-jenga) that tests a number of related cases.

Thu Oct 03 16:12:55 2013 The RT System itself - Status changed from 'new' to 'open'

Thu Oct 03 16:33:57 2013 dmuey [...] cpan.org - Correspondence added

On Thu Oct 03 16:12:55 2013, haarg wrote: Show quoted text

> You shouldn't be using around new. If you need to adjust the way > parameters are accepted in new, use BUILDARGS. > > It may be good to provide some protection against someone doing this. > I have a branch in progress (non-moo-jenga) that tests a number of > related cases.

Thank, makes sense. I don't see a way to get BUILDARGS (or BUILD or FOREIGNBUILDARGS FWIW) to behave like around though which is really what I am going for. For example, Role::Singleton::New wants to make new() return the cached object if we have one and call the original new (and cache that for next time) when it does not. (Role::Singleton adds a separate constructor, …::New means to add the behavior to new() directly). Maybe I need to defined new() via sub and call SUPER->new, hmmmm

Thu Oct 03 16:41:46 2013 haarg [...] haarg.org - Correspondence added

On Thu Oct 03 16:33:57 2013, DMUEY wrote: Show quoted text

> On Thu Oct 03 16:12:55 2013, haarg wrote:

> > You shouldn't be using around new. If you need to adjust the way > > parameters are accepted in new, use BUILDARGS. > > > > It may be good to provide some protection against someone doing this. > > I have a branch in progress (non-moo-jenga) that tests a number of > > related cases.

> > Thank, makes sense. I don't see a way to get BUILDARGS (or BUILD or > FOREIGNBUILDARGS FWIW) to behave like around though which is really > what I am going for. > > For example, Role::Singleton::New wants to make new() return the > cached object if we have one and call the original new (and cache that > for next time) when it does not. (Role::Singleton adds a separate > constructor, …::New means to add the behavior to new() directly). > > Maybe I need to defined new() via sub and call SUPER->new, hmmmm

I'd strongly recommend just using a different method name for that, and letting new behave as normal.

Thu Oct 03 16:54:24 2013 dmuey [...] cpan.org - Correspondence added

Show quoted text

> I'd strongly recommend just using a different method name for that, > and letting new behave as normal.

Yeah, I get that, which is what that is what Role::Multiton and Role::Singleton do. Still, it's been handy to also be able to get new() to act how you want also.

Fri Oct 04 13:14:58 2013 dmuey [...] cpan.org - Correspondence added

The more I think about it, philosophical ideas about 'around new' aside, it does highlight a deficiency between around() and Moo objects. If it was at least consistent (e.g. didn't change behavior with different order/existence of has() calls-action at a distance is bad) it'd be better but the way it is one can't rely on around() for any Moo objects. For example, say we use around for a sort of hook system; who knows if it will work or not with any given class. While tests would help detect regressions (on classes we control but not necessarily on classes that extend it that we don't control) it'd just mean we'd have to redo how hooks worked when they got tripped). With all of that in mind what is your take on using 'around new' as a test bed to ensure that around() works w/ Moo under all circumstances (i.e. has before/has after/extends/with/etc)?

Fri Oct 04 14:17:48 2013 haarg [...] haarg.org - Correspondence added

This isn't really a deficiency in around, or at least not something that wouldn't impact any other alternative. new is a special case, and it gets generated on the fly by Moo when it is called. There are some edge cases in this behavior, and I haven't found solutions for all of them yet. around will be reliable on any other method you use it on.

Mon Oct 07 15:31:01 2013 dmuey [...] cpan.org - Correspondence added

Show quoted text

> new is a special case, and it gets generated on the fly by Moo when it is called.

thank you for the info I appreciate it. Would you all be interested in Moo having a mechanism that'd let us emulate around() for new() that’d get triggered by Moo once Moo is done generating new()? e.g.: something along these lines (obviously simplified and incomplete pseudo code): my $final_new; … Moo builds up new() here like it does … if ($class->can('FINALIZE_NEW')) { $final_new = $class->FINALIZE_NEW( $final_new ); } … $final_new gets installed as a$class->new … In the meantime I'll caveat the my 2 Role::*::New role's ASAP. thanks again

Thu Feb 05 12:53:29 2015 haarg [...] haarg.org - Correspondence added

Moo will now detect the cases where using an around new will cause problems and throw an error. We aren't interested in adding additional complexity to allow wrapping new. Fixed in 1.999_001.

Thu Feb 05 12:53:36 2015 haarg [...] haarg.org - Status changed from 'open' to 'patched'

Wed Feb 11 11:42:10 2015 dmuey [...] cpan.org - Correspondence added

On Thu Feb 05 12:53:29 2015, haarg wrote: Show quoted text

> Moo will now detect the cases where using an around new will cause > problems and throw an error. We aren't interested in adding > additional complexity to allow wrapping new. > > Fixed in 1.999_001.

excellent, that sounds like the way to go!

Sat Apr 11 14:32:48 2015 haarg [...] haarg.org - Correspondence added

Fixed in 2.000000

Sat Apr 11 14:32:54 2015 haarg [...] haarg.org - Status changed from 'patched' to 'resolved'

Sat Apr 11 14:32:55 2015 haarg [...] haarg.org - Fixed in 2.000000 added