Re: cleaning perl code

We currently only run perlcritic at severity level 5, which is fairly
permissive. I'd like to reduce that, ideally to, say, level 3, which is
what I use for the buildfarm code.

But let's start by going to severity level 4. Give this perlcriticrc,
derived from the buildfarm's:

# for policy descriptions see
# https://metacpan.org/release/Perl-Critic

severity = 4

theme = core

# allow octal constants with leading zeros
[-ValuesAndExpressions::ProhibitLeadingZeros]

# allow assignments to %ENV and %SIG without 'local'
[Variables::RequireLocalizedPunctuationVars]
allow = %ENV %SIG

# allow 'no warnings qw(once)
[TestingAndDebugging::ProhibitNoWarnings]
allow = once

# allow opened files to stay open for more than 9 lines of code
[-InputOutput::RequireBriefOpen]

Here's a summary of the perlcritic warnings:

     39 Always unpack @_ first
     30 Code before warnings are enabled
     12 Subroutine "new" called using indirect syntax
      9 Multiple "package" declarations
      9 Expression form of "grep"
      7 Symbols are exported by default
      5 Warnings disabled
      4 Magic variable "$/" should be assigned as "local"
      4 Comma used to separate statements
      2 Readline inside "for" loop
      2 Pragma "constant" used
      2 Mixed high and low-precedence booleans
      2 Don't turn off strict for large blocks of code
      1 Magic variable "@a" should be assigned as "local"
      1 Magic variable "$|" should be assigned as "local"
      1 Magic variable "$\" should be assigned as "local"
      1 Magic variable "$?" should be assigned as "local"
      1 Magic variable "$," should be assigned as "local"
      1 Magic variable "$"" should be assigned as "local"
      1 Expression form of "map"

which isn't a huge number.

I'm going to start posting patches to address these issues, and when
we're done we can lower the severity level and start again on the level
3s :-)

cheers

andrew

--

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

On Thu, Apr 9, 2020 at 11:44 AM Andrew Dunstan
<andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
> We currently only run perlcritic at severity level 5, which is fairly
> permissive. I'd like to reduce that, ideally to, say, level 3, which is
> what I use for the buildfarm code.
>
> But let's start by going to severity level 4.

I continue to be skeptical of perlcritic. I think it complains about a
lot of things which don't matter very much. We should consider whether
the effort it takes to keep it warning-clean has proportionate
benefits.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2020-04-09 19:47, Robert Haas wrote:
> On Thu, Apr 9, 2020 at 11:44 AM Andrew Dunstan
> <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>> We currently only run perlcritic at severity level 5, which is fairly
>> permissive. I'd like to reduce that, ideally to, say, level 3, which is
>> what I use for the buildfarm code.
>>
>> But let's start by going to severity level 4.
>
> I continue to be skeptical of perlcritic. I think it complains about a
> lot of things which don't matter very much. We should consider whether
> the effort it takes to keep it warning-clean has proportionate
> benefits.

Let's see what the patches look like. At least some of the warnings
look reasonable, especially in the sense that they are things casual
Perl programmers might accidentally do wrong.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 4/9/20 2:26 PM, Peter Eisentraut wrote:
> On 2020-04-09 19:47, Robert Haas wrote:
>> On Thu, Apr 9, 2020 at 11:44 AM Andrew Dunstan
>> <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>>> We currently only run perlcritic at severity level 5, which is fairly
>>> permissive. I'd like to reduce that, ideally to, say, level 3, which is
>>> what I use for the buildfarm code.
>>>
>>> But let's start by going to severity level 4.
>>
>> I continue to be skeptical of perlcritic. I think it complains about a
>> lot of things which don't matter very much. We should consider whether
>> the effort it takes to keep it warning-clean has proportionate
>> benefits.
>
> Let's see what the patches look like.  At least some of the warnings
> look reasonable, especially in the sense that they are things casual
> Perl programmers might accidentally do wrong.

OK, I'll prep one or two. I used to be of Robert's opinion, but I've
come around some on it.

cheers

andrew

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

On Thu, Apr 09, 2020 at 11:44:11AM -0400, Andrew Dunstan wrote:
>      39 Always unpack @_ first

Requiring a "my @args = @_" does not improve this code:

sub CreateSolution
{
...
if ($visualStudioVersion eq '12.00')
{
return new VS2013Solution(@_);
}

>      30 Code before warnings are enabled

Sounds good. We already require "use strict" before code. Requiring "use
warnings" in the exact same place does not impose much burden.

>      12 Subroutine "new" called using indirect syntax

No, thanks. "new VS2013Solution(@_)" and "VS2013Solution->new(@_)" are both
fine; enforcing the latter is an ongoing waste of effort.

>       9 Multiple "package" declarations

This is good advice if you're writing for CPAN, but it would make PostgreSQL
code worse by having us split affiliated code across multiple files.

>       9 Expression form of "grep"

No, thanks. I'd be happier with the opposite, requiring grep(/x/, $arg)
instead of grep { /x/ } $arg. Neither is worth enforcing.

>       7 Symbols are exported by default

This is good advice if you're writing for CPAN. For us, it just adds typing.

>       5 Warnings disabled
>       4 Magic variable "$/" should be assigned as "local"
>       4 Comma used to separate statements
>       2 Readline inside "for" loop
>       2 Pragma "constant" used
>       2 Mixed high and low-precedence booleans
>       2 Don't turn off strict for large blocks of code
>       1 Magic variable "@a" should be assigned as "local"
>       1 Magic variable "$|" should be assigned as "local"
>       1 Magic variable "$\" should be assigned as "local"
>       1 Magic variable "$?" should be assigned as "local"
>       1 Magic variable "$," should be assigned as "local"
>       1 Magic variable "$"" should be assigned as "local"
>       1 Expression form of "map"

I looked less closely at the rest, but none give me a favorable impression.

In summary, among those warnings, I see non-negative value in "Code before
warnings are enabled" only. While we're changing this, I propose removing
Subroutines::RequireFinalReturn. Implicit return values were not a material
source of PostgreSQL bugs, yet we've allowed this to litter our code:

$ find src -name '*.p[lm]'| xargs grep -n '^.return;' | wc -l
194

On 2020-04-11 06:30, Noah Misch wrote:
> In summary, among those warnings, I see non-negative value in "Code before
> warnings are enabled" only.

Now that you put it like this, that was also my impression when I first
introduced the level 5 warnings and then decided to stop there.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Noah Misch <noah(at)leadboat(dot)com> writes:
> In summary, among those warnings, I see non-negative value in "Code before
> warnings are enabled" only. While we're changing this, I propose removing
> Subroutines::RequireFinalReturn.

If it's possible to turn off just that warning, then +several.
It's routinely caused buildfarm failures, yet I can detect exactly
no value in it. If there were sufficient cross-procedural analysis
backing it to detect whether any caller examines the subroutine's
result value, then it'd be worth having. But there isn't, so those
extra returns are just pedantic verbosity.

regards, tom lane

On 4/11/20 12:30 AM, Noah Misch wrote:
> On Thu, Apr 09, 2020 at 11:44:11AM -0400, Andrew Dunstan wrote:
>>      39 Always unpack @_ first
> Requiring a "my @args = @_" does not improve this code:
>
> sub CreateSolution
> {
> ...
> if ($visualStudioVersion eq '12.00')
> {
> return new VS2013Solution(@_);
> }
>
>>      30 Code before warnings are enabled
> Sounds good. We already require "use strict" before code. Requiring "use
> warnings" in the exact same place does not impose much burden.
>
>>      12 Subroutine "new" called using indirect syntax
> No, thanks. "new VS2013Solution(@_)" and "VS2013Solution->new(@_)" are both
> fine; enforcing the latter is an ongoing waste of effort.
>
>>       9 Multiple "package" declarations
> This is good advice if you're writing for CPAN, but it would make PostgreSQL
> code worse by having us split affiliated code across multiple files.
>
>>       9 Expression form of "grep"
> No, thanks. I'd be happier with the opposite, requiring grep(/x/, $arg)
> instead of grep { /x/ } $arg. Neither is worth enforcing.
>
>>       7 Symbols are exported by default
> This is good advice if you're writing for CPAN. For us, it just adds typing.
>
>>       5 Warnings disabled
>>       4 Magic variable "$/" should be assigned as "local"
>>       4 Comma used to separate statements
>>       2 Readline inside "for" loop
>>       2 Pragma "constant" used
>>       2 Mixed high and low-precedence booleans
>>       2 Don't turn off strict for large blocks of code
>>       1 Magic variable "@a" should be assigned as "local"
>>       1 Magic variable "$|" should be assigned as "local"
>>       1 Magic variable "$\" should be assigned as "local"
>>       1 Magic variable "$?" should be assigned as "local"
>>       1 Magic variable "$," should be assigned as "local"
>>       1 Magic variable "$"" should be assigned as "local"
>>       1 Expression form of "map"
> I looked less closely at the rest, but none give me a favorable impression.

I don't have a problem with some of this. OTOH, it's nice to know what
we're ignoring and what we're not.

What I have prepared is first a patch that lowers the severity level to
3 but implements policy exceptions so that nothing is broken. Then 3
patches. One fixes the missing warnings pragma and removes shebang -w
switches, so we are quite consistent about how we do this. I gather we
are agreed about that one. The next one fixes those magic variable
error. That includes using some more idiomatic perl, and in one case
just renaming a couple of variables that are fairly opaque anyway. The
last one fixes the mixture of high and low precedence boolean operators,
the inefficient <FOO> inside a foreach loop,  and the use of commas to
separate statements, and relaxes the policy about large blocks with 'no
strict'.

Since I have written them they are attached, for posterity if nothing
else. :-)

>
>
> In summary, among those warnings, I see non-negative value in "Code before
> warnings are enabled" only. While we're changing this, I propose removing
> Subroutines::RequireFinalReturn. Implicit return values were not a material
> source of PostgreSQL bugs, yet we've allowed this to litter our code:
>

That doesn't mean it won't be a source of problems in future, I've
actually been bitten by this in the past.

cheers

andrew

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

> On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:

Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:

>
> The
> last one fixes the mixture of high and low precedence boolean operators,

I did not spot examples of this in your diffs, but I assume you mean to prohibit conditionals like:

if ($a || $b and $c || $d)

As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?

> and the use of commas to separate statements

I don't understand the prejudice against commas used this way. What is wrong with:

$i++, $j++ if defined $k;

rather than:

if (defined $k)
{
$i++;
$j++;
}

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/11/20 12:28 PM, Mark Dilger wrote:
>
>> On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
> Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:

Not at all.

>
>> The
>> last one fixes the mixture of high and low precedence boolean operators,
> I did not spot examples of this in your diffs, but I assume you mean to prohibit conditionals like:
>
> if ($a || $b and $c || $d)
>
> As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?

The docs say:

Conway advises against combining the low-precedence booleans ( |and
or not| ) with the high-precedence boolean operators ( |&& || !| )
in the same expression. Unless you fully understand the differences
between the high and low-precedence operators, it is easy to
misinterpret expressions that use both. And even if you do
understand them, it is not always clear if the author actually
intended it.

|next| |if| |not ||$foo| ||| ||$bar||;  ||#not ok|
|next| |if| |!||$foo| ||| ||$bar||;     ||#ok|
|next| |if| |!( ||$foo| ||| ||$bar| |); ||#ok|

I don't feel terribly strongly about it, but personally I just about
never use the low precendence operators, and mostly prefer to resolve
precedence issue with parentheses.

>
>> and the use of commas to separate statements
> I don't understand the prejudice against commas used this way. What is wrong with:
>
> $i++, $j++ if defined $k;
>
> rather than:
>
> if (defined $k)
> {
> $i++;
> $j++;
> }
>

I don't think the example is terribly clear. I have to look at it and
think "Does it do $i++ if $k isn't defined?"

In the cases we actually have there isn't even any shorthand advantage
like this. There are only a couple of cases.

cheers

andrew

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

Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
> On 4/11/20 12:30 AM, Noah Misch wrote:
>> In summary, among those warnings, I see non-negative value in "Code before
>> warnings are enabled" only. While we're changing this, I propose removing
>> Subroutines::RequireFinalReturn. Implicit return values were not a material
>> source of PostgreSQL bugs, yet we've allowed this to litter our code:

> That doesn't mean it won't be a source of problems in future, I've
> actually been bitten by this in the past.

Yeah, as I recall, the reason for the restriction is that if you fall out
without a "return", what's returned is the side-effect value of the last
statement, which might be fairly surprising. Adding explicit "return;"
guarantees an undef result. So when this does prevent a bug it could
be a pretty hard-to-diagnose one. The problem is that it's a really
verbose/pedantic requirement for subs that no one ever examines the
result value of.

Is there a way to modify the test so that it only complains when
the final return is missing and there are other return(s) with values?
That would seem like a more narrowly tailored check.

regards, tom lane

On 4/11/20 12:48 PM, Tom Lane wrote:
> Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
>> On 4/11/20 12:30 AM, Noah Misch wrote:
>>> In summary, among those warnings, I see non-negative value in "Code before
>>> warnings are enabled" only. While we're changing this, I propose removing
>>> Subroutines::RequireFinalReturn. Implicit return values were not a material
>>> source of PostgreSQL bugs, yet we've allowed this to litter our code:
>> That doesn't mean it won't be a source of problems in future, I've
>> actually been bitten by this in the past.
> Yeah, as I recall, the reason for the restriction is that if you fall out
> without a "return", what's returned is the side-effect value of the last
> statement, which might be fairly surprising. Adding explicit "return;"
> guarantees an undef result. So when this does prevent a bug it could
> be a pretty hard-to-diagnose one. The problem is that it's a really
> verbose/pedantic requirement for subs that no one ever examines the
> result value of.
>
> Is there a way to modify the test so that it only complains when
> the final return is missing and there are other return(s) with values?
> That would seem like a more narrowly tailored check.
>
>

Not AFAICS:
<https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::RequireFinalReturn>

That would probably require writing a replacement module. Looking at the
source if this module I think it might be possible, although I don't
know much of the internals of perlcritic.

cheers

andrew

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

> On Apr 11, 2020, at 9:47 AM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>
>
> On 4/11/20 12:28 PM, Mark Dilger wrote:
>>
>>> On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>> Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about this effort:
>
>
> Not at all.
>
>
>>
>>> The
>>> last one fixes the mixture of high and low precedence boolean operators,
>> I did not spot examples of this in your diffs, but I assume you mean to prohibit conditionals like:
>>
>> if ($a || $b and $c || $d)
>>
>> As I understand it, perl introduced low precedence operators precisely to allow this. Why disallow it?
>
>
> The docs say:
>
>
> Conway advises against combining the low-precedence booleans ( |and
> or not| ) with the high-precedence boolean operators ( |&& || !| )
> in the same expression. Unless you fully understand the differences
> between the high and low-precedence operators, it is easy to
> misinterpret expressions that use both. And even if you do
> understand them, it is not always clear if the author actually
> intended it.
>
> |next| |if| |not ||$foo| ||| ||$bar||; ||#not ok|
> |next| |if| |!||$foo| ||| ||$bar||; ||#ok|
> |next| |if| |!( ||$foo| ||| ||$bar| |); ||#ok|

I don't think any of those three are ok, from a code review perspective, but it's not because high and low precedence operators were intermixed.

>>
>>> and the use of commas to separate statements
>> I don't understand the prejudice against commas used this way. What is wrong with:
>>
>> $i++, $j++ if defined $k;
>>
>> rather than:
>>
>> if (defined $k)
>> {
>> $i++;
>> $j++;
>> }
>>
>
>
> I don't think the example is terribly clear. I have to look at it and
> think "Does it do $i++ if $k isn't defined?"

It works like the equivalent C-code:

if (k)
i++, j++;

which to my eyes is also fine.

I'm less concerned with which perlcritic features you enable than I am with accidentally submitting perl which looks fine to me but breaks the build. I mostly use perl from within TAP tests, which I run locally before submission to the project. Can your changes be integrated into the TAP_TESTS makefile target so that I get local errors about this stuff and can fix it before submitting a regression test to -hackers?

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
> On 4/11/20 12:48 PM, Tom Lane wrote:
>> Is there a way to modify the test so that it only complains when
>> the final return is missing and there are other return(s) with values?
>> That would seem like a more narrowly tailored check.

> Not AFAICS:
> <https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::RequireFinalReturn>

Yeah, the list of all policies in the parent page doesn't offer any
promising alternatives either :-(

BTW, this bit in the policy's man page seems pretty disheartening:

Be careful when fixing problems identified by this Policy; don't
blindly put a return; statement at the end of every subroutine.

since I'd venture that's *exactly* what we've done every time perlcritic
moaned about this. I wonder what else the author expected would happen.

> That would probably require writing a replacement module. Looking at the
> source if this module I think it might be possible, although I don't
> know much of the internals of perlcritic.

I doubt we want to go maintaining our own perlcritic policies; aside from
the effort involved, it'd become that much harder for anyone to reproduce
the results.

regards, tom lane

Mark Dilger <mark(dot)dilger(at)enterprisedb(dot)com> writes:
> I'm less concerned with which perlcritic features you enable than I am with accidentally submitting perl which looks fine to me but breaks the build. I mostly use perl from within TAP tests, which I run locally before submission to the project. Can your changes be integrated into the TAP_TESTS makefile target so that I get local errors about this stuff and can fix it before submitting a regression test to -hackers?

As far as that goes, I think crake is just running

src/tools/perlcheck/pgperlcritic

which you can do for yourself as long as you've got perlcritic
installed.

regards, tom lane

On Sat, Apr 11, 2020 at 11:14:52AM -0400, Tom Lane wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > In summary, among those warnings, I see non-negative value in "Code before
> > warnings are enabled" only. While we're changing this, I propose removing
> > Subroutines::RequireFinalReturn.
>
> If it's possible to turn off just that warning, then +several.

We'd not get that warning if src/tools/perlcheck/pgperlcritic stopped enabling
it by name, so it is possible to turn off by removing lines from that config.

> It's routinely caused buildfarm failures, yet I can detect exactly
> no value in it. If there were sufficient cross-procedural analysis
> backing it to detect whether any caller examines the subroutine's
> result value, then it'd be worth having. But there isn't, so those
> extra returns are just pedantic verbosity.

Agreed.

On Sat, Apr 11, 2020 at 12:13:08PM -0400, Andrew Dunstan wrote:
> --- a/src/tools/msvc/Project.pm
> +++ b/src/tools/msvc/Project.pm
> @@ -420,13 +420,10 @@ sub read_file
> {
> my $filename = shift;
> my $F;
> - my $t = $/;
> -
> - undef $/;
> + local $/ = undef;
> open($F, '<', $filename) || croak "Could not open file $filename\n";
> my $txt = <$F>;
> close($F);
> - $/ = $t;

+1 for this and for the other three hunks like it. The resulting code is
shorter and more robust, so this is a good one-time cleanup. It's not
important to mandate this style going forward, so I wouldn't change
perlcriticrc for this one.

> --- a/src/tools/version_stamp.pl
> +++ b/src/tools/version_stamp.pl
> @@ -1,4 +1,4 @@
> -#! /usr/bin/perl -w
> +#! /usr/bin/perl
>
> #################################################################
> # version_stamp.pl -- update version stamps throughout the source tree
> @@ -21,6 +21,7 @@
> #
>
> use strict;
> +use warnings;

This and the other "use warnings" additions look good. I'm assuming you'd
change perlcriticrc like this:

+[TestingAndDebugging::RequireUseWarnings]
+severity = 5

On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Noah Misch <noah(at)leadboat(dot)com> writes:
> > In summary, among those warnings, I see non-negative value in "Code before
> > warnings are enabled" only. While we're changing this, I propose removing
> > Subroutines::RequireFinalReturn.
>
> If it's possible to turn off just that warning, then +several.
> It's routinely caused buildfarm failures, yet I can detect exactly
> no value in it. If there were sufficient cross-procedural analysis
> backing it to detect whether any caller examines the subroutine's
> result value, then it'd be worth having. But there isn't, so those
> extra returns are just pedantic verbosity.

We've actually gone out of our way to enable that particular warning.
See src/tools/perlcheck/perlcriticrc.

The idea of that warning is not entirely without merit, but in
practice it's usually pretty clear whether a function is intended to
return anything or not, and it's unlikely that someone is going to
rely on the return value when they really shouldn't be doing so. I'd
venture to suggest that the language is lax about this sort of thing
precisely because it isn't very important, and thus not worth
bothering users about.

I agree with Noah's comment about CPAN: it would be worth being more
careful about things like this if we were writing code that was likely
to be used by a wide variety of people and a lot of code over which we
have no control and which we do not get to even see. But that's not
the case here. It does not seem worth stressing the authors of TAP
tests over such things.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 4/12/20 3:22 PM, Robert Haas wrote:
> On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Noah Misch <noah(at)leadboat(dot)com> writes:
>>> In summary, among those warnings, I see non-negative value in "Code before
>>> warnings are enabled" only. While we're changing this, I propose removing
>>> Subroutines::RequireFinalReturn.
>>
>> If it's possible to turn off just that warning, then +several.
>> It's routinely caused buildfarm failures, yet I can detect exactly
>> no value in it. If there were sufficient cross-procedural analysis
>> backing it to detect whether any caller examines the subroutine's
>> result value, then it'd be worth having. But there isn't, so those
>> extra returns are just pedantic verbosity.
>
> I agree with Noah's comment about CPAN: it would be worth being more
> careful about things like this if we were writing code that was likely
> to be used by a wide variety of people and a lot of code over which we
> have no control and which we do not get to even see. But that's not
> the case here. It does not seem worth stressing the authors of TAP
> tests over such things.

FWIW, pgBackRest used Perl Critic when we were distributing Perl code
but stopped when our Perl code was only used for integration testing.
Perhaps that was the wrong call but we decided the extra time required
to run it was not worth the benefit. Most new test code is written in C
and the Perl test code is primarily in maintenance mode now.

When we did use Perl Critic we set it at level 1 (--brutal) and then
wrote an exception file for the stuff we wanted to ignore. The advantage
of this is that if new code violated a policy that did not already have
an exception we could evaluate it and either add an exception or modify
the code. In practice this was pretty rare, but we also had a short
excuse for many exceptions and a list of exceptions that should be
re-evaluated in the future.

About the time we introduced Perl Critic we were already considering the
C migration so most of the exceptions stayed.

Just in case it is useful, I have attached our old policy file with
exceptions and excuses (when we had one).

Regards,
--
-David
david(at)pgmasters(dot)net

On 4/12/20 4:12 PM, David Steele wrote:
> On 4/12/20 3:22 PM, Robert Haas wrote:
>> On Sat, Apr 11, 2020 at 11:15 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> Noah Misch <noah(at)leadboat(dot)com> writes:
>>>> In summary, among those warnings, I see non-negative value in "Code
>>>> before
>>>> warnings are enabled" only.  While we're changing this, I propose
>>>> removing
>>>> Subroutines::RequireFinalReturn.
>>>
>>> If it's possible to turn off just that warning, then +several.
>>> It's routinely caused buildfarm failures, yet I can detect exactly
>>> no value in it.  If there were sufficient cross-procedural analysis
>>> backing it to detect whether any caller examines the subroutine's
>>> result value, then it'd be worth having.  But there isn't, so those
>>> extra returns are just pedantic verbosity.
>>
>> I agree with Noah's comment about CPAN: it would be worth being more
>> careful about things like this if we were writing code that was likely
>> to be used by a wide variety of people and a lot of code over which we
>> have no control and which we do not get to even see. But that's not
>> the case here. It does not seem worth stressing the authors of TAP
>> tests over such things.
>
> FWIW, pgBackRest used Perl Critic when we were distributing Perl code
> but stopped when our Perl code was only used for integration testing.
> Perhaps that was the wrong call but we decided the extra time required
> to run it was not worth the benefit. Most new test code is written in
> C and the Perl test code is primarily in maintenance mode now.
>
> When we did use Perl Critic we set it at level 1 (--brutal) and then
> wrote an exception file for the stuff we wanted to ignore. The
> advantage of this is that if new code violated a policy that did not
> already have an exception we could evaluate it and either add an
> exception or modify the code. In practice this was pretty rare, but we
> also had a short excuse for many exceptions and a list of exceptions
> that should be re-evaluated in the future.
>
> About the time we introduced Perl Critic we were already considering
> the C migration so most of the exceptions stayed.
>
> Just in case it is useful, I have attached our old policy file with
> exceptions and excuses (when we had one).
>
>

That's a pretty short list for --brutal, well done. I agree there is
value in keeping documented the policies you're not complying with.
Maybe the burden of that is too much for this use, that's up to the
project to decide.

For good or ill we now have a significant investment in perl code - I
just looked and it's 180 files with 38,135 LOC, and that's not counting
the catalog data files, so we have some interest in keeping it fairly clean.

I did something similar to what's above with the buildfarm code,
although on checking now I find it's a bit out of date for the sev 1 and
2 warnings, so I'm fixing that. Having said that, my normal target is
level 3.

The absolutely minimal things I want to do are a) fix the code that
we're agreed on fixing (use of warnings, idiomatic use of $/), and b)
fix the output format to include the name of the policy being violated.

cheers

andrew

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

On 4/12/20 6:24 PM, Andrew Dunstan wrote:
> On 4/12/20 4:12 PM, David Steele wrote:
>>
>> Just in case it is useful, I have attached our old policy file with
>> exceptions and excuses (when we had one).
>
> That's a pretty short list for --brutal, well done. I agree there is
> value in keeping documented the policies you're not complying with.
> Maybe the burden of that is too much for this use, that's up to the
> project to decide.

Thanks! Perl is, well Perl, and we made a lot of effort to keep it as
clean and consistent as possible.

Obviously I'm +1 on documenting all the exceptions.

> For good or ill we now have a significant investment in perl code - I
> just looked and it's 180 files with 38,135 LOC, and that's not counting
> the catalog data files, so we have some interest in keeping it fairly clean.

Agreed. According to cloc pgBackRest still has 26,744 lines of Perl (not
including comments or whitespace) so we're in the same boat.

> The absolutely minimal things I want to do are a) fix the code that
> we're agreed on fixing (use of warnings, idiomatic use of $/), and b)
> fix the output format to include the name of the policy being violated.

We found limiting results and being very verbose about the violation was
extremely helpful:

perlcritic --quiet --verbose=8 --brutal --top=10 \
--verbose "[%p] %f: %m at line %l, column %c. %e. (Severity: %s)\n"
--profile=test/lint/perlcritic.policy \
<files>

--
-David
david(at)pgmasters(dot)net

On 4/12/20 3:42 AM, Noah Misch wrote:
> On Sat, Apr 11, 2020 at 12:13:08PM -0400, Andrew Dunstan wrote:
>> --- a/src/tools/msvc/Project.pm
>> +++ b/src/tools/msvc/Project.pm
>> @@ -420,13 +420,10 @@ sub read_file
>> {
>> my $filename = shift;
>> my $F;
>> - my $t = $/;
>> -
>> - undef $/;
>> + local $/ = undef;
>> open($F, '<', $filename) || croak "Could not open file $filename\n";
>> my $txt = <$F>;
>> close($F);
>> - $/ = $t;
> +1 for this and for the other three hunks like it. The resulting code is
> shorter and more robust, so this is a good one-time cleanup. It's not
> important to mandate this style going forward, so I wouldn't change
> perlcriticrc for this one.
>
>> --- a/src/tools/version_stamp.pl
>> +++ b/src/tools/version_stamp.pl
>> @@ -1,4 +1,4 @@
>> -#! /usr/bin/perl -w
>> +#! /usr/bin/perl
>>
>> #################################################################
>> # version_stamp.pl -- update version stamps throughout the source tree
>> @@ -21,6 +21,7 @@
>> #
>>
>> use strict;
>> +use warnings;
> This and the other "use warnings" additions look good. I'm assuming you'd
> change perlcriticrc like this:
>
> +[TestingAndDebugging::RequireUseWarnings]
> +severity = 5

OK, I've committed all that stuff. I think that takes care of the
non-controversial part of what I proposed :-)

cheers

andrew

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

On 4/13/20 12:47 PM, Andrew Dunstan wrote:
>
> OK, I've committed all that stuff. I think that takes care of the
> non-controversial part of what I proposed :-)
>
>

OK, it seems there is a majority of people commenting in this thread in
favor of not doing more except to reverse the policy of requiring
subroutine returns. I'll do that shortly. In the spirit of David
Steele's contribution, here is a snippet that when added to the
perlcriticrc would allow us to pass at the "brutal" setting (severity
1). But I'm not proposing to add this, it's just here so anyone
interested can see what's involved.

One of the things that's a bit sad is that perlcritic doesn't generally
let you apply policies to a given set of files or files matching some
pattern. It would be nice, for instance, to be able to apply some
additional standards to strategic library files like PostgresNode.pm,
TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
to apply higher standards to library files than to, say, a TAP test
script. The only easy way I can see to do that would be to have two
different perlcriticrc files and adjust pgperlcritic to make two runs.
If people think that's worth it I'll put a little work into it. If not,
I'll just leave things here.

cheers

andrew

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

On 2020-Apr-14, Andrew Dunstan wrote:

> One of the things that's a bit sad is that perlcritic doesn't generally
> let you apply policies to a given set of files or files matching some
> pattern. It would be nice, for instance, to be able to apply some
> additional standards to strategic library files like PostgresNode.pm,
> TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
> to apply higher standards to library files than to, say, a TAP test
> script. The only easy way I can see to do that would be to have two
> different perlcriticrc files and adjust pgperlcritic to make two runs.
> If people think that's worth it I'll put a little work into it. If not,
> I'll just leave things here.

I think being more strict about it in strategic files (I'd say that's
Catalog.pm plus src/test/perl/*.pm) might be a good idea. Maybe give it
a try and see what comes up.

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

On 4/14/20 4:44 PM, Alvaro Herrera wrote:
> On 2020-Apr-14, Andrew Dunstan wrote:
>
>> One of the things that's a bit sad is that perlcritic doesn't generally
>> let you apply policies to a given set of files or files matching some
>> pattern. It would be nice, for instance, to be able to apply some
>> additional standards to strategic library files like PostgresNode.pm,
>> TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
>> to apply higher standards to library files than to, say, a TAP test
>> script. The only easy way I can see to do that would be to have two
>> different perlcriticrc files and adjust pgperlcritic to make two runs.
>> If people think that's worth it I'll put a little work into it. If not,
>> I'll just leave things here.
> I think being more strict about it in strategic files (I'd say that's
> Catalog.pm plus src/test/perl/*.pm) might be a good idea. Maybe give it
> a try and see what comes up.
>

OK, in fact those files are in reasonably good shape. I also took a pass
through the library files in src/tools/msvc, which had a few more issues.

Here's a patch that does the stricter testing for those library files,
and fixes them so we get a clean pass

This brings to an end my perl gardening project.

cheers

andrew

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

On Wed, Apr 15, 2020 at 03:43:36PM -0400, Andrew Dunstan wrote:
> On 4/14/20 4:44 PM, Alvaro Herrera wrote:
> > On 2020-Apr-14, Andrew Dunstan wrote:
> >> One of the things that's a bit sad is that perlcritic doesn't generally
> >> let you apply policies to a given set of files or files matching some
> >> pattern. It would be nice, for instance, to be able to apply some
> >> additional standards to strategic library files like PostgresNode.pm,
> >> TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
> >> to apply higher standards to library files than to, say, a TAP test
> >> script. The only easy way I can see to do that would be to have two
> >> different perlcriticrc files and adjust pgperlcritic to make two runs.
> >> If people think that's worth it I'll put a little work into it. If not,
> >> I'll just leave things here.
> > I think being more strict about it in strategic files (I'd say that's
> > Catalog.pm plus src/test/perl/*.pm) might be a good idea. Maybe give it
> > a try and see what comes up.
>
> OK, in fact those files are in reasonably good shape. I also took a pass
> through the library files in src/tools/msvc, which had a few more issues.

It would be an unpleasant surprise to cause a perlcritic buildfarm failure by
moving a function, verbatim, from a non-strategic file to a strategic file.
Having two Perl style regimes in one tree is itself a liability.

> --- a/src/backend/catalog/Catalog.pm
> +++ b/src/backend/catalog/Catalog.pm
> @@ -67,7 +67,7 @@ sub ParseHeader
> if (!$is_client_code)
> {
> # Strip C-style comments.
> - s;/\*(.|\n)*\*/;;g;
> + s;/\*(?:.|\n)*\*/;;g;

This policy against unreferenced groups makes the code harder to read, and the
chance of preventing a bug is too low to justify that.

> --- a/src/tools/perlcheck/pgperlcritic
> +++ b/src/tools/perlcheck/pgperlcritic
> @@ -14,7 +14,21 @@ PERLCRITIC=${PERLCRITIC:-perlcritic}
>
> . src/tools/perlcheck/find_perl_files
>
> -find_perl_files | xargs $PERLCRITIC \
> +flist=`mktemp`
> +find_perl_files > $flist
> +
> +pattern='src/test/perl/|src/backend/catalog/Catalog.pm|src/tools/msvc/[^/]*.pm'

I don't find these files to be especially strategic, and I'm mostly shrugging
about the stricter policy's effect on code quality. -1 for this patch.

On 4/15/20 11:01 PM, Noah Misch wrote:
> On Wed, Apr 15, 2020 at 03:43:36PM -0400, Andrew Dunstan wrote:
>> On 4/14/20 4:44 PM, Alvaro Herrera wrote:
>>> On 2020-Apr-14, Andrew Dunstan wrote:
>>>> One of the things that's a bit sad is that perlcritic doesn't generally
>>>> let you apply policies to a given set of files or files matching some
>>>> pattern. It would be nice, for instance, to be able to apply some
>>>> additional standards to strategic library files like PostgresNode.pm,
>>>> TestLib.pm and Catalog.pm. There are good reasons as suggested upthread
>>>> to apply higher standards to library files than to, say, a TAP test
>>>> script. The only easy way I can see to do that would be to have two
>>>> different perlcriticrc files and adjust pgperlcritic to make two runs.
>>>> If people think that's worth it I'll put a little work into it. If not,
>>>> I'll just leave things here.
>>> I think being more strict about it in strategic files (I'd say that's
>>> Catalog.pm plus src/test/perl/*.pm) might be a good idea. Maybe give it
>>> a try and see what comes up.
>> OK, in fact those files are in reasonably good shape. I also took a pass
>> through the library files in src/tools/msvc, which had a few more issues.
> It would be an unpleasant surprise to cause a perlcritic buildfarm failure by
> moving a function, verbatim, from a non-strategic file to a strategic file.
> Having two Perl style regimes in one tree is itself a liability.

Honestly, I think you're reaching here.

>
>> --- a/src/backend/catalog/Catalog.pm
>> +++ b/src/backend/catalog/Catalog.pm
>> @@ -67,7 +67,7 @@ sub ParseHeader
>> if (!$is_client_code)
>> {
>> # Strip C-style comments.
>> - s;/\*(.|\n)*\*/;;g;
>> + s;/\*(?:.|\n)*\*/;;g;
> This policy against unreferenced groups makes the code harder to read, and the
> chance of preventing a bug is too low to justify that.

Non-capturing groups are also more efficient, and are something perl
programmers should be familiar with.

In fact, there's a much better renovation of semantics of this
particular instance, which is to make . match \n using the s modifier:

    s;/\*.*\*/;;gs;

It would also be more robust using non-greedy matching:

    s;/\*.*?\*/;;gs

After I wrote the above I went and looked at what we do the buildfarm
code to strip comments when looking for typedefs, and it's exactly that,
so at least I'm consistent :-)

I don't care that much if we throw this whole thing away. This was sent
in response to Alvaro's suggestion to "give it a try and see what comes up".

cheers

andrew

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

Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
> On 4/15/20 11:01 PM, Noah Misch wrote:
>> It would be an unpleasant surprise to cause a perlcritic buildfarm failure by
>> moving a function, verbatim, from a non-strategic file to a strategic file.
>> Having two Perl style regimes in one tree is itself a liability.

> Honestly, I think you're reaching here.

I think that argument is wrong, actually. Moving a function from a single
use-case into a library (with, clearly, the intention for it to have more
use-cases) is precisely the time when any weaknesses in its original
implementation might be exposed. So extra scrutiny seems well warranted.

Whether the "extra scrutiny" involved in perlcritic's higher levels
is actually worth anything is a different debate, though, and so far
it's not looking like it's worth much :-(

regards, tom lane

On Thu, Apr 16, 2020 at 08:50:35AM -0400, Andrew Dunstan wrote:
>
> It would also be more robust using non-greedy matching:

This seems more important.
I don't know how/where this is being used, but if it has input like:

/* one */
something;
/* two */

With the old expression 'something;' would be stripped away.
Is that an issue where this this is used? Why are we parsing
these headers?

Garick

On 4/16/20 10:20 AM, Hamlin, Garick L wrote:
> On Thu, Apr 16, 2020 at 08:50:35AM -0400, Andrew Dunstan wrote:
>> It would also be more robust using non-greedy matching:
> This seems more important.
> I don't know how/where this is being used, but if it has input like:
>
> /* one */
> something;
> /* two */
>
> With the old expression 'something;' would be stripped away.
> Is that an issue where this this is used? Why are we parsing
> these headers?
>

It's not quite as bad as that, because we're doing it line by line
rather than on a whole file that's been slurped in. Multiline comments
are handled using some redo logic. But

    /* one */ something(); /* two */

would all be removed. Of course, we hope we don't have anything so
horrible, but still ...

cheers

andrew

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

On 2020-Apr-16, Hamlin, Garick L wrote:

> With the old expression 'something;' would be stripped away.
> Is that an issue where this this is used? Why are we parsing
> these headers?

These are files from which bootstrap catalog data is generated, which is
why we parse from Perl; but also where C structs are declared, which is
why they're C.

I think switching to non-greedy is a win in itself. Non-capturing
parens is probably a wash (this doesn't run often so the performance
argument isn't very interesting).

An example. This eval in Catalog.pm

+ ## no critic (ProhibitStringyEval)
+ ## no critic (RequireCheckingReturnValueOfEval)
+ eval '$hash_ref = ' . $_;

is really weird stuff generally speaking, and the fact that we have to
mark it specially for critic is a good indicator of that -- it serves as
documentation. Catalog.pm is all a huge weird hack, but it's a critically
important hack. Heck, what about RequireCheckingReturnValueOfEval --
should we instead consider actually checking the return value of eval?
It would seem to make sense, would it not? (Not for this patch, though
-- I would be fine with just adding the nocritic line now, and removing
it later while fixing that).

All in all, I think it's a positive value in having this code be checked
with a bit more strength -- checks that are pointless in, say, t/00*.pl
prove files.

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

On 4/16/20 11:12 AM, Alvaro Herrera wrote:
> On 2020-Apr-16, Hamlin, Garick L wrote:
>
>> With the old expression 'something;' would be stripped away.
>> Is that an issue where this this is used? Why are we parsing
>> these headers?
> These are files from which bootstrap catalog data is generated, which is
> why we parse from Perl; but also where C structs are declared, which is
> why they're C.
>
> I think switching to non-greedy is a win in itself. Non-capturing
> parens is probably a wash (this doesn't run often so the performance
> argument isn't very interesting).

Yeah, I'm inclined to fix this independently of the perlcritic stuff.
The change is more readable and more correct as well as being perlcritic
friendly.

I might take a closer look at Catalog.pm.

Meanwhile, the other regex highlighted in the patch, in Solution.pm:

if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\], \[([^\]]*)\],
\[([^\]]+)\]/)

is sufficiently horrid that I think we should see if we can rewrite it,
maybe as an extended regex. And a better fix here instead of marking the
fourth group as non-capturing would be simply to get rid of the parens
altogether. The serve no purpose at all.

>
> An example. This eval in Catalog.pm
>
> + ## no critic (ProhibitStringyEval)
> + ## no critic (RequireCheckingReturnValueOfEval)
> + eval '$hash_ref = ' . $_;
>
> is really weird stuff generally speaking, and the fact that we have to
> mark it specially for critic is a good indicator of that -- it serves as
> documentation. Catalog.pm is all a huge weird hack, but it's a critically
> important hack. Heck, what about RequireCheckingReturnValueOfEval --
> should we instead consider actually checking the return value of eval?
> It would seem to make sense, would it not? (Not for this patch, though
> -- I would be fine with just adding the nocritic line now, and removing
> it later while fixing that).

+1

>
> All in all, I think it's a positive value in having this code be checked
> with a bit more strength -- checks that are pointless in, say, t/00*.pl
> prove files.

thanks

cheers

andrew

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

> On Apr 16, 2020, at 2:07 PM, Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> wrote:
>
>
> On 4/16/20 11:12 AM, Alvaro Herrera wrote:
>> On 2020-Apr-16, Hamlin, Garick L wrote:
>>
>>> With the old expression 'something;' would be stripped away.
>>> Is that an issue where this this is used? Why are we parsing
>>> these headers?
>> These are files from which bootstrap catalog data is generated, which is
>> why we parse from Perl; but also where C structs are declared, which is
>> why they're C.
>>
>> I think switching to non-greedy is a win in itself. Non-capturing
>> parens is probably a wash (this doesn't run often so the performance
>> argument isn't very interesting).
>
>
> Yeah, I'm inclined to fix this independently of the perlcritic stuff.
> The change is more readable and more correct as well as being perlcritic
> friendly.
>
>
> I might take a closer look at Catalog.pm.
>
>
> Meanwhile, the other regex highlighted in the patch, in Solution.pm:
>
>
> if (/^AC_INIT\(\[([^\]]+)\], \[([^\]]+)\], \[([^\]]+)\], \[([^\]]*)\],
> \[([^\]]+)\]/)
>
>
> is sufficiently horrid that I think we should see if we can rewrite it,

my $re = qr/
\[ # literal opening bracket
( # Capture anything but a closing bracket
(?> # without backtracking
[^\]]+
)
)
\] # literal closing bracket
/x;
if (/^AC_INIT\($re, $re, $re, $re, $re/)

> maybe as an extended regex. And a better fix here instead of marking the
> fourth group as non-capturing would be simply to get rid of the parens
> altogether. The serve no purpose at all.

But then you'd have to use something else in position 4, which complicates the code.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Apr 16, 2020 at 09:53:46AM -0400, Tom Lane wrote:
> Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> writes:
> > On 4/15/20 11:01 PM, Noah Misch wrote:
> >> It would be an unpleasant surprise to cause a perlcritic buildfarm failure by
> >> moving a function, verbatim, from a non-strategic file to a strategic file.
> >> Having two Perl style regimes in one tree is itself a liability.
>
> > Honestly, I think you're reaching here.
>
> I think that argument is wrong, actually. Moving a function from a single
> use-case into a library (with, clearly, the intention for it to have more
> use-cases) is precisely the time when any weaknesses in its original
> implementation might be exposed. So extra scrutiny seems well warranted.

Moving a function to a library does call for various scrutiny. I don't think
it calls for replacing "no warnings;" with "no warnings; ## no critic", but
that observation is subordinate to your other point:

> Whether the "extra scrutiny" involved in perlcritic's higher levels
> is actually worth anything is a different debate, though, and so far
> it's not looking like it's worth much :-(

Yeah, this is the central point. Many proposed style conformance changes are
(a) double-entry bookkeeping to emphasize the author's sincerity and (b) regex
performance optimization. Those are not better for libraries than for
non-libraries, and I think they decrease code quality.

Even if such policies were better for libraries, the proposed patch applies
them to .pm files with narrow audiences. If DBD::Pg were in this tree, that
would be a different conversation.