B::Deparse scoping problem with for loop #7390

p5pRT · 2004-06-28T08:39:55Z

Migrated from rt.perl.org#30504 (status was 'resolved')

Searchable as RT30504$

p5pRT · 2004-06-28T08:39:55Z

From T.E.Hofmann@gmx.de

Created by T.E.Hofmann@gmx.de

~ 111 > perl -le '$_="default" for my ($foo, $bar, $baz); print $foo'
default
~ 112 > perl -MO=Deparse -le '$_="default" for my ($foo, $bar, $baz); print $foo' | perl
-e syntax OK

~ 113 > perl -MO=Deparse -le '$_="default" for my ($foo, $bar, $baz); print $foo'
BEGIN { $/ = "\n"; $\ = "\n"; }
foreach $_ (my($foo, $bar, $baz)) {
$_ = 'default';
}
print $foo;
-e syntax OK

so B::Deparse gets the scoping wrong. This is similar (the same?) to ticket #22073.

Also see discussion at Perl Monks

http://www.perlmonks.com/index.pl?node_id=369512

where this originally came up in a conversation between tilly and runrig.

Perl Info


Flags:
    category=library
    severity=low

This perlbug was built using Perl v5.8.0 - Tue Oct  8 16:54:37 UTC 2002
It is being executed now by  Perl v5.8.0 - Tue Oct  8 16:45:50 UTC 2002.

Site configuration information for perl v5.8.0:

Configured by root at Tue Oct  8 16:45:50 UTC 2002.

Summary of my perl5 (revision 5.0 version 8 subversion 0) configuration:
  Platform:
    osname=linux, osvers=2.4.19, archname=i586-linux-thread-multi
    uname='linux amdsim5 2.4.19 #1 wed mar 27 13:57:05 utc 2002 i686 unknown '
    config_args='-ds -e -Dprefix=/usr -Dusethreads -Di_db -Di_dbm -Di_ndbm -Di_gdbm -Duseshrplib=true'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=define use5005threads=undef useithreads=define usemultiplicity=define
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=undef use64bitall=undef uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O3 --pipe',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing'
    ccversion='', gccversion='3.2', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=1234
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long', ivsize=4, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =''
    libpth=/lib /usr/lib /usr/local/lib
    libs=-lnsl -ldl -lm -lpthread -lc -lcrypt -lutil
    perllibs=-lnsl -ldl -lm -lpthread -lc -lcrypt -lutil
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.2.5'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-rdynamic -Wl,-rpath,/usr/lib/perl5/5.8.0/i586-linux-thread-multi/CORE'
    cccdlflags='-fPIC', lddlflags='-shared'

Locally applied patches:
    


@INC for perl v5.8.0:
    /usr/lib/perl5/5.8.0/i586-linux-thread-multi
    /usr/lib/perl5/5.8.0
    /usr/lib/perl5/site_perl/5.8.0/i586-linux-thread-multi
    /usr/lib/perl5/site_perl/5.8.0
    /usr/lib/perl5/site_perl
    .


Environment for perl v5.8.0:
    HOME=/home/tohofman
    LANG=de_DE@euro
    LANGUAGE (unset)
    LC_COLLATE=POSIX
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/tohofman/bin:/opt/kde3/bin:/opt/gnome/bin:/usr/games:/home/tohofman/bin:/usr/bin/X11:/usr/local/bin:/usr/bin:/usr/X11R6/bin:/bin:/usr/lib/java/jre/bin:.:.:/usr/lib/java/jre/bin:/opt/gnome/bin
    PERL_BADLANG (unset)
    SHELL=/bin/tcsh

p5pRT · 2004-06-29T01:27:51Z

From smcc@mit.edu

"TH" == Torsten Hofmann writes:

TH> ~ 111 > perl -le '$_="default" for my ($foo, $bar, $baz); print $foo'
TH> default
TH> ~ 112 > perl -MO=Deparse -le '$_="default" for my ($foo, $bar, $baz); print $foo' | perl
TH> -e syntax OK

TH> ~ 113 > perl -MO=Deparse -le '$_="default" for my ($foo, $bar, $baz); print $foo'
TH> BEGIN { $/ = "\n"; $\ = "\n"; }
TH> foreach $_ (my($foo, $bar, $baz)) {
TH> $_ = 'default';
TH> }
TH> print $foo;
TH> -e syntax OK

TH> so B::Deparse gets the scoping wrong. This is similar (the same?)
TH> to ticket #22073.

I don't think it's very closely related to #22037: "for (;;)" and
"foreach" loops are more similar in name than in implementation.
There, the scope is lost because the loop is thrown away; here, the
problem is that there never was a scope to begin with.

TH> Also see discussion at Perl Monks

TH> http://www.perlmonks.com/index.pl?node_id=369512

TH> where this originally came up in a conversation between tilly and
TH> runrig.

Interesting. My first reaction was that this "for my" construction
might best just be disallowed, since it's confusingly unsimilar to the
"for my $x (...) {...}' construction, and it seems perverse to declare
variables in the condition of a loop that would be inaccessible
there. But of course, someone's already recommending it as an idiom.

I think the right fix is to teach B::Deparse about the difference
between one-line for loops and real ones. I've attached a
proof-of-concept patch that shows how that might be done, though it
still has a problem with extra semicolons showing up. (On a tangential
connection to bug #22073, one-line for loops are another situation
where you can get two consecutive nextstates).

(The first hunk of the patch is totally unrelated, a note about
something else broken I noticed while testing the second hunk.)

-- Stephen

Inline Patch

--- ext/B/B/Deparse.pm.orig	2004-06-28 20:28:31.000000000 -0400
+++ ext/B/B/Deparse.pm	2004-06-28 21:01:54.000000000 -0400
@@ -1013,6 +1013,8 @@
 	and not $self->{'avoid_local'}{$$op}) {
 	my $our_local = ($op->private & OPpLVAL_INTRO) ? "local" : "our";
 	if( $our_local eq 'our' ) {
+	    # XXX This assertion fails code with non-ASCII identifiers,
+	    # like ./ext/Encode/t/jperl.t
 	    die "Unexpected our($text)\n" unless $text =~ /^\W(\w+::)*\w+\z/;
 	    $text =~ s/(\w+::)+//;
 	}
@@ -2514,8 +2516,13 @@
 	} elsif ($var->name eq "gv") {
 	    $var = "\$" . $self->deparse($var, 1);
 	}
-	$head = "foreach $var ($ary) ";
 	$body = $kid->first->first->sibling; # skip OP_AND and OP_ITER
+	if (!is_state $body->first and $body->first->name ne "stub") {
+	    confess unless $var eq '$_';
+	    $body = $body->first;
+	    return $self->deparse($body, 2) . " foreach ($ary)";
+	}
+	$head = "foreach $var ($ary) ";
     } elsif ($kid->name eq "null") { # while/until
 	$kid = $kid->first;
 	my $name = {"and" => "while", "or" => "until"}->{$kid->name};

p5pRT · 2004-06-29T01:27:52Z

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

p5pRT · 2004-06-29T10:30:28Z

From @demerphq

I think the right fix is to teach B::Deparse about the
difference between one-line for loops and real ones. I've
attached a proof-of-concept patch that shows how that might
be done, though it still has a problem with extra semicolons
showing up. (On a tangential connection to bug #22073,
one-line for loops are another situation where you can get
two consecutive nextstates).

(The first hunk of the patch is totally unrelated, a note
about something else broken I noticed while testing the second hunk.)

Just curious, but what about the case of:

my $x=0 if 0;

This is currently "optimized away" but the scoping effect on $x is not. And
thus it deparses as '???'

D:\Development>perl -MO=Deparse -e "use strict; my $x=1 if 0; $x||='boo';
print $x"
'???';
$x ||= 'boo';
print $x;
-e syntax OK

Which is also wrong I should think....

Yves

p5pRT · 2004-06-29T11:43:47Z

From @rgs

Orton, Yves wrote:

Just curious, but what about the case of:

my $x=0 if 0;

This is currently "optimized away" but the scoping effect on $x is not. And
thus it deparses as '???'

Currently B::Deparse has no easy way to detect this situation. (maybe
could it walk the pads for uninitialized entries, but that would be
heavy and error-prone.)

But "my $x if 0" is evil anyway :)

p5pRT · 2004-06-29T12:14:58Z

From @demerphq

Currently B::Deparse has no easy way to detect this situation. (maybe
could it walk the pads for uninitialized entries, but that would be
heavy and error-prone.)

But "my $x if 0" is evil anyway :)

Agreed. :-)

But the fact is that a lot of folks assume that the output from B::Deparse
is verbatim truth about what their code does and as such efforts should be
made to either document what constructs are known to be handled incorrectly,
or preferably fix them.

If the latter is too much effort for the payoff, such as for an evil
construct then I think it should at least be explicitly documented as not
being handled correctly. If only to settle the recurring endless debates and
discussions about whether B::Deparse is right or wrong, not to mention any
bug reports about something that the p5p crew has decided isnt worth the
effort.

Having said that I admit ive been too distracted with other things to review
exactly what the latest B::Deparse docs say, if this is already documented
then my apologies to all concerned.

Yves

p5pRT · 2004-06-29T12:34:45Z

From @rgs

Orton, Yves wrote:

Currently B::Deparse has no easy way to detect this situation. (maybe
could it walk the pads for uninitialized entries, but that would be
heavy and error-prone.)

But "my $x if 0" is evil anyway :)

Agreed. :-)

But the fact is that a lot of folks assume that the output from B::Deparse
is verbatim truth about what their code does and as such efforts should be
made to either document what constructs are known to be handled incorrectly,
or preferably fix them.

If the latter is too much effort for the payoff, such as for an evil
construct then I think it should at least be explicitly documented as not
being handled correctly. If only to settle the recurring endless debates and
discussions about whether B::Deparse is right or wrong, not to mention any
bug reports about something that the p5p crew has decided isnt worth the
effort.

Having said that I admit ive been too distracted with other things to review
exactly what the latest B::Deparse docs say, if this is already documented
then my apologies to all concerned.

Yes, it is, now :) (change #23010 to bleadperl, thanks)

p5pRT · 2004-06-29T20:27:35Z

@rspier - Status changed from 'open' to 'resolved'

p5pRT · 2004-06-29T21:47:23Z

From @iabyn

On Tue, Jun 29, 2004 at 01:17:57PM +0100, Orton, Yves wrote:

Currently B::Deparse has no easy way to detect this situation. (maybe
could it walk the pads for uninitialized entries, but that would be
heavy and error-prone.)

But "my $x if 0" is evil anyway :)

Agreed. :-)

Note also that in bleed you now get a warning for this construct:

$ perl591 -we 'my $x if 0'
Deprecated use of my() in false conditional at -e line 1.

--
"I do not resent critisism, even when, for the sake of emphasis,
it parts for the time with reality".
-- Winston Churchill, House of Commons, 22nd Jan 1941.

p5pRT · 2004-07-05T17:15:01Z

From @rgs

Stephen McCamant wrote:

I think the right fix is to teach B::Deparse about the difference
between one-line for loops and real ones. I've attached a
proof-of-concept patch that shows how that might be done, though it
still has a problem with extra semicolons showing up. (On a tangential
connection to bug #22073, one-line for loops are another situation
where you can get two consecutive nextstates).

Thanks, applied to bleadperl as #23046.

--- ext/B/B/Deparse.pm.orig 2004-06-28 20:28:31.000000000 -0400
+++ ext/B/B/Deparse.pm 2004-06-28 21:01:54.000000000 -0400

p5pRT · 2004-07-06T23:13:42Z

From at@altlinux.ru

On Mon, Jul 05, 2004 at 10:15:00AM -0700, Rafael Garcia-Suarez wrote:

Change 23046 by rgs@valis on 2004/07/05 16:53:54

Subject&#8203;: Re&#8203;: \[perl \#30504\] B&#8203;::Deparse scoping problem with for loop 
From&#8203;: Stephen McCamant \<smcc@&#8203;MIT\.EDU>
Date&#8203;: Mon\, 28 Jun 2004 18&#8203;:26&#8203;:24 \-0700
Message\-ID&#8203;: \<16608\.50496\.787002\.560481@&#8203;apocalypse\.OCF\.Berkeley\.EDU>

Affected files ...

... //depot/perl/ext/B/B/Deparse.pm#146 edit

Hello,
This change seems to break things that worked fine before:

While deparsing /home/at/tmp/perl-buildroot/usr/share/perl5/ExtUtils/MM_Unix.pm near line 1302,
at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 2521
B::Deparse::loop_common('B::Deparse=HASH(0x84a2200)', 'B::BINOP=SCALAR(0x8545fcc)', 0.5, '') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 2578
B::Deparse::pp_leaveloop('B::Deparse=HASH(0x84a2200)', 'B::BINOP=SCALAR(0x8545fcc)', 0.5) called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 789
B::Deparse::deparse('B::Deparse=HASH(0x84a2200)', 'B::BINOP=SCALAR(0x8545fcc)', 0.5) called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 1122
B::Deparse::lineseq('B::Deparse=HASH(0x84a2200)', 'undef', 'B::COP=SCALAR(0x851d1d0)', 'B::BINOP=SCALAR(0x85238f8)', 'B::COP=SCALAR(0x8556b60)', 'B::BINOP=SCALAR(0x8545ec4)', 'B::COP=SCALAR(0x804e94c)', 'B::BINOP=SCALAR(0x8545fcc)') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 850
B::Deparse::deparse_sub('B::Deparse=HASH(0x84a2200)', 'B::CV=SCALAR(0x81ad9ac)') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 348
B::Deparse::next_todo('B::Deparse=HASH(0x84a2200)') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 1351
B::Deparse::seq_subs('B::Deparse=HASH(0x84a2200)', 5318) called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 1340
B::Deparse::cop_subs('B::Deparse=HASH(0x84a2200)', 'B::COP=SCALAR(0x847f044)') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 1363
B::Deparse::pp_nextstate('B::Deparse=HASH(0x84a2200)', 'B::COP=SCALAR(0x847f044)', 0) called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 789
B::Deparse::deparse('B::Deparse=HASH(0x84a2200)', 'B::COP=SCALAR(0x847f044)', 0) called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 1192
B::Deparse::deparse_root('B::Deparse=HASH(0x84a2200)', 'B::LISTOP=SCALAR(0x847e674)') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 647
B::Deparse::__ANON__() called at (eval 1) line 31
O::CHECK() called at /home/at/tmp/perl-buildroot/usr/share/perl5/ExtUtils/MM_Unix.pm line 0
eval {...} called at /home/at/tmp/perl-buildroot/usr/share/perl5/ExtUtils/MM_Unix.pm line 0
CHECK failed--call queue aborted.

Differences ...

==== //depot/perl/ext/B/B/Deparse.pm#146 (text) ====
Index: perl/ext/B/B/Deparse.pm
--- perl/ext/B/B/Deparse.pm#145~~23042~~ Mon Jul 5 07:54:15 2004
+++ perl/ext/B/B/Deparse.pm Mon Jul 5 09:53:54 2004
@@ -1013,6 +1013,8 @@
and not $self->{'avoid_local'}{$$op}) {
my $our_local = ($op->private & OPpLVAL_INTRO) ? "local" : "our";
if( $our_local eq 'our' ) {
+ # XXX This assertion fails code with non-ASCII identifiers,
+ # like ./ext/Encode/t/jperl.t
die "Unexpected our($text)\n" unless $text =~ /^\W(\w+::)*\w+\z/;
$text =~ s/(\w+::)+//;
}
@@ -2514,8 +2516,13 @@
} elsif ($var->name eq "gv") {
$var = "\$" . $self->deparse($var, 1);
}
- $head = "foreach $var ($ary) ";
$body = $kid->first->first->sibling; # skip OP_AND and OP_ITER
+ if (!is_state $body->first and $body->first->name ne "stub") {
+ confess unless $var eq '$_';
+ $body = $body->first;
+ return $self->deparse($body, 2) . " foreach ($ary)";
+ }
+ $head = "foreach $var ($ary) ";
} elsif ($kid->name eq "null") { # while/until
$kid = $kid->first;
my $name = {"and" => "while", "or" => "until"}->{$kid->name};
End of Patch.

p5pRT · 2004-07-07T04:55:58Z

From smcc@mit.edu

"AT" == Alexey Tourbin <at@altlinux.ru> writes:

AT> On Mon, Jul 05, 2004 at 10:15:00AM -0700, Rafael Garcia-Suarez wrote:
P4> Change 23046 by rgs@valis on 2004/07/05 16:53:54
P4>
P4> Subject: Re: [perl #30504] B::Deparse scoping problem with for loop
P4> From: Stephen McCamant <smcc@MIT.EDU>
P4> Date: Mon, 28 Jun 2004 18:26:24 -0700
P4> Message-ID: <16608.50496.787002.560481@apocalypse.OCF.Berkeley.EDU>
P4>
P4> Affected files ...
P4>
P4> ... //depot/perl/ext/B/B/Deparse.pm#146 edit

AT> Hello,
AT> This change seems to break things that worked fine before:

AT> While deparsing /home/at/tmp/perl-buildroot/usr/share/perl5/ExtUtils/MM_Unix.pm near line 1302,
AT> at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 2521
AT> B::Deparse::loop_common('B::Deparse=HASH(0x84a2200)', 'B::BINOP=SCALAR(0x8545fcc)', 0.5, '') called at /home/at/tmp/perl-buildroot/usr/lib/perl5/B/Deparse.pm line 2578
AT> [...]

Indeed. The confession is essentially an assertion failure, saying
that something the code heuristically guessed to be a one-line for
turned out to have a variable other than $_, indicating a bug with the
heuristic. In particular, I think the problem is foreach loops with
continue blocks; try the following smaller test case:

% ./perl -Ilib -MO=Deparse -e 'for $x (0) { $x } continue { $y }'

The continue block gives the body of the loop a different shape,
confusing the test. I'll try to work on an improved patch tomorrow.

Two meta-level notes:

* I'm ambivalent about this sort of half-baked change going into
bleadperl. On one hand, it inconveniences people and makes me feel
bad to be responsible for buggy code. On the other hand, it's a way
of getting the code tested in a way it wouldn't otherwise. I trust
that the people applying my patches are also aware of this tradeoff,
and weighing the choices accordingly.

* B::Deparse really needs better regression tests: more extensive than
the spot checks of deparse.t, but faster and without the many known
failures of "make test.deparse". There are a variety of different
ways this might be achieved; something I'll try to think about if I
can find a larger block of free time.

-- Stephen

p5pRT · 2004-07-07T10:11:35Z

From @nwc10

On Tue, Jul 06, 2004 at 09:55:10PM -0700, Stephen McCamant wrote:

* I'm ambivalent about this sort of half-baked change going into
bleadperl. On one hand, it inconveniences people and makes me feel
bad to be responsible for buggy code. On the other hand, it's a way
of getting the code tested in a way it wouldn't otherwise. I trust
that the people applying my patches are also aware of this tradeoff,
and weighing the choices accordingly.

If experimental stuff isn't the point of blead, what is?

Not enough people are using blead with real programs, which means that
there's not enough feedback about what experimental changes didn't actually
work. Without confidence that proposed improvements are safe, it's
increasingly unlikely that they'll ever get into a released stable version
(in the short term 5.8.x, and in the longer term 5.10, given that releasing
a 5.10 *at all* means that the code base has to become stabilised)

For example perl 5.8.5 contains bug fixes for 2 optimisations put into 5.8.4
that didn't tickle any bugs while in blead, hence seemed well tested and
safe. This is embarrassing, and I don't want it to happen again. If the
community in general, and the subscribers to p5p in particular don't start
testing development code on blead, then the safest way to ensure that 5.8.6
etc are stable is not to include any non-bugfixes. Which will reduce the
motivation for perl development, stagnate 5.8.x and unchecked would postpone
5.10 indefinitely.

Nicholas Clark

p5pRT closed this as completed Jun 29, 2004

p5pRT added Severity Low distro-Linux type-library labels Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

B::Deparse scoping problem with for loop #7390

B::Deparse scoping problem with for loop #7390

p5pRT commented Jun 28, 2004

p5pRT commented Jun 28, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

p5pRT commented Jul 5, 2004

p5pRT commented Jul 6, 2004

p5pRT commented Jul 7, 2004

p5pRT commented Jul 7, 2004

B::Deparse scoping problem with for loop #7390

B::Deparse scoping problem with for loop #7390

Comments

p5pRT commented Jun 28, 2004

p5pRT commented Jun 28, 2004

From T.E.Hofmann@gmx.de

Created by T.E.Hofmann@gmx.de

p5pRT commented Jun 29, 2004

From smcc@mit.edu

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

From @demerphq

p5pRT commented Jun 29, 2004

From @rgs

p5pRT commented Jun 29, 2004

From @demerphq

p5pRT commented Jun 29, 2004

From @rgs

p5pRT commented Jun 29, 2004

p5pRT commented Jun 29, 2004

From @iabyn

p5pRT commented Jul 5, 2004

From @rgs

p5pRT commented Jul 6, 2004

From at@altlinux.ru

p5pRT commented Jul 7, 2004

From smcc@mit.edu

p5pRT commented Jul 7, 2004

From @nwc10