Bug #14250 for Class-Std: No Class::Std behaviours for runtime loaded classes

Sun Aug 21 21:44:45 2005 Guest - Ticket created

Subject:

No Class::Std behaviours for runtime loaded classes

Class::Std classes don't retain all of their behaviour when they are "use eval'd" at runtime. For example CUMULATIVE, PRIVATE and RESTRICTED attributes have no effect for classes loaded at runtime. This can lead to subtle bugs where you believe CUMULATIVE is working, but because of how the classes were originally loaded, the behaviour was not enabled, since the CHECK block was not executed. There are cases where I don't know precisely what classes will be used until run-time, or when I'm writing test cases with Test::More and load the class using use_ok(). Attached is a patch to corrects this problem. It installs an "import" method in the Class::Std derived class that reloads all the Class::Std behaviours. The loading systems used to be inside the CHECK block, but they have been moved into their own subroutines. The CHECK block still executes, but it uses the new subroutines to set up everything. I looked for a way to do it without using a CHECK block at all, but I could not find an approach that would work in all instances.. if you have any ideas I'd love to hear them, and I'd be glad to implement them and supply a patch. I tried re-installing all the behaviour when the new() constructor was used (being careful not to wastefully repeat the process more than once), but I couldn't get all the test cases to pass with that approach.

diff -urN Class-Std-0.0.4/lib/Class/Std.pm Class-Std-NEW/lib/Class/Std.pm --- Class-Std-0.0.4/lib/Class/Std.pm 2005-08-06 21:19:22.000000000 -0700 +++ Class-Std-NEW/lib/Class/Std.pm 2005-08-21 18:10:30.000000000 -0700 @@ -10,8 +10,6 @@ *ID = \&Scalar::Util::refaddr; -my (%attribute, %cumulative, %anticumulative, %restricted, %private, %overload); - my @exported_subs = qw( new DESTROY @@ -25,10 +23,14 @@ my $caller = caller; no strict 'refs'; - *{ $caller . '::ident' } = \&Scalar::Util::refaddr; + *{ $caller . '::ident' } = __PACKAGE__->can('ID'); + *{ $caller . '::import' } = __PACKAGE__->can('_install_handlers'); + for my $sub ( @exported_subs ) { - *{ $caller . '::' . $sub } = \&{$sub}; + *{ $caller . '::' . $sub } = __PACKAGE__->can($sub); } + + return; } sub _find_sub { @@ -73,6 +75,8 @@ *_extract_get = _extractor_for_pair_named('get'); *_extract_set = _extractor_for_pair_named('set'); +my %attribute; + sub MODIFY_HASH_ATTRIBUTES { my ($package, $referent, @attrs) = @_; for my $attr (@attrs) { @@ -86,7 +90,7 @@ no strict 'refs'; *{$package.'::get_'.$getter} = sub { return $referent->{ID($_[0])}; - } + }; } if ($setter = _extract_set($config)) { no strict 'refs'; @@ -97,7 +101,7 @@ my $old_val = $referent->{ID($self)}; $referent->{ID($self)} = $new_val; return $old_val; - } + }; } } undef $attr; @@ -149,6 +153,9 @@ CODIFY => sprintf( $STD_OVERLOADER, q{&{}} ), ); + +my (%cumulative, %anticumulative, %restricted, %private, %overload); + sub MODIFY_CODE_ATTRIBUTES { my ($package, $referent, @attrs) = @_; for my $attr (@attrs) { @@ -158,10 +165,10 @@ elsif ($attr =~ m/\A CUMULATIVE \s* [(] \s* BASE \s* FIRST \s* [)] \z/xms) { push @{$anticumulative{$package}}, $referent; } - elsif ($attr =~ m/\A RESTRICTED \z/xms) { + elsif ($attr eq 'RESTRICTED') { push @{$restricted{$package}}, $referent; } - elsif ($attr =~ m/\A PRIVATE \z/xms) { + elsif ($attr eq 'PRIVATE') { push @{$private{$package}}, $referent; } elsif (exists $OVERLOADER_FOR{$attr}) { @@ -224,9 +231,20 @@ } CHECK { - my (%cumulative_named, %anticumulative_named); + _install_handlers(); +} + +sub _install_handlers { + _install_restricted(); + _install_private(); + _install_cumulative(); + _install_anticumulative(); + _install_overload(); + return; +} - # Implement restricted methods (only callable within hierarchy)... +# Implement restricted methods (only callable within hierarchy)... +sub _install_restricted { for my $package (keys %restricted) { for my $sub_ref (@{$restricted{$package}}) { my $name = _find_sub($package, $sub_ref); @@ -234,18 +252,24 @@ no strict 'refs'; my $sub_name = $package.'::'.$name; my $original = *{$sub_name}{CODE} - or croak "Restricted method ${package}::$name() declared ", + or croak "Restricted method $sub_name() declared ", 'but not defined'; *{$sub_name} = sub { my $caller = caller; goto &{$original} if $caller->isa($package) || $package->isa($caller); croak "Can't call restricted method $sub_name() from class $caller"; - } + }; } } - # Implement private methods (only callable from class itself)... + %restricted = (); + + return; +} + +# Implement private methods (only callable from class itself)... +sub _install_private { for my $package (keys %private) { for my $sub_ref (@{$private{$package}}) { my $name = _find_sub($package, $sub_ref); @@ -253,16 +277,24 @@ no strict 'refs'; my $sub_name = $package.'::'.$name; my $original = *{$sub_name}{CODE} - or croak "Private method ${package}::$name() declared ", + or croak "Private method $sub_name() declared ", 'but not defined'; *{$sub_name} = sub { my $caller = caller; goto &{$original} if $caller eq $package; croak "Can't call private method $sub_name() from class $caller"; - } + }; } } + %private = (); + + return; +} + +my %cumulative_named; + +sub _install_cumulative { for my $package (keys %cumulative) { for my $sub_ref (@{$cumulative{$package}}) { my $name = _find_sub($package, $sub_ref); @@ -298,7 +330,15 @@ }; } } + + %cumulative = (); + + return; +} + +my %anticumulative_named; +sub _install_anticumulative { for my $package (keys %anticumulative) { for my $sub_ref (@{$anticumulative{$package}}) { my $name = _find_sub($package, $sub_ref); @@ -346,16 +386,25 @@ } } + %anticumulative = (); + + return; +} + +sub _install_overload { for my $package (keys %overload) { - foreach my $operation (@{ $overload{$package} }) { + for my $operation (@{ $overload{$package} }) { my ($referent, $attr) = @$operation; local $^W; my $method = _find_sub($package, $referent); eval sprintf $OVERLOADER_FOR{$attr}, ($package)x2; die "Internal error: $@" if $@; } - delete $overload{$package}; } + + %overload = (); + + return; } sub new { @@ -426,7 +475,6 @@ return grep { $seen{$_}++ } @_; } - sub _mislabelled { my (@names) = map { qq{'$_'} } uniq @_; diff -urN Class-Std-0.0.4/t/runtime_load.t Class-Std-NEW/t/runtime_load.t --- Class-Std-0.0.4/t/runtime_load.t 1969-12-31 16:00:00.000000000 -0800 +++ Class-Std-NEW/t/runtime_load.t 2005-08-21 18:10:03.000000000 -0700 @@ -0,0 +1,72 @@ +use Test::More tests => 11; + +use strict; +use warnings FATAL => 'all'; +use lib 't/lib'; + +my $class = 'TestClass'; + +USE: { + # suppress the 'Too late to run CHECK block' warning + local $SIG{__WARN__} = sub {}; + use_ok $class; +} + +my $obj; + +NEW: { + my $method = 'new'; + + can_ok $class, $method; + isa_ok $obj = $class->$method, $class; +} + +CUMULATIVE: { + my $method = 'cumulative'; + + can_ok $obj, $method; + + is( + $obj->$method, + 'TestClassTestClass::Base', + 'CUMULATIVE is enabled', + ); +} + +INSTALLED_PRIVATE: { + my $method = 'private'; + + eval "package $class; sub $method : PRIVATE {}"; + is $@, '', 'no errors installing $method method'; + + # Reload Class::Std handlers + Class::Std::_install_handlers(); + + can_ok $obj, $method; + + eval { $obj->$method }; + like( + $@, + qr/Can\'t call private method/, + 'PRIVATE constraint works', + ); +} + +INSTALLED_RESTRICTED: { + my $method = 'restricted'; + + eval "package $class; sub $method : RESTRICTED {}"; + is $@, '', 'no errors installing method'; + + # Reload Class::Std handlers + Class::Std::_install_handlers(); + + can_ok $obj, $method; + + eval { $obj->$method }; + like( + $@, + qr/Can\'t call restricted method/, + 'RESTRICTED constraint works', + ); +} diff -urN Class-Std-0.0.4/t/lib/TestClass.pm Class-Std-NEW/t/lib/TestClass.pm --- Class-Std-0.0.4/t/lib/TestClass.pm 1969-12-31 16:00:00.000000000 -0800 +++ Class-Std-NEW/t/lib/TestClass.pm 2005-08-21 18:06:25.000000000 -0700 @@ -0,0 +1,15 @@ +package TestClass; + +use Class::Std; + +our @ISA = qw(TestClass::Base); + +sub cumulative : CUMULATIVE { __PACKAGE__ } + +package TestClass::Base; + +use Class::Std; + +sub cumulative : CUMULATIVE { __PACKAGE__ } + +1;

Thu Aug 25 08:35:56 2005 LMEYER [...] cpan.org - Correspondence added

From:

luke [...] daeron.com

I'd like to first note that this problem is disastrous for use with mod_perl, where delayed loading is the common case, not the exception. Show quoted text

> Attached is a patch to corrects this problem. It installs an "import" > method in the Class::Std derived class that reloads all the > Class::Std behaviours. The loading systems used to be inside the > CHECK block, but they have been moved into their own subroutines. > The CHECK block still executes, but it uses the new subroutines to > set up everything.

I see problems with this implementation. Specifically, as _install_restricted et al create wrappers for for their methods, and this occurs for ALL subs at EACH import -- you're going to have a lot of wrapped wrappers. What you need is an import that creates the wrappers only for the class being imported, and a flag to keep it from happening multiple times (as an import can happen multiple times). Also, import() doesn't always get called. Also also, the class may want to have its own import method and this would preclude it. Show quoted text

> I looked for a way to do it without using a CHECK block at all, but I > could not find an approach that would work in all instances.. if > you have any ideas I'd love to hear them, and I'd be glad to > implement them and supply a patch. I tried re-installing all the > behaviour when the new() constructor was used (being careful not to > wastefully repeat the process more than once), but I couldn't get > all the test cases to pass with that approach.

If the flagged per-package approach is used I don't see why the CHECK block would be necessary. Seems like it could be done as easily in new() and probably should. I'll play with this if I get some time.

Fri Aug 26 02:03:50 2005 LMEYER [...] cpan.org - Correspondence added

From:

luke [...] daeron.com

I've created a simple patch that applies sub attributes at runtime without futzing with import() and without wrapping the wrappers. The CHECK block (which previously adjusted all of the methods for every package) mutated into a sub that adjusts the methods -- once only -- for a single package. This sub is then called by new(), so in essence the adjustment is as lazy as possible. The only problem I can see with this is that *class* (non-object) methods with :attributes could be called before the first new(), i.e. before the attributes have been applied. I'm not sure this is really a problem. If it is, we could just add back the CHECK block, leaving this corner case only for runtime-loaded classes. I created two new test cases, which repeat the cumulative.t and access.t tests with runtime-loaded classes. But Wait, There's More! Sometimes it comes in handy to reload a class (when developing mod_perl applications, for instance). This doesn't work too well when the same attributes and methods come in for processing the second time. So I also patched import() to make Class::Std forget everything about a class when it's (re)loaded. I also created a test case for this. The only problem with *that* would be if someone carelessly did a 'use Class::Std' twice in the same class, thereby wiping out any prior declarations. I think a "don't do that" would suffice for that. It's a shame that coderefs don't make it into the symbol table before MODIFY_CODE_ATTRIBUTES is called, or all of this delayed adjustment could instead happen immediately when the sub is created. Maybe in a future attributes implementation. Anyway, with this patch Class::Std classes can be created and reloaded properly at runtime.

Download Class-Std.tgz
application/x-gzip 21.7k

Message body not shown because it is not plain text.

Fri Aug 26 06:16:18 2005 DKUBB [...] cpan.org - Correspondence added

From:

dan.kubb-cpan [...] autopilotmarketing.com

Show quoted text

> I see problems with this implementation. Specifically, as > _install_restricted et al create wrappers for for their methods, and > this occurs for ALL subs at EACH import -- you're going to have a lot > of wrapped wrappers. What you need is an import that creates the > wrappers only for the class being imported, and a flag to keep it from > happening multiple times (as an import can happen multiple times).

One thing that might not have been clear from the patch is that I reset the %cumulative, %anticumulative and other hashes right after the subs are wrapped the first time, thus making it impossible for a double wrapping. From what I can see this produces the same effective results as deleting the hashes for a specific package inside the import as you did in the patch submitted Aug 25th. One other idea I had that *should* absolutely prevent double wrapping of any kind is to actually bless the wrapper subroutine -- basically making it an CODE-based object, beside installing it into the symbol table. From then on whenever checking a subroutine to see if it has been wrapped, you just need to check to see if its blessed and skip the wrapping process. (never seen anyone bless a subroutine that is later installed in the symbol table, but I see no reason why this won't work) Show quoted text

> Also, import() doesn't always get called. Also also, the class may > want to have its own import method and this would preclude it.

You're right.. it is probably best not to do it in the import method. Hmm, I wonder if maybe a better approach might be to explicitly execute a method within the Class::Std derived class after all its methods are properly named and installed in the symbol table, sort of like this: ------------------------------------------------------------ package My::Class; use strict; use warnings FATAL => 'all'; use Class::Std; { my %name_of : ATTR; sub my_method : CUMULATIVE { # ... } } register_class; # imported by Class::Std ------------------------------------------------------------ This might not be as auto-magical as a CHECK block or setting things up in the new constructor, but being explicit might not be such a bad idea in this case. I haven't tested this, but I see no reason why it wouldn't work. This had the advantage of allowing class and object methods to use Class::Std behaviour's properly as the module user would expect. I don't think a module user should have to know anything about the internals of a module in order to use it. They shouldn't need to know that they can't call class methods and expect them to implement Class::Std behaviours, until the new constructor has been executed for the first time. Combined with the bless'd wrappers or clearing out of the Class::Std lexical hashes after use should prevent double-wrapping of any methods, and allow runtime loaded modules to be used. It should also safely handle cases where the same module is reloaded multiple times.

Thu Sep 01 14:20:23 2005 Guest - Correspondence added

[DKUBB - Fri Aug 26 06:16:18 2005]: Show quoted text

> One thing that might not have been clear from the patch is that I > reset > the %cumulative, %anticumulative and other hashes right after the > subs are wrapped the first time, thus making it impossible for a > double > wrapping.

Hmm, nope, missed that. Show quoted text

> wrapping process. (never seen anyone bless a subroutine that is later > installed in the symbol table, but I see no reason why this won't > work)

Now THAT is an interesting idea that may have interesting uses -- bascially, attach meta-data to your subroutine :-) Feels like a hack though. Show quoted text

> Hmm, I wonder if maybe a better approach might be to explicitly > execute > a method within the Class::Std derived class after all its methods are > properly named and installed in the symbol table, sort of like this:

I was starting to think that might be the case. It makes the usage a bit more hokey, though. It's explicitly doing what CHECK was meant (but fails) to do. On the other hand, it would give you a chance to install anonymous subs with Class::Std attributes into the symbol table as well, which might be useful.

Tue Apr 14 10:47:45 2009 jloverso [...] mathworks.com - Correspondence added

Daniel, Are you looking at addressing this? It still applies to v0.9.

Tue Apr 14 10:47:46 2009 The RT System itself - Status changed from 'new' to 'open'

Tue Apr 14 10:57:50 2009 webmaster [...] simplemood.com - Correspondence added

Subject:	Re: [rt.cpan.org #14250] No Class::Std behaviours for runtime loaded classes
Date:	Tue, 14 Apr 2009 09:57:32 -0500
To:	bug-Class-Std [...] rt.cpan.org
From:	Dan Muey <webmaster [...] simplemood.com>

Yes, as time allows, any input to help expedite things is greatly appreciated! On Apr 14, 2009, at 9:47 AM, John LoVerso via RT wrote: Show quoted text

> Queue: Class-Std > Ticket <URL: https://rt.cpan.org/Ticket/Display.html?id=14250 > > > Daniel, > > Are you looking at addressing this? It still applies to v0.9.

Tue Apr 14 11:46:18 2009 jloverso [...] mathworks.com - Correspondence added

Subject:	CUMULATIVE fails for classes compiled at separate times
RT-Send-CC:	dmuey [...] cpan.org

Attached is a tar file that includes two of Luke's tests from his patch above: t/runtime_access.t t/runtime_cumulative.t These tests work in 0.7, 0.8, and 0.9. The remaining bug (that we just tripped over today) is that CUMULATIVE fails when a runtime-compiled class derives from a class that was compiled earlier. I added some additional tests that show this problem. That are all variants of runtime_cumulative.t (which is just a copy of cumulative.t with the classes 'eval'd). runtime_cumulative2.t compiles some classes when the module is initially loaded, and the rest are eval'd (like in runtime_cumulative.t) - and this is by just moving the 'eval' down a few lines. This fails. runtime_cumulative3.t doesn't include the classes inline, but instead loads them with 'use'. It works. runtime_cumulative3_require.t is the same except that some of the derived classes are loaded at runtime with 'require'. It fails.

Download t_runtime.tgz
application/x-compressed 1.8k

Message body not shown because it is not plain text.

Tue Apr 14 12:27:15 2009 jloverso [...] mathworks.com - Correspondence added

Subject:

Re: CUMULATIVE fails for classes compiled at separate times

I've attached an updated version of Luke's patch so that it applies to and works with 0.9 (it should also apply to 0.8). I adapted it to fit within the framework of how Damian originally fixed most of this in 0.7.

Download Class-Std+runtime.tgz
application/x-compressed 25.5k

Message body not shown because it is not plain text.