RT #79289: False Postive in Perl::Critic::Utils::is_in_void_context()

The ticket is really about BuiltinFunctions::ProhibitVoidGrep, but the
requester's diagnosis of the point of failure and the nature of the
correction was correct. This revision applies the requester's
correction, and adds tests to ProhibitVoidGrep.run and
ProhibitVoidMap.run, since the latter policy had the same problem.

The thing is, it appears to me that most uses of grep and map in slice
subscripts were passing for the wrong reason. Both policies, after
ensuring they have the PPI::Token::Word that they want, call
is_function_call() on the element in question, and accept the element if
this returns false. The first thing that is_function_call() does is to
call is_hash_key(), returning false if is_hash_key() returns true.

Well, is_hash_key() is documented as detecting barewords used as hash
keys. But it returns true when handed the 'grep' token in the following:
    @hash{ grep { /foo/ } keys %hash }
    @hash{ grep /foo/, keys %hash }
This causes the policy to declare a true negative in the above cases,
but to declare it for the wrong reason. The requester, on the other
hand, was doing
    @hash{ grep ( /foo/, keys %hash ) )
which is a violation of a couple other policies, but should not be a
violation of this one. But the parens after the 'grep' were causing
is_hash_key() to fail; consequently is_function_call() succeeded, so the
failure of is_in_void_context() to return false when called on a token
in a subscript was exposed.

diff --git a/Changes b/Changes
index 652632a..ebbcf54 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,6 +1,10 @@
 [X.XXX] Released on XXXX-XX-XX
 
      Policy Changes:
+     * BuiltinFunctions::ProhibitVoidGrep and ::ProhibitVoidMap: grep
+       and map called as functions are now allowed in slice operations.
+       RT #79289
+       Thanks to Wade at Anomaly dot org for the patch.
      * Subroutines::RequireArgUnpacking: Most tests of the size of @_
        are now allowed.  RT #79138
      Other Changes:

diff --git a/MANIFEST b/MANIFEST
index 831d355..ca7cfe6 100644 (file)
--- a/MANIFEST
+++ b/MANIFEST
@@ -212,6 +212,9 @@ lib/Test/Perl/Critic/Policy.pm
 LICENSE
 Makefile.PL
 MANIFEST                       This list of files
+META.json
+META.yml
+README
 t/00_modules.t
 t/01_bad_perlcriticrc
 t/01_config.t
@@ -253,6 +256,7 @@ t/20_policy_pod_spelling.t
 t/20_policy_prohibit_evil_modules.t
 t/20_policy_prohibit_hard_tabs.t
 t/20_policy_prohibit_trailing_whitespace.t
+t/20_policy_prohibit_useless_no_critic.t
 t/20_policy_require_consistent_newlines.t
 t/20_policy_require_tidy_code.t
 t/92_memory_leaks.t
@@ -418,6 +422,3 @@ xt/author/95_kwalitee.t
 xt/author/98_pod_syntax.t
 xt/author/99_pod_coverage.t
 xt/author/generate_without_optional_dependencies_wrappers.PL
-README
-META.yml
-META.json

diff --git a/lib/Perl/Critic/Utils.pm b/lib/Perl/Critic/Utils.pm
index d08456f..139a186 100644 (file)
--- a/lib/Perl/Critic/Utils.pm
+++ b/lib/Perl/Critic/Utils.pm
@@ -884,6 +884,7 @@ sub is_in_void_context {
         return if $parent->isa('PPI::Structure::For');
         return if $parent->isa('PPI::Structure::Condition');
         return if $parent->isa('PPI::Structure::Constructor');
+        return if $parent->isa('PPI::Structure::Subscript');
 
         my $grand_parent = $parent->parent();
         if ($grand_parent) {

diff --git a/t/BuiltinFunctions/ProhibitVoidGrep.run b/t/BuiltinFunctions/ProhibitVoidGrep.run
index ff86e61..840cc2d 100644 (file)
--- a/t/BuiltinFunctions/ProhibitVoidGrep.run
+++ b/t/BuiltinFunctions/ProhibitVoidGrep.run
@@ -48,6 +48,19 @@ grep { spam($_) }
 
 #-----------------------------------------------------------------------------
 
+## name Subscript grep (RT #79289)
+## failures 0
+## cut
+
+my %hash;
+
+delete @hash{ grep { m/ foo /smx } keys %hash };
+delete @hash{ grep m/ foo /smx, keys %hash };
+# The following is the form that was actually failing.
+delete @hash{ grep ( m/ foo /smx, keys %hash ) };
+
+#-----------------------------------------------------------------------------
+
 ##############################################################################
 #      $URL$
 #     $Date$

diff --git a/t/BuiltinFunctions/ProhibitVoidMap.run b/t/BuiltinFunctions/ProhibitVoidMap.run
index 6164995..71538f0 100644 (file)
--- a/t/BuiltinFunctions/ProhibitVoidMap.run
+++ b/t/BuiltinFunctions/ProhibitVoidMap.run
@@ -47,6 +47,19 @@ $self->map('Pennsylvania Ave, Washington, DC');
 
 #-----------------------------------------------------------------------------
 
+## name Subscript map (derived from RT #79289)
+## failures 0
+## cut
+
+my %hash;
+
+delete @hash{ map { uc $_ } keys %hash };
+delete @hash{ map uc( $_ ), keys %hash };
+# This is the form analogous to what failed under RT #79289.
+delete @hash{ map ( uc( $_ ), keys %hash ) };
+
+#-----------------------------------------------------------------------------
+
 ##############################################################################
 #      $URL$
 #     $Date$