Skip Menu |

Preferred bug tracker

Please visit the preferred bug tracker to report your issue.

This queue is for tickets about the Perl-Critic CPAN distribution.

Report information
The Basics
Id: 41938
Status: open
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: jpierce [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: 1.093_03
Fixed in: (no value)

History Show all quoted text
  Wed Dec 24 15:47:59 2008 jpierce [...] cpan.org - Ticket created
Subject: Perl::Critic::Policy::Variables::ProhibitMatchVars exception
Perl::Critic::Policy::Variables::ProhibitMatchVars should permit English match vars if they're (likely) from Regexp::MatchContext
  Wed Dec 24 17:24:45 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #41938] Perl::Critic::Policy::Variables::ProhibitMatchVars exception
Date: Wed, 24 Dec 2008 16:24:29 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Jerrad Pierce via RT wrote: Show quoted text
> Perl::Critic::Policy::Variables::ProhibitMatchVars should permit English > match vars if they're (likely) from Regexp::MatchContext
Ew. I don't see much point to the module. Why not just use captures to get the stuff in front and in back? m/\A(.*)(whatever)(.*)\z/s I don't think use of variables with the same names as those from English should be used because it encourages their use even when that module isn't present.
  Wed Dec 24 17:24:46 2008 The RT System itself - Status changed from 'new' to 'open'
  Wed Dec 24 17:57:23 2008 belg4mit [...] MIT.EDU - Correspondence added
Subject: Re: [rt.cpan.org #41938] Perl::Critic::Policy::Variables::ProhibitMatchVars exception
Date: Wed, 24 Dec 2008 17:52:09 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Jerrad Pierce <belg4mit [...] MIT.EDU>
Show quoted text
>Ew. I don't see much point to the module. Why not just use captures to get >the stuff in front and in back?
... Show quoted text
>I don't think use of variables with the same names as those from English >should be used because it encourages their use even when that module isn't present.
Neither of these opinions really speaks to the *accuracy* of this policy meant to save one from a marginal performance hit, which is what this bug is about. <https://rt.cpan.org/Ticket/Display.html?id=41936> But if you must have some justification of the module's existence in order to ammend this code so that it is robust and accurate: unlike the internal implementation of these variables, ContextMatch results can be assigned to. Besides which you're also forbidding the perfectly legitimate: do{ $PREMATCH = $1; $MATCH =$2; $POSTMATCH = $3 } if /\A(.*)(EXPR)(.*)\Z/ These variable names are logical and self-descriptive, and there's no reason to forbid them so absolutely except for the complexity of accurately checking for the forms described in the documentation. If the current draconian form persists, more accurate documentation and diagnostics are warranted. -- Free map of local environmental resources: http://CambridgeMA.GreenMap.org -- MOTD on Pungenday, the 66th of The Aftermath, in the YOLD 3174: Honi soit la vache qui rit.
  Wed Dec 24 21:14:32 2008 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #41938] Perl::Critic::Policy::Variables::ProhibitMatchVars exception
Date: Wed, 24 Dec 2008 20:14:21 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Jerrad Pierce via RT wrote: Show quoted text
>> I don't think use of variables with the same names as those from English >> should be used because it encourages their use even when that module isn't present.
> > Neither of these opinions really speaks to the *accuracy* of this policy meant > to save one from a marginal performance hit, which is what this bug is about.
[chop] Show quoted text
> Besides which you're also forbidding the perfectly legitimate: > > do{ $PREMATCH = $1; $MATCH =$2; $POSTMATCH = $3 } if /\A(.*)(EXPR)(.*)\Z/
You're misunderstanding me. Due to the history of English, I believe those names should not be used, period. I personally won't change that policy to allow them, but that doesn't mean that you can't convince one of the other P::C developers to make the change.
  Wed Dec 24 21:52:18 2008 belg4mit [...] MIT.EDU - Correspondence added
Subject: Re: [rt.cpan.org #41938] Perl::Critic::Policy::Variables::ProhibitMatchVars exception
Date: Wed, 24 Dec 2008 21:46:05 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Jerrad Pierce <belg4mit [...] MIT.EDU>
Okay, your position is clearer, if not extreme and subjective. As the link in my previous message shows (though I didn't explicitly point it out) the impact of saw ampersand and friends is seriously overstated. Regardless, I also suggested that if the test is going to conintue to completely forbid them, the documentation should better explain why*: Someone at one time made these variables do questionable things so unless you remove this policy we'll never allow you to use these names, even if you're doing so "safely." Even with better documentation perhaps there should be an additional, "laxer" policy that should have a higher severity than the existing (revised severity) policy (as per Perl::Critic::Policy::default_severity)? i.e; Perl::Critic::Policy::Variables::Prohibit*Unsafe*MatchVars * And ideally list the alternatives, something lacking in many of the PODs -- Free map of local environmental resources: http://CambridgeMA.GreenMap.org -- MOTD on Pungenday, the 66th of The Aftermath, in the YOLD 3174: Honi soit la vache qui rit.
  Fri Dec 26 22:50:59 2008 jpierce [...] cpan.org - Correspondence added
Subject: Patches for ProhibitMatchVars exception
I've clarified the text of the POD in ProhibitMatchVars, and changed it to medium severity. The new ProhibitCOREMatchVars is a modified variant (still high severity) of the original ProhibitMatchVars that should allow the various cases previously discussed.
############################################################################## # $URL: http://perlcritic.tigris.org/svn/perlcritic/tags/Perl-Critic-1.092/lib/Perl/Critic/Policy/Variables/ProhibitMatchVars.pm $ # $Date: 2008-09-02 09:43:48 -0700 (Tue, 02 Sep 2008) $ # $Author: thaljef $ # $Revision: 2721 $ ############################################################################## package Perl::Critic::Policy::Variables::ProhibitMatchVars; use 5.006001; use strict; use warnings; use Readonly; use Perl::Critic::Utils qw{ :severities :data_conversion }; use base 'Perl::Critic::Policy'; our $VERSION = '1.092'; #----------------------------------------------------------------------------- Readonly::Scalar my $DESC => q{Match variable used}; Readonly::Scalar my $EXPL => [ 82 ]; Readonly::Array my @FORBIDDEN => qw( $` $& $' $MATCH $PREMATCH $POSTMATCH ); Readonly::Hash my %FORBIDDEN => hashify( @FORBIDDEN ); #----------------------------------------------------------------------------- sub supported_parameters { return () } sub default_severity { return $SEVERITY_MEDIUM } sub default_themes { return qw( core bugs pbp ) } sub applies_to { return qw( PPI::Token::Symbol PPI::Statement::Include ) } #----------------------------------------------------------------------------- sub violates { my ( $self, $elem, undef ) = @_; if (_is_use_english($elem) || #Explicit import of match vars _is_use_english($elem) return $self->violation( $DESC, $EXPL, $elem ); } return; #ok! } #----------------------------------------------------------------------------- sub _is_use_english { my $elem = shift; $elem->isa('PPI::Statement::Include') || return; $elem->type() eq 'use' || return; $elem->module() eq 'English' || return; # Bare, lacking -no_match_vars. Now handled by # Modules::RequireNoMatchVarsWithUseEnglish. return 0 if ($elem =~ m/\A use \s+ English \s* ;\z/xms); return 1 if ($elem =~ m/\$(?:PRE|POST|)MATCH/xms); return; # either "-no_match_vars" or a specific list } sub _is_forbidden_var { my $elem = shift; $elem->isa('PPI::Token::Symbol') || return; return exists $FORBIDDEN{$elem}; } 1; __END__ #----------------------------------------------------------------------------- =pod =head1 NAME Perl::Critic::Policy::Variables::ProhibitMatchVars - Avoid Saw Ampersand et al. =head1 AFFILIATION This Policy is part of the core L<Perl::Critic|Perl::Critic> distribution. =head1 DESCRIPTION Using the "match variables" C<$`>, C<$&>, and/or C<$'> can significantly degrade the performance of perl's regular expression engine. See B<perldoc English> or PBP page 82 for more information. This policy forbids using these variables; the C<English> aliases I<$PREMATCH>, I<$MATCH>, I<$POSTMATCH>; or any other variables with those names. As an alternative, consider the following idiom: /\A(.*?)(PATTERN)(.*)\Z/; Or to avoid throwing off any numbered captures: /PATTERN/; substr($_, 0, $-[0]), substr($_, $-[0], $+[0]-$-[0]), substr($_, $+[0]); This policy used to forbid plain C<use English;> because its aliasing of the match variables involved triggers the aforementioned performance hit. However, the message emitted for that situation was not at all clear and there is now L<Perl::Critic::Policy::Modules::RequireNoMatchVarsWithUseEnglish|Perl::Critic::Policy::Modules::RequireNoMatchVarsWithUseEnglish>, which addresses this situation directly. =head1 CONFIGURATION This Policy is not configurable except for the standard options. =head1 AUTHOR Chris Dolan <cdolan@cpan.org> =head1 COPYRIGHT Copyright (c) 2006-2008 Chris Dolan. All rights reserved. This program is free software; you can redistribute it and/or modify it under the same terms as Perl itself. The full text of this license can be found in the LICENSE file included with this module. =cut # Local Variables: # mode: cperl # cperl-indent-level: 4 # fill-column: 78 # indent-tabs-mode: nil # c-indentation-style: bsd # End: # ex: set ts=8 sts=4 sw=4 tw=78 ft=perl expandtab shiftround :
############################################################################## # $URL: http://perlcritic.tigris.org/svn/perlcritic/tags/Perl-Critic-1.092/lib/Perl/Critic/Policy/Variables/ProhibitMatchVars.pm $ # $Date: 2008-09-02 09:43:48 -0700 (Tue, 02 Sep 2008) $ # $Author: thaljef $ # $Revision: 2721 $ ############################################################################## package Perl::Critic::Policy::Variables::ProhibitCOREMatchVars; use 5.006001; use strict; use warnings; use Readonly; use Perl::Critic::Utils qw{ :severities :data_conversion }; use base 'Perl::Critic::Policy'; our $VERSION = '1.092'; #----------------------------------------------------------------------------- Readonly::Scalar my $DESC => q{Match variable used}; Readonly::Scalar my $EXPL => [ 82 ]; Readonly::Array my @FORBIDDEN => qw( $` $& $' $MATCH $PREMATCH $POSTMATCH ); Readonly::Hash my %FORBIDDEN => hashify( @FORBIDDEN ); #----------------------------------------------------------------------------- sub supported_parameters { return () } sub default_severity { return $SEVERITY_HIGH } sub default_themes { return qw( core bugs ) } sub applies_to { return qw( PPI::Token::Symbol PPI::Statement::Include ) } #----------------------------------------------------------------------------- sub violates { my ( $self, $elem, undef ) = @_; if (_is_forbidden_var($elem) || #Explicit import of match vars _is_use_english($elem) ) { return $self->violation( $DESC, $EXPL, $elem ); } return; #ok! } #----------------------------------------------------------------------------- sub _is_forbidden_var { my $elem = shift; $elem->isa('PPI::Token::Symbol') || return; return exists $FORBIDDEN{$elem}; } sub _is_use_english { my $elem = shift; $elem->isa('PPI::Statement::Include') || return; $elem->type() eq 'use' || return; $elem->module() eq 'English' || return; # Bare, lacking -no_match_vars. Now handled by # Modules::RequireNoMatchVarsWithUseEnglish. return 0 if ($elem =~ m/\A use \s+ English \s* ;\z/xms); return 1 if ($elem =~ m/\$(?:PRE|POST|)MATCH/xms); return; # either "-no_match_vars" or a specific list } 1; __END__ #----------------------------------------------------------------------------- =pod =head1 NAME Perl::Critic::Policy::Variables::ProhibitCOREMatchVars - Avoid most forms of Saw Ampersand et al. =head1 AFFILIATION This Policy is part of the core L<Perl::Critic|Perl::Critic> distribution. =head1 DESCRIPTION Using the "match variables" C<$`>, C<$&>, and/or C<$'> can significantly degrade the performance of perl's regular expression engine. See B<perldoc English> or PBP page 82 for more details. This policy forbids using them or their C<English> aliases, but it does permit dopplegangers such as the variable interface to the more featureful L<Regexp::MatchContext|Regexp::MatchContext>, and the following idiom which is a low-cost alternative to the native match variables: do{ $PREMATCH = $1; $MATCH =$2; $POSTMATCH = $3 } if /\A(.*)(EXPR)(.*)\Z/ If you fear potential confusion from these exceptions, L<Perl::Critic::Policy::Variables::ProhibitMatchVars>. =head1 CONFIGURATION This Policy is not configurable except for the standard options. =head1 AUTHOR Jerrad Pierce <jpierce@cpan.org> =head1 COPYRIGHT Copyright (c) 2006-2008 Chris Dolan. All rights reserved. This program is free software; you can redistribute it and/or modify it under the same terms as Perl itself. The full text of this license can be found in the LICENSE file included with this module. =cut # Local Variables: # mode: cperl # cperl-indent-level: 4 # fill-column: 78 # indent-tabs-mode: nil # c-indentation-style: bsd # End: # ex: set ts=8 sts=4 sw=4 tw=78 ft=perl expandtab shiftround :
--- ProhibitMatchVars.bak Fri Dec 26 20:51:09 2008 +++ ProhibitMatchVars.pm Fri Dec 26 22:41:35 2008 @@ -28,7 +28,7 @@ #----------------------------------------------------------------------------- sub supported_parameters { return () } -sub default_severity { return $SEVERITY_HIGH } +sub default_severity { return $SEVERITY_MEDIUM } sub default_themes { return qw( core bugs pbp ) } sub applies_to { return qw( PPI::Token::Symbol PPI::Statement::Include ) } @@ -37,7 +37,9 @@ sub violates { my ( $self, $elem, undef ) = @_; - if (_is_use_english($elem) || _is_forbidden_var($elem)) { + if (_is_use_english($elem) || + #Explicit import of match vars + _is_use_english($elem) return $self->violation( $DESC, $EXPL, $elem ); } return; #ok! @@ -75,7 +77,7 @@ =head1 NAME -Perl::Critic::Policy::Variables::ProhibitMatchVars - Avoid C<$`>, C<$&>, C<$'> and their English equivalents. +Perl::Critic::Policy::Variables::ProhibitMatchVars - Avoid Saw Ampersand et al. =head1 AFFILIATION @@ -86,14 +88,24 @@ =head1 DESCRIPTION -Using the "match variables" C<$`>, C<$&>, and/or C<$'> can -significantly degrade the performance of a program. This policy -forbids using them or their English equivalents. See B<perldoc -English> or PBP page 82 for more information. - -It used to forbid plain C<use English;> because it ends up causing the -performance side-effects of the match variables. However, the message -emitted for that situation was not at all clear and there is now +Using the "match variables" C<$`>, C<$&>, and/or C<$'> can significantly +degrade the performance of perl's regular expression engine. +See B<perldoc English> or PBP page 82 for more information. + +This policy forbids using these variables; the C<English> aliases +I<$PREMATCH>, I<$MATCH>, I<$POSTMATCH>; or any other variables with +those names. As an alternative, consider the following idiom: + + /\A(.*?)(PATTERN)(.*)\Z/; + +Or to avoid throwing off any numbered captures: + + /PATTERN/; + substr($_, 0, $-[0]), substr($_, $-[0], $+[0]-$-[0]), substr($_, $+[0]); + +This policy used to forbid plain C<use English;> because its aliasing of the +match variables involved triggers the aforementioned performance hit. However, +the message emitted for that situation was not at all clear and there is now L<Perl::Critic::Policy::Modules::RequireNoMatchVarsWithUseEnglish|Perl::Critic::Policy::Modules::RequireNoMatchVarsWithUseEnglish>, which addresses this situation directly.