Bug #97604 for Moose: Don't use Moose::Exporter's meta_lookup callback in Moose::Exporter itself when it's not needed

Tue Jul 29 15:47:04 2014 DROLSKY [...] cpan.org - Ticket created

Subject:

Replace Moose::Exporter's meta_lookup allback option by looking at init_meta return value

This is mostly a note to remind myself of my thoughts on this ... While I was working on MX::Role::Parameterized I had some issues with its use of meta_lookup. It does some dangerous and fragile things to make sure that Moose::Exporter and other code sees the right meta object while it's in the process of generating a parameterized role from a parameterizable role. It occurred to me that we can ensure we have the right meta object by simply capturing the return value of init_meta() calls and passing that object around to where it's needed. This needs more investigation, but if I'm right we could simplify the internals a fair bit, and make MXRP less bug-prone at the same time.

Tue Jul 29 16:24:58 2014 doy [...] tozt.net - Correspondence added

CC:	;
Subject:	Re: [rt.cpan.org #97604] Replace Moose::Exporter's meta_lookup allback option by looking at init_meta return value
Date:	Tue, 29 Jul 2014 16:24:38 -0400
To:	Dave Rolsky via RT <bug-Moose [...] rt.cpan.org>
From:	Jesse Luehrs <doy [...] tozt.net>

On Tue, Jul 29, 2014 at 03:47:05PM -0400, Dave Rolsky via RT wrote: Show quoted text

> It occurred to me that we can ensure we have the right meta object by > simply capturing the return value of init_meta() calls and passing > that object around to where it's needed. > > This needs more investigation, but if I'm right we could simplify the > internals a fair bit, and make MXRP less bug-prone at the same time.

init_meta is actually not required to return anything - the return value has never been used in the past, and there are quite a few existing modules that don't bother to return anything. Additionally, init_meta was intended to eventually just go away, since all of its functionality has been moved into import (unless you actually override it). I don't think any of the modules that do return something return anything other than the $metaclass parameter, though. In any case, I don't think this is correct, because the point of meta_lookup is that MXRP wants to be able to export both 'parameter' and 'method' keywords into the same package, but have them operate on different metaclasses - 'parameter' is used outside of the role block, and applies to the Parameterizable metaclass, and 'method' is used inside the role block, and applies to the Parameterized metaclass. This can't be handled just by special-casing things in the 'method' keyword definition because it also re-exports everything in Moose::Role, so those keywords also need a way to know that they should be acting on the Parameterized metaclass and not the Parameterizable metaclass. I don't know how you would encode this information other than by having something where you could request "which metaclass should I be using now?". -doy

Tue Jul 29 16:24:59 2014 The RT System itself - Status changed from 'new' to 'open'

Thu Jul 31 15:29:30 2014 DROLSKY [...] cpan.org - Subject changed from 'Replace Moose::Exporter's meta_lookup allback option by looking at init_meta return value' to 'Don't use Moose::Exporter's meta_lookup callback in Moose::Exporter itself when it's not needed'

Thu Jul 31 15:30:43 2014 DROLSKY [...] cpan.org - Correspondence added

On Tue Jul 29 16:24:58 2014, doy@tozt.net wrote: Show quoted text

> On Tue, Jul 29, 2014 at 03:47:05PM -0400, Dave Rolsky via RT wrote:

> > It occurred to me that we can ensure we have the right meta object by > > simply capturing the return value of init_meta() calls and passing > > that object around to where it's needed. > > > > This needs more investigation, but if I'm right we could simplify the > > internals a fair bit, and make MXRP less bug-prone at the same time.

> > init_meta is actually not required to return anything - the return value > has never been used in the past, and there are quite a few existing > modules that don't bother to return anything. Additionally, init_meta > was intended to eventually just go away, since all of its functionality > has been moved into import (unless you actually override it). > > I don't think any of the modules that do return something return > anything other than the $metaclass parameter, though. > > In any case, I don't think this is correct, because the point of > meta_lookup is that MXRP wants to be able to export both 'parameter' and > 'method' keywords into the same package, but have them operate on > different metaclasses - 'parameter' is used outside of the role block, > and applies to the Parameterizable metaclass, and 'method' is used > inside the role block, and applies to the Parameterized metaclass. This > can't be handled just by special-casing things in the 'method' keyword > definition because it also re-exports everything in Moose::Role, so > those keywords also need a way to know that they should be acting on the > Parameterized metaclass and not the Parameterizable metaclass. I don't > know how you would encode this information other than by having > something where you could request "which metaclass should I be using > now?".

Doy, you're right. I changed the title of ticket to better reflect my goal. Basically, I don't think we should use meta_lookup _in Moose::Exporter itself_ where we don't need to. We should be able to simply pass the $meta object itself around in some spots. This reduces the possibility of bugs that come from complicated meta_lookup situations like the one MXRP presents.