Bug #79783 for Moose: Method delegation - ask forgiveness, not permission

Fri Sep 21 10:03:03 2012 perl [...] toby.ink - Ticket created

Subject:

Method delegation - ask forgiveness, not permission

``` { package Thingy; has fh => (is => 'ro', handles => [qw(getline)]); } use IO::Handle; open my $fh, '<', 'some-file.txt'; my $thingy = Thingy->new(fh => $fh); print $thingy->fh->getline; # works print $thingy->getline; # bails ``` The reason the delegated method fails is because Moose::Meta::Method::Delegation::_initialize_body checks that the proxy (i.e. the file handle in this case) is blessed before calling the method (and it's not). Yes, there are workarounds, but workarounds suck by definition. A better solution would be to not check whether the proxy is blessed and just go ahead and call the delegated method. This could be done in an eval block if you wish to avoid the default Perl method-not-found error and supply a Moosey one instead. Also related is RT #46614.

Fri Sep 21 10:05:49 2012 doy [...] tozt.net - Correspondence added

Subject:	Re: [rt.cpan.org #79783] Method delegation - ask forgiveness, not permission
Date:	Fri, 21 Sep 2012 09:05:27 -0500
To:	Toby Inkster via RT <bug-Moose [...] rt.cpan.org>
From:	Jesse Luehrs <doy [...] tozt.net>

On Fri, Sep 21, 2012 at 10:03:04AM -0400, Toby Inkster via RT wrote: Show quoted text

> Fri Sep 21 10:03:03 2012: Request 79783 was acted upon. > Transaction: Ticket created by TOBYINK > Queue: Moose > Subject: Method delegation - ask forgiveness, not permission > Broken in: 2.0603, 2.0604 > Severity: Normal > Owner: Nobody > Requestors: mail@tobyinkster.co.uk > Status: new > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=79783 > > > > ``` > { > package Thingy; > has fh => (is => 'ro', handles => [qw(getline)]); > } > > use IO::Handle; > open my $fh, '<', 'some-file.txt'; > my $thingy = Thingy->new(fh => $fh); > > print $thingy->fh->getline; # works > print $thingy->getline; # bails > ``` > > The reason the delegated method fails is because > Moose::Meta::Method::Delegation::_initialize_body checks that the proxy > (i.e. the file handle in this case) is blessed before calling the > method (and it's not). > > Yes, there are workarounds, but workarounds suck by definition. > > A better solution would be to not check whether the proxy is blessed > and just go ahead and call the delegated method. This could be done in > an eval block if you wish to avoid the default Perl method-not-found > error and supply a Moosey one instead. > > Also related is RT #46614.

The check is there so that we get an error at compile time instead of runtime, which is important. The check should just be expanded to also accept filehandles. -doy

Fri Sep 21 10:05:51 2012 The RT System itself - Status changed from 'new' to 'open'

Fri Sep 21 10:46:13 2012 autarch [...] urth.org - Correspondence added

Subject:	Re: [rt.cpan.org #79783] Method delegation - ask forgiveness, not permission
Date:	Fri, 21 Sep 2012 09:45:59 -0500 (CDT)
To:	Jesse Luehrs via RT <bug-Moose [...] rt.cpan.org>
From:	Dave Rolsky <autarch [...] urth.org>

On Fri, 21 Sep 2012, Jesse Luehrs via RT wrote: Show quoted text

> The check is there so that we get an error at compile time instead of > runtime, which is important. The check should just be expanded to also > accept filehandles.

I don't see how this happens at compile time. The body of the delegated accessor doesn't get executed until it's first called. I think the reason we're checking is to provide a nice error. -dave /*============================================================ http://VegGuide.org http://blog.urth.org Your guide to all that's veg House Absolute(ly Pointless) ============================================================*/

Sat Sep 22 18:08:49 2012 perl [...] toby.ink - Correspondence added

The attached test case illustrates that the check is done at run-time, when the delegated method is called; and that Moose's nice error messages are only issued in certain situations.

Subject:

moose-delegation.pl

use Test::More; use Test::Exception; { package Local::Class; use Moose; has attr => ( is => 'rw', isa => 'Any', handles => { delegated => 'method', }, ); sub method { return 'ok'; } } my $obj = Local::Class->new(); # nice error throws_ok { $obj->delegated } qr{the value of attr is not defined}; $obj->attr(42); # not nice error; just the Perl default throws_ok { $obj->delegated } qr{without a package or object reference}; $obj->attr([]); # nice error throws_ok { $obj->delegated } qr{is not an object}; $obj->attr( Local::Class->new ); # no error lives_ok { $obj->delegated }; done_testing();

Fri Oct 05 11:52:09 2012 perl [...] toby.ink - Correspondence added

On 2012-09-21T15:03:03+01:00, TOBYINK wrote: Show quoted text

> A better solution would be to not check whether the proxy is blessed > and just go ahead and call the delegated method. This could be done in > an eval block if you wish to avoid the default Perl method-not-found > error and supply a Moosey one instead.

Actually, the eval block solution is a non-starter as it catches too many errors - e.g. exceptions deliberately thrown by the delegated method.

Fri Oct 05 11:58:43 2012 perl [...] toby.ink - Correspondence added

Maybe, in "_initialize_body" replace this: if ($error) with this: if ($error and not eval { $proxy->can($method_to_call) }) There are edge cases it doesn't cover, but it should be an improvement over the current situations.

Mon Oct 08 20:42:18 2012 perl [...] toby.ink - Correspondence added

RT-Send-CC:

doy [...] tozt.net

OK, patch attached which allows delegation to filehandles.

Subject:

Moose-filehandle-delegation-tobyink-20121009.diff

diff --git a/lib/Moose/Meta/Method/Delegation.pm b/lib/Moose/Meta/Method/Delegation.pm index 32436d6..e8cbb2e 100644 --- a/lib/Moose/Meta/Method/Delegation.pm +++ b/lib/Moose/Meta/Method/Delegation.pm @@ -90,17 +90,44 @@ sub _initialize_body { : ref($proxy) && !blessed($proxy) ? qq{ is not an object (got '$proxy')} : undef; + unshift @_, @{ $self->curried_arguments }; + if ($error) { - $self->throw_error( - "Cannot delegate $handle_name to $method_to_call because " - . "the value of " - . $self->associated_attribute->name - . $error, - method_name => $method_to_call, - object => $instance - ); + local $@; + my @return; + if (wantarray) { + @return = eval { $proxy->$method_to_call(@_) }; + } + elsif (defined wantarray) { + push @return, scalar eval { $proxy->$method_to_call(@_) }; + } + else { + eval { $proxy->$method_to_call(@_); 1 }; + } + + if (not $@) { + return @return; + } + + # These errors we don't want to catch and rethrow. We want + # to substitute a more Moose-specific error message. + my $r1 = qr{^Can't locate object method "$method_to_call" via package}; + my $r2 = qr{^Can't call method "$method_to_call" on (unblessed reference|an undefined value)}; + + if ($@ =~ $r1 or $@ =~ $r2) { + $self->throw_error( + "Cannot delegate $handle_name to $method_to_call because " + . "the value of " + . $self->associated_attribute->name + . $error, + method_name => $method_to_call, + object => $instance + ); + } + + die $@; } - unshift @_, @{ $self->curried_arguments }; + $proxy->$method_to_call(@_); }; } diff --git a/t/attributes/attribute_delegation.t b/t/attributes/attribute_delegation.t index 733542c..2a557b4 100644 --- a/t/attributes/attribute_delegation.t +++ b/t/attributes/attribute_delegation.t @@ -482,4 +482,20 @@ is($car->stop, 'Engine::stop', '... got the right value from ->stop'); ); } +{ + use IO::Handle; + + { + package Local::Delegation::Test; + use Moose; + has attr => (is => 'ro', handles => [qw/ print /]); + } + + my $d; + open my $fh, '>', \$d; + Local::Delegation::Test->new(attr => $fh)->attr->print("1"); + Local::Delegation::Test->new(attr => $fh)->print("2"); + is($d, "12", "can delegate to non-blessed references"); +} + done_testing;