Thread: [PATCH] Clear up perlcritic 'missing return' warning

[PATCH] Clear up perlcritic 'missing return' warning

From
Mike Blackwell
Date:
This is the first in a series of patches to reduce the number of warnings from perlcritic.   The current plan is to submit a separate patch for each warning or small set of related warnings, to make reviewing more straightforward.

This particular patch addresses the warning caused by falling off the end of a subroutine rather than explicitly returning.  It also contains additions to the perlcritic configuration file to ignore a couple of warnings that seem acceptable, bringing it in line with the buildfarm's configuration for those warnings.

Thanks to Andrew for assistance getting my environment set up.

Mike Blackwell
Attachment

Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Michael Paquier
Date:
On Mon, May 21, 2018 at 08:07:03PM -0500, Mike Blackwell wrote:
> This particular patch addresses the warning caused by falling off the end
> of a subroutine rather than explicitly returning.

Do we really want to make that a requirement?  Making sure that there is
a return clause if a subroutine returns a result makes sense to me, but
making it mandatory if the subroutine does not return anything and
callers don't expect it do is not really a gain in my opinion.  And this
maps with any C code.

Just today, I coded a perl subroutine which does not return any
results...  This is most likely going to be forgotten.
--
Michael

Attachment

Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Mike Blackwell
Date:
On Tue, May 22, 2018 at 3:32 AM, Michael Paquier <michael@paquier.xyz> wrote:

<snip> And this
maps with any C code.

The important differences here are:
  *) Declaring a C function as void prevents returning a value.  The intent not to return a value is clear to any caller and is enforced by the compiler.  There is no equivalent protection in Perl.
  *) Falling off the end of a C function that returns a type other than void has undefined results.  Perl will always return the value of the last statement executed.

Because Perl does allow returning a value without explicitly using return, it's easy to write code that breaks if an unwary person adds a line to the end of the subroutine.  There's a common constructor incantation that has this problem.  It's a real gotcha for C programmers just starting to poke around in Perl code.  

This difference also allows users of .pm modules to abuse the API of a method intended to be "void", if the value returned falling off the end happens to seem useful, leading to breakage if the method's code changes in the future.
 
This is most likely going to be forgotten.

That's what perlcritic is for. :)

Mike

Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Andrew Dunstan
Date:
On Tue, May 22, 2018 at 4:35 PM, Mike Blackwell <maiku41@gmail.com> wrote:
> On Tue, May 22, 2018 at 3:32 AM, Michael Paquier <michael@paquier.xyz>
> wrote:
>>
>>
>> <snip> And this
>> maps with any C code.
>
>
> The important differences here are:
>   *) Declaring a C function as void prevents returning a value.  The intent
> not to return a value is clear to any caller and is enforced by the
> compiler.  There is no equivalent protection in Perl.
>   *) Falling off the end of a C function that returns a type other than void
> has undefined results.  Perl will always return the value of the last
> statement executed.
>
> Because Perl does allow returning a value without explicitly using return,
> it's easy to write code that breaks if an unwary person adds a line to the
> end of the subroutine.  There's a common constructor incantation that has
> this problem.  It's a real gotcha for C programmers just starting to poke
> around in Perl code.
>
> This difference also allows users of .pm modules to abuse the API of a
> method intended to be "void", if the value returned falling off the end
> happens to seem useful, leading to breakage if the method's code changes in
> the future.
>
>>
>> This is most likely going to be forgotten.
>
>
> That's what perlcritic is for. :)
>
> Mike
>

I should also point out that Mike posted on this subject back on May
11, and nobody but me replied.

And yes, the idea is that if we do this then we adopt a perlcritic
policy that calls it out when we forget.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Alvaro Herrera
Date:
On 2018-May-23, Andrew Dunstan wrote:

> And yes, the idea is that if we do this then we adopt a perlcritic
> policy that calls it out when we forget.

If we can have a buildfarm animal that runs perlcritic that starts green
(and turns yellow with any new critique), with a config that's also part
of our tree, so we can easily change it as we see fit, it should be
good.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Andrew Dunstan
Date:
On Wed, May 23, 2018 at 1:45 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2018-May-23, Andrew Dunstan wrote:
>
>> And yes, the idea is that if we do this then we adopt a perlcritic
>> policy that calls it out when we forget.
>
> If we can have a buildfarm animal that runs perlcritic that starts green
> (and turns yellow with any new critique), with a config that's also part
> of our tree, so we can easily change it as we see fit, it should be
> good.
>


Should be completely trivial to do. We already have the core
infrastructure - I added it a week or two ago.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Andrew Dunstan
Date:

On 05/23/2018 02:00 PM, Andrew Dunstan wrote:
> On Wed, May 23, 2018 at 1:45 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> On 2018-May-23, Andrew Dunstan wrote:
>>
>>> And yes, the idea is that if we do this then we adopt a perlcritic
>>> policy that calls it out when we forget.
>> If we can have a buildfarm animal that runs perlcritic that starts green
>> (and turns yellow with any new critique), with a config that's also part
>> of our tree, so we can easily change it as we see fit, it should be
>> good.
>>
>
> Should be completely trivial to do. We already have the core
> infrastructure - I added it a week or two ago.
>


Not quite trivial but it's done - see 
<https://github.com/PGBuildFarm/client-code/commit/92f94ba7df8adbcbdb08f0138d8b5e686611ba1f>.

crake is now set up to run this - see 
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2018-05-26%2014%3A32%3A19&stg=perl-check>

So, are there any other objections?

The patch Mike supplied doesn't give us a clean run (at least on the 
machine I tested on), since it turns down the severity level to 4 but 
leaves some items unfixed. I propose to enable this policy at level 5 
for now, and then remove that when we can go down to level 4 cleanly, 
and use its default setting at that stage.

cheers

andrew

-- 
Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Alvaro Herrera
Date:
On 2018-May-26, Andrew Dunstan wrote:

> Not quite trivial but it's done - see
<https://github.com/PGBuildFarm/client-code/commit/92f94ba7df8adbcbdb08f0138d8b5e686611ba1f>.
> 
> crake is now set up to run this - see
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2018-05-26%2014%3A32%3A19&stg=perl-check>
> 
> So, are there any other objections?
> 
> The patch Mike supplied doesn't give us a clean run (at least on the machine
> I tested on), since it turns down the severity level to 4 but leaves some
> items unfixed. I propose to enable this policy at level 5 for now, and then
> remove that when we can go down to level 4 cleanly, and use its default
> setting at that stage.

Just to be clear -- this is done, right?  And we plan no further
enhancements in perlcritic area for pg11?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Re: [PATCH] Clear up perlcritic 'missing return' warning

From
Andrew Dunstan
Date:

On 06/11/2018 12:34 PM, Alvaro Herrera wrote:
> On 2018-May-26, Andrew Dunstan wrote:
>
>> Not quite trivial but it's done - see
<https://github.com/PGBuildFarm/client-code/commit/92f94ba7df8adbcbdb08f0138d8b5e686611ba1f>.
>>
>> crake is now set up to run this - see
<https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=crake&dt=2018-05-26%2014%3A32%3A19&stg=perl-check>
>>
>> So, are there any other objections?
>>
>> The patch Mike supplied doesn't give us a clean run (at least on the machine
>> I tested on), since it turns down the severity level to 4 but leaves some
>> items unfixed. I propose to enable this policy at level 5 for now, and then
>> remove that when we can go down to level 4 cleanly, and use its default
>> setting at that stage.
> Just to be clear -- this is done, right?  And we plan no further
> enhancements in perlcritic area for pg11?
>



Yes, modulo 
https://www.postgresql.org/message-id/f3c12e2c-618f-cb6f-082b-a2f604dbe73f%402ndQuadrant.com

I am hoping that we can get the perlcritic severity down to level 3 with 
a combination of policy settings and code remediation during the release 
12 dev cycle.

cheers

andrew


-- 

Andrew Dunstan                https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services