inheritance turns back and bites

[dada]

can you tell me, looking at the following code:

1. what is wrong
2. the most elegant way to fix it
3. what can be done to avoid similar errors

UPDATE: this is bad code, I know. it's just a completely fake example I made up for this post. it's also deliberately contrived, just to make the real problem less apparent (and where's the fun, otherwise? :-). you have been warned. don't do this at home.

package SomeData;

# UPDATE: the first version of "new" doesn't seem to 
# work on some platform/version combination (and it's 
# way too evil anyway). but since this is not the main 
# issue, I will use a more "sane" version :-)

# sub new { bless \pop, shift }
sub new { my $self = pop; bless \$self, shift }

sub value { ${shift;} }

sub add {
    my($self, $type, $value) = @_;
    if($type eq 'number') { return $$self += $value; }
    if($type eq 'string') { return $$self .= $value; }
    die "only string or number allowed";
}

sub increment {
    my $self = shift;
    $self->add(number => 1);
}

package MyNumber;

use base qw( SomeData );

sub add {
    my($self, $value) = @_;
    die "not a number" unless $value =~ /^\d+$/;
    $$self += $value;
}

sub subtract {
    my($self, $value) = @_;
    die "not a number" unless $value =~ /^\d+$/;
    $$self -= $value;
}

package main;

my $answer = MyNumber->new(42);
print $answer->value(), "\n";

$answer->subtract(1);
print $answer->value(), "\n";

$answer->increment();
print $answer->value(), "\n";

[borisz]

you expect that increment call SomeData::add, but it calls MyNumber::add. Because it is a MyNumber object. If you do not want that, write in package SomeData

sub increment {
    my $self = shift;
    $self->SomeData::add(number => 1);
}

[esskar]

Always use SUPER instead of the parent-class-name; so in the case of changeing the parent-class, you won't have to replace all the "SomeData::" stuff to the new parent-class-name.
SUPER is pretty super.

[borisz]

But SUPER is wrong here, I like to use SomeData or build something with __PACKAGE__.

[broquaint]

1. The $answer object is using its own method instead of SomeData's method
2. Use Class::MultiMethods, or less elegantly

   sub add {
     @_ > 2 && return $_[0]->SUPER::add(@_[1 .. $#_]);
     ...
   }
   

3. Don't use parameter based dispatch in perl ;)

[Abigail-II]

Well, you can't modify constants. Your problem isn't any different than:

    bless \42, "whatever";

or

    sub foo {$_ [0] += 10}
    foo 42;

Furthermore, the use of bless \pop, shift; is questionable, as there's no order of evaluation defined for the comma in list context.

Abigail

[ysth]

That's what I thought at first glance, but after a moment realized that pop, shift on a 2+ element array is going to get the last and first elements regardless of order of evaluation.

Nevertheless, it's a bad way to do it, since if the method call is given no parameters, the bless will default to package "main" (with a warning).

[dragonchild]

For the specific question, a better option might be to use overload and do dispatch based on the value you get. Something like:

package SomeData;

use overload
    '+' => 'add',
    '-' => 'subtract',
        fallback => 1;

sub add
{
    my ($x, $y) = @_[0..1];
    ($x, $y) = ($y, $x) if $_[2];

    $self->{is_number}
        ? $x + $y
        : $x . $y;
}

Now, personally, I would never overload + to act as string concatenation. Concatenation isn't commutative, which breaks the implicit contract most people have with addition. As well, how do you define multiplication for strings?

[tinita]

as others said already, in MyNumber, you have implemented the method add different from the one in SomeData. you have to decide which one you want.

also, in the new method of SomeData, you have a little error. if you do
sub new { bless \pop, shift }
it's like doing
sub new { bless \42, "MyNumber" }
and you can't bless a constant into an object. so do my $arg = pop; bless \$arg, shift

[dada]

it's like doing sub new { bless \42, "MyNumber" }

no, it's like doing:

sub new { bless $_[-1], $_[0] }

which I can do. arguably a good idea, but I can. and in fact, that part works, at least in my perl interpreter. (note: the code is deliberately "strange" just to hide a bit the real problem :-).

[tinita]

hmm, see Re: Re: Re: inheritance turns back and bites

[borisz]

sub new { bless \pop, shift } it's like doing sub new { bless \42, "MyNumber" } and you can't bless a constant into an object. so do my $arg = pop; bless \$arg, shift

I do not think this is wrong, since \pop is like \pop @_ and whatever I pop it is pushed and a pushed constant is a scalar.

[tinita]

sorry, but:


perl -wle'
sub new { bless \pop, shift }
my $main = new ("ClassName", 42);'
Modification of a read-only value attempted at -e line 2.

This is perl, v5.6.1 built for i586-linux
This is perl, v5.8.0 built for i686-linux


am i overlooking something?

Update: same result with
This is perl, v5.8.2 built for i686-linux

[John M. Dlugosz]

Nobody seems to have addressed the actual question, about the underlying reason for this being a mistake and how to avoid it.

Basically, you used the same method name in a derived class as is used in the base class, and that accidently overrides a function that just happens to have the same name but is used for a different purpose, takes different parameters, etc.

The first thing that comes to mind is to make sure all members are documented or follow a naming convention for "internal use" items. But, you can still hit that problem with an internal-use function! All methods that are called using the virtual dispatch mechanism, even those that are private, must be documented so a writer of a derived class is aware of them and will not re-use those names.

—John

[stvn]

Disclaimer
Sorry, but first i have to rant. This is not nessecarily 100% about your code, but about a particular Perl-OO idiom I see often on this site.

Maybe, I'm a prude, but I just don't understand how anyone could think that this idiom:

# sub new { bless \pop, shift }
sub new { my $self = pop; bless \$self, shift }

is good OO. It completely externalizes the internals of the object in the worst way by allowing the caller of "new" to essentially create them. It does no form of parameter checking at all, and is lazy in the worst way (creates more problems later on, instead of solves them).

Your code breaks in silently if I do any of these things:

MyNumber->new([ 0 .. 100 ]);
MyNumber->new({ test => 1});
MyNumber->new(*STDOUT);

Now of course, these are "unrealistic" examples because your classname and documentation would indicate doing such things were just plain stupid. But, in a larger program (especially with a number of developers) variables may get trampled on, or return values are changed, or any other such issue. If the eventual argument to your constructor is just such a vicitim, your object willingly accepts it without complaint. And when your code suddenly die's (possibly) far away from where the actual bug was created, you may waste hours tracking it down.

Now onto your questions:

1. what is wrong
2. the most elegant way to fix it
3. what can be done to avoid similar errors

Question "a" has already been answered, its because "increment" is using "add" from MyNumber and not from SomeData.

Question "b": My suggestions is to not try and combine string handling and number handling into the same base class. Meaning that SomeData doesn't make alot of sense to me. Especially when you later derive MyNumber from it, and then change the number and type of parameters accepted by "add". This creates a very confusing issue/problem/bug in that MyNumber::add no longer acts the same as SomeData::add and therefore changes the interface.

In a more static language like Java or C# these two methods would be considered different since their method signatures are different. In those langauges your code would work correctly as the call to "add" in "increment" would dispatch correctly to the two argument "add". But alas, this is perl, so I would suggest changing your class design to be more approriate to perls ways and means.

Also, the concept of "increment" is clearly not appropriate to a string. I realize that you specifically call $self->add(number => 1) in the code, but it would seem to me that SomeData is intended to be a string/number handling hybrid base class of some kind, and that implies to me that SomeData should only implement methods appropriate to the both of strings and numbers.

Question "c": see answer to question "b".

-stvn

[dada]

time to clarify things up :-)

I would like to thank everyone that answered to this rather silly post, and took part in a sort of bad joke of mine.

I had prepared a very different kind of posting, talking about inheritance, interfaces and code maintenance, but in the end I decided to try to be thought-provoking and so posed the question as a sort of quiz. and I'm sorry if this caused more harm than good :-).

almost everybody spotted the error in redefining the interface (or the signature, if you prefer to think in terms of it) for a method in a derived class.

the rule I wanted to point out, which in Perl (due to its typeless and prototypeless nature, that we all sometimes love) may easily go overlooked (I, for one, didn't had it so clear in mind before stepping on it):

Don't change the interface of methods you overload from a base class (that is, you can do things differently internally, but both input and output should be consistent with the original method) or inheritance will turn back and bite you.

and that's my exposition of point c. regarding point b (a way to fix this), many (here and in other forums) suggested checking the number, and possibly the nature, of parameters in MyNumber::add, as per broquaint's:

@_ > 2 && return $_[0]->SUPER::add(@_[1..$#_]);

this has the disadvantage that one can still write: $answer->add(string => 'xxx'), and this may very well be what the author of MyNumber wanted to avoid.

one could check the value of the second parameter, and die unless it is number, but this seems rather kludgy.

a slightly less inelegant way to fix this problem could be for example:

sub add {
    # check if the Big Boss is calling us...
    if( (caller(1))[3] =~ /^SomeData::/) {
        # we feel guilty, so let's hide
    return shift->SUPER::add(@_);
    }
    # ...back to our scheduled job
    my($self, $value) = @_;
    die "not a number" unless $value =~ /^\d+$/;
    $$self += $value;
}

but still broquaint's hint on Class::Multimethods wins this thread's elegance award.

as a final note, I will also unveil where really I encountered the problem (so that you can finally forget about my broken example :-). I was taking a look at Net::ICal for a project of mine, and I found that Net::ICal::Time, which is intended to be just a wrapper class for Date::ICal, defines a method:

add($duration)

but Date::ICal has an add method, which has this synopsis (as well as others):

add(duration => $duration)

and that's exactly our point a. the code in Net::ICal::Time::add ends with:

return $self->SUPER::add(duration=>$duration);

but as we saw, this is not enough to be safe.

to further underline how sneaky this bug can be, I will also add this: the code in Date::ICal::new, which Net::ICal::Time delegates to, calls offset, which in turns calls add (which is really Net::ICal::Time::add) only when it has to do some adjustment to account for timezones.

and that's all for today. I hope you found all this somewhat entertaining :-)