Обсуждение: 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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.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@leadboat.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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.com> wrote:
> Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about
thiseffort:
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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.com> wrote:
>
>
> On 4/11/20 12:28 PM, Mark Dilger wrote:
>>
>>> On Apr 11, 2020, at 9:13 AM, Andrew Dunstan <andrew.dunstan@2ndquadrant.com> wrote:
>> Hi Andrew. I appreciate your interest and efforts here. I hope you don't mind a few questions/observations about
thiseffort:
>
>
> 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
operatorswere 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
fineto 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
andcan fix it before submitting a regression test to -hackers?
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Andrew Dunstan <andrew.dunstan@2ndquadrant.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.dilger@enterprisedb.com> writes:
> I'm less concerned with which perlcritic features you enable than I am with accidentally submitting perl which looks
fineto 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
andcan 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@leadboat.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@sss.pgh.pa.us> wrote: > Noah Misch <noah@leadboat.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@sss.pgh.pa.us> wrote: >> Noah Misch <noah@leadboat.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@pgmasters.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@sss.pgh.pa.us> wrote: >>> Noah Misch <noah@leadboat.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@pgmasters.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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.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.dunstan@2ndquadrant.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.