I love this question because it identifies a problem that is a common pitfall. I've even discovered it in the past creeping into code at work.
Look at this code:
sub foo {
return $bar or $baz
}
Pass that through B::Deparse:
perl -MO=Deparse -e 'sub foo {return $bar or $baz}'
sub foo {
$baz unless return $bar;
}
The code return $bar has to be executed before its value can be calculated for the unless condition. But return exits the subroutine, and leaves "unless" hanging. Control passes out of the sub. Game over for the unless statement. It never gets an opportunity to regain control of flow, so $baz could never be hit.
We could run very similar code through Deparse using the || operator:
perl -MO=Deparse -e 'sub foo {return $bar || $baz}'
sub foo {
return $bar || $baz;
}
Here we find none of the surprising behavior, the expression $bar || $baz has to get evaluated first, then the return happens.
The return could be coerced to work "as you wish" by using parenthesis, too:
sub foo {
return ($bar or $baz);
}
We can pass that through B::Deparse as well:
perl -MO=Deparse -e 'sub foo {return ($bar or $baz)}'
sub foo {
return $bar || $baz;
}
Look at that: It parsed down to the || operator.
We had another one pop up in some code a year or so ago. It's not exactly the same, but in a way similar:
sub foo {
my $filename = shift;
my $content = do {
local $/;
open my $fh, '<', $filename or die $!;
return scalar(<$fh>);
}
$content = s/abc/def/g;
return $content;
}
Spot the problem? You shouldn't use return in a do block unless you actually intend to return out of the surrounding subroutine. The writer of that code knew that do blocks return something to their caller. So they used a return to make it explicit (rather than falling back to the last thing that got evaluated in the do block). But that's incorrect; the return returned out of the foo subroutine, leaving the assignment to $content hanging, and the remainder of the subroutine unreachable. In many of the cases I spotted of this, it wasn't apparent what was happening because the caller of foo() still got a return value that seemed reasonable. I think in the actual case where I saw it, the context was poor-man's caching:
sub get_foo {
my $self = shift;
return $self->{'foo'} ||= do {
# expensive operation;
return $result;
}
}
Here the biggest clue there was a problem was that the call to that accessor took longer than it should on any call after the first time it was called. That's rather hard to test for, though a test that breaks encapsulation and looks at the object's guts would have noticed.
| 