Обсуждение: Cleaning up perl code

Hello hackers,

Please look at a bunch of unused variables and a couple of other defects
I found in the perl code, maybe you'll find them worth fixing:
contrib/amcheck/t/001_verify_heapam.pl
$result # unused since introduction in 866e24d47
unused sub:
get_toast_for # not used since 860593ec3

contrib/amcheck/t/002_cic.pl
$result # orphaned since 7f580aa5d

src/backend/utils/activity/generate-wait_event_types.pl
$note, $note_name # unused since introduction in fa8892847

src/bin/pg_dump/t/003_pg_dump_with_server.pl
$cmd, $stdout, $stderr, $result # unused since introduction in 2f9eb3132

src/bin/pg_dump/t/005_pg_dump_filterfile.pl
$cmd, $stdout, $stderr, $result # unused since introduciton in a5cf808be

src/test/modules/ldap_password_func/t/001_mutated_bindpasswd.pl
$slapd, $ldap_schema_dir # unused since introduction in 419a8dd81

src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
$clearpass # orphaned since b846091fd

src/test/perl/PostgreSQL/Test/AdjustUpgrade.pm
$ostmt # unused since introduction in 47bb9db75

src/test/recovery/t/021_row_visibility.pl
$ret # unused since introduction in 7b28913bc

src/test/recovery/t/032_relfilenode_reuse.pl
$ret # unused since introduction in e2f65f425

src/test/recovery/t/035_standby_logical_decoding.pl
$stdin, $ret, $slot # unused since introduction in fcd77d532
$subscriber_stdin, $subscriber_stdout, $subscriber_stderr # unused since introduction in 376dc8205

src/test/subscription/t/024_add_drop_pub.pl
invalid reference in a comment:
tab_drop_refresh -> tab_2 # introduced with 1046a69b3
(invalid since v6-0001-...patch in the commit's thread)

src/tools/msvc_gendef.pl
@def # unused since introduction in 933b46644?

I've attached a patch with all of these changes (tested with meson build
on Windows and check-world on Linux).

Best regards,
Alexander

Alexander Lakhin <exclusion@gmail.com> writes:

> Hello hackers,
>
> Please look at a bunch of unused variables and a couple of other defects
> I found in the perl code, maybe you'll find them worth fixing:

Nice cleanup!  Did you use some static analysis tool, or did look for
them manually?  If I add [Variables::ProhibitUnusedVariables] to
src/tools/perlcheck/perlcriticrc, it finds a few more, see the attached
patch.

The scripts parsing errcodes.txt really should be refactored into using
a common module, but that's a patch for another day.

- ilmari

From 6b096a39753338bb91add5fcf1ed963024e58c15 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Dagfinn=20Ilmari=20Manns=C3=A5ker?= <ilmari@ilmari.org>
Date: Mon, 20 May 2024 19:55:20 +0100
Subject: [PATCH] Prohibit unused variables

---
 src/pl/plpgsql/src/generate-plerrcodes.pl | 6 ++----
 src/pl/plpython/generate-spiexceptions.pl | 6 ++----
 src/pl/tcl/generate-pltclerrcodes.pl      | 6 ++----
 src/tools/perlcheck/perlcriticrc          | 2 ++
 src/tools/pgindent/pgindent               | 2 +-
 5 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/src/pl/plpgsql/src/generate-plerrcodes.pl b/src/pl/plpgsql/src/generate-plerrcodes.pl
index 1c662bc967..e969a4b33e 100644
--- a/src/pl/plpgsql/src/generate-plerrcodes.pl
+++ b/src/pl/plpgsql/src/generate-plerrcodes.pl
@@ -23,10 +23,8 @@
     # Skip section headers
     next if /^Section:/;
 
-    die unless /^([^\s]{5})\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/;
-
-    (my $sqlstate, my $type, my $errcode_macro, my $condition_name) =
-      ($1, $2, $3, $4);
+    my ($type, $errcode_macro, $condition_name) =
+        /^[^\s]{5}\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/ or die;
 
     # Skip non-errors
     next unless $type eq 'E';
diff --git a/src/pl/plpython/generate-spiexceptions.pl b/src/pl/plpython/generate-spiexceptions.pl
index f0c5142be3..984017f212 100644
--- a/src/pl/plpython/generate-spiexceptions.pl
+++ b/src/pl/plpython/generate-spiexceptions.pl
@@ -23,10 +23,8 @@
     # Skip section headers
     next if /^Section:/;
 
-    die unless /^([^\s]{5})\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/;
-
-    (my $sqlstate, my $type, my $errcode_macro, my $condition_name) =
-      ($1, $2, $3, $4);
+    my ($type, $errcode_macro, $condition_name) =
+        /^[^\s]{5}\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/ or die;
 
     # Skip non-errors
     next unless $type eq 'E';
diff --git a/src/pl/tcl/generate-pltclerrcodes.pl b/src/pl/tcl/generate-pltclerrcodes.pl
index fcac4d00a6..58eb6afefe 100644
--- a/src/pl/tcl/generate-pltclerrcodes.pl
+++ b/src/pl/tcl/generate-pltclerrcodes.pl
@@ -23,10 +23,8 @@
     # Skip section headers
     next if /^Section:/;
 
-    die unless /^([^\s]{5})\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/;
-
-    (my $sqlstate, my $type, my $errcode_macro, my $condition_name) =
-      ($1, $2, $3, $4);
+    my ($type, $errcode_macro, $condition_name) =
+        /^[^\s]{5}\s+([EWS])\s+([^\s]+)(?:\s+)?([^\s]+)?/ or die;
 
     # Skip non-errors
     next unless $type eq 'E';
diff --git a/src/tools/perlcheck/perlcriticrc b/src/tools/perlcheck/perlcriticrc
index 4739e9f4f1..6053dfcc2a 100644
--- a/src/tools/perlcheck/perlcriticrc
+++ b/src/tools/perlcheck/perlcriticrc
@@ -15,6 +15,8 @@ verbose = %f: %m at line %l, column %c.  %e.  ([%p] Severity: %s)\n
 
 # Note: for policy descriptions see https://metacpan.org/dist/Perl-Critic
 
+[Variables::ProhibitUnusedVariables]
+severity = 5
 
 # allow octal constants with leading zeros
 [-ValuesAndExpressions::ProhibitLeadingZeros]
diff --git a/src/tools/pgindent/pgindent b/src/tools/pgindent/pgindent
index 48d83bc434..063ec8ce63 100755
--- a/src/tools/pgindent/pgindent
+++ b/src/tools/pgindent/pgindent
@@ -22,7 +22,7 @@ my $indent_opts =
 my $devnull = File::Spec->devnull;
 
 my ($typedefs_file, $typedef_str, @excludes,
-    $indent, $build, $diff,
+    $indent, $diff,
     $check, $help, @commits,);
 
 $help = 0;
-- 
2.39.2

Hello Dagfinn,

Thank you for paying attention to it and improving the possible fix!

20.05.2024 23:39, Dagfinn Ilmari Mannsåker wrote:
> Nice cleanup!  Did you use some static analysis tool, or did look for
> them manually?

I reviewed my collection of unica I gathered for several months, but had
found some of them too minor/requiring more analysis.
Then I added more with perlcritic's policy UnusedVariables, and also
checked for unused subs with a script from blogs.perl.org (and it confirmed
my only previous find of that kind).

>   If I add [Variables::ProhibitUnusedVariables] to
> src/tools/perlcheck/perlcriticrc, it finds a few more, see the attached
> patch.

Yes, I saw unused $sqlstates, but decided that they are meaningful enough
to stay. Though maybe enabling ProhibitUnusedVariables justifies fixing
them too.

> The scripts parsing errcodes.txt really should be refactored into using
> a common module, but that's a patch for another day.

Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf,
$server_config unused since d39a49c1e) aside for another day too.

Best regards,
Alexander

On Tue, May 21, 2024 at 06:00:00AM +0300, Alexander Lakhin wrote:
> I reviewed my collection of unica I gathered for several months, but had
> found some of them too minor/requiring more analysis.
> Then I added more with perlcritic's policy UnusedVariables, and also
> checked for unused subs with a script from blogs.perl.org (and it confirmed
> my only previous find of that kind).

Nice catches from both of you.  The two ones in
generate-wait_event_types.pl are caused by me, actually.

Not sure about the changes in the errcodes scripts, though.  The
current state of thing can be also useful when it comes to debugging
the parsing, and it does not hurt to keep the parsing rules the same
across the board.

>> The scripts parsing errcodes.txt really should be refactored into using
>> a common module, but that's a patch for another day.
>
> Agree, and I would leave 005_negotiate_encryption.pl (with $node_conf,
> $server_config unused since d39a49c1e) aside for another day too.

I'm not sure about these ones as each one of these scripts have their
own local tweaks.  Now, if there is a cleaner picture with a .pm
module I don't see while reading the whole, why not as long as it
improves the code.
--
Michael

On Tue, May 21, 2024 at 02:33:16PM +0900, Michael Paquier wrote:
> Nice catches from both of you.  The two ones in
> generate-wait_event_types.pl are caused by me, actually.
>
> Not sure about the changes in the errcodes scripts, though.  The
> current state of thing can be also useful when it comes to debugging
> the parsing, and it does not hurt to keep the parsing rules the same
> across the board.

For now, I have staged for commit the attached, that handles most of
the changes from Alexander (msvc could go for more cleanup?).  I'll
look at the changes from Dagfinn after that, including if perlcritic
could be changed.  I'll handle the first part when v18 opens up, as
that's cosmetic.

The incorrect comment in 024_add_drop_pub.pl has been fixed as of
53785d2a2aaa, as that was a separate issue.
--
Michael