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

return 0 or die; #9523

Closed
p5pRT opened this issue Oct 11, 2008 · 72 comments
Closed

return 0 or die; #9523

p5pRT opened this issue Oct 11, 2008 · 72 comments

Comments

@p5pRT
Copy link

p5pRT commented Oct 11, 2008

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

Searchable as RT59802$

@p5pRT
Copy link
Author

p5pRT commented Oct 11, 2008

From rvtol@xs4all.nl

Created by rvtol@xs4all.nl

  C<return doit() or die "did it wrong";>

  Would be nice if that warns "unreachable statement".

Perl Info

Flags:
    category=core
    severity=low

Site configuration information for perl v5.8.6:

Configured by rj at Thu Feb  3 20:42:58 CET 2005.

Summary of my perl5 (revision 5 version 8 subversion 6) configuration:
  Platform:
    osname=freebsd, osvers=4.10-release-p2, archname=i386-freebsd-64int
    uname='freebsd xs0.xs4all.nl 4.10-release-p2 freebsd 4.10-release-p2 #1: thu aug 12 12:43:13 cest 2004 kai@xs0.xs4all.nl:usrobjusrsrcsysgeneric i386 '
    config_args='-sde -Dprefix=/usr/local -Darchlib=/usr/local/lib/perl5/5.8.6/mach -Dprivlib=/usr/local/lib/perl5/5.8.6 -Dman3dir=/usr/local/lib/perl5/5.8.6/perl/man/man3 -Dman1dir=/usr/local/man/man1 -Dsitearch=/usr/local/lib/perl5/site_perl/5.8.6/mach -Dsitelib=/usr/local/lib/perl5/site_perl/5.8.6 -Dscriptdir=/usr/local/bin -Dsiteman3dir=/usr/local/lib/perl5/5.8.6/man/man3 -Dsiteman1dir=/usr/local/man/man1 -Ui_malloc -Ui_iconv -Uinstallusrbinperl -Dcc=cc -Doptimize=-O -pipe  -Duseshrplib -Dccflags=-DAPPLLIB_EXP="/usr/local/lib/perl5/5.8.6/BSDPAN" -Ud_dosuid -Ui_gdbm -Dusethreads=n -Dusemymalloc=y -Duse64bitint'
    hint=recommended, useposix=true, d_sigaction=define
    usethreads=undef use5005threads=undef useithreads=undef usemultiplicity=undef
    useperlio=define d_sfio=undef uselargefiles=define usesocks=undef
    use64bitint=define use64bitall=undef uselongdouble=undef
    usemymalloc=y, bincompat5005=undef
  Compiler:
    cc='cc', ccflags ='-DAPPLLIB_EXP="/usr/local/lib/perl5/5.8.6/BSDPAN" -DHAS_FPSETMASK -DHAS_FLOATINGPOINT_H -fno-strict-aliasing -pipe -I/usr/local/include',
    optimize='-O -pipe ',
    cppflags='-DAPPLLIB_EXP="/usr/local/lib/perl5/5.8.6/BSDPAN" -DHAS_FPSETMASK -DHAS_FLOATINGPOINT_H -fno-strict-aliasing -pipe -I/usr/local/include'
    ccversion='', gccversion='2.95.4 20020320 [FreeBSD]', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='cc', ldflags =' -Wl,-E -L/usr/local/lib'
    libpth=/usr/lib /usr/local/lib
    libs=-lgdbm -lm -lcrypt -lutil
    perllibs=-lm -lcrypt -lutil
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='  -Wl,-R/usr/local/lib/perl5/5.8.6/mach/CORE'
    cccdlflags='-DPIC -fPIC', lddlflags='-shared  -L/usr/local/lib'

Locally applied patches:
    SUIDPERLIO0 - fix PERLIO_DEBUG local root exploit (CAN-2005-0155)
    SUIDPERLIO1 - fix PERLIO_DEBUG buffer overflow (CAN-2005-0156)


@INC for perl v5.8.6:
    /home/r/rvtol/.cpan/lib/i386-freebsd-64int
    /home/r/rvtol/.cpan/lib
    /usr/local/lib/perl5/site_perl/5.8.6/mach
    /usr/local/lib/perl5/site_perl/5.8.6
    /usr/local/lib/perl5/site_perl
    /usr/local/lib/perl5/5.8.6/BSDPAN
    /usr/local/lib/perl5/5.8.6/mach
    /usr/local/lib/perl5/5.8.6
    .


Environment for perl v5.8.6:
    HOME=/home/r/rvtol
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH=/home/r/rvtol/bin/rrd/lib
    LOGDIR (unset)
    PATH=/home/r/rvtol/bin:/usr/local/bin:/bin:/usr/bin:/usr/games:.
    PERL5LIB=:/home/r/rvtol/.cpan/lib
    PERL_BADLANG (unset)
    SHELL=/usr/local/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2008

From @druud62

"rvtol@​xs4all.nl (via RT)" schreef​:

return doit() or die "did it wrong";

Would be nice if that warns "unreachable statement".

Another variant of "unreachable"​:

  perl -wle 'my $x = <>; $x++ or die; print $x'
  perl -wle 'my $x = <>; $x-- or die; print $x'

(am not sure about the relation yet)

--
Affijn, Ruud

"Gewoon is een tijger."

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2008

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

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2008

From Eirik-Berg.Hanssen@allverden.no

"Dr.Ruud" <rvtol+news@​isolution.nl> writes​:

"rvtol@​xs4all.nl (via RT)" schreef​:

return doit() or die "did it wrong";

Would be nice if that warns "unreachable statement".

Another variant of "unreachable"​:

perl -wle 'my $x = <>; $x++ or die; print $x'
perl -wle 'my $x = <>; $x-- or die; print $x'

  How so unreachable?

echo -n | perl -wle 'my $x = <>; $x++ or die; print $x'
echo -n 0 | perl -wle 'my $x = <>; $x++ or die; print $x'
echo 0 | perl -wle 'my $x = <>; $x++ or die; print $x'
echo -n | perl -wle 'my $x = <>; $x-- or die; print $x'
echo -n 0 | perl -wle 'my $x = <>; $x-- or die; print $x'
echo 0 | perl -wle 'my $x = <>; $x-- or die; print $x'

Died at -e line 1.
Died at -e line 1, <> line 1.
1
Died at -e line 1.
Died at -e line 1, <> line 1.
-1

(am not sure about the relation yet)

  (?)

Eirik
--
You just paddle around there awhile, and I'll explain about these poles ...
  -- Sally Brown
Is that in Europe?
  -- Joyce Brown

@p5pRT
Copy link
Author

p5pRT commented Oct 12, 2008

From @druud62

Eirik Berg Hanssen schreef​:

Dr.Ruud​:

Another variant of "unreachable"​:

perl -wle 'my $x = <>; $x++ or die; print $x'
perl -wle 'my $x = <>; $x-- or die; print $x'

How so unreachable?

Oops, please ignore. (forgot to chomp my test values)

--
Affijn, Ruud

"Gewoon is een tijger."

@p5pRT
Copy link
Author

p5pRT commented Oct 21, 2008

From @druud62

"rvtol(AT)xs4all.nl (via RT)" schreef​:

C<return doit() or die "did it wrong";>

Would be nice if that warns "unreachable statement".

Some examples can be seen here​:
  http​://www.google.com/codesearch?q=+lang​:perl+\+return\+.*\+or\+die
  http​://www.google.com/codesearch?q=lang​:perl+return\+\S%2B\s%2Bor

There is one in​:

http​://search.cpan.org/src/GRICHTER/ExtUtils-XSBuilder-0.28/XSBuilder/ParseSource.pm

A multi-line example​:
  http​://search.cpan.org/~markf/Attempt-1.01/lib/Sub/Attempts.pm

--
Affijn, Ruud

"Gewoon is een tijger."

@p5pRT
Copy link
Author

p5pRT commented Oct 23, 2008

From @davidnicol

this one looks like a typical offender​:

<<COPIED
BatchSystem-SBS-0.32/lib/BatchSystem/SBS/Common.pm - 3 identical

  88​: if($simpleLocker){
  return $simpleLocker->trylock($f) or CORE​::die "cannot
lock [$f]​: $!";
  }else{

  97​: if($simpleLocker){
  return $simpleLocker->unlock($f) or CORE​::die "cannot
lock [$f]​: $!";
  }else{

www.cpan.org/authors/id/A/AL/ALEXMASS/BatchSystem-SBS-0.32.tar.gz
COPIED

  Refusing to allow return in non-void contexts would also disallow
legitmate uses such as

  (wearedone and return) or domore;

but how legit is that?

wearedone and return;
domore;

would be a non-violating rewrite.

Alternately, dropping the precedence of return would make C<return
something or die> do what the coder wants.

a low-precedence return would surely expose some bugs in currently
unreachable code, would it break anything that is written well?

On Tue, Oct 21, 2008 at 6​:19 PM, Dr.Ruud <rvtol+news@​isolution.nl> wrote​:

Would be nice if that warns "unreachable statement".

Some examples can be seen here​:
http​://www.google.com/codesearch?q=+lang​:perl+\+return\+.*\+or\+die

@p5pRT
Copy link
Author

p5pRT commented Oct 24, 2008

From olav@genebio.com

Hello David,

I do not understand the problem.,,

That's true that I was supposing the precedence of the return.

I slightly modified the code as
  $simpleLocker->trylock($f) and return 1;
  die "cannot lock [$f]​: $!";

would that make this piece of code more secure?
to what extent?
cheers
Alex

David Nicol wrote​:

this one looks like a typical offender​:

<<COPIED
BatchSystem-SBS-0.32/lib/BatchSystem/SBS/Common.pm - 3 identical

88&#8203;:   if\($simpleLocker\)\{
        return $simpleLocker\->trylock\($f\) or CORE&#8203;::die  "cannot

lock [$f]​: $!";
}else{

wearedone and return;
domore;

would be a non-violating rewrite.

Alternately, dropping the precedence of return would make C<return
something or die> do what the coder wants.

a low-precedence return would surely expose some bugs in currently
unreachable code, would it break anything that is written well?

On Tue, Oct 21, 2008 at 6​:19 PM, Dr.Ruud <rvtol+news@​isolution.nl> wrote​:

Would be nice if that warns "unreachable statement".

Some examples can be seen here​:
http​://www.google.com/codesearch?q=+lang​:perl+\+return\+.*\+or\+die

--
Alexandre Masselot, phD
Senior bioinformatician
www.genebio.com
voice​: +41 22 702 99 00

@p5pRT
Copy link
Author

p5pRT commented Oct 24, 2008

From tchrist@perl.com

Return *is* low precedence already.

Compare​:

  return ($a + $b) * $c;

with

  print ($a + $b) * $c;

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @jkeenan

I reviewed this 5-year-old ticket tonight. I don't see any claim that
there is a bug in perl, nor do I see a request for a change in
documentation or source code. All I see is some back and forth that
would better be handled on IRC or on perlmonks.

I am taking this ticket for the purpose of closing it in seven days --
unless someone can see something I haven't and takes the ticket over.

Thank you very much.
Jim Keenan

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @demerphq

On 19 February 2013 02​:51, James E Keenan via RT
<perlbug-followup@​perl.org> wrote​:

I reviewed this 5-year-old ticket tonight. I don't see any claim that
there is a bug in perl, nor do I see a request for a change in
documentation or source code. All I see is some back and forth that
would better be handled on IRC or on perlmonks.

I am taking this ticket for the purpose of closing it in seven days --
unless someone can see something I haven't and takes the ticket over.

Well, Im not going to take it over, but IMO​:

return $x
  or die "...";

should warn or something.

So maybe the ticket should be marked stalled and not closed?

Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @druud62

On 2013-02-19 03​:03, demerphq wrote​:

On 19 February 2013 02​:51, James E Keenan via RT
<perlbug-followup@​perl.org> wrote​:

I reviewed this 5-year-old ticket tonight. I don't see any claim that
there is a bug in perl, nor do I see a request for a change in
documentation or source code. All I see is some back and forth that
would better be handled on IRC or on perlmonks.

I am taking this ticket for the purpose of closing it in seven days --
unless someone can see something I haven't and takes the ticket over.

Well, Im not going to take it over, but IMO​:

return $x
or die "...";

should warn or something.

So maybe the ticket should be marked stalled and not closed?

Indeed, the requested change is to warn on popular kinds of unreachable
code, for example​:

  return ... or ...;

Or leave it to perlcritic/perltidy?

--
Ruud

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @nwc10

On Tue, Feb 19, 2013 at 11​:44​:21AM +0100, Dr.Ruud wrote​:

On 2013-02-19 03​:03, demerphq wrote​:

Well, Im not going to take it over, but IMO​:

return $x
or die "...";

should warn or something.

So maybe the ticket should be marked stalled and not closed?

Indeed, the requested change is to warn on popular kinds of unreachable
code, for example​:

return ... or ...;

(subexpression that always causes control flow) [operator] ...

where ... is unreachable.

Or leave it to perlcritic/perltidy?

The code that we're suggesting should warn, is buggy as written. I don't
think that that's really a "tidy" issue, or even a style issue.

To my mind, it's a legitimate todo.

Nicholas Clark

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @Leont

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

Well, Im not going to take it over, but IMO​:

return $x
or die "...";

should warn or something.

So maybe the ticket should be marked stalled and not closed?

Yeah, this is always buggy, so we might as well warn.

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @ap

* Leon Timmermans <fawaka@​gmail.com> [2013-02-19 13​:15]​:

Yeah, this is always buggy, so we might as well warn.

Except in Perl poetry. I am sure that Paul Fenwick, for one,
would appreciate `return 1 or die`. :-) </digression>

--
*AUTOLOAD=*_;sub _{s/​::([^​:]*)$/print$1,(",$\/"," ")[defined wantarray]/e;chop;$_}
&Just->another->Perl->hack;
#Aristotle Pagaltzis // <http​://plasmasturm.org/>

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @rjbs

* Leon Timmermans <fawaka@​gmail.com> [2013-02-19T07​:12​:42]

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

return $x
or die "...";

should warn or something.
Yeah, this is always buggy, so we might as well warn.

Agreed. I've made this mistake and then wondered why I didn't get an
unreachable code warning, in the past.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From tchrist@perl.com

"Dr.Ruud" <rvtol+usenet@​isolution.nl> wrote
  on Tue, 19 Feb 2013 11​:44​:21 +0100​:

On 2013-02-19 03​:03, demerphq wrote​:

On 19 February 2013 02​:51, James E Keenan via RT
<perlbug-followup@​perl.org> wrote​:

I reviewed this 5-year-old ticket tonight. I don't see any claim that
there is a bug in perl, nor do I see a request for a change in
documentation or source code. All I see is some back and forth that
would better be handled on IRC or on perlmonks.

I am taking this ticket for the purpose of closing it in seven days --
unless someone can see something I haven't and takes the ticket over.

Well, Im not going to take it over, but IMO​:

return $x
or die "...";

should warn or something.

So maybe the ticket should be marked stalled and not closed?

Indeed, the requested change is to warn on popular kinds of unreachable
code, for example​:

return ... or ...;

Or leave it to perlcritic/perltidy?

What is it that gardenpaths people into making this sort of error, anyway?

I'm serious.

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From tchrist@perl.com

Leon Timmermans <fawaka@​gmail.com> wrote
  on Tue, 19 Feb 2013 13​:12​:42 +0100​:

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

Well, Im not going to take it over, but IMO​:

return $x
or die "...";

should warn or something.

So maybe the ticket should be marked stalled and not closed?

Yeah, this is always buggy, so we might as well warn.

Precedence is different on return than anywhere else in Perl.
Try explaining why this elicits warning 1​:

  sub t1 { return(0) || warn "warning 1" }
  sub t2 { return(0) or warn "warning 2" }
  $x = t1();
  $y = t2();

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From tchrist@perl.com

Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote
  on Tue, 19 Feb 2013 09​:59​:58 EST​:

* Leon Timmermans <fawaka@​gmail.com> [2013-02-19T07​:12​:42]

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

return $x
or die "...";

should warn or something.

Yeah, this is always buggy, so we might as well warn.

Agreed. I've made this mistake and then wondered why I
didn't get an unreachable code warning, in the past.

Again I ask​: what is it that leads anyone ever to make that
particular error? I do not understand.

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @demerphq

On 19 February 2013 17​:02, Tom Christiansen <tchrist@​perl.com> wrote​:

Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote
on Tue, 19 Feb 2013 09​:59​:58 EST​:

* Leon Timmermans <fawaka@​gmail.com> [2013-02-19T07​:12​:42]

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

return $x
or die "...";

should warn or something.

Yeah, this is always buggy, so we might as well warn.

Agreed. I've made this mistake and then wondered why I
didn't get an unreachable code warning, in the past.

Again I ask​: what is it that leads anyone ever to make that
particular error? I do not understand.

Who cares? People do it and it causes trouble and Perl should probably
be able to figure out how to warn about it.

cheers,
Yves

--
perl -Mre=debug -e "/just|another|perl|hacker/"

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @Hugmeir

On Tue, Feb 19, 2013 at 1​:02 PM, Tom Christiansen <tchrist@​perl.com> wrote​:

Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote
on Tue, 19 Feb 2013 09​:59​:58 EST​:

* Leon Timmermans <fawaka@​gmail.com> [2013-02-19T07​:12​:42]

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

return $x
or die "...";

should warn or something.

Yeah, this is always buggy, so we might as well warn.

Agreed. I've made this mistake and then wondered why I
didn't get an unreachable code warning, in the past.

Again I ask​: what is it that leads anyone ever to make that
particular error? I do not understand.

Playing the devil's advocate, how about​: Blind code standardization,
the sort that goes "every function should have an explicit return!"

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From tchrist@perl.com

demerphq <demerphq@​gmail.com> wrote
  on Tue, 19 Feb 2013 17​:10​:14 +0100​:

On 19 February 2013 17​:02, Tom Christiansen <tchrist@​perl.com> wrote​:

Ricardo Signes <perl.p5p@​rjbs.manxome.org> wrote
on Tue, 19 Feb 2013 09​:59​:58 EST​:

* Leon Timmermans <fawaka@​gmail.com> [2013-02-19T07​:12​:42]

On Tue, Feb 19, 2013 at 3​:03 AM, demerphq <demerphq@​gmail.com> wrote​:

return $x
or die "...";

should warn or something.

Yeah, this is always buggy, so we might as well warn.

Agreed. I've made this mistake and then wondered why I
didn't get an unreachable code warning, in the past.

Again I ask​: what is it that leads anyone ever to make that
particular error? I do not understand.

Who cares? People do it and it causes trouble and Perl should probably
be able to figure out how to warn about it.

The reason it matters is because I have no idea what that is supposed
to mean.

There are uncountably many similar constructs that Perl does not warn
on either, like

  $ perl -cwe 'die "ABC"; die "XYZ"'
  -e syntax OK

  $ perl -cwe 'die("ABC") || die("XYZ")'
  -e syntax OK

  $ perl -cwe 'die("ABC") && die("XYZ")'
  -e syntax OK

None of which are caught and all of which should be. Remember

  /* NOTREACHED */

I want to understand what it is that the people who make this
error were thinking, and how they came to think that.

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @Leont

On Tue, Feb 19, 2013 at 7​:25 PM, Tom Christiansen <tchrist@​perl.com> wrote​:

The reason it matters is because I have no idea what that is supposed
to mean.

There are uncountably many similar constructs that Perl does not warn
on either, like

$ perl \-cwe 'die "ABC"; die "XYZ"'
\-e syntax OK

$ perl \-cwe 'die\("ABC"\) || die\("XYZ"\)'
\-e syntax OK

$ perl \-cwe 'die\("ABC"\) && die\("XYZ"\)'
\-e syntax OK

None of which are caught and all of which should be. Remember

/\* NOTREACHED \*/

I want to understand what it is that the people who make this
error were thinking, and how they came to think that.

It's a simple precedence mistake.

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @Leont

On Tue, Feb 19, 2013 at 3​:08 PM, Aristoteles Pagaltzis <pagaltzis@​gmx.de> wrote​:

Except in Perl poetry. I am sure that Paul Fenwick, for one,
would appreciate `return 1 or die`. :-) </digression>

Fortunately, one usually doesn't run poetry with warnings enabled ;-)

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 19, 2013

From @ap

* Tom Christiansen <tchrist@​perl.com> [2013-02-19 17​:00]​:

Precedence is different on return than anywhere else in Perl.
Try explaining why this elicits warning 1​:

sub t1 { return(0) || warn "warning 1" }
sub t2 { return(0) or warn "warning 2" }
$x = t1();
$y = t2();

This smells to me like a super-special parse reminiscent of how Perl
parses

  sort foo($x)

which has cost me way too many hairs over the years. I run into it just
rarely enough to never suspect the real problem, not only because the
rarity means I forget, but also because it almost invariably happens in
a chain of data structure-munging list ops, so the symptom is a weird
number of elements returned at the end, and at some very surprising
deref level. So I end up on a long goose chase of conjectures every time
before I finally start converging on the real problem and eventually
remember with a start that `sort` is hatefully super-special. Ugh.

Err.

I’m afraid I’m not sure any of that is even relevant to your point… all
I have is the suspicion that this is a similar case. But I feel a need
to rant about `sort` whenever I am reminded of this aspect of it; see
above for why.

--
*AUTOLOAD=*_;sub _{s/​::([^​:]*)$/print$1,(",$\/"," ")[defined wantarray]/e;chop;$_}
&Just->another->Perl->hack;
#Aristotle Pagaltzis // <http​://plasmasturm.org/>

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2013

From tchrist@perl.com

Aristotle Pagaltzis <pagaltzis@​gmx.de> wrote
  on Wed, 20 Feb 2013 00​:06​:02 +0100​:

* Tom Christiansen <tchrist@​perl.com> [2013-02-19 17​:00]​:

Precedence is different on return than anywhere else in Perl.
Try explaining why this elicits warning 1​:

sub t1 { return(0) || warn "warning 1" }
sub t2 { return(0) or warn "warning 2" }
$x = t1();
$y = t2();

This smells to me like a super-special parse reminiscent of how Perl
parses

sort foo\($x\)

which has cost me way too many hairs over the years. I run into it just
rarely enough to never suspect the real problem, not only because the
rarity means I forget, but also because it almost invariably happens in
a chain of data structure-munging list ops, so the symptom is a weird
number of elements returned at the end, and at some very surprising
deref level. So I end up on a long goose chase of conjectures every time
before I finally start converging on the real problem and eventually
remember with a start that `sort` is hatefully super-special. Ugh.

Err.

I’m afraid I’m not sure any of that is even relevant to your point… all
I have is the suspicion that this is a similar case. But I feel a need
to rant about `sort` whenever I am reminded of this aspect of it; see
above for why.

The thing about return is that it is design to make it hard to make
precedence errors with. You can even

  return ($a + 5) * 9;

and have it do the right thing. I just had never heard of anybody
making a precedence error with it before, because it tries so hard
to make that impossible.

Leave it that skunky "or" thing to screw that up. Thank goodness
I never used it, and so never have precedence problems. :)

--tom

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2013

From @rjbs

* Tom Christiansen <tchrist@​perl.com> [2013-02-19T11​:02​:02]

Again I ask​: what is it that leads anyone ever to make that
particular error? I do not understand.

I don't remember how it came about. I assume it was the result of
absent-minded refactoring, where​:

  my $x = foo or die;
  return $x;

turned into

  return foo or die;

...but I can't remember. I only remember that I've gotten that sort of
nonsense in my code. If I find evidence of commits fixing it or, Heaven
forbid, I make that mistake again, I'll try to let you know how it came about.

I do think it's useful to know what kind of confusion of ideas that could
provoke such a mistake.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2013

From @jandubois

On Tue, Feb 19, 2013 at 4​:54 PM, Ricardo Signes
<perl.p5p@​rjbs.manxome.org> wrote​:

I don't remember how it came about. I assume it was the result of
absent-minded refactoring, where​:

my $x = foo or die;
return $x;

turned into

return foo or die;

Just for the record, I've also seen

  return foo() and bar(); # supposed to return TRUE only when
both foo() and bar() are true

in the wild (can't remember where). Which I guess proves Tom's point
that adding and/or didn't make things safer for beginners.

Cheers,
-Jan

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2013

From @Leont

On Wed, Feb 20, 2013 at 2​:07 AM, Jan Dubois <jand@​activestate.com> wrote​:

in the wild (can't remember where). Which I guess proves Tom's point
that adding and/or didn't make things safer for beginners.

People write bugs on any precedence level, I don't think that that
argument is convincing.

Leon

@p5pRT
Copy link
Author

p5pRT commented Feb 20, 2013

From tchrist@perl.com

Leon Timmermans <fawaka@​gmail.com> wrote
  on Wed, 20 Feb 2013 02​:51​:49 +0100​:

On Wed, Feb 20, 2013 at 2​:07 AM, Jan Dubois <jand@​activestate.com> wrote​:

in the wild (can't remember where). Which I guess proves Tom's point
that adding and/or didn't make things safer for beginners.

People write bugs on any precedence level, I don't think that that
argument is convincing.

The argument is that the only people who do not make precedence mistakes
are the people who use parentheses. You are not convinced?

Thinking that you -- and everybody else! -- always know, remember, and
correctly apply all 24 (twenty-four) different precedence levels in
all situations without using parens is not realistic, and it leads
to error.

  1​: left terms and list operators (leftward)
  2​: left ->
  3​: nonassoc ++ --
  4​: right **
  5​: right ! ~ \ and unary + and -
  6​: left =~ !~
  7​: left * / % x
  8​: left + - .
  9​: left << >>
  10​: nonassoc named unary operators
  11​: nonassoc < > <= >= lt gt le ge
  12​: nonassoc == != <=> eq ne cmp ~~
  13​: left &
  14​: left | ^
  15​: left &&
  16​: left || //
  17​: nonassoc .. ...
  18​: right ?​:
  19​: right = += -= *= etc.
  20​: left , =>
  21​: nonassoc list operators (rightward)
  22​: right not
  23​: left and
  24​: left or xor

That is the reasoning behind "just use parens". Once you start doing that,
all of these precedence "problems" disappear. And so do the funny bits
at the bottom of the table. :)

--tom

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @nthykier

0001-op.c-Add-op_folded-to-BASEOP.patch
From 422e79d529540c15c6795d9a252329541eb05244 Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Wed, 17 Jul 2013 20:59:54 +0200
Subject: [PATCH 1/2] op.c: Add op_folded to BASEOP

Add a new member, op_folded, to BASEOP.  It is replacement for
OPpCONST_FOLDED (which can only be set on OP_CONST).  At the moment
OPpCONST_FOLDED remains, as it is exposed in B (e.g. B::Concise relies
on it).

Signed-off-by: Niels Thykier <niels@thykier.net>
---
 op.c   |   14 ++++++++++----
 op.h   |    7 +++++--
 toke.c |    1 +
 3 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/op.c b/op.c
index d5323a0..a9ee2d1 100644
--- a/op.c
+++ b/op.c
@@ -3345,7 +3345,10 @@ S_fold_constants(pTHX_ OP *o)
     if (type == OP_RV2GV)
 	newop = newGVOP(OP_GV, 0, MUTABLE_GV(sv));
     else
+    {
 	newop = newSVOP(OP_CONST, OPpCONST_FOLDED<<8, MUTABLE_SV(sv));
+	newop->op_folded = 1;
+    }
     op_getmad(o,newop,'f');
     return newop;
 
@@ -5880,6 +5883,8 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
 		other->op_flags |= OPf_SPECIAL;
 	    else if (other->op_type == OP_CONST)
 		other->op_private |= OPpCONST_FOLDED;
+
+	    other->op_folded = 1;
 	    return other;
 	}
 	else {
@@ -6041,6 +6046,7 @@ Perl_newCONDOP(pTHX_ I32 flags, OP *first, OP *trueop, OP *falseop)
 	    live->op_flags |= OPf_SPECIAL;
 	else if (live->op_type == OP_CONST)
 	    live->op_private |= OPpCONST_FOLDED;
+	live->op_folded = 1;
 	return live;
     }
     NewOp(1101, logop, 1, LOGOP);
@@ -8651,7 +8657,7 @@ Perl_ck_ftst(pTHX_ OP *o)
 	const OPCODE kidtype = kid->op_type;
 
 	if (kidtype == OP_CONST && (kid->op_private & OPpCONST_BARE)
-	 && !(kid->op_private & OPpCONST_FOLDED)) {
+	 && !kid->op_folded) {
 	    OP * const newop = newGVOP(type, OPf_REF,
 		gv_fetchsv(kid->op_sv, GV_ADD, SVt_PVIO));
 #ifdef PERL_MAD
@@ -9236,7 +9242,7 @@ Perl_ck_listiob(pTHX_ OP *o)
 	kid = kid->op_sibling;
     else if (kid && !kid->op_sibling) {		/* print HANDLE; */
 	if (kid->op_type == OP_CONST && kid->op_private & OPpCONST_BARE
-	 && !(kid->op_private & OPpCONST_FOLDED)) {
+	 && !kid->op_folded) {
 	    o->op_flags |= OPf_STACKED;	/* make it a filehandle */
 	    kid = newUNOP(OP_RV2GV, OPf_REF, scalar(kid));
 	    cLISTOPo->op_first->op_sibling = kid;
@@ -10603,8 +10609,8 @@ Perl_ck_trunc(pTHX_ OP *o)
 	if (kid->op_type == OP_NULL)
 	    kid = (SVOP*)kid->op_sibling;
 	if (kid && kid->op_type == OP_CONST &&
-	    (kid->op_private & (OPpCONST_BARE|OPpCONST_FOLDED))
-			     == OPpCONST_BARE)
+	    (kid->op_private & OPpCONST_BARE) &&
+	    !kid->op_folded)
 	{
 	    o->op_flags |= OPf_SPECIAL;
 	    kid->op_private &= ~OPpCONST_STRICT;
diff --git a/op.h b/op.h
index 5d1a771..dcfd5be 100644
--- a/op.h
+++ b/op.h
@@ -23,7 +23,8 @@
  *	op_static	tell op_free() to skip PerlMemShared_free(), when
  *                      !op_slabbed.
  *	op_savefree	on savestack via SAVEFREEOP
- *	op_spare	Three spare bits
+ *	op_folded	Result/remainder of a constant fold operation.
+ *	op_spare	Two spare bits
  *	op_flags	Flags common to all operations.  See OPf_* below.
  *	op_private	Flags peculiar to a particular operation (BUT,
  *			by default, set to the number of children until
@@ -56,7 +57,8 @@ typedef PERL_BITFIELD16 Optype;
     PERL_BITFIELD16 op_slabbed:1;	\
     PERL_BITFIELD16 op_savefree:1;	\
     PERL_BITFIELD16 op_static:1;	\
-    PERL_BITFIELD16 op_spare:3;		\
+    PERL_BITFIELD16 op_folded:1;	\
+    PERL_BITFIELD16 op_spare:2;		\
     U8		op_flags;		\
     U8		op_private;
 #endif
@@ -257,6 +259,7 @@ Deprecated.  Use C<GIMME_V> instead.
 #define	OPpCONST_STRICT		8	/* bareword subject to strict 'subs' */
 #define OPpCONST_ENTERED	16	/* Has been entered as symbol. */
 #define OPpCONST_BARE		64	/* Was a bare word (filehandle?). */
+/* Replaced by op_folded in perl itself, still used by B/B::Concise etc. */
 #define OPpCONST_FOLDED		128	/* Result of constant folding */
 
 /* Private for OP_FLIP/FLOP */
diff --git a/toke.c b/toke.c
index 1615cb6..883b881 100644
--- a/toke.c
+++ b/toke.c
@@ -7391,6 +7391,7 @@ Perl_yylex(pTHX)
 			SvREFCNT_dec(((SVOP*)pl_yylval.opval)->op_sv);
 			((SVOP*)pl_yylval.opval)->op_sv = SvREFCNT_inc_simple(sv);
 			pl_yylval.opval->op_private = OPpCONST_FOLDED;
+			pl_yylval.opval->op_folded = 1;
 			pl_yylval.opval->op_flags |= OPf_SPECIAL;
 			TOKEN(WORD);
 		    }
-- 
1.7.10.4

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @nthykier

0002-op.c-Warn-on-return-a-or-b-perl-59802.patch
From 1c7ceaba32801cdb7b754628fc6996b952a5b79f Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Mon, 15 Jul 2013 22:25:19 +0200
Subject: [PATCH 2/2] op.c: Warn on "return $a or $b" [perl #59802]

Add a warning for the (likely) unintended use of "return $a or $b"
(and similar expressions), which perl parses as "(return $a) || $b"
(which is effectively just "return $a;").

Note this warning is triggered by some modules (e.g. Test::Builder).
These are not fixed by this commit.

Signed-off-by: Niels Thykier <niels@thykier.net>
---
 op.c              |   38 ++++++++++++++++++++++++++++++++++++++
 pod/perldiag.pod  |   22 ++++++++++++++++++++++
 t/lib/warnings/op |   37 +++++++++++++++++++++++++++++++++++++
 3 files changed, 97 insertions(+)

diff --git a/op.c b/op.c
index a9ee2d1..e2e5be6 100644
--- a/op.c
+++ b/op.c
@@ -5830,6 +5830,44 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
     first = *firstp;
     other = *otherp;
 
+    /* [perl #59802]: Warn about things like "return $a or $b", which
+       is parsed as "(return $a) or $b" rather than "return ($a or
+       $b)".  NB: This also applies to xor, which is why we do it
+       here.
+     */
+    switch (first->op_type) {
+    case OP_NEXT:
+    case OP_LAST:
+    case OP_REDO:
+	/* XXX: Perhaps we should emit a stronger warning for these.
+	   Even with the high-precedence operator they don't seem to do
+	   anything sensible.
+
+	   But until we do, fall through here.
+         */
+    case OP_RETURN:
+    case OP_DIE:
+    case OP_EXIT:
+    case OP_GOTO:
+	/* XXX: Currently we allow people to "shoot themselves in the
+	   foot" by explicitly writing "(return $a) or $b".
+
+	   Warn unless we are looking at the result from folding or if
+	   the programmer explicitly grouped the operators like this.
+	   The former can occur with e.g.
+
+		use constant FEATURE => ( $] >= ... );
+		sub { not FEATURE and return or do_stuff(); }
+	 */
+	if (!first->op_folded && !(first->op_flags & OPf_PARENS))
+	    Perl_ck_warner(aTHX_ packWARN(WARN_SYNTAX),
+	                   "Possible precedence issue with control flow operator");
+	/* XXX: Should we optimze this to "return $a;" (i.e. remove
+	   the "or $b" part)?
+	*/
+	break;
+    }
+
     if (type == OP_XOR)		/* Not short circuit, but here by precedence. */
 	return newBINOP(type, flags, scalar(first), scalar(other));
 
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 5165599..7347969 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -4229,6 +4229,28 @@ higher precedence of C<==>.  This is probably not what you want.  (If you
 really meant to write this, disable the warning, or, better, put the
 parentheses explicitly and write C<$x & ($y == 0)>).
 
+=item Possible precedence issue with control flow operator
+
+(W syntax) There is a possible problem with the mixing of a control
+flow operator (e.g. C<return>) and a low-precedence operator like
+C<or>.  Consider:
+
+    sub { return $a or $b; }
+
+This is parsed as:
+
+    sub { (return $a) or $b; }
+
+Which is effectively just:
+
+    sub { return $a; }
+
+Either use parentheses or the high-precedence variant of the operator.
+
+Note this may be also triggered for constructs like:
+
+    sub { 1 if die; }
+
 =item Possible unintended interpolation of $\ in regex
 
 (W ambiguous) You said something like C<m/$\/> in a regex.
diff --git a/t/lib/warnings/op b/t/lib/warnings/op
index c38bcde..7a5ae47 100644
--- a/t/lib/warnings/op
+++ b/t/lib/warnings/op
@@ -1575,3 +1575,40 @@ OPTION regex
 ?(?s).*
 Subroutine DynaLoader::dl_error redefined at \(eval 2\) line 2\.
 ########
+# op.c
+use warnings;
+sub do_warn_1 { return $a or $b; }
+sub do_warn_2 { return $a and $b; }
+sub do_warn_3 { return $a xor $b; }
+sub do_warn_4 { die $a or $b; }
+sub do_warn_5 { die $a and $b; }
+sub do_warn_6 { die $a xor $b; }
+# These get re-written to "(return/die $a) and $b"
+sub do_warn_7 { $b if return $a; }
+sub do_warn_8 { $b if die $a; }
+
+use constant FEATURE => 1;
+use constant MISSING_FEATURE => 0;
+
+sub dont_warn_1 { return ($a or $b); }
+sub dont_warn_2 { return ($a and $b); }
+sub dont_warn_3 { return ($a xor $b); }
+
+sub dont_warn_4 { MISSING_FEATURE and return or dont_warn_3(); }
+sub dont_warn_5 { FEATURE || return and dont_warn_3(); }
+sub dont_warn_6 { not FEATURE and return or dont_warn_3(); }
+sub dont_warn_7 { !MISSING_FEATURE || return and dont_warn_3(); }
+
+# These are weird, but at least not ambiguous.
+sub dont_warn_8  { (return $a) or $b; }
+sub dont_warn_9  { (return $a) and $b; }
+sub dont_warn_10 { (return $a) xor $b; }
+EXPECT
+Possible precedence issue with control flow operator at - line 3.
+Possible precedence issue with control flow operator at - line 4.
+Possible precedence issue with control flow operator at - line 5.
+Possible precedence issue with control flow operator at - line 6.
+Possible precedence issue with control flow operator at - line 7.
+Possible precedence issue with control flow operator at - line 8.
+Possible precedence issue with control flow operator at - line 10.
+Possible precedence issue with control flow operator at - line 11.
-- 
1.7.10.4

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @cpansprout

On Wed Jul 17 12​:37​:07 2013, niels@​thykier.net wrote​:

On 2013-07-17 18​:56, Father Chrysostomos via RT wrote​:

On Wed Jul 17 01​:30​:56 2013, niels@​thykier.net wrote​:

[...]

You wrote "also set op->op_folded" and later imply the change could
simplify the code by using the same bit.
Just to confirm here, did you want the "op_folded" member replace
OPpCONST_FOLDED and (for regexp/tr operators) OPf_SPECIAL?

That would be nice. But it is not a prerequisite for getting your patch
in. :-)

And if so,
should OPf_SPECIAL still be set in these cases since it is a public
flag
or should it be cleared so we can repurpose it?

I think (off the top of my head) that B​::Deparse currently uses it. So
B​::Deparse may have to change. I don’t remember now.

OPf_SPECIAL is a private flag, at least in terms of its use, but stored
with the public flags.

Included is a patch to add the op_folded member and my previous patch
has been rebased on top of it.

RE​: op_folded-patch - I made OPpCONST_FOLDED redundant internally in the
code (i.e. perl does the right thing even without it). However, I left
it in (and kept it maintained) for now until B​::Concise can be updated.
I presume that will involve exposing op_folded via B.pm, which I have
left as "future work" for now.
I made no attempt to have it replace OPf_SPECIAL on the regexp/tr
operators either.

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 17, 2013

From @xdg

On Wed, Jul 17, 2013 at 4​:12 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

The patch is in the new Perl-Toolchain-Gang repo.

I'll try to ship it tonight.

David

--
David Golden <xdg@​xdg.me>
Take back your inbox! → http​://www.bunchmail.com/
Twitter/IRC​: @​xdg

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @xdg

  Parse-CPAN-Meta-1.4405.tar.gz

has entered CPAN as

  file​: $CPAN/authors/id/D/DA/
DAGOLDEN/Parse-CPAN-Meta-1.4405.tar.gz
  size​: 20876 bytes
  md5​: f9025240afbd3fd98e34964609732b8b

On Wed, Jul 17, 2013 at 4​:59 PM, David Golden <xdg@​xdg.me> wrote​:

On Wed, Jul 17, 2013 at 4​:12 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

The patch is in the new Perl-Toolchain-Gang repo.

I'll try to ship it tonight.

David

--
David Golden <xdg@​xdg.me>
Take back your inbox! → http​://www.bunchmail.com/
Twitter/IRC​: @​xdg

--
David Golden <xdg@​xdg.me>
Take back your inbox! → http​://www.bunchmail.com/
Twitter/IRC​: @​xdg

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @cpansprout

On Wed Jul 17 18​:47​:35 2013, xdg@​xdg.me wrote​:

Parse\-CPAN\-Meta\-1\.4405\.tar\.gz

has entered CPAN as

file​: $CPAN/authors/id/D/DA/
DAGOLDEN/Parse-CPAN-Meta-1.4405.tar.gz
size​: 20876 bytes
md5​: f9025240afbd3fd98e34964609732b8b

Thank you.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @nthykier

On 2013-07-17 22​:12, Father Chrysostomos via RT wrote​:

On Wed Jul 17 12​:37​:07 2013, niels@​thykier.net wrote​:

[...]

Included is a patch to add the op_folded member and my previous patch
has been rebased on top of it.

RE​: op_folded-patch - I made OPpCONST_FOLDED redundant internally in the
code (i.e. perl does the right thing even without it). However, I left
it in (and kept it maintained) for now until B​::Concise can be updated.
I presume that will involve exposing op_folded via B.pm, which I have
left as "future work" for now.
I made no attempt to have it replace OPf_SPECIAL on the regexp/tr
operators either.

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

Revised warning patch attached with more tests. Thanks to Reini Urban
for smoke testing CPAN and reminding me that exit is a unary operator
(so it is not even safe to use "||" with exit).

~Niels

@p5pRT
Copy link
Author

p5pRT commented Jul 18, 2013

From @nthykier

0002-op.c-Warn-on-return-a-or-b-perl-59802.patch
From cb322ab05e51647edb4db431f1757b9631ef5001 Mon Sep 17 00:00:00 2001
From: Niels Thykier <niels@thykier.net>
Date: Mon, 15 Jul 2013 22:25:19 +0200
Subject: [PATCH 2/2] op.c: Warn on "return $a or $b" [perl #59802]

Add a warning for the (likely) unintended use of "return $a or $b"
(and similar expressions), which perl parses as "(return $a) || $b"
(which is effectively just "return $a;").

Note this warning is triggered by some modules (e.g. Test::Builder).
These are not fixed by this commit.

Signed-off-by: Niels Thykier <niels@thykier.net>
---
 op.c              |   38 ++++++++++++++++++
 pod/perldiag.pod  |   22 ++++++++++
 t/lib/warnings/op |  115 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 175 insertions(+)

diff --git a/op.c b/op.c
index a9ee2d1..0459968 100644
--- a/op.c
+++ b/op.c
@@ -5830,6 +5830,44 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp)
     first = *firstp;
     other = *otherp;
 
+    /* [perl #59802]: Warn about things like "return $a or $b", which
+       is parsed as "(return $a) or $b" rather than "return ($a or
+       $b)".  NB: This also applies to xor, which is why we do it
+       here.
+     */
+    switch (first->op_type) {
+    case OP_NEXT:
+    case OP_LAST:
+    case OP_REDO:
+	/* XXX: Perhaps we should emit a stronger warning for these.
+	   Even with the high-precedence operator they don't seem to do
+	   anything sensible.
+
+	   But until we do, fall through here.
+         */
+    case OP_RETURN:
+    case OP_EXIT:
+    case OP_DIE:
+    case OP_GOTO:
+	/* XXX: Currently we allow people to "shoot themselves in the
+	   foot" by explicitly writing "(return $a) or $b".
+
+	   Warn unless we are looking at the result from folding or if
+	   the programmer explicitly grouped the operators like this.
+	   The former can occur with e.g.
+
+		use constant FEATURE => ( $] >= ... );
+		sub { not FEATURE and return or do_stuff(); }
+	 */
+	if (!first->op_folded && !(first->op_flags & OPf_PARENS))
+	    Perl_ck_warner(aTHX_ packWARN(WARN_SYNTAX),
+	                   "Possible precedence issue with control flow operator");
+	/* XXX: Should we optimze this to "return $a;" (i.e. remove
+	   the "or $b" part)?
+	*/
+	break;
+    }
+
     if (type == OP_XOR)		/* Not short circuit, but here by precedence. */
 	return newBINOP(type, flags, scalar(first), scalar(other));
 
diff --git a/pod/perldiag.pod b/pod/perldiag.pod
index 5165599..7347969 100644
--- a/pod/perldiag.pod
+++ b/pod/perldiag.pod
@@ -4229,6 +4229,28 @@ higher precedence of C<==>.  This is probably not what you want.  (If you
 really meant to write this, disable the warning, or, better, put the
 parentheses explicitly and write C<$x & ($y == 0)>).
 
+=item Possible precedence issue with control flow operator
+
+(W syntax) There is a possible problem with the mixing of a control
+flow operator (e.g. C<return>) and a low-precedence operator like
+C<or>.  Consider:
+
+    sub { return $a or $b; }
+
+This is parsed as:
+
+    sub { (return $a) or $b; }
+
+Which is effectively just:
+
+    sub { return $a; }
+
+Either use parentheses or the high-precedence variant of the operator.
+
+Note this may be also triggered for constructs like:
+
+    sub { 1 if die; }
+
 =item Possible unintended interpolation of $\ in regex
 
 (W ambiguous) You said something like C<m/$\/> in a regex.
diff --git a/t/lib/warnings/op b/t/lib/warnings/op
index c38bcde..d858f32 100644
--- a/t/lib/warnings/op
+++ b/t/lib/warnings/op
@@ -1575,3 +1575,118 @@ OPTION regex
 ?(?s).*
 Subroutine DynaLoader::dl_error redefined at \(eval 2\) line 2\.
 ########
+# op.c
+use warnings;
+sub do_warn_1  { return $a or $b; }
+sub do_warn_2  { return $a and $b; }
+sub do_warn_3  { return $a xor $b; }
+sub do_warn_4  { die $a or $b; }
+sub do_warn_5  { die $a and $b; }
+sub do_warn_6  { die $a xor $b; }
+sub do_warn_7  { exit $a or $b; }
+sub do_warn_8  { exit $a and $b; }
+sub do_warn_9  { exit $a xor $b; }
+
+# Since exit is an unary operator, it is even stronger than
+# || and &&.
+sub do_warn_10 { exit $a || $b; }
+sub do_warn_11 { exit $a && $b; }
+
+sub do_warn_12 { goto $a or $b; }
+sub do_warn_13 { goto $a and $b; }
+sub do_warn_14 { goto $a xor $b; }
+sub do_warn_15 { next $a or $b while(1);  }
+sub do_warn_16 { next $a and $b while(1); }
+sub do_warn_17 { next $a xor $b while(1); }
+sub do_warn_18 { last $a or $b while(1);  }
+sub do_warn_19 { last $a and $b while(1); }
+sub do_warn_20 { last $a xor $b while(1); }
+sub do_warn_21 { redo $a or $b while(1); }
+sub do_warn_22 { redo $a and $b while(1); }
+sub do_warn_23 { redo $a xor $b while(1); }
+# These get re-written to "(return/die $a) and $b"
+sub do_warn_24 { $b if return $a; }
+sub do_warn_25 { $b if die $a; }
+EXPECT
+Possible precedence issue with control flow operator at - line 3.
+Possible precedence issue with control flow operator at - line 4.
+Possible precedence issue with control flow operator at - line 5.
+Possible precedence issue with control flow operator at - line 6.
+Possible precedence issue with control flow operator at - line 7.
+Possible precedence issue with control flow operator at - line 8.
+Possible precedence issue with control flow operator at - line 9.
+Possible precedence issue with control flow operator at - line 10.
+Possible precedence issue with control flow operator at - line 11.
+Possible precedence issue with control flow operator at - line 15.
+Possible precedence issue with control flow operator at - line 16.
+Possible precedence issue with control flow operator at - line 18.
+Possible precedence issue with control flow operator at - line 19.
+Possible precedence issue with control flow operator at - line 20.
+Possible precedence issue with control flow operator at - line 21.
+Possible precedence issue with control flow operator at - line 22.
+Possible precedence issue with control flow operator at - line 23.
+Possible precedence issue with control flow operator at - line 24.
+Possible precedence issue with control flow operator at - line 25.
+Possible precedence issue with control flow operator at - line 26.
+Possible precedence issue with control flow operator at - line 27.
+Possible precedence issue with control flow operator at - line 28.
+Possible precedence issue with control flow operator at - line 29.
+Possible precedence issue with control flow operator at - line 31.
+Possible precedence issue with control flow operator at - line 32.
+########
+# op.c
+#  (same as above, except these should not warn)
+use constant FEATURE => 1;
+use constant MISSING_FEATURE => 0;
+
+sub dont_warn_1  { MISSING_FEATURE and return or dont_warn_3(); }
+sub dont_warn_2  { FEATURE || return and dont_warn_3(); }
+sub dont_warn_3  { not FEATURE and return or dont_warn_3(); }
+sub dont_warn_4  { !MISSING_FEATURE || return and dont_warn_3(); }
+sub dont_warn_5  { MISSING_FEATURE and die or dont_warn_3(); }
+sub dont_warn_6  { FEATURE || die and dont_warn_3(); }
+sub dont_warn_7  { not FEATURE and die or dont_warn_3(); }
+sub dont_warn_8  { !MISSING_FEATURE || die and dont_warn_3(); }
+sub dont_warn_9  { MISSING_FEATURE and goto $a or dont_warn_3(); }
+sub dont_warn_10 { FEATURE || goto $a and dont_warn_3(); }
+sub dont_warn_11 { not FEATURE and goto $a or dont_warn_3(); }
+sub dont_warn_12 { !MISSING_FEATURE || goto $a and dont_warn_3(); }
+
+sub dont_warn_13 { MISSING_FEATURE and exit $a or dont_warn_3(); }
+sub dont_warn_14 { FEATURE || exit $a and dont_warn_3(); }
+sub dont_warn_15 { not FEATURE and exit $a or dont_warn_3(); }
+sub dont_warn_16 { !MISSING_FEATURE || exit $a and dont_warn_3(); }
+
+sub dont_warn_17 { MISSING_FEATURE and next or dont_warn_3() while(1); }
+sub dont_warn_18 { FEATURE || next and dont_warn_3() while(1); }
+sub dont_warn_19 { not FEATURE and next or dont_warn_3() while(1); }
+sub dont_warn_20 { !MISSING_FEATURE || next and dont_warn_3() while(1); }
+sub dont_warn_21 { MISSING_FEATURE and redo or dont_warn_3() while(1); }
+sub dont_warn_22 { FEATURE || redo and dont_warn_3() while(1); }
+sub dont_warn_23 { not FEATURE and redo or dont_warn_3() while(1); }
+sub dont_warn_24 { !MISSING_FEATURE || redo and dont_warn_3() while(1); }
+sub dont_warn_25 { MISSING_FEATURE and last or dont_warn_3() while(1); }
+sub dont_warn_26 { FEATURE || last and dont_warn_3() while(1); }
+sub dont_warn_27 { not FEATURE and last or dont_warn_3() while(1); }
+sub dont_warn_28 { !MISSING_FEATURE || last and dont_warn_3() while(1); }
+
+# These are weird, but at least not ambiguous.
+sub dont_warn_29 { return ($a or $b); }
+sub dont_warn_30 { return ($a and $b); }
+sub dont_warn_31 { return ($a xor $b); }
+sub dont_warn_32 { die ($a or $b); }
+sub dont_warn_33 { die ($a and $b); }
+sub dont_warn_34 { die ($a xor $b); }
+sub dont_warn_35 { goto ($a or $b); }
+sub dont_warn_36 { goto ($a and $b); }
+sub dont_warn_37 { goto ($a xor $b); }
+sub dont_warn_38 { next ($a or $b) while(1);  }
+sub dont_warn_39 { next ($a and $b) while(1); }
+sub dont_warn_40 { next ($a xor $b) while(1); }
+sub dont_warn_41 { last ($a or $b) while(1);  }
+sub dont_warn_42 { last ($a and $b) while(1); }
+sub dont_warn_43 { last ($a xor $b) while(1); }
+sub dont_warn_44 { redo ($a or $b) while(1);  }
+sub dont_warn_45 { redo ($a and $b) while(1); }
+sub dont_warn_46 { redo ($a xor $b) while(1); }
+EXPECT
-- 
1.7.10.4

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @rurban

On Thu, Jul 18, 2013 at 6​:05 PM, Niels Thykier <niels@​thykier.net> wrote​:

On 2013-07-17 22​:12, Father Chrysostomos via RT wrote​:

On Wed Jul 17 12​:37​:07 2013, niels@​thykier.net wrote​:

Included is a patch to add the op_folded member and my previous patch
has been rebased on top of it.

RE​: op_folded-patch - I made OPpCONST_FOLDED redundant internally in the
code (i.e. perl does the right thing even without it). However, I left
it in (and kept it maintained) for now until B​::Concise can be updated.
I presume that will involve exposing op_folded via B.pm, which I have
left as "future work" for now.
I made no attempt to have it replace OPf_SPECIAL on the regexp/tr
operators either.

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

Revised warning patch attached with more tests. Thanks to Reini Urban
for smoke testing CPAN and reminding me that exit is a unary operator
(so it is not even safe to use "||" with exit).

LGTM.

These are the bugs we found so far​:

Perl-Critic [cpan #87032] wrong return precedence in
Perl​::Critic​::Utils​::interpolate
YAML-Syck [cpan #87034] precedence error in inc/Test/Builder.pm
DBI [cpan #87029] Possible precedence issue with control flow operator
at ./t/16destroy.t
Git-CPAN-Patch [cpan #86950] wrong return or precedence (fixed)
Apache-SWIT [cpan #86949] return expr or precedence error
Parse-CPAN-Meta - fixed with 1.4405

JSON-PP (no ticket yet)
Test-Simple (no ticket yet)

I needed the following patch to extend B for folded (and the other
missing fields),
and to fix dump.

And the 2nd patch is a hint for the upstream maintainers.

When I fixed all B modules to use the new B, we can get rid of
the OPpCONST_FOLDED and have one more private bit free for CONSTs.
B​::C and ByteLoader already done. B-Generate has more problems with blead.

--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @rurban

0001-more-op_folded-support-B-dump.patch
From 9c97840a3a4c7538643548513ff282fb21a3ecde Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Thu, 18 Jul 2013 14:50:35 -0500
Subject: [PATCH 1/2] more op_folded support: B, dump

also add more B::OP accessors for the missing bitfields
---
 dump.c     |  5 +++--
 ext/B/B.xs | 28 ++++++++++++++++++++++++++--
 op.h       |  2 ++
 3 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/dump.c b/dump.c
index 6ba4fd2..4720e30 100644
--- a/dump.c
+++ b/dump.c
@@ -861,13 +861,14 @@ S_op_private_to_names(pTHX_ SV *tmpsv, U32 optype, U32 op_private) {
         if (o->op_slabbed)  sv_catpvs(tmpsv, ",SLABBED");               \
         if (o->op_savefree) sv_catpvs(tmpsv, ",SAVEFREE");              \
         if (o->op_static)   sv_catpvs(tmpsv, ",STATIC");                \
-        if (!xml)                                                        \
+        if (o->op_folded)   sv_catpvs(tmpsv, ",FOLDED");                \
+        if (!xml)                                                       \
             Perl_dump_indent(aTHX_ level, file, "FLAGS = (%s)\n",       \
                             SvCUR(tmpsv) ? SvPVX_const(tmpsv) + 1 : "");\
         else                                                            \
             PerlIO_printf(file, " flags=\"%s\"",                        \
                           SvCUR(tmpsv) ? SvPVX(tmpsv) + 1 : "");        \
-        SvREFCNT_dec_NN(tmpsv);                                            \
+        SvREFCNT_dec_NN(tmpsv);                                         \
     }
 
 #if !defined(PERL_MAD)
diff --git a/ext/B/B.xs b/ext/B/B.xs
index 0b2ecae..a17f876 100644
--- a/ext/B/B.xs
+++ b/ext/B/B.xs
@@ -670,7 +670,7 @@ struct OP_methods {
     STR_WITH_LEN("targ"),    PADOFFSETp, offsetof(struct op, op_targ),   /* 2*/
     STR_WITH_LEN("flags"),   U8p,    offsetof(struct op, op_flags),      /* 3*/
     STR_WITH_LEN("private"), U8p,    offsetof(struct op, op_private),    /* 4*/
-    STR_WITH_LEN("first"),   OPp,    offsetof(struct unop, op_first),     /* 5*/
+    STR_WITH_LEN("first"),   OPp,    offsetof(struct unop, op_first),    /* 5*/
     STR_WITH_LEN("last"),    OPp,    offsetof(struct binop, op_last),    /* 6*/
     STR_WITH_LEN("other"),   OPp,    offsetof(struct logop, op_other),   /* 7*/
     STR_WITH_LEN("pmreplstart"), 0, -1,                                  /* 8*/
@@ -730,6 +730,14 @@ struct OP_methods {
     STR_WITH_LEN("warnings"),0,       -1,                                /*44*/
     STR_WITH_LEN("io"),      0,       -1,                                /*45*/
     STR_WITH_LEN("hints_hash"),0,     -1,                                /*46*/
+#if PERL_VERSION >= 17
+    STR_WITH_LEN("slabbed"), 0,       -1,                                /*47*/
+    STR_WITH_LEN("savefree"),0,       -1,                                /*48*/
+    STR_WITH_LEN("static"),  0,       -1,                                /*49*/
+#if PERL_VERSION >= 19
+    STR_WITH_LEN("folded"),  0,       -1,                                /*50*/
+#endif
+#endif
 };
 
 #include "const-c.inc"
@@ -1001,6 +1009,10 @@ next(o)
 	B::COP::warnings     = 44
 	B::COP::io           = 45
 	B::COP::hints_hash   = 46
+	B::OP::slabbed       = 47
+	B::OP::savefree      = 48
+	B::OP::static        = 49
+	B::OP::folded        = 50
     PREINIT:
 	char *ptr;
 	SV *ret;
@@ -1076,10 +1088,22 @@ next(o)
 	    case 30: /* type  */
 	    case 31: /* opt   */
 	    case 32: /* spare */
-	    /* These 3 are all bitfields, so we can't take their addresses */
+#if PERL_VERSION >= 17
+	    case 47: /* slabbed  */
+	    case 48: /* savefree */
+	    case 49: /* static   */
+#if PERL_VERSION >= 19
+	    case 50: /* folded   */
+#endif
+#endif
+	    /* These are all bitfields, so we can't take their addresses */
 		ret = sv_2mortal(newSVuv((UV)(
 				      ix == 30 ? o->op_type
 		                    : ix == 31 ? o->op_opt
+		                    : ix == 47 ? o->op_slabbed
+		                    : ix == 48 ? o->op_savefree
+		                    : ix == 49 ? o->op_static
+		                    : ix == 50 ? o->op_folded
 		                    :            o->op_spare)));
 		break;
 	    case 33: /* children */
diff --git a/op.h b/op.h
index dcfd5be..ffb7178 100644
--- a/op.h
+++ b/op.h
@@ -656,6 +656,8 @@ struct loop {
 #  define OpREFCNT_dec(o)		(--(o)->op_targ)
 #endif
 
+#define OpFOLDED(o) ((o)->type == OP_CONST ? (o)->op_private & OPpCONST_FOLDED : (o)->op_folded)
+
 /* flags used by Perl_load_module() */
 #define PERL_LOADMOD_DENY		0x1	/* no Module */
 #define PERL_LOADMOD_NOIMPORT		0x2	/* use Module () */
-- 
1.8.3.1

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @rurban

0002-External-TODOs-for-59802-cpan-JSON-PP-cpan-Test-Simp.patch
From 72758296adbd0c6b838d1a041381b5954bacecd1 Mon Sep 17 00:00:00 2001
From: Reini Urban <rurban@x-ray.at>
Date: Thu, 18 Jul 2013 11:00:57 -0500
Subject: [PATCH 2/2] External TODOs for #59802 cpan/JSON-PP, cpan/Test-Simple

---
 cpan/JSON-PP/lib/JSON/PP.pm          | 2 +-
 cpan/Test-Simple/lib/Test/Builder.pm | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/cpan/JSON-PP/lib/JSON/PP.pm b/cpan/JSON-PP/lib/JSON/PP.pm
index e9e65b1..12b7d51 100644
--- a/cpan/JSON-PP/lib/JSON/PP.pm
+++ b/cpan/JSON-PP/lib/JSON/PP.pm
@@ -1564,7 +1564,7 @@ sub _incr_parse {
     $self->{incr_text} = substr( $self->{incr_text}, $p );
     $self->{incr_p} = 0;
 
-    return $obj or '';
+    return $obj || '';
 }
 
 
diff --git a/cpan/Test-Simple/lib/Test/Builder.pm b/cpan/Test-Simple/lib/Test/Builder.pm
index cb4335f..18e90f5 100644
--- a/cpan/Test-Simple/lib/Test/Builder.pm
+++ b/cpan/Test-Simple/lib/Test/Builder.pm
@@ -915,7 +915,7 @@ sub _is_dualvar {
 
     no warnings 'numeric';
     my $numval = $val + 0;
-    return $numval != 0 and $numval ne $val ? 1 : 0;
+    return ($numval != 0 and $numval ne $val) ? 1 : 0;
 }
 
 =item B<is_eq>
-- 
1.8.3.1

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @nthykier

On 2013-07-19 06​:22, Reini Urban wrote​:

On Thu, Jul 18, 2013 at 6​:05 PM, Niels Thykier <niels@​thykier.net> wrote​:

[...]

Revised warning patch attached with more tests. Thanks to Reini Urban
for smoke testing CPAN and reminding me that exit is a unary operator
(so it is not even safe to use "||" with exit).

LGTM.

These are the bugs we found so far​:

Perl-Critic [cpan #87032] wrong return precedence in
Perl​::Critic​::Utils​::interpolate
YAML-Syck [cpan #87034] precedence error in inc/Test/Builder.pm
DBI [cpan #87029] Possible precedence issue with control flow operator
at ./t/16destroy.t
Git-CPAN-Patch [cpan #86950] wrong return or precedence (fixed)
Apache-SWIT [cpan #86949] return expr or precedence error
Parse-CPAN-Meta - fixed with 1.4405

Great. :)

JSON-PP (no ticket yet)
Test-Simple (no ticket yet)

I believe these are filed​:

* https://rt.cpan.org/Public/Bug/Display.html?id=86948
* Test-More/test-more#385

I needed the following patch to extend B for folded (and the other
missing fields),
and to fix dump.

And the 2nd patch is a hint for the upstream maintainers.

When I fixed all B modules to use the new B, we can get rid of
the OPpCONST_FOLDED and have one more private bit free for CONSTs.
B​::C and ByteLoader already done. B-Generate has more problems with blead.

--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

Sounds good to me. :)

~Niels

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @rurban

CPAN updates on this after the night run​:

+ DBI [cpan #87029] (fixed)
* Git-CPAN-Patch [cpan #86950] (fixed)
* Parse-CPAN-Meta (fixed)

* Perl-Critic [cpan #87032]
* YAML-Syck [cpan #87034]
* Apache-SWIT [cpan #86949]
+ JSON-PP [cpan #86948]
+ HTML-Mason [cpan #87050]
+ IO-Async [cpan #87051]
+ IO-Socket-SSL [cpan #87052]
+ Test-Simple [cpan #87053] + Test-More/test-more#385

all patches also in my distroprefs​:
https://github.com/rurban/distroprefs
--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @cpansprout

On Wed Jul 17 12​:37​:07 2013, niels@​thykier.net wrote​:

On 2013-07-17 18​:56, Father Chrysostomos via RT wrote​:

On Wed Jul 17 01​:30​:56 2013, niels@​thykier.net wrote​:

[...]

You wrote "also set op->op_folded" and later imply the change could
simplify the code by using the same bit.
Just to confirm here, did you want the "op_folded" member replace
OPpCONST_FOLDED and (for regexp/tr operators) OPf_SPECIAL?

That would be nice. But it is not a prerequisite for getting your patch
in. :-)

And if so,
should OPf_SPECIAL still be set in these cases since it is a public
flag
or should it be cleared so we can repurpose it?

I think (off the top of my head) that B​::Deparse currently uses it. So
B​::Deparse may have to change. I don’t remember now.

OPf_SPECIAL is a private flag, at least in terms of its use, but stored
with the public flags.

Included is a patch to add the op_folded member and my previous patch
has been rebased on top of it.

RE​: op_folded-patch - I made OPpCONST_FOLDED redundant internally in the
code (i.e. perl does the right thing even without it). However, I left
it in (and kept it maintained) for now until B​::Concise can be updated.
I presume that will involve exposing op_folded via B.pm, which I have
left as "future work" for now.
I made no attempt to have it replace OPf_SPECIAL on the regexp/tr
operators either.

Thank you. I have applied the op_folded patch as 3513c74.

The warning patch will again have to wait, as your more thorough version
of the patch triggers other warnings, causing other test failures, as I
am sure you well know.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @cpansprout

On Thu Jul 18 21​:23​:27 2013, rurban wrote​:

On Thu, Jul 18, 2013 at 6​:05 PM, Niels Thykier <niels@​thykier.net> wrote​:

On 2013-07-17 22​:12, Father Chrysostomos via RT wrote​:

On Wed Jul 17 12​:37​:07 2013, niels@​thykier.net wrote​:

Included is a patch to add the op_folded member and my previous patch
has been rebased on top of it.

RE​: op_folded-patch - I made OPpCONST_FOLDED redundant internally
in the
code (i.e. perl does the right thing even without it). However, I
left
it in (and kept it maintained) for now until B​::Concise can be
updated.
I presume that will involve exposing op_folded via B.pm, which I have
left as "future work" for now.
I made no attempt to have it replace OPf_SPECIAL on the regexp/tr
operators either.

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

Revised warning patch attached with more tests. Thanks to Reini Urban
for smoke testing CPAN and reminding me that exit is a unary operator
(so it is not even safe to use "||" with exit).

LGTM.

These are the bugs we found so far​:

Perl-Critic [cpan #87032] wrong return precedence in
Perl​::Critic​::Utils​::interpolate
YAML-Syck [cpan #87034] precedence error in inc/Test/Builder.pm
DBI [cpan #87029] Possible precedence issue with control flow operator
at ./t/16destroy.t
Git-CPAN-Patch [cpan #86950] wrong return or precedence (fixed)
Apache-SWIT [cpan #86949] return expr or precedence error
Parse-CPAN-Meta - fixed with 1.4405

JSON-PP (no ticket yet)
Test-Simple (no ticket yet)

I needed the following patch to extend B for folded (and the other
missing fields),
and to fix dump.

And the 2nd patch is a hint for the upstream maintainers.

When I fixed all B modules to use the new B, we can get rid of
the OPpCONST_FOLDED and have one more private bit free for CONSTs.
B​::C and ByteLoader already done. B-Generate has more problems with blead.

Thank you. I have applied the first patch as 3164fde, but I
omitted the OpFOLDED macro, because I don’t understand its purpose (or
need). Niels’ patch changes the core not to use OPpCONST_FOLDED at all,
so having a macro to check that doesn’t help. If it is for B​::C’s sake,
then it won’t be present in older perls anyway, so you will still have
to #define it in B​::C.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jul 19, 2013

From @rurban

On Fri, Jul 19, 2013 at 12​:30 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Thu Jul 18 21​:23​:27 2013, rurban wrote​:

On Thu, Jul 18, 2013 at 6​:05 PM, Niels Thykier <niels@​thykier.net> wrote​:

On 2013-07-17 22​:12, Father Chrysostomos via RT wrote​:

On Wed Jul 17 12​:37​:07 2013, niels@​thykier.net wrote​:

Included is a patch to add the op_folded member and my previous patch
has been rebased on top of it.

RE​: op_folded-patch - I made OPpCONST_FOLDED redundant internally
in the
code (i.e. perl does the right thing even without it). However, I
left
it in (and kept it maintained) for now until B​::Concise can be
updated.
I presume that will involve exposing op_folded via B.pm, which I have
left as "future work" for now.
I made no attempt to have it replace OPf_SPECIAL on the regexp/tr
operators either.

Those patches look good. Thank you. Now we just have to wait for
Parse​::CPAN​::Meta.

Revised warning patch attached with more tests. Thanks to Reini Urban
for smoke testing CPAN and reminding me that exit is a unary operator
(so it is not even safe to use "||" with exit).

LGTM.

These are the bugs we found so far​:

Perl-Critic [cpan #87032] wrong return precedence in
Perl​::Critic​::Utils​::interpolate
YAML-Syck [cpan #87034] precedence error in inc/Test/Builder.pm
DBI [cpan #87029] Possible precedence issue with control flow operator
at ./t/16destroy.t
Git-CPAN-Patch [cpan #86950] wrong return or precedence (fixed)
Apache-SWIT [cpan #86949] return expr or precedence error
Parse-CPAN-Meta - fixed with 1.4405

JSON-PP (no ticket yet)
Test-Simple (no ticket yet)

I needed the following patch to extend B for folded (and the other
missing fields),
and to fix dump.

And the 2nd patch is a hint for the upstream maintainers.

When I fixed all B modules to use the new B, we can get rid of
the OPpCONST_FOLDED and have one more private bit free for CONSTs.
B​::C and ByteLoader already done. B-Generate has more problems with blead.

Thank you. I have applied the first patch as 3164fde, but I
omitted the OpFOLDED macro, because I don’t understand its purpose (or
need). Niels’ patch changes the core not to use OPpCONST_FOLDED at all,
so having a macro to check that doesn’t help. If it is for B​::C’s sake,
then it won’t be present in older perls anyway, so you will still have
to #define it in B​::C.

Thanks and I'm fine with that. It thought it's a cool API for the time
when we have both.
But I haven't found a usecase for it yet.

So missing are​:

commit 715975678ef3c57515eb6282cdaab53da6afbb46
Author​: Niels Thykier <niels@​thykier.net>
Date​: Mon Jul 15 22​:25​:19 2013 +0200

  op.c​: Warn on "return $a or $b" [perl #59802]

  Add a warning for the (likely) unintended use of "return $a or $b"
  (and similar expressions), which perl parses as "(return $a) || $b"
  (which is effectively just "return $a;").

  Note this warning is triggered by some modules (e.g. Test​::Builder).
  These are not fixed by this commit.

  Signed-off-by​: Niels Thykier <niels@​thykier.net>

commit c1f7aff8b732c59411569240ce4ffc7a83080e33
Author​: Reini Urban <rurban@​x-ray.at>
Date​: Thu Jul 11 12​:09​:15 2013 -0500

  Return B​::HEK for B​::CV​::GV of lexical subs

  A lexsub has a hek instead of a gv.
  Provide a ref to a PV for the name in the new B​::HEK class.
  This crashed previously accessing the not existing SvFLAGS of the hek.

--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

@p5pRT
Copy link
Author

p5pRT commented Jul 22, 2013

From @rjbs

* Niels Thykier <niels@​thykier.net> [2013-07-16T04​:58​:01]

Here is a revised patch.
[…]
The warning is now emitted for any of the following ops​:
OP_{NEXT,LAST,REDO,RETURN,DIE,EXIT,GOTO}

Thanks, Niels, this warning is something I'm very happy to see finally
happening!

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Aug 20, 2013

From @cpansprout

On Mon Jul 22 14​:59​:51 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Niels Thykier <niels@​thykier.net> [2013-07-16T04​:58​:01]

Here is a revised patch.
[…]
The warning is now emitted for any of the following ops​:
OP_{NEXT,LAST,REDO,RETURN,DIE,EXIT,GOTO}

Thanks, Niels, this warning is something I'm very happy to see finally
happening!

This is currently blocked by Test​::Builder. If we are to have this
warning, we probably ought to have it present in bleadperl for as long
as possible, in order to shake out CPAN bugs.

Would it be acceptable to break the usual rule and patch Test​::Builder
directly in blead (assigning it a previously unused dev version)?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 23, 2013

From @rjbs

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2013-08-20T11​:51​:46]

Would it be acceptable to break the usual rule and patch Test​::Builder
directly in blead (assigning it a previously unused dev version)?

I've just poked Schwern both via GitHub and another channel. Let's give him a
day or two to state his intentions before patching blead... which we should do
if needed. (and if you do that, please add a ticket to #116923 which I will
follow up on)

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2013

From @rjbs

* Ricardo Signes <perl.p5p@​rjbs.manxome.org> [2013-08-22T22​:17​:24]

I've just poked Schwern both via GitHub and another channel. Let's give him
a day or two to state his intentions before patching blead... which we should
do if needed. (and if you do that, please add a ticket to #116923 which I
will follow up on)

FWIW, Schwern is now AFK for a week or so.

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2013

From @cpansprout

On Sun Aug 25 21​:28​:46 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Ricardo Signes <perl.p5p@​rjbs.manxome.org> [2013-08-22T22​:17​:24]

I've just poked Schwern both via GitHub and another channel. Let's
give him
a day or two to state his intentions before patching blead... which
we should
do if needed. (and if you do that, please add a ticket to #116923
which I
will follow up on)

FWIW, Schwern is now AFK for a week or so.

What does AFK stand for? Asking for kippers?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2013

From @Tux

On Mon, 26 Aug 2013 00​:09​:12 -0700, "Father Chrysostomos via RT"
<perlbug-followup@​perl.org> wrote​:

On Sun Aug 25 21​:28​:46 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Ricardo Signes <perl.p5p@​rjbs.manxome.org> [2013-08-22T22​:17​:24]

I've just poked Schwern both via GitHub and another channel. Let's
give him
a day or two to state his intentions before patching blead... which
we should
do if needed. (and if you do that, please add a ticket to #116923
which I
will follow up on)

FWIW, Schwern is now AFK for a week or so.

What does AFK stand for? Asking for kippers?

Away From Keyboard

http​://www.acronymfinder.com/Away-From-Keyboard-(AFK).html

--
H.Merijn Brand http​://tux.nl Perl Monger http​://amsterdam.pm.org/
using perl5.00307 .. 5.19 porting perl5 on HP-UX, AIX, and openSUSE
http​://mirrors.develooper.com/hpux/ http​://www.test-smoke.org/
http​://qa.perl.org http​://www.goldmark.org/jeff/stupid-disclaimers/

@p5pRT
Copy link
Author

p5pRT commented Aug 26, 2013

From @rurban

On Tue, Aug 20, 2013 at 10​:51 AM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

On Mon Jul 22 14​:59​:51 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Niels Thykier <niels@​thykier.net> [2013-07-16T04​:58​:01]

Here is a revised patch.
[…]
The warning is now emitted for any of the following ops​:
OP_{NEXT,LAST,REDO,RETURN,DIE,EXIT,GOTO}

Thanks, Niels, this warning is something I'm very happy to see finally
happening!

This is currently blocked by Test​::Builder. If we are to have this
warning, we probably ought to have it present in bleadperl for as long
as possible, in order to shake out CPAN bugs.

All possible CPAN bugs have already been reported by smoking
CPAN with a patched bleadperl with these warning enabled last month.
The fastest response was from DBI, the slowest seems to be Test​::Builder.
Now only BlackPAN is the problem. Our own company blackpan was okay.

See http​://blogs.perl.org/users/rurban/2013/07/smoking-cpan.html
All necessary distroprefs patches are in my repo
https://github.com/rurban/distroprefs

Would it be acceptable to break the usual rule and patch Test​::Builder
directly in blead (assigning it a previously unused dev version)?

--
Reini Urban
http​://cpanel.net/ http​://www.perl-compiler.org/

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2013

From @cpansprout

On Thu Aug 22 19​:17​:59 2013, perl.p5p@​rjbs.manxome.org wrote​:

* Father Chrysostomos via RT <perlbug-followup@​perl.org> [2013-08-
20T11​:51​:46]

Would it be acceptable to break the usual rule and patch
Test​::Builder
directly in blead (assigning it a previously unused dev version)?

I've just poked Schwern both via GitHub and another channel. Let's
give him a
day or two to state his intentions before patching blead... which we
should do
if needed. (and if you do that, please add a ticket to #116923 which
I will
follow up on)

I have applied Niels’ warning patch now as 9da2d04 (thank you).
His precedence patch has been applied as 13c65ef.

I have opened ticket #119825 to track this.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Sep 15, 2013

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

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

1 participant