Skip Menu |
 

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 79231
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: martin.sarfy [...] sokordia.cz
Cc:
AdminCc:

Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)



Subject: "return undef" policy
Date: Sun, 26 Aug 2012 20:13:06 +0200
To: bug-Perl-Critic [...] rt.cpan.org
From: Martin Šárfy <martin.sarfy [...] sokordia.cz>
Hi, I am getting complains when I use "return undef". I do not think that a simple "return" is always correct. Imagine following functions: sub check { return undef; } # returns boolean sub list { return () } # returns array sub find { return wantarray ? () : undef } # returns an object or undef If you always use "return;", then check() and find() will not work as expected. On the other hand, if you always use "return undef", then list() will be broken. The safest method is to always use "return wantarray ? () : undef", but it's too much typing. I think it's a bug if Perl::Critic is always requiring "return;", because functions returning "object or undef" are quite common and soon or later you will use such function in a list context (in push or in hash definition). E.g.: sub find { return } # perfectly OK for Perl::Critic my $x = { object => find(), foo => "bar" }; print "foo => ".$x->{foo}."\n"; # whoops, does NOT print "bar" Martin -- Mgr. Martin Šárfy Sokordia, s.r.o. tel.: +420 608-883-159 martin.sarfy@sokordia.cz http://www.sokordia.cz
Download (untitled) / with headers
text/plain 1.1k
On Sun Aug 26 14:13:46 2012, martin.sarfy@sokordia.cz wrote: Show quoted text
> Hi, > > I am getting complains when I use "return undef". > > I do not think that a simple "return" is always correct. Imagine following > functions: > > sub check { return undef; } # returns boolean > sub list { return () } # returns array > sub find { return wantarray ? () : undef } # returns an object or undef > > If you always use "return;", then check() and find() will not work as > expected. On the other hand, if you always use "return undef", then list() > will be broken. > > The safest method is to always use "return wantarray ? () : undef", but > it's too much typing. > > I think it's a bug if Perl::Critic is always requiring "return;", because > functions returning "object or undef" are quite common and soon or later > you will use such function in a list context (in push or in hash > definition). E.g.: > > sub find { return } # perfectly OK for Perl::Critic > my $x = { object => find(), foo => "bar" }; > print "foo => ".$x->{foo}."\n"; # whoops, does NOT print "bar" >
Your wantarray method won’t solve that. These are all equivalent: return; return (); return wantarray ? () : undef;
Subject: Re: [rt.cpan.org #79231] "return undef" policy
Date: Mon, 27 Aug 2012 12:36:10 +0200
To: bug-Perl-Critic [...] rt.cpan.org
From: Martin Šárfy <martin.sarfy [...] sokordia.cz>
Download (untitled) / with headers
text/plain 1.6k
On Sun, Aug 26, 2012 at 10:11 PM, Father Chrysostomos via RT < bug-Perl-Critic@rt.cpan.org> wrote: Show quoted text
> <URL: https://rt.cpan.org/Ticket/Display.html?id=79231 > > > On Sun Aug 26 14:13:46 2012, martin.sarfy@sokordia.cz wrote:
> > Hi, > > > > I am getting complains when I use "return undef". > > > > I do not think that a simple "return" is always correct. Imagine
> following
> > functions: > > > > sub check { return undef; } # returns boolean > > sub list { return () } # returns array > > sub find { return wantarray ? () : undef } # returns an object or undef > > > > If you always use "return;", then check() and find() will not work as > > expected. On the other hand, if you always use "return undef", then
> list()
> > will be broken. > > > > The safest method is to always use "return wantarray ? () : undef", but > > it's too much typing. > > > > I think it's a bug if Perl::Critic is always requiring "return;", because > > functions returning "object or undef" are quite common and soon or later > > you will use such function in a list context (in push or in hash > > definition). E.g.: > > > > sub find { return } # perfectly OK for Perl::Critic > > my $x = { object => find(), foo => "bar" }; > > print "foo => ".$x->{foo}."\n"; # whoops, does NOT print "bar" > >
> > Your wantarray method won’t solve that. These are all equivalent: > > return; > return (); > return wantarray ? () : undef; >
You're right. But in find() above, "return undef" must be used, but Perl::Critic does not like it and is suggesting erroneous solution. (Because of this, I broke my code trying to make it clean...) M. -- Martin Sarfy
Download (untitled) / with headers
text/plain 3.6k
On Mon Aug 27 06:36:49 2012, martin.sarfy@sokordia.cz wrote: Show quoted text
> On Sun, Aug 26, 2012 at 10:11 PM, Father Chrysostomos via RT < > bug-Perl-Critic@rt.cpan.org> wrote: >
> > <URL: https://rt.cpan.org/Ticket/Display.html?id=79231 > > > > > On Sun Aug 26 14:13:46 2012, martin.sarfy@sokordia.cz wrote:
> > > Hi, > > > > > > I am getting complains when I use "return undef". > > > > > > I do not think that a simple "return" is always correct. Imagine
> > following
> > > functions: > > > > > > sub check { return undef; } # returns boolean > > > sub list { return () } # returns array > > > sub find { return wantarray ? () : undef } # returns an object or
> undef
> > > > > > If you always use "return;", then check() and find() will not work
> as
> > > expected. On the other hand, if you always use "return undef",
> then
> > list()
> > > will be broken. > > > > > > The safest method is to always use "return wantarray ? () :
> undef", but
> > > it's too much typing. > > > > > > I think it's a bug if Perl::Critic is always requiring "return;",
> because
> > > functions returning "object or undef" are quite common and soon or
> later
> > > you will use such function in a list context (in push or in hash > > > definition). E.g.: > > > > > > sub find { return } # perfectly OK for Perl::Critic > > > my $x = { object => find(), foo => "bar" }; > > > print "foo => ".$x->{foo}."\n"; # whoops, does NOT print "bar" > > >
> > > > Your wantarray method won’t solve that. These are all equivalent: > > > > return; > > return (); > > return wantarray ? () : undef; > >
> > You're right. But in find() above, "return undef" must be used, but > Perl::Critic does not like it and is suggesting erroneous solution. > > (Because of this, I broke my code trying to make it clean...) > > M. >
Any time you make a code change you risk breaking something. Making code changes recommended by Perl::Critic is no different. I broke a bunch of my own code implementing Subroutines::RequireArgUnpacking, mostly by missing 'shift' operations that used the default argument. Subroutines::ProhibitExplicitReturnUndef is a Perl Best Practices policy -- that is, it implements a recommendation in that book. But the policy is not a replacement for the book, and you should see the book for full details of why Damien Conway thinks it is a good thing to do. Given compliance to this policy, There's More Than One Way To Do It. My favorite (because it works also for subroutines you have no control over, such as localtime()) is to force scalar context. In your find() example, that would be sub find { return } # perfectly OK for Perl::Critic my $x = { object => scalar find(), foo => "bar" }; # ------ print "foo => ".$x->{foo}."\n"; # DOES print "bar" I also note that, if you follow another Perl::Critic policy and 'use warnings,' Perl in fact warns you that you have an odd number of values in a hash in your original code. That said, you may not agree with either the policy or my recommendation on how to code with it. That's OK -- Perl::Critic is intended to be a tool, not a straitjacket. I myself do not use all policies. If you don't agree with this policy, you have two ways to deal with it: * If you like the idea in general, but have certain subroutines that you always want to return a single value from, regardless of context, you can tell Perl::Critic "I meant to do that", as follows: sub find { return undef; ## no critic (ProhibitExplicitReturnUndef) } * If you simply believe the policy should never be applied to your code, put the following line in your .perlcriticrc configuration file. [-Subroutines::ProhibitExplicitReturnUndef] Tom
Subject: Re: [rt.cpan.org #79231] "return undef" policy
Date: Mon, 10 Sep 2012 15:21:51 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 1.2k
On Aug 26, 2012, at 11:13 AM, Martin Šárfy via RT wrote: Show quoted text
> functions returning "object or undef" are quite common and soon or later > you will use such function in a list context (in push or in hash definition).
That is true, so there are some cases where this rule does not apply. You have to make a judgement based on what the function can return and how you typically intend it to be used. If a function only makes sense as a scalar, then returning an explicit undef is probably fine... sub some_scalar_function { return undef } %hash = (key => some_scalar_function); but if the function could also (or only) return a list, then you'll usually want to return an empty list instead... sub some_list_function { return } for (some_list_function) {…} and then it is up to the caller to change the context when necessary... %hash = (key => scalar some_list_function); The gist of the ProhibitReturnUndef Policy is to discourage list functions from returning undef when called in list context. This actually prevents quite a few bugs, especially when those functions encounter unusual (but valid) edge cases like an empty file or an empty query result. I'll try and improve the documentation for this Policy. -Jeff


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.