From: gfx <gfuji@cpan.org>
Date: Thu, 8 Oct 2009 08:40:35 +0000 (+0900)
Subject: Re-implement role composition, which was implemented but broken
X-Git-Tag: 0.37_04~17
X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?a=commitdiff_plain;h=71e7b544ac5b18ce7b09b9a95723b1a4e28b4321;p=gitmo%2FMouse.git

Re-implement role composition, which was implemented but broken
---

diff --git a/lib/Mouse/Meta/Module.pm b/lib/Mouse/Meta/Module.pm
index a4ddb38..3069250 100755
--- a/lib/Mouse/Meta/Module.pm
+++ b/lib/Mouse/Meta/Module.pm
@@ -121,6 +121,19 @@ sub has_method {
     return $code && $self->_code_is_mine($code);
 }
 
+sub get_method_body{
+    my($self, $method_name) = @_;
+
+    defined($method_name)
+        or $self->throw_error('You must define a method name');
+
+    return $self->{methods}{$method_name} ||= do{
+        my $code = do{ no strict 'refs'; *{$self->{package} . '::' . $method_name}{CODE} };
+
+        $code && $self->_code_is_mine($code) && $code;
+    };
+}
+
 sub get_method{
     my($self, $method_name) = @_;
 
diff --git a/lib/Mouse/Meta/Role.pm b/lib/Mouse/Meta/Role.pm
index bfa99cb..62382c4 100644
--- a/lib/Mouse/Meta/Role.pm
+++ b/lib/Mouse/Meta/Role.pm
@@ -41,7 +41,9 @@ sub get_required_method_list{
 
 sub add_required_methods {
     my($self, @methods) = @_;
-    push @{$self->{required_methods}}, @methods;
+    my %required = map{ $_ => 1 } @{$self->{required_methods}};
+    push @{$self->{required_methods}}, grep{ !$required{$_}++ && !$self->has_method($_) } @methods;
+    return;
 }
 
 sub requires_method {
@@ -56,93 +58,38 @@ sub add_attribute {
     $self->{attributes}->{$name} = (@_ == 1) ? $_[0] : { @_ };
 }
 
-sub _canonicalize_apply_args{
-    my($self, $applicant, %args) = @_;
-
-    if($applicant->isa('Mouse::Meta::Class')){ # Application::ToClass
-        $args{_to} = 'class';
-    }
-    elsif($applicant->isa('Mouse::Meta::Role')){ # Application::ToRole
-        $args{_to} = 'role';
-    }
-    else{ # Appplication::ToInstance
-        $args{_to} = 'class';
-
-        my $metaclass = $applicant->meta->create_anon_class(
-            superclasses => [ref $applicant],
-            cache        => 1,
-        );
-        bless $applicant, $metaclass->name; # rebless
-
-        $applicant = $metaclass;
-    }
-
-    if($args{alias} && !exists $args{-alias}){
-        $args{-alias} = $args{alias};
-    }
-    if($args{excludes} && !exists $args{-excludes}){
-        $args{-excludes} = $args{excludes};
-    }
-
-    if(my $excludes = $args{-excludes}){
-        $args{-excludes} = {}; # replace with a hash ref
-        if(ref $excludes){
-            %{$args{-excludes}} = (map{ $_ => undef } @{$excludes});
-        }
-        else{
-            $args{-excludes}{$excludes} = undef;
-        }
-    }
-
-    return( $applicant, \%args );
-}
-
 sub _check_required_methods{
-    my($role, $class, $args, @other_roles) = @_;
+    my($role, $applicant, $args) = @_;
 
-    if($args->{_to} eq 'class'){
-        my $class_name = $class->name;
+    if($args->{_to} eq 'role'){
+        $applicant->add_required_methods($role->get_required_method_list);
+    }
+    else{ # to class or instance
+        my $class_name = $applicant->name;
         my $role_name  = $role->name;
         my @missing;
         foreach my $method_name(@{$role->{required_methods}}){
-            if(!$class_name->can($method_name)){
-                my $has_method      = 0;
-
-                foreach my $another_role_spec(@other_roles){
-                    my $another_role_name = $another_role_spec->[0];
-                    if($role_name ne $another_role_name && $another_role_name->can($method_name)){
-                        $has_method = 1;
-                        last;
-                    }
-                }
-
-                push @missing, $method_name if !$has_method;
+            if(!($class_name->can($method_name) || exists $args->{to_be_provided}{$method_name})){
+                push @missing, $method_name;
             }
         }
         if(@missing){
-            $class->throw_error("'$role_name' requires the "
+            $role->throw_error("'$role_name' requires the "
                 . (@missing == 1 ? 'method' : 'methods')
                 . " "
                 . english_list(map{ sprintf q{'%s'}, $_ } @missing)
                 . " to be implemented by '$class_name'");
         }
     }
-    else {
-        # apply role($role) to role($class)
-        foreach my $method_name($role->get_required_method_list){
-            next if $class->has_method($method_name); # already has it
-            $class->add_required_methods($method_name);
-        }
-    }
 
     return;
 }
 
 sub _apply_methods{
-    my($role, $class, $args) = @_;
+    my($role, $applicant, $args) = @_;
 
     my $role_name  = $role->name;
-    my $class_name = $class->name;
+    my $class_name = $applicant->name;
 
     my $alias    = $args->{-alias};
     my $excludes = $args->{-excludes};
@@ -150,24 +97,25 @@ sub _apply_methods{
     foreach my $method_name($role->get_method_list){
         next if $method_name eq 'meta';
 
-        my $code = $role_name->can($method_name);
+        my $code = $role->get_method_body($method_name);
 
-        if(!exists $excludes->{$method_name}){
-            if(!$class->has_method($method_name)){
-                $class->add_method($method_name => $code);
+        if($excludes && !exists $excludes->{$method_name}){
+            if(!$applicant->has_method($method_name)){
+                # The third argument, $role, is used in Role::Composite
+                $applicant->add_method($method_name => $code, $role);
             }
         }
 
         if($alias && $alias->{$method_name}){
             my $dstname = $alias->{$method_name};
 
-            my $dstcode = do{ no strict 'refs'; *{$class_name . '::' . $dstname}{CODE} };
+            my $dstcode = $applicant->get_method_body($dstname);
 
             if(defined($dstcode) && $dstcode != $code){
-                $class->throw_error("Cannot create a method alias if a local method of the same name exists");
+                $role->throw_error("Cannot create a method alias if a local method of the same name exists");
             }
             else{
-                $class->add_method($dstname => $code);
+                $applicant->add_method($dstname => $code, $role);
             }
         }
     }
@@ -176,41 +124,30 @@ sub _apply_methods{
 }
 
 sub _apply_attributes{
-    my($role, $class, $args) = @_;
+    my($role, $applicant, $args) = @_;
 
-    if ($args->{_to} eq 'class') {
-        # apply role to class
-        for my $attr_name ($role->get_attribute_list) {
-            next if $class->has_attribute($attr_name);
-
-            my $spec = $role->get_attribute($attr_name);
-
-            $class->add_attribute($attr_name => %{$spec});
-        }
-    }
-    else {
-        # apply role to role
-        for my $attr_name ($role->get_attribute_list) {
-            next if $class->has_attribute($attr_name);
+    for my $attr_name ($role->get_attribute_list) {
+        next if $applicant->has_attribute($attr_name);
 
-            my $spec = $role->get_attribute($attr_name);
-            $class->add_attribute($attr_name => $spec);
-        }
+        $applicant->add_attribute($attr_name => $role->get_attribute($attr_name));
     }
-
     return;
 }
 
 sub _apply_modifiers{
-    my($role, $class, $args) = @_;
+    my($role, $applicant, $args) = @_;
 
     for my $modifier_type (qw/override before around after/) {
+        my $modifiers = $role->{"${modifier_type}_method_modifiers"}
+            or next;
+
         my $add_modifier = "add_${modifier_type}_method_modifier";
-        my $modifiers    = $role->{"${modifier_type}_method_modifiers"};
 
-        while(my($method_name, $modifier_codes) = each %{$modifiers}){
+        foreach my $method_name (keys %{$modifiers}){
+            my $modifier_codes = $modifiers->{$method_name};
             foreach my $code(ref($modifier_codes) eq 'ARRAY' ? @{$modifier_codes} : $modifier_codes){
-                $class->$add_modifier($method_name => $code);
+                next if $applicant->{"_$modifier_type"}{$code}++; # skip applied modifiers
+                $applicant->$add_modifier($method_name => $code);
             }
         }
     }
@@ -218,12 +155,12 @@ sub _apply_modifiers{
 }
 
 sub _append_roles{
-    my($role, $class, $args) = @_;
+    my($role, $applicant, $args) = @_;
 
-    my $roles = ($args->{_to} eq 'class') ? $class->roles : $class->get_roles;
+    my $roles = ($args->{_to} eq 'role') ? $applicant->get_roles : $applicant->roles;
 
     foreach my $r($role, @{$role->get_roles}){
-        if(!$class->does_role($r->name)){
+        if(!$applicant->does_role($r->name)){
             push @{$roles}, $r;
         }
     }
@@ -234,116 +171,72 @@ sub _append_roles{
 sub apply {
     my $self      = shift;
     my $applicant = shift;
-    my $args;
-
-    ($applicant, $args) = $self->_canonicalize_apply_args($applicant, @_);
-
-    $self->_check_required_methods($applicant, $args);
-    $self->_apply_methods($applicant, $args);
-    $self->_apply_attributes($applicant, $args);
-    $self->_apply_modifiers($applicant, $args);
-    $self->_append_roles($applicant, $args);
-    return;
-}
-
-sub combine_apply {
-    my($role_class, $applicant, @roles) = @_;
-
-    ($applicant) = $role_class->_canonicalize_apply_args($applicant);
 
-    # check conflicting
-    my %method_provided;
-    my @method_conflicts;
-    my %attr_provided;
-    my %override_provided;
+    my %args = (@_ == 1) ? %{ $_[0] } : @_;
 
-    foreach my $role_spec (@roles) {
-        my $role      = $role_spec->[0]->meta;
-        my $role_name = $role->name;
-
-        # methods
-        foreach my $method_name($role->get_method_list){
-            next if $applicant->has_method($method_name); # manually resolved
-
-            my $code = do{ no strict 'refs'; \&{ $role_name . '::' . $method_name } };
+    if($applicant->isa('Mouse::Meta::Class')){  # Application::ToClass
+        $args{_to} = 'class';
+    }
+    elsif($applicant->isa('Mouse::Meta::Role')){ # Application::ToRole
+        $args{_to} = 'role';
+    }
+    else{                                       # Appplication::ToInstance
+        $args{_to} = 'instance';
 
-            my $c = $method_provided{$method_name};
+        my $metaclass = $applicant->meta->create_anon_class(
+            superclasses => [ref $applicant],
+            cache        => 1,
+        );
+        bless $applicant, $metaclass->name; # rebless
 
-            if($c && $c->[0] != $code){
-                push @{$c}, $role;
-                push @method_conflicts, $c;
-            }
-            else{
-                $method_provided{$method_name} = [$code, $method_name, $role];
-            }
-        }
+        $applicant = $metaclass;
+    }
 
-        # attributes
-        foreach my $attr_name($role->get_attribute_list){
-            my $attr = $role->get_attribute($attr_name);
-            my $c    = $attr_provided{$attr_name};
-            if($c && $c != $attr){
-                $role_class->throw_error("We have encountered an attribute conflict with '$attr_name' "
-                                   . "during composition. This is fatal error and cannot be disambiguated.")
-            }
-            else{
-                $attr_provided{$attr_name} = $attr;
-            }
-        }
+    if($args{alias} && !exists $args{-alias}){
+        $args{-alias} = $args{alias};
+    }
+    if($args{excludes} && !exists $args{-excludes}){
+        $args{-excludes} = $args{excludes};
+    }
 
-        # override modifiers
-        foreach my $method_name($role->get_method_modifier_list('override')){
-            my $override = $role->get_override_method_modifier($method_name);
-            my $c        = $override_provided{$method_name};
-            if($c && $c != $override){
-                $role_class->throw_error( "We have encountered an 'override' method conflict with '$method_name' during "
-                                   . "composition (Two 'override' methods of the same name encountered). "
-                                   . "This is fatal error.")
-            }
-            else{
-                $override_provided{$method_name} = $override;
-            }
-        }
+    # In Moose it is called 'aliased methods'
+    $args{to_be_provided} = {};
+    if(my $alias = $args{-alias}){
+        @{$args{to_be_provided}}{ values %{$alias} } = ();
     }
-    if(@method_conflicts){
-        my $error;
-
-        if(@method_conflicts == 1){
-            my($code, $method_name, @roles) = @{$method_conflicts[0]};
-            $role_class->throw_error(
-                sprintf q{Due to a method name conflict in roles %s, the method '%s' must be implemented or excluded by '%s'},
-                    english_list(map{ sprintf q{'%s'}, $_->name } @roles), $method_name, $applicant->name
-            );
+    @{ $args{to_be_provided} }{ $self->get_method_list } = ();
+
+    if(my $excludes = $args{-excludes}){
+        $args{-excludes} = {}; # replace with a hash ref
+        if(ref $excludes){
+            %{$args{-excludes}} = (map{ $_ => undef } @{$excludes});
         }
         else{
-            @method_conflicts = sort { $a->[0] cmp $b->[0] } @method_conflicts; # to avoid hash-ordering bugs
-            my $methods = english_list(map{ sprintf q{'%s'}, $_->[1] } @method_conflicts);
-            my $roles   = english_list(
-                map{ sprintf q{'%s'}, $_->name }
-                map{ my($code, $method_name, @roles) = @{$_}; @roles } @method_conflicts
-            );
-
-            $role_class->throw_error(
-                sprintf q{Due to method name conflicts in roles %s, the methods %s must be implemented or excluded by '%s'},
-                    $roles, $methods, $applicant->name
-            );
+            $args{-excludes}{$excludes} = undef;
         }
     }
 
-    foreach my $role_spec (@roles) {
-        my($role_name, $args) = @{$role_spec};
+    $self->_check_required_methods($applicant, \%args);
+    $self->_apply_attributes($applicant, \%args);
+    $self->_apply_methods($applicant, \%args);
+    $self->_apply_modifiers($applicant, \%args);
+    $self->_append_roles($applicant, \%args);
+    return;
+}
+
 
-        my $role = $role_name->meta;
+sub combine {
+    my($role_class, @role_specs) = @_;
 
-        ($applicant, $args) = $role->_canonicalize_apply_args($applicant, %{$args});
+    require 'Mouse/Meta/Role/Composite.pm'; # we don't want to create its namespace
 
-        $role->_check_required_methods($applicant, $args, @roles);
-        $role->_apply_methods($applicant, $args);
-        $role->_apply_attributes($applicant, $args);
-        $role->_apply_modifiers($applicant, $args);
-        $role->_append_roles($applicant, $args);
+    my $composite = Mouse::Meta::Role::Composite->create_anon_role();
+
+    foreach my $role_spec (@role_specs) {
+        my($role_name, $args) = @{$role_spec};
+        $role_name->meta->apply($composite, %{$args});
     }
-    return;
+    return $composite;
 }
 
 for my $modifier_type (qw/before after around/) {
diff --git a/lib/Mouse/Meta/Role/Composite.pm b/lib/Mouse/Meta/Role/Composite.pm
new file mode 100644
index 0000000..02a0510
--- /dev/null
+++ b/lib/Mouse/Meta/Role/Composite.pm
@@ -0,0 +1,116 @@
+package Mouse::Meta::Role::Composite;
+use Mouse::Util qw(english_list); # enables strict and warnings
+use Mouse::Meta::Role;
+our @ISA = qw(Mouse::Meta::Role);
+
+sub get_method_list{
+    my($self) = @_;
+    return keys %{ $self->{methods} };
+}
+
+sub add_method {
+    my($self, $method_name, $code, $role) = @_;
+
+    if( ($self->{methods}{$method_name} || 0) == $code){
+        # This role already has the same method.
+        return;
+    }
+
+    if($method_name ne 'meta'){
+        my $roles = $self->{composed_roles_by_method}{$method_name} ||= [];
+        push @{$roles}, $role;
+        if(@{$roles} > 1){
+            $self->{conflicting_methods}{$method_name}++;
+        }
+    }
+
+    $self->{methods}{$method_name} = $code;
+    # no need to add a subroutine to the stash
+    return;
+}
+
+sub get_method_body {
+    my($self, $method_name) = @_;
+    return $self->{methods}{$method_name};
+}
+
+sub has_method {
+    # my($self, $method_name) = @_;
+    return 0; # to fool _apply_methods() in combine()
+}
+
+sub has_attribute{
+    # my($self, $method_name) = @_;
+    return 0; # to fool _appply_attributes() in combine()
+}
+
+sub has_override_method_modifier{
+    # my($self, $method_name) = @_;
+    return 0; # to fool _apply_modifiers() in combine()
+}
+
+sub add_attribute{
+    my($self, $attr_name, $spec) = @_;
+
+    my $existing = $self->{attributes}{$attr_name};
+    if($existing && $existing != $spec){
+        $self->throw_error("We have encountered an attribute conflict with '$attr_name' "
+                         . "during composition. This is fatal error and cannot be disambiguated.");
+    }
+    $self->SUPER::add_attribute($attr_name, $spec);
+    return;
+}
+
+sub add_override_method_modifier{
+    my($self, $method_name, $code) = @_;
+
+    my $existing = $self->{override_method_modifiers}{$method_name};
+    if($existing && $existing != $code){
+        $self->throw_error( "We have encountered an 'override' method conflict with '$method_name' during "
+                          . "composition (Two 'override' methods of the same name encountered). "
+                          . "This is fatal error.")
+    }
+    $self->SUPER::add_override_method_modifier($method_name, $code);
+    return;
+}
+
+# components of apply()
+
+sub _apply_methods{
+    my($self, $applicant, $args) = @_;
+
+    if(exists $self->{conflicting_methods}){
+        my $applicant_class_name = $applicant->name;
+
+        my @conflicting = sort grep{ !$applicant_class_name->can($_) } keys %{ $self->{conflicting_methods} };
+
+        if(@conflicting == 1){
+            my $method_name = $conflicting[0];
+            my @roles       = sort @{ $self->{composed_roles_by_method}{$method_name} };
+            $self->throw_error(
+               sprintf q{Due to a method name conflict in roles %s, the method '%s' must be implemented or excluded by '%s'},
+                   english_list(map{ sprintf q{'%s'}, $_->name } @roles), $method_name, $applicant->name
+            );
+        }
+        elsif(@conflicting > 1){
+            my $methods = english_list(map{ sprintf q{'%s'}, $_ } @conflicting);
+
+            my %seen;
+            my $roles   = english_list(
+                sort map{ my $name = $_->name; $seen{$name}++ ? () : sprintf q{'%s'}, $name }
+                map{ @{$_} } @{ $self->{composed_roles_by_method} }{@conflicting}
+            );
+
+            $self->throw_error(
+               sprintf q{Due to method name conflicts in roles %s, the methods %s must be implemented or excluded by '%s'},
+                   $roles, $methods, $applicant->name
+            );
+        }
+    }
+
+    $self->SUPER::_apply_methods($applicant, $args);
+    return;
+}
+1;
+__END__
+
diff --git a/lib/Mouse/Util.pm b/lib/Mouse/Util.pm
index c31e115..5f5d8de 100644
--- a/lib/Mouse/Util.pm
+++ b/lib/Mouse/Util.pm
@@ -2,6 +2,7 @@ package Mouse::Util;
 use Mouse::Exporter; # enables strict and warnings
 
 use Carp qw(confess);
+use Scalar::Util qw(blessed);
 use B ();
 
 use constant _MOUSE_VERBOSE => !!$ENV{MOUSE_VERBOSE};
@@ -266,7 +267,7 @@ sub is_class_loaded {
 
 
 sub apply_all_roles {
-    my $meta = Mouse::Meta::Class->initialize(shift);
+    my $applicant = blessed($_[0]) ? shift : Mouse::Meta::Class->initialize(shift);
 
     my @roles;
 
@@ -281,15 +282,15 @@ sub apply_all_roles {
         my $role_name = $roles[-1][0];
         load_class($role_name);
         ( $role_name->can('meta') && $role_name->meta->isa('Mouse::Meta::Role') )
-            || $meta->throw_error("You can only consume roles, $role_name(".$role_name->meta.") is not a Mouse role");
+            || $applicant->meta->throw_error("You can only consume roles, $role_name(".$role_name->meta.") is not a Mouse role");
     }
 
     if ( scalar @roles == 1 ) {
         my ( $role, $params ) = @{ $roles[0] };
-        $role->meta->apply( $meta, ( defined $params ? %$params : () ) );
+        $role->meta->apply( $applicant, ( defined $params ? %$params : () ) );
     }
     else {
-        Mouse::Meta::Role->combine_apply($meta, @roles);
+        Mouse::Meta::Role->combine(@roles)->apply($applicant);
     }
     return;
 }