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: 75329
Status: open
Priority: 0/
Queue: Perl-Critic

People
Owner: Nobody in particular
Requestors: dolmen [...] cpan.org
Cc:
AdminCc:

Bug Information
Severity: Wishlist
Broken in: 1.117
Fixed in: (no value)

Attachments
Modules-ProhibitInheritingExporter-2.patch
Modules-ProhibitInheritingExporter.patch



Subject: New policy: Modules::ProhibitInheritingExporter
Download (untitled) / with headers
text/plain 472b
As a follow-up to bug #75300 I wrote a policy to report modules that use Exporter by inheriting from it (use base 'Exporter') instead of just importing its 'import' sub (use Exporter 'import'). The name of the policy is Modules::ProhibitInheritingExporter. Patch attached. Comments welcome. This is my first policy and english is not my native language. Also I'm not sure about the naming, the severity and the themes. -- Olivier Mengué - http://perlresume.org/DOLMEN
Subject: Modules-ProhibitInheritingExporter.patch
Index: lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm =================================================================== --- lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm (revision 0) +++ lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm (revision 0) @@ -0,0 +1,118 @@ +############################################################################## +# $URL$ +# $Date$ +# $Author$ +# $Revision$ +############################################################################## + +package Perl::Critic::Policy::Modules::ProhibitInheritingExporter; + +use 5.006001; +use strict; +use warnings; +use Readonly; + +use Perl::Critic::Utils qw{ :severities }; +use base 'Perl::Critic::Policy'; + +our $VERSION = '1.117'; + +#----------------------------------------------------------------------------- + +Readonly::Scalar my $DESC => q{Module inherits from Exporter}; +Readonly::Scalar my $EXPL => q{Use "use Exporter 'import'" instead}; ## no critic (RequireInterpolation) + +#----------------------------------------------------------------------------- + +sub supported_parameters { return () } +sub default_severity { return $SEVERITY_MEDIUM } +sub default_themes { return qw( core maintenance performance ) } +sub applies_to { return 'PPI::Document' } + +#----------------------------------------------------------------------------- + +sub violates { + my ( $self, $elem, $doc ) = @_; + + my $includes_ref = $doc->find('PPI::Statement::Include'); + return if not $includes_ref; #ok + # This covers both C<use Exporter;> and C<use base 'Exporter';> + my @inherit_exporter = + grep { + m/ + use \s+ (?: base|parent ) \b .* (?<! [:\w]) Exporter (?: :: ) ? (?! [:\w] ) + /xms + } @{ $includes_ref }; + + if ( @inherit_exporter ) { + return $self->violation( $DESC, $EXPL, $inherit_exporter[0] ); + } + return; #ok +} + +1; + +__END__ + +#----------------------------------------------------------------------------- + +=pod + +=head1 NAME + +Perl::Critic::Policy::Modules::ProhibitInheritingExporter - Use Exporter via C<use Exporter 'import'> instead of C<use base 'Exporter'>. + +=head1 AFFILIATION + +This Policy is part of the core L<Perl::Critic|Perl::Critic> +distribution. + + +=head1 DESCRIPTION + +Old version of L<Exporter> recommended to use it with C<our @ISA = 'Exporter'> +as the only way (C<use base 'Exporter'> and C<use parent 'Exporter'> are +alternative way or writing this, but the same under). This is an easy way to +make the 'import' method of Exporter available in the importing package. +However the C<Exporter> package contains also other functions that will +consequently be also available as methodes of your package without you +explicitely knowing it. This can lead to problems hard to debug. + +The best practice is to import only the C<Exporter::import> sub into your +package. + + package Foo; + + use base 'Exporter'; # not ok + use parent 'Exporter'; # not ok + use Exporter 'import'; # ok + + +=head1 CONFIGURATION + +This Policy is not configurable except for the standard options. + + +=head1 AUTHOR + +Olivier MenguE<eacute> <dolmen@cpan.org> + + +=head1 COPYRIGHT + +Copyright (c) 2012 Olivier MenguE<eacute>. 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 : Property changes on: lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm ___________________________________________________________________ Added: svn:keywords + URL Date Author Revision Index: t/Modules/ProhibitInheritingExporter.run =================================================================== --- t/Modules/ProhibitInheritingExporter.run (revision 0) +++ t/Modules/ProhibitInheritingExporter.run (revision 0) @@ -0,0 +1,134 @@ +## name Basic failure, "use base 'Exporter';" +## failures 1 +## cut + +use base 'Exporter'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use parent 'Exporter';" +## failures 1 +## cut + +use parent 'Exporter'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, 'use base "Exporter";' +## failures 1 +## cut + +use base "Exporter"; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, 'use base Exporter;' (bareword) +## failures 1 +## cut + +use base Exporter; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, 'use base Exporter::;' (bareword) +## failures 1 +## cut + +use base Exporter::; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use base qq<Exporter>;" +## failures 1 +## cut + +use base qw<Exporter>; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use base qw< Exporter >;" +## failures 1 +## cut + +use base qw< Exporter >; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- +## name Basic failure, "use base qw< Exporter >;" +## failures 1 +## cut + +use base qw< Exporter:: >; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use parent 0.001 'Exporter';" +## failures 1 +## cut + +use parent 0.001 'Exporter'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic pass, "use Exporter 'import';" +## failures 0 +## cut + +use Exporter 'import'; +our @EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Pass, 'use base 'Exporter::Heavy';' (not bare Exporter) +## failures 0 +## cut + +use base 'Exporter::Heavy'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- +## name Pass, 'use base Exporter::Heavy;' (bareword, not bare Exporter) +## failures 0 +## cut + +use base Exporter::Heavy; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- +## name Pass, 'use base Moose::Exporter;' (bareword, not bare Exporter) +## failures 0 +## cut + +use Moose (); +use base Moose::Exporter; + +#----------------------------------------------------------------------------- +## name No 'use' at all +## failures 0 +## cut + +print 123; # no use at all; for test coverage + +############################################################################## +# $URL$ +# $Date$ +# $Author$ +# $Revision$ +############################################################################## + +# 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 : Property changes on: t/Modules/ProhibitInheritingExporter.run ___________________________________________________________________ Added: svn:keywords + URL Date Author Revision
Download (untitled) / with headers
text/plain 199b
Here is an improved patch. The policy now applies to every PPI::Statement::Include instead of to the whole document. That should be more efficient. -- Olivier Mengué - http://perlresume.org/DOLMEN
Subject: Modules-ProhibitInheritingExporter-2.patch
Index: lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm =================================================================== --- lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm (révision 0) +++ lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm (révision 0) @@ -0,0 +1,111 @@ +############################################################################## +# $URL$ +# $Date$ +# $Author$ +# $Revision$ +############################################################################## + +package Perl::Critic::Policy::Modules::ProhibitInheritingExporter; + +use 5.006001; +use strict; +use warnings; +use Readonly; + +use Perl::Critic::Utils qw{ :severities }; +use base 'Perl::Critic::Policy'; + +our $VERSION = '1.117'; + +#----------------------------------------------------------------------------- + +Readonly::Scalar my $DESC => q{Module inherits from Exporter}; +Readonly::Scalar my $EXPL => q{Use "use Exporter 'import'" instead}; ## no critic (RequireInterpolation) + +#----------------------------------------------------------------------------- + +sub supported_parameters { return () } +sub default_severity { return $SEVERITY_MEDIUM } +sub default_themes { return qw( core maintenance performance ) } +sub applies_to { return 'PPI::Statement::Include' } + +#----------------------------------------------------------------------------- + +sub violates { + my ( $self, $elem, undef ) = @_; + + if ($elem->type eq 'use' && "$elem" =~ + m/ + use \s+ (?: base|parent ) \b .* [^:\w] Exporter (?: :: ) ? (?! [:\w] ) + /xms ) { + return $self->violation( $DESC, $EXPL, $elem ); + } + return; #ok +} + +1; + +__END__ + +#----------------------------------------------------------------------------- + +=pod + +=head1 NAME + +Perl::Critic::Policy::Modules::ProhibitInheritingExporter - Use Exporter via C<use Exporter 'import'> instead of C<use base 'Exporter'>. + +=head1 AFFILIATION + +This Policy is part of the core L<Perl::Critic|Perl::Critic> +distribution. + + +=head1 DESCRIPTION + +Old version of L<Exporter> recommended to use it with C<our @ISA = 'Exporter'> +as the only way (C<use base 'Exporter'> and C<use parent 'Exporter'> are +alternative way or writing this, but the same under). This is an easy way to +make the 'import' method of Exporter available in the importing package. +However the C<Exporter> package contains also other functions that will +consequently be also available as methodes of your package without you +explicitely knowing it. This can lead to problems hard to debug. + +The best practice is to import only the C<Exporter::import> sub into your +package. + + package Foo; + + use base 'Exporter'; # not ok + use parent 'Exporter'; # not ok + use Exporter 'import'; # ok + + +=head1 CONFIGURATION + +This Policy is not configurable except for the standard options. + + +=head1 AUTHOR + +Olivier MenguE<eacute> <dolmen@cpan.org> + + +=head1 COPYRIGHT + +Copyright (c) 2012 Olivier MenguE<eacute>. 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 : Modification de propriétés sur lib/Perl/Critic/Policy/Modules/ProhibitInheritingExporter.pm ___________________________________________________________________ Ajouté : svn:keywords + URL Date Author Revision Index: t/Modules/ProhibitInheritingExporter.run =================================================================== --- t/Modules/ProhibitInheritingExporter.run (révision 0) +++ t/Modules/ProhibitInheritingExporter.run (révision 0) @@ -0,0 +1,134 @@ +## name Basic failure, "use base 'Exporter';" +## failures 1 +## cut + +use base 'Exporter'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use parent 'Exporter';" +## failures 1 +## cut + +use parent 'Exporter'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, 'use base "Exporter";' +## failures 1 +## cut + +use base "Exporter"; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, 'use base Exporter;' (bareword) +## failures 1 +## cut + +use base Exporter; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, 'use base Exporter::;' (bareword) +## failures 1 +## cut + +use base Exporter::; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use base qq<Exporter>;" +## failures 1 +## cut + +use base qw<Exporter>; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use base qw< Exporter >;" +## failures 1 +## cut + +use base qw< Exporter >; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- +## name Basic failure, "use base qw< Exporter >;" +## failures 1 +## cut + +use base qw< Exporter:: >; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic failure, "use parent 0.001 'Exporter';" +## failures 1 +## cut + +use parent 0.001 'Exporter'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Basic pass, "use Exporter 'import';" +## failures 0 +## cut + +use Exporter 'import'; +our @EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- + +## name Pass, 'use base 'Exporter::Heavy';' (not bare Exporter) +## failures 0 +## cut + +use base 'Exporter::Heavy'; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- +## name Pass, 'use base Exporter::Heavy;' (bareword, not bare Exporter) +## failures 0 +## cut + +use base Exporter::Heavy; +@EXPORT = qw(foo bar); + +#----------------------------------------------------------------------------- +## name Pass, 'use base Moose::Exporter;' (bareword, not bare Exporter) +## failures 0 +## cut + +use Moose (); +use base Moose::Exporter; + +#----------------------------------------------------------------------------- +## name No 'use' at all +## failures 0 +## cut + +print 123; # no use at all; for test coverage + +############################################################################## +# $URL$ +# $Date$ +# $Author$ +# $Revision$ +############################################################################## + +# 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 : Modification de propriétés sur t/Modules/ProhibitInheritingExporter.run ___________________________________________________________________ Ajouté : svn:keywords + URL Date Author Revision
Subject: Re: [rt.cpan.org #75329] New policy: Modules::ProhibitInheritingExporter
Date: Mon, 27 Feb 2012 10:05:32 -0800
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
Download (untitled) / with headers
text/plain 307b
On Feb 27, 2012, at 2:28 AM, Olivier Mengué via RT wrote: Show quoted text
> Here is an improved patch. The policy now applies to every > PPI::Statement::Include instead of to the whole document. That should be > more efficient.
Thanks. I'll take a look at this after I've sorted out all the MANIFEST business. -Jeff
Download (untitled) / with headers
text/plain 478b
Please review this patch. Le 2012-02-27 19:05:45, jeff@imaginative-software.com a écrit : Show quoted text
> > On Feb 27, 2012, at 2:28 AM, Olivier Mengué via RT wrote: >
> > Here is an improved patch. The policy now applies to every > > PPI::Statement::Include instead of to the whole document. That
> should be
> > more efficient.
> > Thanks. I'll take a look at this after I've sorted out all the > MANIFEST business. > > -Jeff >
-- Olivier Mengué - http://perlresume.org/DOLMEN
Subject: Re: [rt.cpan.org #75329] New policy: Modules::ProhibitInheritingExporter
Date: Wed, 25 Jul 2012 23:09:05 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <elliotjs [...] cpan.org>
Download (untitled) / with headers
text/plain 639b
On 7/25/12 4:31 AM, Olivier Mengué via RT wrote: Show quoted text
> Please review this patch.
This really needs to check for the version of Exporter available; given the difficulty in getting this correct, the perl version will need to be a proxy. So, this policy cannot complain unless there's an explicit "use 5.008something" (I have dim memories of it being 5.8.1, but it's been a lonnnnnng time) or higher, i.e. P::C::Document::highest_explicit_perl_version() will need to be checked as in BuiltinFunctions::ProhibitLvalueSubstr, InputOutput::ProhibitTwoArgOpen, TestingAndDebugging::RequireUseWarnings, and Variables::RequireLexicalLoopIterators.
RT-Send-CC: elliotjs [...] cpan.org, jeff [...] imaginative-software.com
Download (untitled) / with headers
text/plain 1.2k
Le 2012-07-26 06:09:22, ELLIOTJS a écrit : Show quoted text
> This really needs to check for the version of Exporter available; > given the difficulty in getting this correct, the perl version will > need to be a proxy. So, this policy cannot complain unless there's > an explicit "use 5.008something" (I have dim memories of it being > 5.8.1, but it's been a lonnnnnng time) or higher, i.e. > P::C::Document::highest_explicit_perl_version() will need to be > checked as in BuiltinFunctions::ProhibitLvalueSubstr, > InputOutput::ProhibitTwoArgOpen, > TestingAndDebugging::RequireUseWarnings, and > Variables::RequireLexicalLoopIterators.
This feature of Exporter has been introduced in Exporter 5.57 bundled with Perl 5.8.3, released 8 years ago. Would you prefer if my patch was explicitely recommending "use Exporter 5.57 'import';" ? Note that Perl::Critic itself is requiring Exporter 5.63 as a minimum requirement (inc/Perl/Critic/BuildUtilities.pm line 42). If the user wants to still maintain old code that will have to work on old perls (5.6) and the Exporter of the time (already many irrealistic conditions) and use Perl::Critic, he can still disable the policy. -- Olivier Mengué - http://perlresume.org/DOLMEN
Subject: Re: [rt.cpan.org #75329] [PATCH] New policy: Modules::ProhibitInheritingExporter
Date: Thu, 26 Jul 2012 18:54:13 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <elliotjs [...] cpan.org>
On 7/26/12 9:20 AM, Olivier Mengué via RT wrote: Show quoted text
> This feature of Exporter has been introduced in Exporter 5.57 bundled > with Perl 5.8.3, released 8 years ago. > Would you prefer if my patch was explicitely recommending "use Exporter > 5.57 'import';" ?
It would be better, yes. Show quoted text
> Note that Perl::Critic itself is requiring Exporter 5.63 as a minimum > requirement (inc/Perl/Critic/BuildUtilities.pm line 42).
Yes, but it supports looking at code as old as 5.0. Perl::Critic's runtime is not necessarily the one used by the code being looked at. (I've actually proposed bumping P::C's minimum requirements in the past.) Show quoted text
> If the user wants to still maintain old code that will have to work on > old perls (5.6) and the Exporter of the time (already many irrealistic > conditions) and use Perl::Critic, he can still disable the policy.
No. The P::C policy is that it is better for there to be false negatives than for there to be false positives. Unless we know what version of Exporter the code /itself/ is going to use, then we can't ding it. We've got this issue with other Policies and they deal with it.


This service is sponsored and maintained by Best Practical Solutions and runs on Perl.org infrastructure.

Please report any issues with rt.cpan.org to rt-cpan-admin@bestpractical.com.