Using 'sane' to refer to other event loops in the AE entry is fine, using it to refer to people in the JSON::XS entry less so. "more collaborative maintainer" would be clearer anyway.

• File::HomeDir: has many features that usually aren't needed (and has additional prereqs for darwin, which can cause issues in a cpanfile.snapshot when some team members are on darwin and others aren't): for the normal homedir case, simply use this (simplifying if windows or older perls aren't supported):

    my $homedir = $^O eq 'MSWin32' && "$]" < 5.016
      ? $ENV{HOME} || $ENV{USERPROFILE}
      : (<~>)[0];

• Test::Requires -> Test::Needs: (quoted from docs) "Test::Requires will act as a "use" statement (despite its name), calling the import sub. Under "RELEASE_TESTING", it will BAIL_OUT if a module fails to load rather than using a normal test fail. It also doesn't distinguish between missing modules and broken modules."

• FindBin -> Dir::Self - I'm sure there was an article somewhere talking about what's wrong with FindBin, but I haven't been able to find it - nevertheless Dir::Self is much simpler code.

foo and baz get caught correctly. bat is ignored correctly because it's an empty prototype. bar should be caught but is not.

This looks like the same problem as Perl-Critic/Perl-Critic#978

#!/usr/bin/perl

use warnings;
use strict;
use 5.010;


sub foo($$) {}

sub bar :prototype($$) {}

sub bat() {}

sub baz($$) {}

$ perlcritic --s Community::Prototypes foo.pl
foo.pl: Using function prototypes.
    8:sub foo($$) {}

foo.pl: Using function prototypes.
    14:sub baz($$) {}

use strict;
use warnings;

my @xs = (
    ['foo'],
    ['bar', 'baz'],
);

@xs = sort {
    my ($k1) = @{$a};
    my ($k2) = @{$b};
    $k1 cmp $k2
} @xs;

$ perlcritic bug.pl
Using $a or $b outside sort() at line 10, column 18.  $a and $b are special package variables for use in sort() and related functions. Declaring them as lexicals like "my $a" may break sort(). Use different variable names.  (Severity: 4)
Using $a or $b outside sort() at line 11, column 18.  $a and $b are special package variables for use in sort() and related functions. Declaring them as lexicals like "my $a" may break sort(). Use different variable names.  (Severity: 4)

:-(

The Freenode:: policies were recently renamed to Community::*, with backward compatibility aliases in the Freenode:: namespace. The backward compatibility aliases seem to be treated as distinct policies by perlcritic core, which means they're all being processed twice and configuration for Freenode::foo isn't applied to Community::foo or vice versa.

I noticed this when my project's CI broke because all of the Freenode:: policies that we turned off, got turned back on under their Community:: names. The most straightforward workaround seems to be to rewrite the main perlcritic configuration in terms of the Community:: names and then also list every single one of the Freenode:: names as disabled.

I have a module that imports subroutine signatures into the caller. Unfortunately, it still triggers it as prototypes.

I would like a configuration to allow me to pick which modules also import signatures.

Thanks. :)

Consider this program foo.pl:

#!/var/perl/bin/perl

use 5.010;
use strict;
use warnings;

print $a;

If I run perlcritic on it, I get two errors:

$ perlcritic -2 -verbose='%f:%l:%c %m (%p)\n'  foo.pl
foo.pl:7:7 Using $a or $b outside sort() (Community::DollarAB)
foo.pl:7:7 Using $a or $b outside sort() (Community::DollarAB)

If I then create a Dev/perlcriticrc file like so:

[-Community::DollarAB]

and rerun I get only one error:

$ perlcritic -2 -verbose='%f:%l:%c %m (%p)\n' -p Dev/perlcriticrc foo.pl
foo.pl:7:7 Using $a or $b outside sort() (Community::DollarAB)

If I add

[-Freenode::DollarAB]

to my perlcriticrc, then it runs clean.

It looks like the Freenode::DollarAB aliasing to Community::DollarAB means I now have two policies checking on my code, and that I need to silence both of them. :-(

The suggestion to use if (%bar){} here should probably be replaced with if (keys %bar){}, as using a hash directly for this purpose is several orders of magnitude slower in some instances. Perl seems to be optimized to deal with the specific case of if (%hash){}, but the performance drops dramatically for, say, using unless, which isn't present with the keys %hash variant:

zoffix@:~$ perl -Mojo -wlE 'my %h = 1..1000_000; n { for(1..100){ unless(%h ){} } } 100'
4.80698 wallclock secs ( 4.79 usr +  0.01 sys =  4.80 CPU) @ 20.83/s (n=100)
zoffix@:~$ perl -Mojo -wlE 'my %h = 1..1000_000; n { for(1..100){ unless(keys %h){} } } 100'
0.000820875 wallclock secs ( 0.00 usr +  0.00 sys =  0.00 CPU)

Autoderef (push/pop/shift/unshift/splice/keys/values/each on a ref) was considered experimental since added in 5.14.0 and removed in 5.24.0.

Similar to https://metacpan.org/pod/Perl::Critic::Policy::Modules::RequireFilenameMatchesPackage but only requiring that one such package exists in the file.

#!/usr/bin/perl
use 5.14.2;
use warnings;
our $VERSION = "0.00";
say "hello";

Here 'use 5.14.2' implies 'use strict' (plus a bit more), and that would be the exception to the "version string used" warning.

$ perlcritic -1 xx.pl
Argument "^E^N^B" isn't numeric in numeric gt (>) at /pro/lib/perl5/site_perl/5.22.0/Perl/Critic/Policy/Freenode/StrictWarnings.pm line 57.
xx.pl#1.1:      [4 - Freenode::StrictWarnings]  Missing strict or warnings      :#!/usr/bin/perl
xx.pl#2.1:      [3 - ValuesAndExpressions::ProhibitVersionStrings]      Version string used     :use 5.14.2;

The policy Perl::Critic::Policy::Freenode::Wantarray does not detect use of CORE::wantarray.

The following snippet results in a perlcritic issue

use strict;
use warnings;

my $foo = {workers => {a => b => c =>4}};

if (keys $foo->workers->%* < 4) {
  print "Warnings";
}

autoderef is deprecated at line 6, column 5. Use of each/keys/pop/push/shift/splice/unshift/values on a reference is an experimental feature that is removed in perl v5.24.0. Dereference the array or hash to use these functions on it. (Severity: 4)

Not sure if this is in scope for Perl::Critic::Freenode namespace, but for a while I was thinking - how nice it would be to have a module that reports deprecated/removed things in future Perl's than the one you are using.

It's a pain to implement and this can be already achieved by running your library against newer Perl versions ( perhaps with the help of Perlbrew or etc. ). Though, this would be great CI/code review tool.

https://metacpan.org/pod/Perl::Critic::Policy::Freenode::AmpersandSubCalls does not say anything about how it is different from https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::ProhibitAmpersandSigils.

Is it indeed different/better? Should users use the Freenode version instead of the core one? When should the user choose one over the other?

Or is it just redundant?

https://metacpan.org/pod/Perl::Critic::Policy::TestingAndDebugging::RequireUseStrict and https://metacpan.org/pod/Perl::Critic::Policy::TestingAndDebugging::RequireUseWarnings

If your policy doesn't do anything different, I'd recommend removing it. If it does, please document the differences.

As found in https://perlmonks.org/?node_id=1223518

$Person->shift( $foo );

results in the violation:

autoderef is deprecated at line 1, column 10.  Use of each/keys/pop/push/shift/splice/unshift/values on a reference is an experimental feature that is removed in perl v5.24.0. Dereference the array or hash to use these functions on it.  (Severity: 4)

This sentence seems broken: https://github.com/Grinnz/Perl-Critic-Freenode/blob/master/lib/Perl/Critic/Policy/Freenode/DiscouragedModules.pm#L19

POE, IO::Async, and Mojo::IOLoop are widely used and interoperable async event loops.

Should the and be a for?

When I put theme = freenode in .perlcriticrc I get an error message from perlcritic stating No policies selected. and exiting with error-code 255.

Shell session showing the issue:

[0]suppy@celestia:~/work/sbotools$ cat .perlcriticrc
theme = freenode
[0]suppy@celestia:~/work/sbotools$ perlcritic SBO-Lib/lib/SBO/Lib.pm
No policies selected.
[255]suppy@celestia:~/work/sbotools$ perlcritic --theme freenode SBO-Lib/lib/SBO/Lib.pm
SBO-Lib/lib/SBO/Lib.pm source OK
[0]suppy@celestia:~/work/sbotools$ env | grep -i perl
PERL5LIB=/home/suppy/perl5/lib/perl5
PERL_MB_OPT=--install_base "/home/suppy/perl5"
PATH=/home/suppy/perl5/bin:/home/suppy/.commands/:/usr/local/bin:/usr/bin:/bin:/usr/games:/usr/lib64/kde4/libexec:/usr/lib64/qt/bin:/usr/share/texmf/bin
PERL_LOCAL_LIB_ROOT=/home/suppy/perl5
PERL_MM_OPT=INSTALL_BASE=/home/suppy/perl5
[0]suppy@celestia:~/work/sbotools$

The double diamond (<<>>) operator is a safer version of diamond.

It would be great to support that too.

With the rename to Community (which I applaud), I saw Miscellanea::ProhibitUselessNoCritic violations where I hadn't before. When I removed the ## no critic (Freenode::Wantarray) I got the Community::Wantarray violation. Instead changing the ## no critic (Community::Wantarray) cleared the violations.

This is the CI fail:
https://github.com/perlwasm/Wasm/pull/112/checks?check_run_id=2694106399
and this is the workaround:
perlwasm/Wasm@62d30e1
(the commit also includes a change in the perlcritic rc file, but that wasn't required to clear the error, as per local testing)

Prompted by https://www.reddit.com/r/perl/comments/c4lhaq/and_most_damning_of_all_the_resulting_code_was/erx5hno/

The punchline is, of course, the 100 kB file keeper in http://www.ibiblio.org/pub/Linux/search/keeper-1.54.tar.gz. The code is so eyewateringly bad, it crashes Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles. esr made a career out of bashing Perl, always blaming the language, never his own lack of skill.

$ cat minimal
#!/usr/bin/perl

open(CHECK, '/foo');
$ perlcritic minimal -s InputOutput::ProhibitBarewordFileHandles
Fatal error while critiquing "minimal": Not an ARRAY reference at /var/perl5.20.3/lib/site_perl/5.20.3/Perl/Critic/Policy/InputOutput/ProhibitBarewordFileHandles.pm line 35.

$ perlcritic minimal -s Freenode::BarewordFileHandles
Fatal error while critiquing "minimal": Not an ARRAY reference at /var/perl5.20.3/lib/site_perl/5.20.3/Perl/Critic/Policy/Freenode/BarewordFilehandles.pm line 47.

See also Perl-Critic/Perl-Critic#878

EmptyReturn currently does not catch statements like return if $foo; This should be doable, it just needs to return the violation if the next element is a perl builtin which is a statement modifier.

Technically you just outlawed Try::Tiny.

I'd say prototypes with an & should be at least a lower level of violation since those are sometimes useful.

use constant CONSTANT => "foo";
my ($foo) = "foobar" =~ /(${\CONSTANT})/;

...yields

test.pl:2:25: /\C/ is deprecated. Use of the \C character class in regular expressions is deprecated in perl v5.20.0. To examine a string's UTF-8-encoded byte representation, encode it to UTF-8. [Community::DeprecatedFeatures]

Perl 5.34, Perl::Critic::Community v1.0.2, Perl::Critic 1.140

If both Community and Freenode are enabled, Community settings seem to be ignored.

(As background, I don't use themes. I run against everything which I've installed, unless I explicitly exclude it. It's not currently documented in the top-level Community documentation that the same policies are also installed under the Freenode name. This caused me a bit of confusion during my debugging, as I was seeing Freenode policies and had never explicitly installed them.)

The following concocted code and perlcritic configuration illustrate the issue.

test code:

package Test;
use List::Util;
our $VERSION = 0.01;
1;

perlcritic.rc

verbose = 9
severity = brutal

exclude = TestingAndDebugging

[Community::StrictWarnings]
extra_importers = List::Util

I expect this to pass Community::StrictWarnings, but perlcritic returns:
[Community::StrictWarnings] Missing strict or warnings at line 1, near 'package Test;'. (Severity: 4)

I discovered that if I either

1. explicitly exclude Freenode; or
2. duplicate the settings under the Freenode::StrictWarnings namespace,

then the tests pass. It's not clear to me if the Freenode shims are just being reported as Community in the output or that they are actually hijacking Community's settings. Regardless, something is awry.

Thanks,
Diab

Hello,

This module advertises 'return ()' as correct and 'return' as incorrect, and actually reports the latter as a violation if I read the code correctly. This is misleading because these two forms are syntaxically completely equivalent. While I understand that this could be the object of an internal stylistic policy when writing the sub, it has no implication to the problem your Perl::Critic policy is trying to advert : being surprised by an unpredictable number of return values when calling the sub in list context.

I suggest you to collect all the return points of any sub and report a violation if they are possibly inconsistent (like having both a 'return' and a 'return $foo'). In particular, note that a conditional at the end of a sub yields a scalar value.

Vincent

Sometimes you have no choice but to use a module on one of the discouraged lists. Right now, you have to turn the entire policy off to make perlcritic happy with that. It would be nice if it were possible to configure these policies with a list of specific modules to be allowed.

Example: I have a whole bunch of scripts that can only depend on core library modules, nothing from CPAN. They use FindBin; in the context it's used, it does the Right Thing. I'd like to be able to write something like

[Community::DiscouragedModules]
allow = FindBin

Add a policy for indirect object method calls, with $HIGHEST severity. The problem with this is there's no reliable way to detect it without compiling the code, which means it will be a dynamic policy that requires --allow-unsafe.

https://metacpan.org/pod/Perl::Critic::Policy::Dynamic::NoIndirect

I came across this lint:

Used module List::MoreUtils at line 6, column 1. List::MoreUtils is a far more complex distribution than it needs to be. Use List::SomeUtils instead, or see List::Util or List::UtilsBy for alternatives.

and to me this reads as "this library is very fully featured and has everything you need :) don't use it!". I assume that since there is a lint for it there is a very good reason to avoid it. Maybe the message could include some information about the hazard?

HTML::Template is awful but if you're stuck with it anyway, ::Pro allows method calls and therefore is a nice first step towards actually having a model to use with other templates.

Similar to #11, I don't see any indication of when I should use Freenode::Prototypes instead of Subroutines::ProhibitSubroutinePrototypes.

http://search.cpan.org/~dbook/Perl-Critic-Freenode/lib/Perl/Critic/Policy/Freenode/Prototypes.pm

vs.

http://search.cpan.org/~thaljef/Perl-Critic-1.125/lib/Perl/Critic/Policy/Subroutines/ProhibitSubroutinePrototypes.pm

common::sense doesn't export a full set of warnings so doesn't count; I think sanity.pm does count but I dunno if it has enough revdeps to matter.

However the policy definitely needs to be configurable since I commonly end up with e.g.

package MyApp::Class;

use Import::Into;
use Moo ();
use MyApp::Utils ();

sub import {
    MyApp::Utils->import::into(1);
    Moo->import::into(1);
}

and a similar MyApp::Role - and obviously I'd then want the policy to accept those.

Requested by @plicease, a policy to catch use of \d to match digits where [0-9] was meant. Since it is valid but uncommon to match unicode digits this would be a severity 1 policy. Possibly also allowed with the /u flag indicating unicode is expected.

https://perldoc.pl/perldata#Multi-dimensional-array-emulation is a perl 4 feature that still exists in perl 5, but in most cases is only used accidentally. We should add a low to medium severity policy to detect it.

"The default place to put the next value or input record when a <FH> , readline, readdir or each
operation's result is tested by itself as the sole criterion of a while test. Outside a while test, this will not happen." - referring to $_

https://metacpan.org/pod/distribution/perl/pod/perlvar.pod#pod-_

Many of the syntax removals in https://metacpan.org/pod/release/XSAWYERX/perl-5.25.1/pod/perldelta.pod#Core-Enhancements should be detectable.

Thank you for a very useful policy of DollarAB, it is very helpful, let us catch a couple potential bugs in our codebase.

There's a caveat though- we're using pairwise in our code, and DollarAB complains about it. pairwise is exported by List::MoreUtils and List::AllUtils — granted, there's another policy that discourages using List::MoreUtils but getting rid of that dependency is not feasible for us at this time.

Can you please either add pairwise to the list, or, better yet, make the policy extensible so that the users could redefine the 'list of sorters' to suit their needs?

Thanks!