kareila: Taking refuge from falling debris under a computer desk. (computercrash)
kareila ([personal profile] kareila) wrote in [site community profile] dw_dev2010-01-12 07:45 am
Entry tags:

the hazards of coding for webservers vs. shell scripts

When I was doing some code cleanup a few months ago, I saw some lines that looked like this:

my $foo;
$foo = $bar unless $baz;


I decided that looked redundant and changed it to:

my $foo = $bar unless $baz;

Take my advice: don't combine "my" statements and postfix conditionals. Ever.

I've updated the programming guidelines to warn people away from this as well.

The manifestation of this particular bug was to cause the $foo variable to be referenced in global scope if the conditional was false. In that situation, the assigned value would persist across execution threads, such that if two people in a row had a true $baz, the second user would see the first user's value of $foo. Even with use warnings and use strict in effect, Perl apparently thinks this is perfectly fine.

ETA: [personal profile] exor674 says this command will find it (but not in BML files):

perlcritic --single-policy ProhibitConditionalDeclarations
janinedog: (Default)

[personal profile] janinedog 2010-01-12 04:29 pm (UTC)(link)
Weird, I'd never heard of this. I assume something like this is okay?

my $foo = $baz ? $woo : $bar;
exor674: Computer Science is my girlfriend (Default)

[personal profile] exor674 2010-01-12 07:11 pm (UTC)(link)
Yeah, that's totally fine.
afuna: Cat under a blanket. Text: "Cats are just little people with Fur and Fangs" (Default)

[personal profile] afuna 2010-01-13 12:29 am (UTC)(link)
Yup, that works fine. I think because in that, the my isn't conditional.

It's the difference between:

unless ( $baz ) {
    my $foo = $bar;
}


And

my $foo;

if ( $baz ) {
  $foo = $woo;
} else {
  $foo = $bar;
}
Edited (fix negation, add line) 2010-01-13 00:30 (UTC)
janinedog: (Default)

[personal profile] janinedog 2010-01-13 12:36 am (UTC)(link)
Interesting. I know that I never wrote stuff like "my $foo = $bar unless $baz", but that was simply because it's not clear what $foo is in the case that it's not $bar (undef? or does it just not exist at all? or something else?). And I like my code to be clear, which is why I generally use a ternary if I want to do a simple assignment on one line.

I never realized it actually causes problems like this, though!
alierak: (Default)

[personal profile] alierak 2010-01-18 07:56 pm (UTC)(link)
For the sake of completeness: take either of those examples and follow it with "# stuff that uses $foo" and you'll see why the "my" is silently ignored in the first one. If $foo were localized to the conditional, it would go out of scope before you ever got to use it.

The sort of pathological case we need to prevent looks like this:

my $foo = $bar unless $baz;
# stuff that uses $foo without checking $baz
exor674: Computer Science is my girlfriend (Default)

[personal profile] exor674 2010-01-12 07:12 pm (UTC)(link)
technically, you need to do:

perlcritic --single-policy ProhibitConditionalDeclarations cgi-bin/*

(or any other directory you wanna run over)
jadelennox: Senora Sabasa Garcia, by Goya (Default)

[personal profile] jadelennox 2010-01-12 07:38 pm (UTC)(link)
Huh. Nice catch.

Conway's Perl Best Practices' general reaction to postfix conditionals in any circumstance is STAB STAB STABBITY STAB, which is why perlcritic can be set to warn against them, but I know this is direct opposition to our coding guidelines.
vlion: cut of the flammarion woodcut, colored (Default)

[personal profile] vlion 2010-01-12 09:45 pm (UTC)(link)
I've usually found postfix conditions to wind up being more of a pain than otherwise; they tend to get factored into prefix conditionals.
sophie: A cartoon-like representation of a girl standing on a hill, with brown hair, blue eyes, a flowery top, and blue skirt. ☀ (Default)

[personal profile] sophie 2010-01-18 05:09 pm (UTC)(link)
I must admit, I'm mostly the same; I also don't tend to use unless at all, as my brain prefers consistency; I can deal with negated ifs.

But of course I stick to the coding guidelines for DW.
vlion: cut of the flammarion woodcut, colored (Default)

[personal profile] vlion 2010-01-12 09:46 pm (UTC)(link)
Awkward.

Good to know though. I'm somewhat surprised that it persists across threads though.
exor674: Computer Science is my girlfriend (Default)

[personal profile] exor674 2010-01-12 09:49 pm (UTC)(link)
It doesn't persist across threads, it persists across requests as long as two users get the same Apache process.
thorfinn: <user name="seedy_girl"> and <user name="thorfinn"> (Default)

[personal profile] thorfinn 2010-01-13 02:30 am (UTC)(link)
I actually highly recommend adopting perlcritic must pass with no warnings (as a simpler to validate version of "must obey all of the rules in Damian Conway's Perl Best Practices book") and perltidy with Damian Conway's .perltidyrc outright as enforced checkin on Perl code.

There are entirely good reasons to avoid some of the rules in perlcritic, yes. The thing is, it is a whole lot simpler and easier to accept the Conway Perl::Critic ruleset wholesale, than it is to try and go around making individual exceptions.

It's almost certainly not worth going around and changing the entire existing codebase, but it is something that can be done on new checkins.

Alternately, it's possible to set up a .perlcriticrc and .perltidyrc to suit the DW coding standard, and then pass that around.

Perltidy in particular saves a lot of manual inspection for formatting standards and Perl::Critic similarly for coding style standards, especially if you turn on the option that prints the explanation from the book for any rule failures.

[personal profile] andychilton 2010-01-20 11:58 am (UTC)(link)
I like using conditionals but have never used them on a declaration. Sometimes I just think things are too important to be hidden away somewhere and declarations deserve to have their own line (hence I haven't ever been bitten by this bug) :)