THRIFT-5064: Introduce Perl::Critic into the SCA

kpumuk · kpumuk · commit 426bf4983b5d · 2026-05-03T19:01:25.000-04:00
diff --git a/.github/workflows/sca.yml b/.github/workflows/sca.yml
@@ -275,6 +275,46 @@ jobs:
         run: |
           ./vendor/bin/phpstan analyse -c lib/php/phpstan.neon --no-progress --error-format=github
 
+  lib-perl:
+    needs: compiler
+    runs-on: ubuntu-24.04
+    steps:
+      - uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
+        with:
+          persist-credentials: false
+
+      - name: Install dependencies
+        run: |
+          sudo apt-get update -yq
+          # shellcheck disable=SC2086
+          sudo apt-get install -y --no-install-recommends g++ $BUILD_DEPS libperl-critic-perl
+
+      - name: Configure
+        run: |
+          ./bootstrap.sh
+          # shellcheck disable=SC2086
+          ./configure $CONFIG_ARGS_FOR_SCA
+
+      - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1
+        with:
+          name: thrift-compiler
+          path: compiler/cpp
+
+      - name: Generate Perl thrift files
+        run: |
+          chmod a+x compiler/cpp/thrift
+          compiler/cpp/thrift -version
+          make -j"$(nproc)" -C lib/perl precross
+          make -j"$(nproc)" -C test/perl precross
+          make -j"$(nproc)" -C tutorial/perl precross
+
+      - name: Run perlcritic
+        run: |
+          find lib/perl test/perl tutorial/perl test/audit -iname '*.p[lm]' -type f \
+            ! -path 'lib/perl/blib/*' \
+            ! -path 'lib/perl/t/*' \
+            -print0 | xargs -0 perlcritic
+
   lib-ruby:
     needs: compiler
     runs-on: ubuntu-24.04
diff --git a/.perlcriticrc b/.perlcriticrc
@@ -0,0 +1 @@
+verbose = 1
diff --git a/compiler/cpp/src/thrift/generate/t_perl_generator.cc b/compiler/cpp/src/thrift/generate/t_perl_generator.cc
@@ -244,7 +244,8 @@ void t_perl_generator::init_generator() {
   f_types_ << autogen_comment() << perl_includes();
 
   // Print header
-  f_consts_ << autogen_comment() << "package " << perl_namespace(program_) << "Constants;" << '\n'
+  f_consts_ << autogen_comment() << "package " << perl_namespace(program_)
+            << "Constants;  ## no critic (RequireFilenameMatchesPackage)" << '\n'
             << perl_includes() << '\n';
 }
 
@@ -292,7 +293,8 @@ void t_perl_generator::generate_typedef(t_typedef* ttypedef) {
  * @param tenum The enumeration
  */
 void t_perl_generator::generate_enum(t_enum* tenum) {
-  f_types_ << "package " << perl_namespace(program_) << tenum->get_name() << ";" << '\n';
+  f_types_ << "package " << perl_namespace(program_) << tenum->get_name()
+           << ";  ## no critic (RequireFilenameMatchesPackage)" << '\n';
 
   vector<t_enum_value*> constants = tenum->get_constants();
   vector<t_enum_value*>::iterator c_iter;
@@ -458,7 +460,8 @@ void t_perl_generator::generate_perl_struct_definition(ostream& out,
   const vector<t_field*>& members = tstruct->get_members();
   vector<t_field*>::const_iterator m_iter;
 
-  out << "package " << perl_namespace(tstruct->get_program()) << tstruct->get_name() << ";\n";
+  out << "package " << perl_namespace(tstruct->get_program()) << tstruct->get_name()
+      << ";  ## no critic (RequireFilenameMatchesPackage)\n";
   if (is_exception) {
     out << "use base qw(Thrift::TException);\n";
   }
@@ -719,7 +722,8 @@ void t_perl_generator::generate_service_processor(t_service* tservice) {
   indent_up();
 
   // Generate the header portion
-  f_service_ << "package " << perl_namespace(program_) << service_name_ << "Processor;" << '\n'
+  f_service_ << "package " << perl_namespace(program_) << service_name_
+             << "Processor;  ## no critic (RequireFilenameMatchesPackage)" << '\n'
              << '\n' << "use strict;" << '\n' << extends_processor << '\n' << '\n';
 
   if (extends.empty()) {
@@ -943,7 +947,8 @@ void t_perl_generator::generate_service_interface(t_service* tservice) {
                  + "If);";
   }
 
-  f_service_ << "package " << perl_namespace(program_) << service_name_ << "If;" << '\n' << '\n'
+  f_service_ << "package " << perl_namespace(program_) << service_name_
+             << "If;  ## no critic (RequireFilenameMatchesPackage)" << '\n' << '\n'
              << "use strict;" << '\n' << extends_if << '\n' << '\n';
 
   indent_up();
@@ -968,7 +973,8 @@ void t_perl_generator::generate_service_rest(t_service* tservice) {
     extends_if = "use base qw(" + perl_namespace(extends_s->get_program()) + extends_s->get_name()
                  + "Rest);";
   }
-  f_service_ << "package " << perl_namespace(program_) << service_name_ << "Rest;" << '\n' << '\n'
+  f_service_ << "package " << perl_namespace(program_) << service_name_
+             << "Rest;  ## no critic (RequireFilenameMatchesPackage)" << '\n' << '\n'
              << "use strict;" << '\n' << extends_if << '\n' << '\n';
 
   if (extends.empty()) {
@@ -1030,7 +1036,8 @@ void t_perl_generator::generate_service_client(t_service* tservice) {
     extends_client = "use base qw(" + extends + "Client);";
   }
 
-  f_service_ << "package " << perl_namespace(program_) << service_name_ << "Client;" << '\n' << '\n'
+  f_service_ << "package " << perl_namespace(program_) << service_name_
+             << "Client;  ## no critic (RequireFilenameMatchesPackage)" << '\n' << '\n'
              << extends_client << '\n' << "use base qw(" << perl_namespace(program_)
              << service_name_ << "If);" << '\n';
 
diff --git a/lib/perl/Makefile.am b/lib/perl/Makefile.am
@@ -96,8 +96,15 @@ PERL_GEN = \
 	$(BENCHMARK_GEN) \
 	$(AGGR_GEN)
 
+PERL_PRECROSS_GEN = \
+	gen-perl/ThriftTest/Constants.pm \
+	gen-perl/BenchmarkService.pm \
+	gen-perl2/Aggr.pm
+
 BUILT_SOURCES = $(PERL_GEN)
 
+precross: $(PERL_PRECROSS_GEN)
+
 check-local: $(PERL_GEN)
 	$(PERL) -Iblib/lib -I@abs_srcdir@ -I@builddir@/gen-perl2 -I@builddir@/gen-perl \
 		@abs_srcdir@/test.pl @abs_srcdir@/t/*.t
diff --git a/lib/perl/lib/Thrift/Exception.pm b/lib/perl/lib/Thrift/Exception.pm
@@ -24,7 +24,7 @@ use warnings;
 use Thrift;
 use Thrift::Type;
 
-package Thrift::TException;
+package Thrift::TException;  ## no critic (RequireFilenameMatchesPackage)
 use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");
 
 use overload '""' => sub {
diff --git a/lib/perl/lib/Thrift/MessageType.pm b/lib/perl/lib/Thrift/MessageType.pm
@@ -26,7 +26,7 @@ use Thrift;
 #
 # Message types for RPC
 #
-package Thrift::TMessageType;
+package Thrift::TMessageType;  ## no critic (RequireFilenameMatchesPackage)
 use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");
 
 use constant CALL      => 1;
diff --git a/lib/perl/lib/Thrift/MultiplexedProcessor.pm b/lib/perl/lib/Thrift/MultiplexedProcessor.pm
@@ -27,7 +27,7 @@ use Thrift::MultiplexedProtocol;
 use Thrift::Protocol;
 use Thrift::ProtocolDecorator;
 
-package Thrift::StoredMessageProtocol;
+package Thrift::StoredMessageProtocol;  ## no critic (RequireFilenameMatchesPackage)
 use base qw(Thrift::ProtocolDecorator);
 use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");
 
diff --git a/lib/perl/lib/Thrift/Protocol.pm b/lib/perl/lib/Thrift/Protocol.pm
@@ -28,7 +28,7 @@ use Thrift::Type;
 #
 # Protocol exceptions
 #
-package Thrift::TProtocolException;
+package Thrift::TProtocolException;  ## no critic (RequireFilenameMatchesPackage)
 use base('Thrift::TException');
 use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");
 
diff --git a/lib/perl/lib/Thrift/ServerSocket.pm b/lib/perl/lib/Thrift/ServerSocket.pm
@@ -89,7 +89,6 @@ sub accept
         return $result;
     }
 
-    return undef;
 }
 
 sub close
diff --git a/lib/perl/lib/Thrift/Transport.pm b/lib/perl/lib/Thrift/Transport.pm
@@ -27,7 +27,7 @@ use Thrift::Exception;
 #
 # Transport exceptions
 #
-package Thrift::TTransportException;
+package Thrift::TTransportException;  ## no critic (RequireFilenameMatchesPackage)
 use base('Thrift::TException');
 use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");
 
@@ -177,4 +177,3 @@ sub close
 
 
 1;
-
diff --git a/lib/perl/lib/Thrift/Type.pm b/lib/perl/lib/Thrift/Type.pm
@@ -26,7 +26,7 @@ use Thrift;
 #
 # Data types that can be sent via Thrift
 #
-package Thrift::TType;
+package Thrift::TType;  ## no critic (RequireFilenameMatchesPackage)
 use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");
 
 use constant STOP   => 0;
diff --git a/test/audit/thrift_audit_test.pl b/test/audit/thrift_audit_test.pl
@@ -199,7 +199,7 @@ sub auditNonBreakingChanges
 
 # -----------------------------------------------------------------------------------------------------
 # call thriftAuditTool script
-sub callThriftAuditTool ( $ )
+sub callThriftAuditTool
 {
     my $args = shift;
 
@@ -210,7 +210,7 @@ ( $ )
     return ($exitCode,$output);
 }
 
-sub getMessageSubString( $ )
+sub getMessageSubString
 {
     my $fileName = shift;
     my %lookupTable = (
diff --git a/tutorial/perl/Makefile.am b/tutorial/perl/Makefile.am
@@ -22,6 +22,8 @@ gen-perl/tutorial/Calculator.pm gen-perl/shared/SharedService.pm: $(top_srcdir)/
 
 all-local: gen-perl/tutorial/Calculator.pm
 
+precross: gen-perl/tutorial/Calculator.pm
+
 tutorialserver: all
 	${PERL} PerlServer.pl
 

Original file line number	Diff line number	Diff line change
`@@ -26,7 +26,7 @@ use Thrift;`
`26`	`26`	`#`
`27`	`27`	`# Message types for RPC`
`28`	`28`	`#`
`29`		`-package Thrift::TMessageType;`
	`29`	`+package Thrift::TMessageType; ## no critic (RequireFilenameMatchesPackage)`
`30`	`30`	`use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");`
`31`	`31`
`32`	`32`	`use constant CALL => 1;`
Original file line number	Diff line number	Diff line change
`@@ -28,7 +28,7 @@ use Thrift::Type;`
`28`	`28`	`#`
`29`	`29`	`# Protocol exceptions`
`30`	`30`	`#`
`31`		`-package Thrift::TProtocolException;`
	`31`	`+package Thrift::TProtocolException; ## no critic (RequireFilenameMatchesPackage)`
`32`	`32`	`use base('Thrift::TException');`
`33`	`33`	`use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");`
`34`	`34`
Original file line number	Diff line number	Diff line change
`@@ -89,7 +89,6 @@ sub accept`
`89`	`89`	`return $result;`
`90`	`90`	`}`
`91`	`91`
`92`		`- return undef;`
`93`	`92`	`}`
`94`	`93`
`95`	`94`	`sub close`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,7 @@ use Thrift::Exception;`
`27`	`27`	`#`
`28`	`28`	`# Transport exceptions`
`29`	`29`	`#`
`30`		`-package Thrift::TTransportException;`
	`30`	`+package Thrift::TTransportException; ## no critic (RequireFilenameMatchesPackage)`
`31`	`31`	`use base('Thrift::TException');`
`32`	`32`	`use version 0.77; our $VERSION = version->declare("$Thrift::VERSION");`
`33`	`33`
`@@ -177,4 +177,3 @@ sub close`
`177`	`177`
`178`	`178`
`179`	`179`	`1;`
`180`		`-`