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: 28676
Status: open
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: MJCARMAN [...] cpan.org
Cc:
AdminCc:
Bug Information
Severity: Normal
Broken in: 1.061
Fixed in: (no value)

History Show all quoted text
  Sun Aug 05 14:57:16 2007 MJCARMAN [...] cpan.org - Ticket created
Subject: False positive in TestingAndDebugging::RequireUseStrict
I'm getting a false positive for violating the TestingAndDebugging::RequireUseStrict policy. I tend to start my scripts and modules with this: BEGIN { require 5.006 } use strict; use warnings; I've been doing this for so long that I can no longer remember the exact reason for using a BEGIN block instead of C<use VERSION>. IIRC it behaves better on old versions of perl. The documentation for the policy says that it's okay to have a C<require> before C<use strict>. That's really all I have here but the BEGIN block seems to be throwing Perl::Critic off.
  Sun Aug 05 15:32:47 2007 MJCARMAN [...] cpan.org - Correspondence added
From: mjcarman [...] mchsi.com
Fixing this is as simple as adding return 0 if $elem->isa('PPI::Statement::Scheduled'); into _isnt_include_or_package(). That causes my earlier example to pass. Things like this still fail: BEGIN {require 5.006; $foo = $bar; } use strict; I think that ignoring scheduled blocks completely is okay -- it's the statements inside we care about.
  Tue Jan 01 00:34:19 2008 thaljef [...] cpan.org - Correspondence added
Sorry for the slow reply. I don't feel terribly strong about it, but think that might create some problems, as it would allow stuff like... BEGIN { require $module_path }; use strict; Of course, that would fail spectacularly if you tried to compile it. But part of the reason for static analysis is to identify problems *before* you even compile the code. I'll think about it some more, but right now, I'm inclined to leave the policy as-is. If you can't take advantage of "use VERSION" for some reason, then adding a "## no critic" comment would help readers know that you had a reason for doing something out of the ordinary. BEGIN { require 5.006 }; ## no critic (UseStrict) Or could you just put the BEGIN block after strictures are enabled? use strict; BEGIN{ require 5.006 }; ... -Jeff
  Tue Jan 01 00:34:21 2008 The RT System itself - Status changed from 'new' to 'open'
  Tue Jan 01 23:44:01 2008 MJCARMAN [...] cpan.org - Correspondence added
From: mjcarman [...] mchsi.com
I don't see how your potential problem is a problem at all. The current policy explicitly allows having a C<require> (or C<use>, or C<package>) before "use strict." All my proposed patch does is to ignore scheduled blocks. It's just the blocks themselves which are ignored, not the contents of them. Basically, it stops treating a BEGIN block as a statement -- which it really isn't anyway. "## no critic" comments are fine when I'm deliberately flouting a policy, but I don't think I'm doing that here. I can't put the BEGIN after the strictures. The reason for writing it the way I do is that it will fail gracefully when the prerequisite isn't met. i.e. it will die with "Perl 5.6 required--this is only X.Y..." instead of choking on the "use strict" as a syntax error. Those ancient versions of Perl are precisely who I'm adding this snippet of code for, so I have to cater to their needs, not what newer versions allow.
  Wed Jan 02 14:20:28 2008 MJCARMAN [...] cpan.org - Correspondence added
From: mjcarman [...] mchsi.com
Correction: "use strict" won't cause a syntax error. (I was thinking of "use warnings", which was added in v5.6.) My stance on everything else still holds, though. :)
  Wed Jan 02 14:21:36 2008 MJCARMAN [...] cpan.org - Correspondence added
From: mjcarman [...] mchsi.com
Correction: "use strict" won't cause a syntax error. (I was thinking of "use warnings", which was added in v5.6.) My stance on everything else still holds, though. :)
  Tue Feb 01 12:54:55 2011 DOHERTY [...] cpan.org - Correspondence added
On Sun Aug 05 15:32:47 2007, MJCARMAN wrote: Show quoted text
> Fixing this is as simple as adding > > return 0 if $elem->isa('PPI::Statement::Scheduled'); > > into _isnt_include_or_package(). That causes my earlier example to
pass. Works here too (_statement_isnt_include_or_package though). Show quoted text
> I think that ignoring scheduled blocks completely is okay -- it's the > statements inside we care about.
Another example that /should/ be ok but isn't: setting the $VERSION in a BEGIN block: package My::Package; BEGIN { $App::perlhl::VERSION = '0.002'; } use strict; use warnings; #... The same change should be made in RequireUseWarnings.
  Tue Feb 01 13:04:42 2011 DOHERTY [...] cpan.org - Correspondence added
On Tue Feb 01 12:54:55 2011, DOHERTY wrote: Show quoted text
> Works here too (_statement_isnt_include_or_package though).
...or maybe not.
  Thu Feb 17 08:38:52 2011 DOHERTY [...] cpan.org - Correspondence added
Subject: [PATCH] False positive in TestingAndDebugging::RequireUseStrict
On Tue Feb 01 12:54:55 2011, DOHERTY wrote: Show quoted text
> Works here too (_statement_isnt_include_or_package though).
Patch attached.
Subject: rt28676.patch
Index: lib/Perl/Critic/Policy/TestingAndDebugging/RequireUseWarnings.pm =================================================================== --- lib/Perl/Critic/Policy/TestingAndDebugging/RequireUseWarnings.pm (revision 4042) +++ lib/Perl/Critic/Policy/TestingAndDebugging/RequireUseWarnings.pm (working copy) @@ -125,6 +125,7 @@ my ($elem) = @_; return 0 if $elem->isa('PPI::Statement::Package'); return 0 if $elem->isa('PPI::Statement::Include'); + return 0 if $elem->isa('PPI::Statement::Scheduled'); return 1; } Index: lib/Perl/Critic/Policy/TestingAndDebugging/RequireUseStrict.pm =================================================================== --- lib/Perl/Critic/Policy/TestingAndDebugging/RequireUseStrict.pm (revision 4042) +++ lib/Perl/Critic/Policy/TestingAndDebugging/RequireUseStrict.pm (working copy) @@ -134,6 +134,7 @@ my ($elem) = @_; return 0 if $elem->isa('PPI::Statement::Package'); return 0 if $elem->isa('PPI::Statement::Include'); + return 0 if $elem->isa('PPI::Statement::Scheduled'); return 1; }
  Thu Feb 17 09:12:22 2011 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #28676] [PATCH] False positive in TestingAndDebugging::RequireUseStrict
Date: Thu, 17 Feb 2011 08:12:11 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
This patch is not sufficient. It only checks that the code is a scheduled subroutine. It does not check that the contents of subroutine do anything more than require. BEGIN { $x = 57 } use strict; won't fail with this patch.
  Thu Feb 17 09:16:53 2011 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #28676] False positive in TestingAndDebugging::RequireUseStrict
Date: Thu, 17 Feb 2011 08:16:44 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Show quoted text
> Of course, that would fail spectacularly if you tried to compile it. > But part of the reason for static analysis is to identify problems > *before* you even compile the code.
Wrong. Static analysis is in addition to compilation. In particular, for any code that isn't compilable, consider P::C critic behavior to be undefined and not a bug.
  Wed Feb 23 13:58:21 2011 wyant [...] cpan.org - Correspondence added
On Thu Feb 17 09:12:22 2011, clonezone wrote: Show quoted text
> This patch is not sufficient. It only checks that the code is a > scheduled subroutine. It does not check that the contents of > subroutine do anything more than require. > > BEGIN { $x = 57 } > use strict; > > won't fail with this patch.
Actually, it turns out that it does fail, because this policy just does a $doc->find( 'PPI::Statement' ) on the document, then paws through the result. Since find() does not care about nesting depth, the '$x = 57' appears in the data fed to _statement_isnt_include_or_package, and we get the error. So this seems to put us back to an editorial decision on whether or not to implement. Maybe with a configuration variable? It would be a bit of bother to filter out "require $module_path", but I presume this could be done by filtering out requires whose first argument is anything but a PPI::Token::Number or a PPI::Token::Word.
  Wed Feb 23 22:15:33 2011 perl [...] galumph.com - Correspondence added
Subject: Re: [rt.cpan.org #28676] False positive in TestingAndDebugging::RequireUseStrict
Date: Wed, 23 Feb 2011 21:15:22 -0600
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
On 2/23/11 12:58 PM, Tom Wyant via RT wrote: Show quoted text
> So this seems to put us back to an editorial decision on whether or > not to implement. Maybe with a configuration variable?
There's no need to complicate the Policy for this sort of thing. I say we simply accept or reject the idea. Show quoted text
> It would be a bit of bother to filter out "require $module_path", but > I presume this could be done by filtering out requires whose first > argument is anything but a PPI::Token::Number or a PPI::Token::Word.
Ugh. Such complication for what I consider to be rare code. Look at the original message for this ticket: Show quoted text
> I've been doing this for so long that I can no longer remember the > exact reason for using a BEGIN block instead of C<use VERSION>. IIRC > it behaves better on old versions of perl.
What, he's cargo-culting from himself? If he can't explain this in the first place, then I see no reason to support it. Just because a particular style is endemic to an individual's code doesn't mean that it should apply everywhere.
  Mon Feb 28 19:12:41 2011 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: [rt.cpan.org #28676] False positive in TestingAndDebugging::RequireUseStrict
Date: Mon, 28 Feb 2011 16:12:24 -0800
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
I don't see why "require 5.8" or "$VERSION = ..." has to be in a BEGIN block. If it really is necessary to achieve some unconventional effect, then I think it deserves a "no critic" annotation to explain it. I also vote for rejecting this bug. -Jeff