Make code meet perlcritic severity level 4.

.github/workflows/linter.yml

@@ -0,0 +1,24 @@
+---
+name: Lint Code Base
+
+defaults:
+  run:
+    shell: bash
+
+on:
+  push:
+    branches-ignore: [main, develop]
+  pull_request:
+
+jobs:
+  perlcritic:
+    name: Lint Perl files with perlcritic
+    runs-on: ubuntu-24.04
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@v6
+      - name: Install dependencies
+        run: sudo apt-get update && sudo apt-get install -y libperl-critic-perl
+      - name: Run perlcritic
+        shell: bash
+        run: perlcritic ./lib

.perlcriticrc

@@ -0,0 +1,10 @@
+severity = 4
+
+# Prototypes are bad, but signatures are good.  Perlcritic doesn't know the difference.
+[-Subroutines::ProhibitSubroutinePrototypes]
+
+# Perlcritic does not know how to properly deal with async/await.
+[-Modules::RequireEndWithOne]
+
+# Disable this rule for now. Webwork uses too many constants.
+[-ValuesAndExpressions::ProhibitConstantPragma]

bin/addcourse

@@ -58,7 +58,6 @@ BEGIN {
 use lib "$ENV{WEBWORK_ROOT}/lib";
 
 use WeBWorK::CourseEnvironment;
-use WeBWorK::File::Classlist;
 use WeBWorK::Utils                   qw(runtime_use cryptPassword);
 use WeBWorK::Utils::CourseManagement qw(addCourse);
 use WeBWorK::File::Classlist         qw(parse_classlist);

bin/importClassList.pl

@@ -15,8 +15,8 @@ BEGIN
 
 use WeBWorK::CourseEnvironment;
 
-use WeBWorK::DB;
-use WeBWorK::File::Classlist;
+use WeBWorK::DB              qw(check_user_id);
+use WeBWorK::File::Classlist qw(parse_classlist);
 use WeBWorK::Utils           qw(cryptPassword);
 use WeBWorK::File::Classlist qw(parse_classlist);
 

lib/Caliper/Actor.pm

@@ -5,7 +5,6 @@ use strict;
 use warnings;
 use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
-use WeBWorK::Debug;
 use Data::Dumper;
 
 use Caliper::ResourceIri;

lib/Caliper/Entity.pm

@@ -8,7 +8,6 @@ use Digest::SHA qw(sha1_base64);
 
 use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
-use WeBWorK::Debug;
 use WeBWorK::Utils::Tags;
 use WeBWorK::Utils::Sets qw(grade_set grade_gateway);
 use Caliper::ResourceIri;

lib/Caliper/Event.pm

@@ -5,7 +5,6 @@ use strict;
 use warnings;
 use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
-use WeBWorK::Debug;
 use Data::Dumper;
 use Data::UUID;
 
@@ -17,7 +16,7 @@ sub add_defaults {
 	my ($c, $event_hash) = @_;
 	my $ce = $c->ce;
 	my $db = $c->db;
-	my $ug = new Data::UUID;
+	my $ug = Data::UUID->new;
 
 	my $user_id     = $c->param('user');
 	my $session_key = $c->param('key');
@@ -43,6 +42,7 @@ sub add_defaults {
 	if (defined($ENV{HTTP_REFERER})) {
 		$event_hash->{'extensions'}{'referer'} = $ENV{HTTP_REFERER};
 	}
+	return;
 }
 
 1;

lib/Caliper/ResourceIri.pm

@@ -5,7 +5,6 @@ use strict;
 use warnings;
 use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
-use WeBWorK::Debug;
 use Data::Dumper;
 
 # Constructor

lib/Caliper/Sensor.pm

@@ -5,7 +5,7 @@ use strict;
 use warnings;
 use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
-use WeBWorK::Debug;
+use WeBWorK::Debug qw(debug);
 use Data::Dumper;
 use Mojo::JSON  qw(encode_json);
 use Time::HiRes qw/gettimeofday/;
@@ -96,6 +96,7 @@ sub sendEvents {
 			debug($response->content);
 		}
 	}
+	return;
 }
 
 sub log_error {
@@ -119,6 +120,7 @@ sub log_error {
 	} else {
 		debug("Error, unable to open caliper error log file '$logfile' in append mode: $!");
 	}
+	return;
 }
 
 sub formatted_timestamp {

lib/Mojolicious/WeBWorK.pm

@@ -148,7 +148,7 @@ sub startup ($app) {
 
 			$c->stash->{orig_sig_warn} = $SIG{__WARN__};
 
-			$SIG{__WARN__} = sub {
+			$SIG{__WARN__} = sub {    ## no critic (Variables::RequireLocalizedPunctuationVars)
 				my ($warning) = @_;
 				chomp $warning;
 				$c->stash->{warnings} .= "$warning\n";
@@ -168,7 +168,8 @@ sub startup ($app) {
 
 	$app->hook(
 		after_dispatch => sub ($c) {
-			$SIG{__WARN__} = ref($c->stash->{orig_sig_warn}) eq 'CODE' ? $c->stash->{orig_sig_warn} : 'DEFAULT';
+			$SIG{__WARN__} =    ## no critic (Variables::RequireLocalizedPunctuationVars)
+				ref($c->stash->{orig_sig_warn}) eq 'CODE' ? $c->stash->{orig_sig_warn} : 'DEFAULT';
 
 			if ($c->isa('WeBWorK::ContentGenerator') && $c->ce) {
 				$c->authen->store_session if $c->authen;

lib/WeBWorK.pm

@@ -29,7 +29,7 @@ use WeBWorK::Authen;
 use WeBWorK::Authz;
 use WeBWorK::CourseEnvironment;
 use WeBWorK::DB;
-use WeBWorK::Debug;
+use WeBWorK::Debug qw(debug);
 use WeBWorK::Upload;
 use WeBWorK::Utils qw(runtime_use);
 use WeBWorK::ContentGenerator::Login;

lib/WeBWorK/Authen.pm

@@ -40,7 +40,7 @@ use Scalar::Util         qw(weaken);
 use Mojo::Util           qw(b64_encode b64_decode);
 use Math::Random::Secure qw(irand);
 
-use WeBWorK::Debug;
+use WeBWorK::Debug       qw(debug);
 use WeBWorK::Utils       qw(x runtime_use utf8Crypt cryptPassword);
 use WeBWorK::Utils::Logs qw(writeCourseLog);
 use WeBWorK::Utils::TOTP;
@@ -76,7 +76,7 @@ the C<%authen> hash, an exception is thrown.
 
 =cut
 
-sub class {
+sub class {    ## no critic (Subroutines::ProhibitBuiltinHomonyms)
 	my ($ce, $type) = @_;
 
 	if (exists $ce->{authen}{$type}) {
@@ -762,7 +762,7 @@ sub create_session {
 
 	if (!$c->stash->{'webwork2.database_session'} || !$c->stash->{'webwork2.database_session'}{user_id}) {
 		my @chars = @{ $ce->{sessionKeyChars} };
-		$newKey = join('', @chars[ map irand(@chars), 1 .. $ce->{sessionKeyLength} ]);
+		$newKey = join('', @chars[ map { irand(@chars) } 1 .. $ce->{sessionKeyLength} ]);
 		$c->stash->{'webwork2.database_session'} =
 			{ user_id => $userID, key => $newKey, timestamp => time, session => {} };
 	} else {

lib/WeBWorK/Authen/Basic_TheLastOption.pm

@@ -1,5 +1,5 @@
 package WeBWorK::Authen::Basic_TheLastOption;
-use base qw/WeBWorK::Authen/;
+use parent qw(WeBWorK::Authen);
 
 =head1 NAME
 
@@ -13,4 +13,7 @@ be the last one in the array of the Authen module hashes.
 
 =cut
 
+use strict;
+use warnings;
+
 1;

lib/WeBWorK/Authen/CAS.pm

@@ -1,11 +1,12 @@
 package WeBWorK::Authen::CAS;
-use base qw/WeBWorK::Authen/;
+use parent qw(WeBWorK::Authen);
 
 use strict;
 use warnings;
+
 use AuthCAS;
 
-use WeBWorK::Debug;
+use WeBWorK::Debug qw(debug);
 #$WeBWorK::Debug::Enabled = 1;
 #$WeBWorK::Debug::Logfile = "/opt/webwork/webwork2/logs/cas-debug.log";
 #$WeBWorK::Debug::AllowSubroutineOutput = "get_credentials";
@@ -98,7 +99,7 @@ sub get_credentials {
 		#my $cas_certs = $ce->{authen}{cas_options}{certs};
 		#my $cas = new AuthCAS(casUrl => $cas_url,
 		#    CAFile => $cas_certs);
-		my $cas = new AuthCAS(%{ $ce->{authen}{cas_options}{AuthCAS_opts} });
+		my $cas = AuthCAS->new(%{ $ce->{authen}{cas_options}{AuthCAS_opts} });
 
 		my $service = $c->req->url->to_string;
 		# Remove the "ticket=..." parameter that the CAS server added
@@ -170,6 +171,8 @@ sub logout_user {
 	my $go_to = $ce->{authen}{cas_options}{AuthCAS_opts}{casUrl} . '/logout';
 	debug("logging out.  Redirecting to $go_to");
 	$self->{redirect} = $go_to;
+
+	return;
 }
 
 1;

lib/WeBWorK/Authen/LDAP.pm

@@ -1,5 +1,5 @@
 package WeBWorK::Authen::LDAP;
-use base qw/WeBWorK::Authen/;
+use parent qw(WeBWorK::Authen);
 
 use strict;
 use warnings;

lib/WeBWorK/Authen/LTIAdvanced.pm

@@ -1,5 +1,5 @@
 package WeBWorK::Authen::LTIAdvanced;
-use base qw/WeBWorK::Authen/;
+use parent qw(WeBWorK::Authen);
 
 =head1 NAME
 
@@ -17,7 +17,7 @@ use Mojo::URL;
 use URI::Escape;
 use Net::OAuth;
 
-use WeBWorK::Debug;
+use WeBWorK::Debug             qw(debug);
 use WeBWorK::Utils::DateTime   qw(formatDateTime);
 use WeBWorK::Utils::Instructor qw(assignSetToUser);
 use WeBWorK::Localize;
@@ -592,7 +592,7 @@ sub create_user {
 	$c->authz->{PermissionLevel} = $newPermissionLevel;    #cache the Permission Level Record.
 
 	# Assign existing sets
-	my @setsToAssign = ();
+	my @setsToAssign;
 
 	my @globalSetIDs = $db->listGlobalSets;
 	my @GlobalSets   = $db->getGlobalSets(@globalSetIDs);

lib/WeBWorK/Authen/LTIAdvanced/SubmitGrade.pm

@@ -13,7 +13,7 @@ use Mojo::UserAgent;
 use UUID::Tiny ':std';
 use Digest::SHA qw(sha1_base64);
 
-use WeBWorK::Debug;
+use WeBWorK::Debug                      qw(debug);
 use WeBWorK::Utils                      qw(wwRound);
 use WeBWorK::Utils::Sets                qw(grade_all_sets);
 use WeBWorK::Authen::LTI::GradePassback qw(getSetPassbackScore);

lib/WeBWorK/Authen/LTIAdvantage.pm

@@ -12,7 +12,7 @@ use strict;
 use warnings;
 use experimental 'signatures';
 
-use WeBWorK::Debug;
+use WeBWorK::Debug qw(debug);
 use WeBWorK::Localize;
 use WeBWorK::Utils::DateTime   qw(formatDateTime);
 use WeBWorK::Utils::Instructor qw(assignSetToUser);

lib/WeBWorK/Authen/Proctor.pm

@@ -1,5 +1,5 @@
 package WeBWorK::Authen::Proctor;
-use base 'WeBWorK::Authen';
+use parent 'WeBWorK::Authen';
 
 =head1 NAME
 

lib/WeBWorK/Authz.pm

@@ -43,14 +43,15 @@ activity, regardless of their permission level.
 
 use strict;
 use warnings;
+use version;
+
 use Carp qw/croak/;
 
 use WeBWorK::Utils::DateTime qw(before);
 use WeBWorK::Utils::Sets     qw(restricted_set_message);
 use WeBWorK::Authen::Proctor;
 use Net::IP;
 use Scalar::Util qw(weaken);
-use version;
 
 ################################################################################
 
@@ -159,6 +160,8 @@ sub setCachedUser {
 	} else {
 		warn "setCachedUser() called with userID undefined.";
 	}
+
+	return;
 }
 
 =item hasPermissions($userID, $activity)
@@ -181,13 +184,10 @@ assumes that the user does not have permission.
 # This currently only uses two of it's arguments, but it accepts any number, in
 # case in the future calculating certain permissions requires more information.
 sub hasPermissions {
-	if (@_ != 3 and not(@_ == 4 and $_[3] eq 'equal')) {
-		shift @_;    # get rid of self
-		my $nargs = @_;
-		croak "hasPermissions called with $nargs arguments instead of the expected 2: '@_'";
-	}
-
 	my ($self, $userID, $activity, $exactness) = @_;
+	croak 'hasPermissions called with the incorrect number of arguments' unless defined $activity;
+	croak 'hasPermissions called with an incorrect exactness argument' if defined $exactness && $exactness ne 'equal';
+
 	if (!defined($exactness)) { $exactness = 'ge'; }
 	my $c  = $self->{c};
 	my $ce = $c->ce;
@@ -519,7 +519,7 @@ sub invalidIPAddress {
 			|| $set->restrict_ip eq ''
 			|| $set->restrict_ip eq 'No');
 
-	my $clientIP = new Net::IP($c->tx->remote_address);
+	my $clientIP = Net::IP->new($c->tx->remote_address);
 
 	# make sure that we're using the non-versioned set name
 	$setName =~ s/,v\d+$//;
@@ -539,7 +539,7 @@ sub invalidIPAddress {
 	) if (!@restrictAddresses);
 
 	# build a set of IP objects to match against
-	my @restrictIPs = (map { new Net::IP($_) } @restrictAddresses);
+	my @restrictIPs = (map { Net::IP->new($_) } @restrictAddresses);
 
 	# and check the clientAddress against these: is $clientIP
 	#    in @restrictIPs?

lib/WeBWorK/ContentGenerator.pm

@@ -768,6 +768,7 @@ sub url ($c, $args) {
 		return $ce->{courseURLs}{$name};
 	} else {
 		warn __PACKAGE__ . "::url: unrecognized type '$type'.\n";
+		return;
 	}
 }
 

lib/WeBWorK/ContentGenerator/CourseAdmin.pm

@@ -14,7 +14,7 @@ use File::stat;
 use Time::localtime;
 
 use WeBWorK::CourseEnvironment;
-use WeBWorK::Debug;
+use WeBWorK::Debug                   qw(debug);
 use WeBWorK::Utils                   qw(cryptPassword trim_spaces);
 use WeBWorK::Utils::CourseManagement qw(
 	addCourse

lib/WeBWorK/ContentGenerator/GatewayQuiz.pm

@@ -16,11 +16,11 @@ use WeBWorK::Utils::DateTime          qw(before between after);
 use WeBWorK::Utils::Files             qw(path_is_subdir);
 use WeBWorK::Utils::Instructor        qw(assignSetVersionToUser);
 use WeBWorK::Utils::Logs              qw(writeLog writeCourseLog);
-use WeBWorK::Utils::ProblemProcessing qw/create_ans_str_from_responses compute_reduced_score/;
+use WeBWorK::Utils::ProblemProcessing qw(create_ans_str_from_responses compute_reduced_score);
 use WeBWorK::Utils::Rendering         qw(getTranslatorDebuggingOptions renderPG);
 use WeBWorK::Utils::Sets              qw(is_restricted);
 use WeBWorK::DB::Utils                qw(global2user fake_set fake_set_version fake_problem);
-use WeBWorK::Debug;
+use WeBWorK::Debug                    qw(debug);
 use PGrandom;
 use WeBWorK::Authen::LTI::GradePassback qw(passbackGradeOnSubmit);
 use Caliper::Sensor;

lib/WeBWorK/ContentGenerator/Instructor/ProblemSetList.pm

@@ -63,7 +63,7 @@ Delete sets:
 
 use Mojo::File;
 
-use WeBWorK::Debug;
+use WeBWorK::Debug             qw(debug);
 use WeBWorK::Utils             qw(x);
 use WeBWorK::Utils::DateTime   qw(getDefaultSetDueDate);
 use WeBWorK::Utils::Instructor qw(assignSetToUser);

lib/WeBWorK/ContentGenerator/Instructor/Scoring.pm

@@ -7,7 +7,7 @@ WeBWorK::ContentGenerator::Instructor::Scoring - Generate scoring data files
 
 =cut
 
-use WeBWorK::Debug;
+use WeBWorK::Debug        qw(debug);
 use WeBWorK::Utils        qw(wwRound x);
 use WeBWorK::Utils::Files qw(readFile);
 use WeBWorK::Utils::JITAR qw(jitar_id_to_seq jitar_problem_adjusted_status);