April 24, 2009

The New Moose Warning and the New Moose Critic

Role Design

A role is something that classes do. Usually, a role encapsulates some piece of behavior or state that can be shared between classes. It is important to understand that roles are not classes. You cannot inherit from a role, and a role cannot be instantiated. We sometimes say that roles are consumed, either by classes or other roles.

A role can be composed into a class. In practical terms, this means that all of the methods and attributes defined in a role are added directly to (we sometimes say "flattened into") the class that consumes the role. These attributes and methods then appear as if they were defined in the class itself. A subclass of the consuming class will inherit all of these methods and attributes.

– Dave Rolsky in Moose::Manual::Roles

Roles can be composed to create new roles. This is not like inheritance; this operation is actually commutative summation. This does not establish a hierarchy, instead it creates a composite role with the methods, attributes, requirements, etc. of its component roles. If two or more roles in the composition provide different methods with the same name, a conflict is generated. Such conflicts do not immediately issue fatal errors, but they must be resolved at composition time. One of the ways a role consumer can resolve such a conflict is by providing its own implementation of the conflicted method name. The method could do whatever the programmer sees fit, but it is most sensible for this custom implementation to invoke the conflicting methods from the component roles.

For example, consider two roles that provide a validate method. A class can consume the summation of these two roles. Since the two component roles each attempted to contribute a validate method, validate is a conflicted method in the composite role. To resolve this conflict, a consuming class must define its own validate method. The class's validate method should generally call the validate method of both roles. However, the class needs some way to disambiguate the two roles' validate methods so that it can call each in turn. For this reason, roles support method aliasing. The consumer can alias each role's validate methods. Aliasing only adds a new name for a method; it does not change the name of any method. That means the original validate methods are still conflicting. The consuming class can provide a validate method to resolve this conflict, whose implementation calls each role's aliased validate.

# this "with" sums the two roles then applies one role to the class
with (
    'Role::Foo' => { alias => { validate => 'validate_foo' } },
    'Role::Bar' => { alias => { validate => 'validate_bar' } },
);

sub validate {
    my $self = shift;
    $self->validate_foo(@_);
    $self->validate_bar(@_);
}

While this may seem like a lot of effort, role design has very nice theoretical and practical properties. Unlike multiple inheritance and mixins, roles issue an error when there is an unresolved naming conflict. The programmer has good tools for resolving conflicts: method aliasing, method exclusion, and method overriding. In our above example, the consuming class could have excluded the validate method from one of the two roles so that there would be no conflict (since only one role is providing a method with that name). Finally, the consuming class could have overridden the conflicted validate method. The overriding implementation may not need to call the component roles' validate methods, perhaps obviating the need for aliasing. The example above used a combination of aliasing and overriding. All of these features are available even when there are no method conflicts, granting additional flexibility to roles.

The New Moose Warning

Ovid has written eleven articles on roles since March 4th (as of this posting). Most of them have praised roles and explained his happiness with how comprehensible his large class role space has become. However, in Not All Features Are Useful (Moose Roles), Ovid describes his frustration at spending a long time trying to discover an unintentional use of the method overriding feature. Which is silent.

Marcus Ramberg was the first to suggest the addition a warning to the Moose core when a class overrides a method from a role it consumes. This way we could push the "frontier" of the beloved collision detection a little further, into boundary between roles and classes. The warning (which was uncannily easy to implement, as if it had existed in a past Alces) would read like:

The Quux class has implicitly overridden the method (validate) from role Role::Foo. If this is intentional, please exclude the method from composition to silence this warning (see Moose::Cookbook::Roles::Recipe2)

I was one of the very few vocal proponents of this new warning. Suggesting to users that they explicitly list which methods they intended to exclude from composition would benefit maintainability. Other developers would not have to guess whether each method override was intentional or not; intention would be evident from the exclusions, or lack thereof. In a large, evolving system, accidental name collisions are bound to happen. I'd much prefer a compile-time warning over spending even ten minutes debugging why my application decided to start connecting to a database named "Shawn Moore".

However, many people were unhappy with this warning, because it complained about perfectly valid semantics. Classes overriding local role methods has been in the design of roles since they were conceived as traits by Smalltalk researchers. Due to limitations in Perl's builtin warning system, the warning was also difficult to disable for a particular scope.

chromatic in particular decried this new warning in his article The Value of a Warning. While I continue to disagree with a few of his specific points, the argument he made in a comment on Ovid's Well, Now I Know (Roles) article struck a chord.

The moment you throw a mandatory default warning on two of the four appropriate and specified uses of roles, you've penalized them. You're subtly encouraging people not to use the most important and most powerful features of roles! You're actively discouraging people from taking advantage of alloorphism using well-established and long-recommended design techniques explicitly made safer and more understandable by roles.

He's absolutely correct. Even subtly discouraging users from perfectly good designs is unacceptable. I reverted the warning.

The New Moose Critic

One of the suggested ideas was to leave the warning disabled by default. For a while I thought it was quite ridiculous to turn Perl's own warnings on in your class, but leave Moose's own warnings off. After all, we have grown as a community to love use warnings; it would be silly to repeat that default-off mistake.

The final section of chromatic's The Value of a Warning held the best compromise. I was dead set against warnings that were off by default, so I didn't give the ideas the thorough consideration they deserved.

Compare that to optional Perl::Critic warnings. If your project decides that a method length of more than 24 lines is a code smell, you can determine that yourself. If you decide that a specified, implemented, and tested feature of Perl 5 is troublesome (tie), you can disallow it yourself locally.

What we need is a set of Perl::Critic policies for Moose. Elliot Shank has already started a Perl::Critic::Moose for, among other things, ensuring (statically) that you make your classes immutable.

Moose stores plenty of data in its metaobjects, rich for plunder. Though nearly all of Perl::Critic performs static analysis, there is precedent for dynamic Perl::Critic policies. We could stand on the shoulders of the Perl::Critic giants so as to reuse and contribute to their configuration system, testing modules, and great community.

Some easily-implemented dynamic Moose policies (of varying severity) could highlight:

  • Classes overriding methods from locally consumed roles
  • Classes overriding methods from their superclasses without using method modifiers (including the override modifier)
  • Declaring builder methods without leading underscores
  • Leaving classes mutable
  • Declaring new methods instead of using the many object construction hooks

I think this is an excellent compromise and I can't wait to begin implementing Moose policies.