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: 33936
Status: rejected
Priority: 0/
Queue: Perl-Critic
People
Owner: Nobody in particular
Requestors: perl [...] galumph.com
Cc:
AdminCc:
Bug Information
Severity: (no value)
Broken in: (no value)
Fixed in: (no value)

History Show all quoted text
  Sun Mar 09 17:56:08 2008 perl [...] galumph.com - Ticket created
CC: Heiko <heiko [...] hexco.de>
Subject: ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks.
Date: Sun, 09 Mar 2008 16:55:49 -0500
To: bug-Perl-Critic [...] rt.cpan.org
From: Elliot Shank <perl [...] galumph.com>
Subroutines::ProhibitNestedSubs gives Nested named subroutine at line 12, column 9. Declaring a named sub inside another named sub does not prevent the inner sub from being global.. (Severity: 5) on this code: sub BEGIN { eval { sub color }; } The purpose is to make color() a no-op under certain runtime situations. I think BEGIN (and possibly CHECK and INIT ) subroutines should be exceptions to this rule. The patch makes an exception for BEGIN subroutines.
--- ProhibitNestedSubs.pm.org 2008-02-25 22:14:31.265625000 +0100 +++ ProhibitNestedSubs.pm.new 2008-02-23 22:58:08.390625000 +0100 @@ -11,7 +11,7 @@ use warnings; use Readonly; -use Perl::Critic::Utils qw{ :severities }; +use Perl::Critic::Utils qw{ :severities :classification }; use base 'Perl::Critic::Policy'; our $VERSION = '1.080'; @@ -38,6 +38,13 @@ my $inner = $elem->find_first('PPI::Statement::Sub'); return if not $inner; + my $barewords = $elem->find('PPI::Token::Word'); + + return if (2 <= scalar @{ $barewords } + && $barewords->[0]->{'content'} eq 'sub' + && is_subroutine_name( $barewords->[1] ) + && $barewords->[1]->{'content'} eq 'BEGIN'); + # Must be a violation... return $self->violation($DESC, $EXPL, $inner); }
  Sun Jul 27 01:01:04 2008 thaljef [...] cpan.org - Correspondence added
RT-Send-CC: heiko [...] hexco.de
On Sun Mar 09 17:56:08 2008, clonezone wrote: Show quoted text
> Subroutines::ProhibitNestedSubs gives > > Nested named subroutine at line 12, column 9. Declaring a named sub > inside another named sub does not prevent the inner sub from being > global.. (Severity: 5) > > on this code: > sub BEGIN > { > eval { sub color }; > } > > The purpose is to make color() a no-op under certain runtime > situations. I think BEGIN (and possibly CHECK and INIT ) subroutines > should be exceptions to this rule.
I don't understand what's supposed to be going on here. What is the purpose of wrapping "sub color" in an eval block? Is color() declared later in this package, or was it declared in a package that had already been loaded? Is color() going to get called at compile-time or run-time (or both)? I guess if I wanted to override a subroutine at run-time I would probably do something like this: *color = sub {...}; And if I wanted to override it at compile-time, then I would wrap it in a BEGIN block just as you did: BEGIN { *color = sub {...} } In any event, it looks like your doing something that's quite out of the ordinary. In which case, the "## no critic" flag is a way to communicate to your colleagues that you know what you're doing. But please enlighten me if I've misunderstood your intention. -Jeff
  Sun Jul 27 01:01:06 2008 The RT System itself - Status changed from 'new' to 'open'
  Mon Jul 28 03:06:04 2008 heiko.eissfeldt [...] siemens.com - Correspondence added
CC: <heiko [...] hexco.de>
Subject: AW: [rt.cpan.org #33936] ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks.
Date: Mon, 28 Jul 2008 09:05:40 +0200
To: <bug-Perl-Critic [...] rt.cpan.org>
From: "Eissfeldt, Heiko" <heiko.eissfeldt [...] siemens.com>
Show quoted text
-----Ursprüngliche Nachricht----- Von: Jeffrey Ryan Thalhammer via RT [mailto:bug-Perl-Critic@rt.cpan.org] Gesendet: Sonntag, 27. Juli 2008 07:01 Cc: heiko@hexco.de Betreff: [rt.cpan.org #33936] ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks. <URL: http://rt.cpan.org/Ticket/Display.html?id=33936 > On Sun Mar 09 17:56:08 2008, clonezone wrote:
> Subroutines::ProhibitNestedSubs gives > > Nested named subroutine at line 12, column 9. Declaring a named sub > inside another named sub does not prevent the inner sub from being > global.. (Severity: 5) > > on this code: > sub BEGIN > { > eval { sub color }; > } > > The purpose is to make color() a no-op under certain runtime > situations. I think BEGIN (and possibly CHECK and INIT ) subroutines > should be exceptions to this rule.
>I don't understand what's supposed to be going on here. What is the >purpose of wrapping "sub color" in an eval block? Is color() declared >later in this package, or was it declared in a package that had already >been loaded? Is color() going to get called at compile-time or run-time >(or both)?
>I guess if I wanted to override a subroutine at run-time I would >probably do something like this:
> *color = sub {...};
>And if I wanted to override it at compile-time, then I would wrap it in >a BEGIN block just as you did:
> BEGIN { *color = sub {...} }
Yes, thanks, that is another (probably better) solution.
>In any event, it looks like your doing something that's quite out of the >ordinary. In which case, the "## no critic" flag is a way to >communicate to your colleagues that you know what you're doing.
>But please enlighten me if I've misunderstood your intention.
My intention was to define a no-op sub 'color' as a fallback, if the supplying module could not be loaded or was not available. So the rest of the app code did not need to change. Thanks for your feedback. Heiko aka Hexcoder
  Tue Jul 29 00:22:09 2008 jeff [...] imaginative-software.com - Correspondence added
Subject: Re: AW: [rt.cpan.org #33936] ProhibitNestedSubs doesn't handle subs declared inside of eval/scheduled blocks.
Date: Mon, 28 Jul 2008 21:21:17 -0700
To: bug-Perl-Critic [...] rt.cpan.org
From: Jeffrey Thalhammer <jeff [...] imaginative-software.com>
On Jul 28, 2008, at 12:06 AM, Eissfeldt, Heiko via RT wrote: Show quoted text
> My intention was to define a no-op sub 'color' as a fallback, > if the supplying module could not be loaded or was not available. > So the rest of the app code did not need to change.
Aahh, now I understand. I can think of some better ways to do that. Let me know If you care to hear my suggestions. I don't think there's anything wrong with the Policy then. A named sub declared inside another sub is always wrong (at least, I can't think of any situations where it would be right). You might have been thinking of this... eval 'sub color{}'; ...which has a different set of problems and violates a different Policy :) -Jeff
  Wed Apr 21 22:15:37 2010 ELLIOTJS [...] cpan.org - Status changed from 'open' to 'rejected'