Bug #14923 for Perl-Critic: Permit 'require' for runtime loading

Tue Oct 04 23:28:14 2005 cpan [...] clotho.com - Ticket created

Subject:

Permit 'require' for runtime loading

[intended to be filed against 0.09, but RT hasn't seen 0.09 yet] A common practice to improve performance is to delay loading a module until it is needed. To do this, a require statement is needed, not a use statement. For example: use File::Slurp; sub load_or_fetch_file { my $name = shift; my $content; if (-f $name) { $content = read_file($name); } else { require LWP::Simple; $content = LWP::Simple::get($baseurl . $name); write_file($name, $content); } return $content; } I propose that the require warning be emitted only when the require statement is at the base level of the module file, i.e. is not in any scope like a subroutine, an eval block or a conditional block. -- Chris

Tue Oct 11 16:35:13 2005 thaljef [...] cpan.org - Taken

Tue Oct 11 16:49:21 2005 thaljef [...] cpan.org - Correspondence added

Chris-- Perl::Critic actually does run-time loading too (but for a slightly different reason). I get around the warnings by using the "no critic" comments. I understand the motivation, but I wonder if Conway might suggest an alternate pattern like this: use File::Slurp; use English qw(-no_match_vars); sub load_or_fetch_file { my $name = shift; if (-f $name) { $content = read_file($name); } else { eval { use LWP::Simple }; die $EVAL_ERROR if $EVAL_ERROR; #Can't load! $content = LWP::Simple::get($name); } return $content; } By using 'eval', you can still postpone loading until run-time, and you can trap loading errors and try to use an alternate module (perhaps FTP in this case).

Tue Oct 11 17:33:40 2005 cpan [...] clotho.com - Correspondence added

[THALJEF - Tue Oct 11 16:49:21 2005]: Show quoted text

> ... > By using 'eval', you can still postpone loading until run-time, and you > can trap loading errors and try to use an alternate module (perhaps FTP > in this case).

Jeff, No, that's not how eval {}; works. Try this for example: % perl -le'print $CGI::VERSION; eval { use CGI; };' 3.05 The "use" statement happens at compile time, even inside an eval. Even Damian uses require for runtime loading: http://search.cpan.org/src/DCONWAY/Class-Std-0.0.4/lib/Class/Std.pm (search for require on that page) -- Chris

Tue Oct 11 18:05:26 2005 thaljef [...] cpan.org - Correspondence added

Doh! You're right. I should have said C<eval "use Module";>. Like this: perl -e 'print "before: $CGI::VERSION\n"; eval "use CGI"; print "after: $CGI::VERSION\n"' I wrote this policy to force my colleagues to write *.pm modules instead of *.pl libraries, so it's not based on any particular Conway practice. Based on his ideas about exception handling, I assumed that Conway would prefer the 'eval' method. But as you pointed out, his code suggests otherwise. I think we should just ask him. Recently, I've seen some modules (like Module::Load) that cover up the syntactic differences between 'use' and 'require'. What do you think of those? -Jeff

Tue Oct 11 23:03:53 2005 Guest - Correspondence added

From:

cdolan [...] cpan.org

[THALJEF - Tue Oct 11 18:05:26 2005]: Show quoted text

> Doh! You're right. I should have said C<eval "use Module";>. Like > this: > > perl -e 'print "before: $CGI::VERSION\n"; eval "use CGI"; print "after: > $CGI::VERSION\n"' > > I wrote this policy to force my colleagues to write *.pm modules > instead of *.pl libraries, so it's not based on any particular Conway > practice. Based on his ideas about exception handling, I assumed that > Conway would prefer the 'eval' method. But as you pointed out, his > code suggests otherwise. I think we should just ask him. > > Recently, I've seen some modules (like Module::Load) that cover up the > syntactic differences between 'use' and 'require'. What do you think > of those? > > -Jeff

eval "..."; carries a performance penalty and hides typos (like eval "use Prel::Critic";) so I oppose it. I prefer require. I've been meaning to look into the ::Load modules, but haven't gotten around to it. As for require with connection to .pl, I think this require Foo::Bar; should be permitted and this require "Foo/Bar.pm"; should be criticized. Can PPI tell the difference easily? -- Chris

Wed Oct 12 03:31:56 2005 thaljef [...] cpan.org - Correspondence added

Show quoted text

> eval "..."; carries a performance penalty and hides typos (like eval > "use Prel::Critic";) so I oppose it. I prefer require.

Ok, I'll buy that. Show quoted text

> As for require with connection to .pl, I think this > require Foo::Bar; > should be permitted and this > require "Foo/Bar.pm"; > should be criticized. Can PPI tell the difference easily?

That sounds good to me, but that probably should be a different Policy module altogether. So maybe the best thing for me to do is just take ProhibitRequireStatements out of the default configuration. People (like me) can still add it to the .perlcriticrc if they want to use it.

Wed Oct 12 03:31:57 2005 thaljef [...] cpan.org - Status changed from 'new' to 'open'

Wed Oct 12 10:21:18 2005 cpan [...] clotho.com - Correspondence added

From:

cdolan [...] cpan.org

[THALJEF - Wed Oct 12 03:31:56 2005]: Show quoted text

>

> > eval "..."; carries a performance penalty and hides typos (like eval > > "use Prel::Critic";) so I oppose it. I prefer require.

> > Ok, I'll buy that. >

> > As for require with connection to .pl, I think this > > require Foo::Bar; > > should be permitted and this > > require "Foo/Bar.pm"; > > should be criticized. Can PPI tell the difference easily?

> > That sounds good to me, but that probably should be a different Policy > module altogether. So maybe the best thing for me to do is just take > ProhibitRequireStatements out of the default configuration. People > (like me) can still add it to the .perlcriticrc if they want to use > it.

OK, I did that. Attached is a new policy module called Modules::ProhibitRequireStrings that accepts 'require Foo::Bar' but rejects 'require "file.pl"'. It requires that the first arg be a bareword. This is my first interaction with PPI, so let me know if there's a better way to do this. -- Chris

diff -Nur Perl-Critic-0.12-orig/lib/Perl/Critic/Config.pm Perl-Critic-0.12-require/lib/Perl/Critic/Config.pm --- Perl-Critic-0.12-orig/lib/Perl/Critic/Config.pm 2005-10-10 22:21:01.000000000 -0500 +++ Perl-Critic-0.12-require/lib/Perl/Critic/Config.pm 2005-10-12 08:47:48.000000000 -0500 @@ -85,6 +85,7 @@ InputOutput::ProhibitBacktickOperators Modules::ProhibitMultiplePackages Modules::ProhibitRequireStatements + Modules::ProhibitRequireStrings Modules::ProhibitSpecificModules Modules::ProhibitUnpackagedCode NamingConventions::ProhibitMixedCaseSubs diff -Nur Perl-Critic-0.12-orig/lib/Perl/Critic/Policy/Modules/ProhibitRequireStrings.pm Perl-Critic-0.12-require/lib/Perl/Critic/Policy/Modules/ProhibitRequireStrings.pm --- Perl-Critic-0.12-orig/lib/Perl/Critic/Policy/Modules/ProhibitRequireStrings.pm 1969-12-31 18:00:00.000000000 -0600 +++ Perl-Critic-0.12-require/lib/Perl/Critic/Policy/Modules/ProhibitRequireStrings.pm 2005-10-12 09:22:06.000000000 -0500 @@ -0,0 +1,81 @@ +package Perl::Critic::Policy::Modules::ProhibitRequireStrings; + +use strict; +use warnings; +use Perl::Critic::Utils; +use Perl::Critic::Violation; +use base 'Perl::Critic::Policy'; + +our $VERSION = '0.12'; +$VERSION = eval $VERSION; ## no critic + +my $desc = q{Deprecated 'require "filename"'}; +my $expl = q{Use 'require Foo::Bar" instead}; + +#---------------------------------------------------------------------------- + +sub violates { + my ( $self, $elem, $doc ) = @_; + $elem->isa('PPI::Statement::Include') && $elem->type() eq 'require' + || return; + if ( !_first_arg_is_bareword($elem) ) { + return Perl::Critic::Violation->new( $desc, $expl, $elem->location() ); + } + return; #ok! +} + +sub _first_arg_is_bareword { + my $elem = shift || return; + + # Find the 'require' token, then find the first non-whitespace token + # If that token is a bareword then succeed, else fail. + + my $looking_for_require = 1; # Initial state + + SEARCH: + for my $token ( $elem->schildren() ) { + if ( $looking_for_require ) { + if ( $token->isa('PPI::Token::Word') && $token eq 'require' ) { + $looking_for_require = 0; # Change state + } + next SEARCH; + } + + if ( $token->isa('PPI::Token::Word') ) { + return 1; + } + elsif ( !$token->isa('PPI::Token::Whitespace') ) { + return; + } + } + return; +} + +1; + +__END__ + +=head1 NAME + +Perl::Critic::Policy::Modules::ProhibitRequireStrings + +=head1 DESCRIPTION + +C<require> has two forms: C<require "dir/file.pl"> and C<require +Foo::Bar>. The former loads and executes the specified file. It is a +legacy from Perl 4 where C<use> and modules did not exist. The latter +is like a runtime version of C<use> that doesn't call the C<import> +method. That is, it loads the specified C<.pm> module if it's not +already loaded. Where possible, C<use> should be employed, but in +some cases a module is not needed during every execution of the +program, so loading it only when truly needed is a performance boost. + +=head1 AUTHOR + +Chris Dolan <cdolan@cpan.org> + +Copyright (c) 2005 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.

Thu Oct 13 03:24:59 2005 Guest - Correspondence added

From:

thaljef [...] cpan.org

I was reading the perldocs on 'use' and 'require' last night and I officially decided that I like your Policy much better than mine. Aside from permitting run-time inclusion and discouraging people from using it in a non-portable way (i.e. hard-coded paths), it also discourages people from writing *.pl libraries (which was my goal all along). So in the next release, ProhibitRequireStrings will replace ProhibitRequireStatements. I might code it a little differently. Rather than searching for a Statement::Include, I'll look for 'require' as a Token::Word and then verify that the parent is a Statement::Include. That way, I don't have to go hunting for the 'require' token again since I already have it in hand. It might look like this (untested): sub violates { my ($self, $elem, $doc) = @_; $elem->isa('PPI::Token::Word') && $elem eq 'require' || return; $elem->parent->isa('PPI::Statement::Include') || return; my $sib = $elem->snext_sibling() || return; if( ! $sib->isa('PPI::Token::Word') ) { #Return violation... } return; #ok! } Since violates() is called in a very tight loop, I think this sort of short-circuting allows you to bail out as soon as possible. But that's just my humble opinion. Let me know if you think my approach is awkward or just plain incorrect. Technically, we should also accommodate the 'require VERSION EXPR' variety. But I think that's probably pretty rare so I'm willing to ingore it for now. Thanks for submitting all the patches. I have to do my real job now, but I'll stick them into Perl::Critic as soon as I can. -Jeff

Thu Oct 13 11:44:14 2005 cpan [...] clotho.com - Correspondence added

From:

cdolan [...] cpan.org

[guest - Thu Oct 13 03:24:59 2005]: Show quoted text

> I was reading the perldocs on 'use' and 'require' last night and I > officially decided that I like your Policy much better than mine. > Aside from permitting run-time inclusion and discouraging people from > using it in a non-portable way (i.e. hard-coded paths), it also > discourages people from writing *.pl libraries (which was my goal all > along). So in the next release, ProhibitRequireStrings will replace > ProhibitRequireStatements. > > I might code it a little differently. Rather than searching for a > Statement::Include, I'll look for 'require' as a Token::Word and then > verify that the parent is a Statement::Include. That way, I don't > have to go hunting for the 'require' token again since I already have > it in hand. It might look like this (untested): > > sub violates { > my ($self, $elem, $doc) = @_; > $elem->isa('PPI::Token::Word') && $elem eq 'require' || return; > $elem->parent->isa('PPI::Statement::Include') || return; > my $sib = $elem->snext_sibling() || return; > if( ! $sib->isa('PPI::Token::Word') ) { > #Return violation... > } > return; #ok! > } > > Since violates() is called in a very tight loop, I think this sort of > short-circuting allows you to bail out as soon as possible. But > that's just my humble opinion. Let me know if you think my approach > is awkward or just plain incorrect. Technically, we should also > accommodate the 'require VERSION EXPR' variety. But I think that's > probably pretty rare so I'm willing to ingore it for now. > > Thanks for submitting all the patches. I have to do my real job now, > but I'll stick them into Perl::Critic as soon as I can. > > -Jeff

Your solution probably works fine, although the ::Word search concerns me due to other problems I've had with the construct (i.e. hashkeys). I think the 'require VERSION EXPR' variety is already covered by the code you've offered since we never look past the first argument. As for efficiency, there's no significant difference between looking for ::Word or ::Include since you're looking at all nodes anyway. The best way to get a performance boost would be for this policy to have a way to tell the caller that it doesn't care about anything that's not a ::Include and then never call the violates() method in the first place. But that's a discussion for another time and another forum. -- Chris

Thu Nov 03 12:38:21 2005 thaljef [...] cpan.org - Status changed from 'open' to 'resolved'

Thu Nov 03 12:38:22 2005 thaljef [...] cpan.org - Fixed in 0.13 added

Bug #14923 for Perl-Critic: Permit 'require' for runtime loading

Preferred bug tracker