Defining a sub within a sub: OK?

[talexb]

Just wondering if the following definition of a sub within a sub is good form or not:

#!/usr/bin/perl -w
#
#  Declare (and call) a sub within a sub.

use strict;
use warnings;

{
    print "In the mainline.\n";

    sub level1 {
        print "Inside level1!\n";
    }
    level1();
    print "Finished!\n";
}

It seems that perlcritic isn't happy with it:

foo@bar:~/dev$ perlcritic --severity=4 subinsub.pl
Subroutine does not end with "return" at line 11, column 5.  See page 
+197 of PBP.  (Severity: 4)

I think this means that it thinks I'm defining a new sub, the old one wasn't terminated properly.

It does produce the output I was expecting:

foo@bar:~/dev$ perl -w subinsub.pl
In the mainline.
Inside level1!
Finished!

Thoughts? Feedback?

[moritz]

As others have pointed out already there's no sub inside a sub in your code. I'll answer the question asked in your title though:

I consider it a bad idea to define a named sub within another sub. It's visible from the outside still, but obeys rather unintuitive scoping rules:

 use strict;
use warnings;

sub a {
    my $x = 3;
    sub b {
        print $x, $/;
    }
}

b();
__END__
Variable "$x" will not stay shared at foo.pl line 7.
Use of uninitialized value $x in print at foo.pl line 7.

There's no sane way in which perl could handle a call to b() from the outside.

However there's nothing wrong with defining an anonymous subroutine inside another sub. Actually it's quite common, and used very often in functional programming.

[Joost]

There's no sane way in which perl could handle a call to b() from the outside.

However there's nothing wrong with defining an anonymous subroutine inside another sub. Actually it's quite common, and used very often in functional programming.

Those two points are related: logically, any call to a() would redefine b() in that code, like my example below, which would only be silly. Also, that would mean that you couldn't call b() before calling a() at least once. But named subroutines are defined only once, at compile time*, which makes the whole construct iffy.

sub a {
    my $x = 3;
    *b = sub {
        print $x, $/;
    };
}

* The reasoning behind sub NAME { } constructs being realized early is so that you can call subroutines even if their definition is "later" in the code.

[moritz]

* The reasoning behind sub NAME { } constructs being realized early is so that you can call subroutines even if their definition is "later" in the code.

That's mostly true, but not very precise.

If a subroutine is defined after it is called, the call has to use parenthesis. If it doesn't, it's interpreted as a string literal (or as an error if strict subs are in effect).

$ perl -E 'say 1, foo; sub foo { 3  }'
1foo
$ perl -E 'sub foo { 3 }; say 1, foo'
13
$ perl -E 'say 1, foo(); sub foo { 3  }'
13

However it the call uses parenthesis, there's no need it has to be know at compile time at all:

$ perl -E 'eval "sub foo { 3 }"; say 1, foo();'
13

But I think for this discussion it's more important to ask when the lexical pad of sub a is being built and destroyed. As long as the a is not part of the call chain, $x is in no lex pad, and sub b can't work in a meaningful way.

[ikegami]

I consider it a bad idea to define a named sub within another sub

I agree 100%. Anon subs are another story. For example, I use constructs such as

sub foo {
    my $shared_with_all_recur = ...;

    local *_recur = sub {
        ...
        _recur(...);
        ...
    };

    _recur(...);
}

[AnomalousMonk]

Aside from the fact that the approach given in your reply is 'cleaner' and therefore, to my mind, preferable to something like

sub foo {
    my $shared_with_all_recur = ...;

    my $_recur;
    $_recur = sub {
        ...
        $_recur->(...);
        ...
    };

    $_recur->(...);
}

is there any advantage to using a localized glob rather than a lexical scalar?

[BrowserUk]

The main problem with nested subs, beyond the misleading apparent scoping of the innermost definition, is that Perl can only handle one level of closure, as (rather obliquely) indicated by the error message:

sub a{ 
    my $x = 'fred'; 
    sub b{ 
        print $x 
    } 
    b(); 
}; 

a();;

Variable "$x" will not stay shared at ...
fred

By the time you leave the definition of sub a(), the variable $x no longer exists, but sub b(), is still callable.

[Porculus]

I think this means that it thinks I'm defining a new sub, the old one wasn't terminated properly.

It means exactly what it says: your sub ends with print, but perlcritic thinks all subs should end with return. (Even if they are not intended to return a value, as in this case; perlcritic thinks that such subs should explicitly return nothing.)

[jwkrahn]

Your code does not define a sub within a sub, but a sub within a code block.

[AnomalousMonk]

Your code does not define a sub within a sub, but a sub within a code block.

Actually, the sub  level1 is defined (and called) within an explicit scope, not within a code block.

Since there are no lexicals also defined within the scope and referred to by the subroutine, the scope has no effect; subroutines themselves are globally scoped.

[jwkrahn]

Subroutine names are package variables and therefore not "globally scoped".    Only certain Perl variables fit the definition of "globally scoped".

the sub level1 is defined within an explicit scope, not within a code block.

Basic BLOCKs

[AnomalousMonk]

As the example below shows, defining a subroutine with sub within another subroutine and then calling it from within the enclosing subroutine works (although I don't think it's considered quite kosher), but really buys you nothing because the enclosed subroutine is still global in scope.

I have seen the trick on PM of local-izing a glob within a subroutine and then defining a named subroutine via that glob, but otherwise the approach for private subroutines seems to be to use anonymous subs defined within a sub.

The statement  my sub foo { ... } produces a  "my sub" not yet implemented ... error in 5.10 (and before AFAIR), so someone is thinking about this!

>perl -wMstrict -le
"print level2();
 sub level1 {
   sub level2 { return 'level2(): foo' }
   print 'level1(): ', level2();
   }
 level1();
 print level2();
"
level2(): foo
level1(): level2(): foo
level2(): foo

Updates:
1. After reading Joost's reply, I remembered the 'stupid local trick' referred to above. Perhaps someone else can say if this approach differs at all (other than by supplying a local named subroutine) from the anonymous-subroutine-assigned-to-lexical-variable approach; I can see no difference.

   >perl -wMstrict -le
   "sub level1 {
      my $x = shift || 'default';
      local *foo = sub { return 'foo' . $x };
      print foo();
      }
    level1('bar');
    level1();
    foo();
   "
   foobar
   foodefault
   Undefined subroutine &main::foo called at -e line 1.
   

2. A 'stupid local trick'? Well, maybe not. See ikegami's detailed reply to Re^3: Defining a sub within a sub: OK? below: there is an advantage.

[Khen1950fx]

I tried running your script using:

perlcritic --harsh --statistics script.pl

The results were, well, more than I thought that I would get:

1 files.
1 subroutines/methods.
8 statements.

14 lines, consisting of:
     2 blank lines.
     1 comment lines.
     0 data lines.
    11 lines of Perl code.
     0 lines of POD.

Average McCabe score of subroutines was 1.00.

17 violations.
Violations per file was 17.000.
Violations per statement was 2.125.
Violations per line of code was 1.214.

6 severity 4 violations.
6 severity 2 violations.
5 severity 1 violations.

5 violations of CodeLayout::RequireConsistentNewlines.
1 violations of CodeLayout::RequireTidyCode.
1 violations of Compatibility::PerlMinimumVersionAndWhy.
1 violations of Editor::RequireEmacsFileVariables.
3 violations of InputOutput::RequireCheckedSyscalls.
3 violations of Miscellanea::RequireRcsKeywords.
1 violations of Modules::RequirePerlVersion.
1 violations of Modules::RequireVersionVar.
1 violations of Subroutines::RequireFinalReturn.

[BrowserUk]

1 violations of Editor::RequireEmacsFileVariables.

Oh My God!!!!!!!!!!

Every perl script I ever wrote contains at least 1 violations". even if it is totally empty! Woe is me.

Actually, without bothering to look up the other violations being commited above, I'm guessing half a dozen others would also be triggered by an empty, or all whitespace source file.

What's next? Will I be penalised because Blond highlights with a hint of purple are just "so 2007"?

I'm really hoping that you thought that the above was worth posting on the basis of an ironic critique of the critic.

[Khen1950fx]

++! I was thinking of something more gothic---black with a hint of neon green:-).