Bug #57638 for Perl-Critic: Subroutines::RequireFinalReturn should ignore constant subs

Wed May 19 13:36:09 2010 dolmen [...] cpan.org - Ticket created

Subject:

Subroutines::RequireFinalReturns should ignore constant subs

The policy Subroutines::RequireFinalReturns should not raise a problem with subs which are in fact constant declaration: sub MY_INTEGER { 1 } sub MY_STRING { "Hello" } -- Olivier Mengué - http://o.mengue.free.fr/

Wed May 19 16:24:10 2010 jeff [...] imaginative-software.com - Correspondence added

Subject:	Re: [rt.cpan.org #57638] Subroutines::RequireFinalReturns should ignore constant subs
Date:	Wed, 19 May 2010 13:23:51 -0700
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Jeffrey Thalhammer <jeff [...] imaginative-software.com>

On May 19, 2010, at 10:36 AM, Olivier 'dolmen' Mengué via RT wrote: Show quoted text

> The policy Subroutines::RequireFinalReturns should not raise a problem > with subs which are in fact constant declaration: > > sub MY_INTEGER { 1 } > sub MY_STRING { "Hello" }

That's an interesting pattern. Why wouldn't you use the constant or Readonly pragmas for those? Do you override those subs (in subclasses, for example)? Detecting a sub that contains only a literal string or number is fairly easy. But in the general case, detecting a constant expression is not trivial... sub foo { oct( 1 + 7 ) . "bar" } That may be nonsensical, but technically, it is a constant expression. So I think what you probably want is something more specific. I could imagine a Policy to enforce that all subs named in ALL_CAPS must not have returns, but that is a slightly different rule. In any case, I doubt we would ever add this to the Perl::Critic core. I think Conway would feel the merits of consistency (i.e. always use "return") are more important than the convenience of not using "return" in a small set of cases. But you could certainly release your own variant of RequireFinalReturns as a plugin :] But if you think I'm wrong, you're welcome to try and persuade me. -Jeff

Wed May 19 16:24:12 2010 The RT System itself - Status changed from 'new' to 'open'

Wed May 19 20:15:08 2010 perl [...] galumph.com - Correspondence added

Subject:	Re: [rt.cpan.org #57638] Subroutines::RequireFinalReturns should ignore constant subs
Date:	Wed, 19 May 2010 19:14:54 -0500
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Elliot Shank <perl [...] galumph.com>

On 5/19/10 12:36 PM, Olivier 'dolmen' Mengué via RT wrote: Show quoted text

> The policy Subroutines::RequireFinalReturns should not raise a problem > with subs which are in fact constant declaration: > > sub MY_INTEGER { 1 } > sub MY_STRING { "Hello" }

Actually, those are not constant declarations; if you want those, you need to use an empty prototype: sub MY_INTEGER () { 1 } sub MY_STRING () { 'Hello' } See "Constant Functions" in perlsub. But using the constant pragma or the Readonly module would be better because other people reading your code wouldn't need to understand why the prototypes need to be there. use constant MY_INTEGER => 1; use Readonly; Readonly::Scalar my $MY_STRING => 'HELLO';

Mon May 24 15:49:23 2010 dolmen [...] cpan.org - Correspondence added

Le Mer 19 Mai 2010 16:24:10, jeff@imaginative-software.com a écrit : Show quoted text

> That's an interesting pattern. Why wouldn't you use the constant or > Readonly pragmas for those? Do you override those subs (in > subclasses, for example)?

The Perl compiler knows how to /inline/ subs which are composed of just one value to return, *and which have a '()' prototype* (I missed this last part in the description of the issue). So the real issue is for: sub MY_INTEGER () { 1 } sub MY_STRING () { "Hello" } So this is a good way to create constants that are portable without requiring 'constant' or 'ReadOnly', and this is also faster at runtime. Try this: perl -MO=Deparse -e 'sub HELLO () { "Hello\n" } print HELLO;' See "Constant functions" in perlsub: http://perldoc.perl.org/perlsub.html#Constant-Functions Show quoted text

> Detecting a sub that contains only a literal string or number is > fairly easy. But in the general case, detecting a constant > expression is not trivial... > > sub foo { oct( 1 + 7 ) . "bar" }

Perl knows tries to inline if the prototype is () sub foo() { oct( 1 + 7 ) . "bar" } The general case is the one that Perl 5 knows how to inline. perl -MO=Deparse -e 'sub foo () { oct( 1 + 7 ) . "bar" } print foo;' Show quoted text

> That may be nonsensical, but technically, it is a constant expression. > So I think what you probably want is something more specific. I > could imagine a Policy to enforce that all subs named in ALL_CAPS > must not have returns, but that is a slightly different rule.

I agree this would be a different, and this is not my issue. Show quoted text

> In any case, I doubt we would ever add this to the Perl::Critic core. > I think Conway would feel the merits of consistency (i.e. always > use "return") are more important than the convenience of not using > "return" in a small set of cases. But you could certainly release > your own variant of RequireFinalReturns as a plugin :] > > But if you think I'm wrong, you're welcome to try and persuade me.

I hope so, now that I've made the exception rule stricter. ;) I just want an exception to the rule for a common Perl idiom which makes programs readable but still portable with old Perl which do not have the 'constant' module. Also the point of the RequireFinalReturn rule is to make code more readable. But the case of a single constant expression is already readable and adding "return" only adds verbosity. So, fixing the simple case of a one-term sub would be a step in the good direction. -- Olivier Mengué - http://o.mengue.free.fr/

Mon May 24 15:49:51 2010 dolmen [...] cpan.org - Subject changed from 'Subroutines::RequireFinalReturns should ignore constant subs' to 'Subroutines::RequireFinalReturn should ignore constant subs'

Mon May 24 18:33:48 2010 dolmen [...] cpan.org - Dependency on ticket #57807 added

Mon May 24 23:22:34 2010 jeff [...] imaginative-software.com - Correspondence added

Subject:	Re: [rt.cpan.org #57638] Subroutines::RequireFinalReturns should ignore constant subs
Date:	Mon, 24 May 2010 20:21:30 -0700
To:	bug-Perl-Critic [...] rt.cpan.org
From:	Jeffrey Thalhammer <jeff [...] imaginative-software.com>

On May 24, 2010, at 12:49 PM, Olivier 'dolmen' Mengué via RT wrote: Show quoted text

> I just want an exception to the rule for a common Perl idiom which makes > programs readable but still portable with old Perl which do not have the > 'constant' module.

This may be the heart of the problem. I think you are basically asking for a Policy that enforces a best practice for a particular version(s) of Perl. And this is certainly a reasonable request, but I think it sends Perl-Critic to the edge of a slippery slope. In my mind, using a recent Perl is an implicit best practice (there is even a Policy plugin that makes it explicit). However, I understand this isn't always feasible. But if we start considering the limitations of older Perls, then all kinds of second-best practices start to emerge. It is perfectly reasonable to try and enforce a second-best practices when the best practice is not available. But I feel this is out of scope for the Perl-Critic core, and I fear there are just too many possible permutations to consider. Moreover, there is much less consensus on what those second-best practices might actually be. Show quoted text

> Also the point of the RequireFinalReturn rule is to make code more > readable. But the case of a single constant expression is already > readable and adding "return" only adds verbosity.

That is debatable. There are several Policies that require you to type more code (ProhibitDoubleSigils, for example). In most cases, the underlying belief is that the verbosity is worth the added clarity it provides. And as I've said before, consistency also has a virtue. I can understand why you might disagree, but I believe your reasons are more subjective than you may realize. Ultimately, I feel your request is too specific to be implemented in the core. And I commend you for seeking to adjust the existing Policy, rather than just disabling it completely. But in this case, I think the right solution is a custom Policy. If you haven't already done so, check out http://search.cpan.org/~elliotjs/Perl-Critic-1.106/lib/Perl/Critic/DEVELOPER.pod and then gaze at http://cpansearch.perl.org/src/ELLIOTJS/Perl-Critic-1.106/lib/Perl/Critic/Policy/Subroutines/RequireFinalReturn.pm. I bet you can write the Policy you want in less than hour! But if Elliot and Tom (the other Perl-Critic maintainers) agree this should be a core feature, then I will yield. Jeffrey Thalhammer Imaginative Software Systems vcard: http://www.imaginative-software.com/contact/jeff.vcf

Tue May 25 19:13:49 2010 wyant [...] cpan.org - Correspondence added

On Mon May 24 23:22:34 2010, jeff@imaginative-software.com wrote: Show quoted text

> > But if Elliot and Tom (the other Perl-Critic maintainers) agree this > should be a core feature, then I will yield.

Thanks for the vote of confidence. I have some sympathy for Mr. Mengué's need to deal with old Perls -- he may have no say about his run time environment, he has a job to do, and I am inclined to help him if I can see a way to do so. On the other hand, the first statement in constant.pm is 'use 5.005;', and it might be reasonable to say that whoever made the decision to stick with Perl 5.004 deserves the consequences of that decision. My concern about implementing a constant subroutine detector is that I am not sure I am smart enough to detect a constant subroutine. Even Mr. Mengué's original examples are not completely straightforward. Before you know that sub { "Hello" } is a constant subroutine, you have to establish that nothing is interpolated into the string. This can be done, of course. But then I think of other things that would have to be checked. They can all be checked as well, but after a while I begin to doubt that I can come up with a reasonably complete list of things to check. Then you get questions like what to do with subroutines. Something like sub { oct 17 } is clearly a constant. Something like sub { rand 17 } is clearly not. So the policy needs to know about built-ins. If it does not, we get a bug report about the former case. The 'constant' pragma does not have this problem because it evaluates the the expression only once. Yes, I'm the author of ValuesAndExpressions::RequireConstantVersion. But if I had thought a little more about it I might well have shied away from that as well. There have been RT tickets on that involving code I would never have thought of using to generate a module version. And a constant subroutine is even less constrained. RequireConstantVersion is out in the wild now, and I have to live with my sins, but that does not mean I have to repeat them. A more robust approach (from the Perl::Critic standpoint, anyway) might be to do what Subroutines::RequireArgUnpacking does -- allow an exception for short subroutines, controlled by a configuration option ('short_subroutine_statements') which defaults to 0. Yes, RequireFinalReturn is a Perl Best Practices policy, but so is RequireArgUnpacking. And done this way there is no implied guarantee that we can detect whatever weird constant subroutine anyone can think up.

Bug #57638 for Perl-Critic: Subroutines::RequireFinalReturn should ignore constant subs

Preferred bug tracker