| Subject: | ProhibitMutatingListFunctions should not prohibit nonmutating tr/// (test case and fix included) |
The tr function is often used just to count characters:
my $count_colons = ($str =~ tr/://);
In this case it does not change the input string. You can also use it
with map or grep, for example
my @bad = grep { tr/A-Za-z//c } @in;
die "nonalphabetic characters in: @bad" if @bad;
Currently ControlStructures::ProhibitMutatingListFunctions bans all use
of tr in a map or grep block. It should complain only if the call
changes the $_ string. A tr with an empty substitution list, and no /s
or /d flag, is the normal perlish way to count characters and should be
allowed.
A patch is attached adding test cases for this and fixing the code. PPI
doesn't provide methods to get the substitution list or flags from the
tr call, so some string matching is needed.
| Subject: | perl-Perl-Critic-1.096-nonmutating-tr.diff |
Index: Perl-Critic/t/ControlStructures/ProhibitMutatingListFunctions.run
===================================================================
--- Perl-Critic/t/ControlStructures/ProhibitMutatingListFunctions.run (revision 3246)
+++ Perl-Critic/t/ControlStructures/ProhibitMutatingListFunctions.run (working copy)
@@ -84,6 +84,15 @@
@bar = map {m/4/} @foo;
@bar = map {my $s=$_; chomp $s; $s} @foo;
+## name Non-mutatating tr///
+## failures 0
+## cut
+
+@bar = map {tr/a-z//} @foo;
+@bar = map {tr/123//c} @foo;
+@bar = map {tr{A-Z}{}c} @foo;
+@bar = map {tr<foo><>c} @foo;
+
#-----------------------------------------------------------------------------
## name Value given for list_funcs passing
Index: Perl-Critic/lib/Perl/Critic/Policy/ControlStructures/ProhibitMutatingListFunctions.pm
===================================================================
--- Perl-Critic/lib/Perl/Critic/Policy/ControlStructures/ProhibitMutatingListFunctions.pm (revision 3246)
+++ Perl-Critic/lib/Perl/Critic/Policy/ControlStructures/ProhibitMutatingListFunctions.pm (working copy)
@@ -153,6 +153,38 @@
return if ! ( $elem->isa('PPI::Token::Regexp::Substitute')
|| $elem->isa('PPI::Token::Regexp::Transliterate') );
+ if ($elem->isa('PPI::Token::Regexp::Transliterate')) {
+ # Not all tr/// operators are mutators. It can be used just to count
+ # characters, typically by having an empty replacement list and no /d
+ # or /s flag.
+ #
+ # PPI doesn't provide methods to check this so we must look at the
+ # string. This is obviously a bit icky, but try to be conservative
+ # when deciding that an expression does not change the string.
+ # There are some operations such as tr/foo/foo/ which do not alter
+ # the string but we don't spot.
+ #
+ my $str = $elem->content;
+ if ($str =~ s/\A(?:tr|y)//) {
+ # It begins tr or y, find the delimiter characters.
+ if ($str =~ /\A(\W).*\1\1c?\z/) {
+ # A single-character delimiter like tr/whatever//.
+ return 0;
+ }
+ if ($str =~ /\A(\W).*(\W)\1\2c?\z/) {
+ # Test for matched delimiters.
+ my $delims = $1 . $2;
+ my @allowed = ('()', '{}', '[]', '<>');
+ foreach (@allowed) {
+ return 0 if $delims eq $_;
+ }
+ }
+ }
+ else {
+ # Strange, but let it go.
+ }
+ }
+
# If the previous sibling does not exist, then
# the regex implicitly binds to $_
my $prevsib = $elem->sprevious_sibling;