Login
RT #34713. Tweaked Subroutines::ProtectPrivateVars
authorJeffrey Ryan Thalhammer <jeff@imaginative-software.com>
Tue, 15 Jul 2008 05:30:16 +0000 (05:30 +0000)
committerJeffrey Ryan Thalhammer <jeff@imaginative-software.com>
Tue, 15 Jul 2008 05:30:16 +0000 (05:30 +0000)
so that "shift->_private_sub()" is allowed.

Changes
lib/Perl/Critic/Policy/Subroutines/ProtectPrivateSubs.pm
t/Subroutines/ProtectPrivateSubs.run

diff --git a/Changes b/Changes
index f98b67b..f1703d8 100644 (file)
--- a/Changes
+++ b/Changes
       Inspired by RT #37632.
     * Subroutines::RequireFinalReturn now includes exec in the set of things
       that mark a successful return.  RT #37672
       Inspired by RT #37632.
     * Subroutines::RequireFinalReturn now includes exec in the set of things
       that mark a successful return.  RT #37672
+    * Subroutines::ProtectPrivateSubs now allows expressions like
+      "shift->_some_private_method();".  Note that this *only* applies
+      to the "shift" function -- a private method call on the right of any
+      other bareword still causes a violation.  RT #34713.
 
     Bug fixes:
     * BuiltinFunctions::ProhibitSleepViaSelect would complain if there were
 
     Bug fixes:
     * BuiltinFunctions::ProhibitSleepViaSelect would complain if there were
index 48deb51..f5aea70 100644 (file)
@@ -34,19 +34,20 @@ sub applies_to           { return 'PPI::Token::Word'     }
 sub violates {
     my ( $self, $elem, undef ) = @_;
 
 sub violates {
     my ( $self, $elem, undef ) = @_;
 
-    my $psib = $elem->sprevious_sibling;
-    my $psib_name = eval { $psib->content };
-    no warnings 'uninitialized';    ## no critic ProhibitNoWarnings
-    if (   $psib_name ne 'package'
-        && $psib_name ne 'require'
-        && $psib_name ne 'use'
-        && (   $self->_is_other_pkg_private_function($elem)
-            || $self->_is_other_pkg_private_method($elem) )
-        )
+    if (my $psib = $elem->sprevious_sibling()) {
+        my $psib_name = $psib->content();
+        return if $psib_name eq 'package';
+        return if $psib_name eq 'require';
+        return if $psib_name eq 'use';
+    }
+
+    if ( $self->_is_other_pkg_private_function($elem)
+         || $self->_is_other_pkg_private_method($elem) )
     {
         return $self->violation( $DESC, $EXPL, $elem );
     }
     {
         return $self->violation( $DESC, $EXPL, $elem );
     }
-    return;                         #ok!
+
+    return;  # ok!
 }
 
 sub _is_other_pkg_private_function {
 }
 
 sub _is_other_pkg_private_function {
@@ -62,8 +63,15 @@ sub _is_other_pkg_private_method {
     $elem =~ m{ \A _\w+ \z }xms || return;
     my $op = $elem->sprevious_sibling() || return;
     $op eq q{->} || return;
     $elem =~ m{ \A _\w+ \z }xms || return;
     my $op = $elem->sprevious_sibling() || return;
     $op eq q{->} || return;
+
     my $pkg = $op->sprevious_sibling() || return;
     $pkg->isa('PPI::Token::Word') || return;
     my $pkg = $op->sprevious_sibling() || return;
     $pkg->isa('PPI::Token::Word') || return;
+
+    # sometimes the previous sib is a keyword, as in:
+    # shift->_private_method();  This is typically used as
+    # shorthand for "my $self=shift; $self->_private_method()"
+    $pkg ne 'shift' || return;
+
     return 1;
 }
 
     return 1;
 }
 
index 10e79cc..b1f7bd1 100644 (file)
@@ -18,6 +18,29 @@ require My::Self::_private;
 
 #-----------------------------------------------------------------------------
 
 
 #-----------------------------------------------------------------------------
 
+## name "shift" followed by private method call
+## failures 0
+## cut
+
+# See http://rt.cpan.org/Ticket/Display.html?id=34713
+# Also, see the test case below for a counter example.
+
+shift->_private_sub();
+shift->_private_sub;
+
+#-----------------------------------------------------------------------------
+
+## name other builtin-function followed by private method call
+## failures 2
+## cut
+
+# See http://rt.cpan.org/Ticket/Display.html?id=34713
+
+pop->_private_sub();
+pop->_private_sub;
+
+#-----------------------------------------------------------------------------
+
 ## name Difficult-to-detect pass
 # This one should be illegal, but it is too hard to distinguish from
 # the next one, which is legal
 ## name Difficult-to-detect pass
 # This one should be illegal, but it is too hard to distinguish from
 # the next one, which is legal