Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
p5pRT opened this issue Aug 19, 2014 · 3 comments
Open

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

p5pRT opened this issue Aug 19, 2014 · 3 comments

Comments

@p5pRT
Copy link

p5pRT commented Aug 19, 2014

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

Searchable as RT122567$

@p5pRT
Copy link
Author

p5pRT commented Aug 19, 2014

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
Copy link
Author

p5pRT commented Aug 21, 2014

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
Copy link
Author

p5pRT commented Aug 21, 2014

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants