return 0 or die; - Githubissues

p5pRT commented 15 years ago

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

Searchable as RT59802$

p5pRT commented 15 years ago

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 commented 15 years ago

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 commented 15 years ago

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

p5pRT commented 15 years ago

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 commented 15 years ago

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 commented 15 years ago

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 commented 15 years ago

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 commented 15 years ago

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 commented 15 years ago

From tchrist@perl.com

Return *is* low precedence already.

Compare:

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

with

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

--tom

p5pRT commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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`. :-) \

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

p5pRT commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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`. :-) \

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

Leon

p5pRT commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

From @Leont

On Wed\, Feb 20\, 2013 at 4:11 AM\, Tom Christiansen \tchrist@perl\.com wrote:

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?

Indeed I am (not convinced) that ((parentheses) always (make code (more readable))). Specially not when you're using multiple sets of parentheses. The precedence levels are such that most of the time you don't need any parentheses. IMHO they're extra noise in your code. They add to the mental effort required for parsing. In case of C\ and C\\, understanding In case of C\ and C\\, understanding they're the absolute lowest levels of precedence will make things much easier to read.

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.

To me that sounds like optimizing writability over readability. Then again\, style discussions are worst then bike-shedding\, there is no ultimate truth\, etcetera…

Leon

p5pRT commented 11 years ago

From @davidnicol

On Tue\, Feb 19\, 2013 at 9:11 PM\, Tom Christiansen \tchrist@perl\.com wrote:

21&#8203;: nonassoc    list operators $rightward$
22&#8203;: right       not
23&#8203;: left        and
24&#8203;: 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

question: would adding a 25th level for C\ break anything\, aside from these C\<return something verbose-short-circuiter somethingelse> instances that currently happen to be in production and working anyway? Maybe C\ could go there too?

answer: yes\, that would break any code that was lazily written by inserting a return or a die within a chain of C\ operators where the coder didn't bother to comment out all the unreachable ones: the code would suddenly return (or throw) different results.

conclusion: it's a feature.

example: given a routine like so:

sub check_for_various_conditions{ critical_condition or severe_condition or medium_condition or mild_condition or 0 }

and the coder who wrote it doesn't want to care about mild conditions any more\, at least during the current phase of the development cycle.

Instead of commenting out the mild_condition line\, someone who proudly understands Perl precedence decides to show off and insert "return 0 or" above it.

Does p5p support that coder?

p5pRT commented 11 years ago

From @mauke

On 19.02.2013 19:25\, Tom Christiansen wrote:

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

Add 'die die die "ABC"' and 'return return "XYZ"' to that list.

Hey\, die and return are idempotent. :-)

-- Lukas Mai \l\.mai@web\.de

p5pRT commented 11 years ago

From @Hugmeir

On Sun\, Mar 3\, 2013 at 12:01 AM\, Lukas Mai \l\.mai@web\.de wrote:

On 19.02.2013 19:25\, Tom Christiansen wrote:
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
Add 'die die die "ABC"' and 'return return "XYZ"' to that list.

Hey\, die and return are idempotent. :-)

BEGIN { *CORE::GLOBAL::die = sub { return unless $really_meant_it++; goto &CORE::die } } die die !!!111;

p5pRT commented 11 years ago

From miles@assyrian.org.uk

On 19 Feb 2013 at 17:07:05\, Jan Dubois wrote:

Just for the record\, I've also seen

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

I ran into this problem last night. The offending line was

return ($self->true_count($idx) > 0) xor ($self->false_count($idx) > 0);

and in this case it's easy to explain what I had in mind: I wanted my function to return the logical xor of those two expressions! Once I'd tracked the problem down it was easy enough to fix by reparenthesising\, but a warning would have been very useful.

Miles

p5pRT commented 10 years ago

From @jkeenan

On Tue Feb 19 02:54:23 2013\, nicholas wrote:

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

Is there anyone actually willing to take on the job of creating a warning that will run when code is unreachable?

Thank you very much. Jim Keenan

p5pRT commented 10 years ago

From @nthykier

Is there anyone actually willing to take on the job of creating a warning that will run when code is unreachable?

Thank you very much. Jim Keenan

Hi\,

Yes\, I have a prototype patch (see attached).

The warning is triggered in a couple of (cpan) modules included in perlcore. This list includes at least Test::Builder\, Parse::CPAN::Meta and JSON::PP. Each of these three modules have received a bug report[1].

I do also see a couple of test failures from applying the patch. From what I can tell\, they are all caused by the new warning showing up in the abovementioned modules. That said\, the new warning does not cause all tests triggering it to fail\, so I may I have missed a buggy module or two.

Thanks for considering\, ~Niels

PS: Please CC

[1] Test::Builder: https://github.com/schwern/test-more/pull/385

Parse::Cpan::Meta: https://rt.cpan.org/Ticket/Display.html?id=86947

JSON::PP: https://rt.cpan.org/Ticket/Display.html?id=86948

p5pRT commented 10 years ago

From @nthykier

0001-op.c-Warn-on-return-a-or-b-perl-59802.patch

```diff From c0b975eae5c835f016a102ca6616432c37105c61 Mon Sep 17 00:00:00 2001 From: Niels Thykier Date: Mon, 15 Jul 2013 22:25:19 +0200 Subject: [PATCH] op.c: Warn on "return $a or $b" [perl #59802] Add a warning for the (likely) unintended use of "return $a or $b", 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 --- op.c | 13 +++++++++++++ pod/perldiag.pod | 17 +++++++++++++++++ t/lib/warnings/op | 17 +++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/op.c b/op.c index d5323a0..97d9214 100644 --- a/op.c +++ b/op.c @@ -5827,6 +5827,19 @@ S_new_logop(pTHX_ I32 type, I32 flags, OP** firstp, OP** otherp) first = *firstp; other = *otherp; + /* [perl #59802]: Warn about "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. + + XXX: Currently we allow people to "shoot themselves in the foot" + by explicitly writing "(return $a) or $b". It is not ambiguous, + but it is probably not very useful either. + */ + if (first->op_type == OP_RETURN && !(first->op_flags & OPf_PARENS)) { + Perl_ck_warner(aTHX_ packWARN(WARN_SYNTAX), + "Possible precedence issue with return"); + } + 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..8f1a7dc 100644 --- a/pod/perldiag.pod +++ b/pod/perldiag.pod @@ -6353,6 +6353,23 @@ what you want, put an & in front.) not get any randomness out of your system. This usually indicates Something Very Wrong. +=item Possible precedence issue with return + +(W syntax) There is a possible problem with the mixing of a C +and a low-precedence operator like C. 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. + =back =head1 SEE ALSO diff --git a/t/lib/warnings/op b/t/lib/warnings/op index c38bcde..2ef5b52 100644 --- a/t/lib/warnings/op +++ b/t/lib/warnings/op @@ -1575,3 +1575,20 @@ OPTION regex ?(?s).* Subroutine DynaLoader::dl_error redefined at $eval 2$ line 2\. ######## +# op.c +use warnings; +sub warn_1 { return $a or $b; } +sub warn_2 { return $a and $b; } +sub warn_3 { return $a xor $b; } + +sub dont_warn_1 { return ($a or $b); } +sub dont_warn_2 { return ($a and $b); } +sub dont_warn_3 { return ($a xor $b); } +# These are weird, but at least not ambiguous. +sub dont_warn_4 { (return $a) or $b; } +sub dont_warn_5 { (return $a) and $b; } +sub dont_warn_6 { (return $a) xor $b; } +EXPECT +Possible precedence issue with return at - line 3. +Possible precedence issue with return at - line 4. +Possible precedence issue with return at - line 5. -- 1.7.10.4 ```

p5pRT commented 10 years ago

From @cpansprout

On Mon Jul 15 15:53:55 2013\, niels@thykier.net wrote:

Is there anyone actually willing to take on the job of creating a warning that will run when code is unreachable?

Thank you very much. Jim Keenan

Hi\,

Yes\, I have a prototype patch (see attached).

The warning is triggered in a couple of (cpan) modules included in perlcore. This list includes at least Test::Builder\, Parse::CPAN::Meta and JSON::PP. Each of these three modules have received a bug report[1].

I do also see a couple of test failures from applying the patch. From what I can tell\, they are all caused by the new warning showing up in the abovementioned modules. That said\, the new warning does not cause all tests triggering it to fail\, so I may I have missed a buggy module or two.

Thanks for considering\, ~Niels

Thank you. The patch looks good (though I haven’t tested it yet). I have one quibble\, though: perldiag’s messages are in alphabetical order\, but the patch does not respect that.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @cpansprout

On Mon Jul 15 18:20:35 2013\, sprout wrote:

On Mon Jul 15 15:53:55 2013\, niels@thykier.net wrote:

Is there anyone actually willing to take on the job of creating a warning that will run when code is unreachable?

Thank you very much. Jim Keenan

Hi\,

Yes\, I have a prototype patch (see attached).

The warning is triggered in a couple of (cpan) modules included in perlcore. This list includes at least Test::Builder\, Parse::CPAN::Meta and JSON::PP. Each of these three modules have received a bug report[1].

Thank you.

I do also see a couple of test failures from applying the patch. From what I can tell\, they are all caused by the new warning showing up in the abovementioned modules.

That is sufficient reason to wait until newer versions are released and then imported into perl.git before applying the patch.

That said\, the new warning does not cause all tests triggering it to fail\, so I may I have missed a buggy module or two.

Thanks for considering\, ~Niels

Thank you. The patch looks good (though I haven’t tested it yet). I have one quibble\, though: perldiag’s messages are in alphabetical order\, but the patch does not respect that.

One more thing: This could be extended to other ops that don’t return\, such as last\, goto\, die\, etc.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @nthykier

On 2013-07-16 05:31\, Father Chrysostomos via RT wrote:

[...]

Thank you. The patch looks good (though I haven’t tested it yet). I have one quibble\, though: perldiag’s messages are in alphabetical order\, but the patch does not respect that.

Here is a revised patch.

The warning has been inserted in its proper place in perldiag.pod.

One more thing: This could be extended to other ops that don’t return\, such as last\, goto\, die\, etc.

The warning is now emitted for any of the following ops: OP_{NEXT\,LAST\,REDO\,RETURN\,DIE\,EXIT\,GOTO}

I noticed and have documented that some possible intended uses of this precedence. An example being:

use constant FEATURE => 0; sub { not FEATURE and return or do_feature(); }

perl correctly parses this as:

sub { (not FEATURE and return) or do_feature(); }

but it still causes a warning. I am not sure how to detect this case\, so I am not able to devise a fix for this false-positive (other than recommending that people add the parentheses themselves to express the intended grouping).

The problem in detecting this is that by the time the warning is emitted\, constant folding has eliminated the context. The steps are (something like):

Start with "not FEATURE # and return or do_feature();". (The "#" denotes the part not parsed/examined yet).

Fold "not FEATURE" to 1 leaving "1 # and return or do_feature();"

Parse/Consume "and return" giving "1 and return # or do_feature();"

Reduce "1 and return #or do_feature();" to "return # or do_feature();"

Parse/Consume "or do_feature();" giving "return or do_feature();"

Warn as "return" is on the left-hand side of a (logical) binary operator (and is not enclosed in parentheses).

~Niels

p5pRT commented 10 years ago

From @nthykier

0001-op.c-Warn-on-return-a-or-b-perl-59802.patch

```diff From 60b596a9f686752e5dea73e480c551b02135821e Mon Sep 17 00:00:00 2001 From: Niels Thykier Date: Mon, 15 Jul 2013 22:25:19 +0200 Subject: [PATCH] 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 --- op.c | 38 ++++++++++++++++++++++++++++++++++++++ pod/perldiag.pod | 25 +++++++++++++++++++++++++ t/lib/warnings/op | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 104 insertions(+) diff --git a/op.c b/op.c index d5323a0..6e9dcd2 100644 --- a/op.c +++ b/op.c @@ -5827,6 +5827,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: Missing some way to detect that the control flow was not + involved in a "FEATURE_TEST && return or do_something()". + + XXX: Currently we allow people to "shoot themselves in the + foot" by explicitly writing "(return $a) or $b". It allows + people to disable the warning in cases like: + + sub { (FEATURE or return) and do_feature() } + + */ + if (!(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..5d810f7 100644 --- a/pod/perldiag.pod +++ b/pod/perldiag.pod @@ -4229,6 +4229,31 @@ 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) and a low-precedence operator like +C. 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; } + + use constant FEATURE => 0; + sub { not FEATURE and return or do_feature(); } + =item Possible unintended interpolation of $\ in regex (W ambiguous) You said something like C in a regex. diff --git a/t/lib/warnings/op b/t/lib/warnings/op index c38bcde..db218b0 100644 --- a/t/lib/warnings/op +++ b/t/lib/warnings/op @@ -1575,3 +1575,44 @@ 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 dt_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); } + +# These first two warns if the parentheses surrounding FEATURE are +# removed. +# - optimially, it wouldn't warn even then since FEATURE is a constant +# being inlined (it could be a result of a "$[ >= ..." test). +sub dont_warn_4 { (MISSING_FEATURE and return) or dont_warn_3(); } +sub dont_warn_5 { (FEATURE or 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 commented 10 years ago

From @nthykier

On 2013-07-16 05:31\, Father Chrysostomos via RT wrote:

[...]

I do also see a couple of test failures from applying the patch. From what I can tell\, they are all caused by the new warning showing up in the abovementioned modules.

That is sufficient reason to wait until newer versions are released and then imported into perl.git before applying the patch.

Certainly.

That said\, the new warning does not cause all tests triggering it to fail\, so I may I have missed a buggy module or two.

Thanks for considering\, ~Niels

Thank you. The patch looks good (though I haven’t tested it yet). I have one quibble\, though: perldiag’s messages are in alphabetical order\, but the patch does not respect that.

One more thing: This could be extended to other ops that don’t return\, such as last\, goto\, die\, etc.

Seems reasonable\, will look into it.

[From previous mail]

Thank you. The patch looks good (though I haven’t tested it yet). I have one quibble\, though: perldiag’s messages are in alphabetical order\, but the patch does not respect that.

Ah\, haven't noticed that. Will fix that in the next revision.

~Niels

p5pRT commented 10 years ago

From @cpansprout

On Tue Jul 16 13:35:28 2013\, niels@thykier.net wrote:

On 2013-07-16 05:31\, Father Chrysostomos via RT wrote:

[...]

Thank you. The patch looks good (though I haven’t tested it yet). I have one quibble\, though: perldiag’s messages are in alphabetical order\, but the patch does not respect that.

Here is a revised patch.

The warning has been inserted in its proper place in perldiag.pod.

One more thing: This could be extended to other ops that don’t return\, such as last\, goto\, die\, etc.

The warning is now emitted for any of the following ops: OP_{NEXT\,LAST\,REDO\,RETURN\,DIE\,EXIT\,GOTO}

I noticed and have documented that some possible intended uses of this precedence. An example being:
use constant FEATURE => 0;
sub \{ not FEATURE and return or do\_feature; \}
perl correctly parses this as:
sub \{ $not FEATURE and return$ or do\_feature; \}
but it still causes a warning. I am not sure how to detect this case\, so I am not able to devise a fix for this false-positive (other than recommending that people add the parentheses themselves to express the intended grouping).

I have an idea: We have three spare bits in op->op_spare. We could use one of them to indicate folding.

We already set OPf_SPECIAL on regexp and tr operators in S_new_logop and newCONDOP to indicate this. We also set OPpCONST_FOLDED on constants in those two places.

If we also set\, say\, op->op_folded to 1 in those same code paths\, then we can detect the fold.

(And we could simplify the code by having match-like ops and constants us the same bit.)

--

Father Chrysostomos

p5pRT commented 10 years ago

From @nthykier

On 2013-07-17 00:54\, Father Chrysostomos via RT wrote:

I have an idea: We have three spare bits in op->op_spare. We could use one of them to indicate folding.

That would be possible\, indeed.

We already set OPf_SPECIAL on regexp and tr operators in S_new_logop and newCONDOP to indicate this. We also set OPpCONST_FOLDED on constants in those two places.

If we also set\, say\, op->op_folded to 1 in those same code paths\, then we can detect the fold.

(And we could simplify the code by having match-like ops and constants us the same bit.)

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? 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?

~Niels

p5pRT commented 10 years ago

From @cpansprout

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

On 2013-07-17 00:54\, Father Chrysostomos via RT wrote:

I have an idea: We have three spare bits in op->op_spare. We could use one of them to indicate folding.

That would be possible\, indeed.

We already set OPf_SPECIAL on regexp and tr operators in S_new_logop and newCONDOP to indicate this. We also set OPpCONST_FOLDED on constants in those two places.

If we also set\, say\, op->op_folded to 1 in those same code paths\, then we can detect the fold.

(And we could simplify the code by having match-like ops and constants us the same bit.)

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.

--

Father Chrysostomos

p5pRT commented 10 years ago

From @nthykier

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.

~Niels

p5pRT commented 10 years ago

From @nthykier

0001-op.c-Add-op_folded-to-BASEOP.patch

```diff From 422e79d529540c15c6795d9a252329541eb05244 Mon Sep 17 00:00:00 2001 From: Niels Thykier 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 --- 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 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 commented 10 years ago

From @nthykier

0002-op.c-Warn-on-return-a-or-b-perl-59802.patch

```diff From 1c7ceaba32801cdb7b754628fc6996b952a5b79f Mon Sep 17 00:00:00 2001 From: Niels Thykier 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 --- 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) and a low-precedence operator like +C. 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 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 commented 10 years ago

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

Perl / perl5

return 0 or die; #9523

From rvtol@xs4all.nl

Created by rvtol@xs4all.nl

From @druud62

From Eirik-Berg.Hanssen@allverden.no

From @druud62

From @druud62

From @davidnicol

From olav@genebio.com

From tchrist@perl.com

From @jkeenan

From @demerphq

From @druud62

From @nwc10

From @Leont

From @ap

From @rjbs

From tchrist@perl.com

From tchrist@perl.com

From tchrist@perl.com

From @demerphq

From @Hugmeir

From tchrist@perl.com

From @Leont

From @Leont

From @ap

From tchrist@perl.com

From @rjbs

From @jandubois

From @Leont

From tchrist@perl.com

From @Leont

From @davidnicol

From @mauke

From @Hugmeir

From miles@assyrian.org.uk

From @jkeenan

From @nthykier

From @nthykier

From @cpansprout

From @cpansprout

From @nthykier

From @nthykier

From @nthykier

From @cpansprout

From @nthykier

From @cpansprout

From @nthykier

From @nthykier

From @nthykier

From @cpansprout