[PATCH] Warn when a conditional my() is used with an assignment

[p5pRT]

Migrated from rt.perl.org#122567 (status was 'open')

Searchable as RT122567$

[p5pRT]

From @lkundrak

The "Deprecated use of my() in false conditional" failed to trigger in case
there was an assignment (my $x = 123 if 0), silently creating "static"
variables without a warning. This aims to correct thit and adds a test case.

The only occassion in perl tree that triggers the warning is in Math​::BigFloat,
fix submitted in [rt.cpan.org #98185].

---

op.c | 21 +++++++++++++++++----
t/lib/warnings/op | 14 ++++++++++++++
2 files changed, 31 insertions(+), 4 deletions(-)

Inline Patch

diff --git a/op.c b/op.c
index f785c55..75abb50 100644
--- a/op.c
+++ b/op.c
@@ -6172,14 +6172,27 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
 	    return other;
 	}
 	else {
-	    /* check for C<my $x if 0>, or C<my($x,$y) if 0> */
+	    /* Check for abuse of my() in false conditional to create a state variable */
 	    const OP *o2 = other;
-	    if ( ! (o2->op_type == OP_LIST
+
+	    /* C<my ($x,$y) if 0> */
+	    if ( ! (((o2->op_type == OP_LIST ) ||
+		     /* C<my @a = (1,2,3) if 0>, C<my ($x,$y) = (1,2) if 0> */
+		     (o2->op_type == OP_AASSIGN && (( o2 = cBINOPx(o2)->op_last)))
+		    )
 		    && (( o2 = cUNOPx(o2)->op_first))
 		    && o2->op_type == OP_PUSHMARK
 		    && (( o2 = OP_SIBLING(o2))) )
-	    )
-		o2 = other;
+	    ) {
+		/* C<my $x = 123 if 0> */
+		if (! (o2->op_type == OP_SASSIGN
+		    && (( o2 = cBINOPx(o2)->op_last )) )
+		) {
+		    /* None of the above. Maybe just C<my $x if 0> */
+		    o2 = other;
+		}
+	    }
+
 	    if ((o2->op_type == OP_PADSV || o2->op_type == OP_PADAV
 			|| o2->op_type == OP_PADHV)
 		&& o2->op_private & OPpLVAL_INTRO
diff --git a/t/lib/warnings/op b/t/lib/warnings/op
index 33ee585..689ebff 100644
--- a/t/lib/warnings/op
+++ b/t/lib/warnings/op
@@ -1640,6 +1640,13 @@ my ($x4) if 0;
 my ($x5,@x6, %x7) if 0;
 0 && my $z1;
 0 && my (%z2);
+my $x1 = length("moo") if 0;
+my @x2 = (1, 2, 3) if 0;
+my %x3 = () if 0;
+my ($x4) = (666) if 0;
+my ($x5,@x6, %x7) = (0,1,length("moo")) if 0;
+0 && my $z1;
+0 && my (%z2);
 # these shouldn't warn
 our $x if 0;
 our $x unless 0;
@@ -1655,6 +1662,13 @@ Deprecated use of my() in false conditional at - line 5.
 Deprecated use of my() in false conditional at - line 6.
 Deprecated use of my() in false conditional at - line 7.
 Deprecated use of my() in false conditional at - line 8.
+Deprecated use of my() in false conditional at - line 9.
+Deprecated use of my() in false conditional at - line 10.
+Deprecated use of my() in false conditional at - line 11.
+Deprecated use of my() in false conditional at - line 12.
+Deprecated use of my() in false conditional at - line 13.
+Deprecated use of my() in false conditional at - line 14.
+Deprecated use of my() in false conditional at - line 15.
 ########
 # op.c
 $[ = 1;
-- 
2.0.4

[p5pRT]

From @iabyn

On Tue, Aug 19, 2014 at 01​:52​:59PM -0700, Lubomir Rintel wrote​:

The "Deprecated use of my() in false conditional" failed to trigger in case
there was an assignment (my $x = 123 if 0), silently creating "static"
variables without a warning. This aims to correct thit and adds a test case.

The only occassion in perl tree that triggers the warning is in Math​::BigFloat,

I'm in two minds about this. Note that the usage in Math​::BigFloat *is*
actually useful​:

  sub DEBUG () { 0; }

  my $steps = 0 if DEBUG;
  ...
  if (DEBUG) {
  $steps++; print "step $steps = $x\n" if $steps % 10 == 0;
  }

With this, the whole 'my $steps = 0' statement is optimised away
at compile time if DEBUG is set to 0. With your proposed patch to
Math​::BigFloat, a useless 'my $steps = 0' is executed each time.

On the other hand for this particular case, optimising away that
statement is probably indetectable in terms of performance improvement.

Also, I added the original 'my $foo if 0' warning, and if Math​::BigFloat
had been written as

  my $steps if DEBUG;
  ...
  $steps = 0 if DEBUG;

then it would have already warned under the existing regime, even though
the 'if DEBUG' is usefully optimising away an unneeded run-time op. So I'm
being a bit hypocritical here.

Possibly a compromise would be to only emit the warning if the /FOLD flag
is present on the constant?

So these would continue warning​:

  my $x if 0;
  my $x if DEBUG;

this would start to warn​:

  my $x = 0 if 0;

but this would continue not warning​:

  my $x = 0 if DEBUG;

--
The warp engines start playing up a bit, but seem to sort themselves out
after a while without any intervention from boy genius Wesley Crusher.
  -- Things That Never Happen in "Star Trek" #17

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'