Nested sub definitions - compile time warning

[p5pRT]

Migrated from rt.perl.org#125809 (status was 'open')

Searchable as RT125809$

[p5pRT]

From @epa

Created by @epa

You can nest subroutine definitions, as

  sub x { sub y {} }

Although at first glance it looks like the definition of y is somehow
'local' to x, this is not really the case. As far as I can tell there
is no advantage to writing it like this rather than putting y outside.

There are uses to making subroutine definitions inside a BLOCK, to
capture variables​:

  { my $x; sub y { ++$x } }

but that does not apply if the block is itself a subroutine definition
(variable will not stay shared). So unless there is something I've
overlooked, nesting one sub definition inside another indicates some
mistake or confusion by the programmer. It should warn at compile
time, and perhaps at some future point become a hard error.

(This would also smooth the path for adding some more useful semantics
to nested subroutine definitions at some later date - but doing so
is not the scope of this bug report.)

Perl Info

Flags:
    category=core
    severity=wishlist

Site configuration information for perl 5.18.4:

Configured by Red Hat, Inc. at Thu Apr  2 16:17:20 UTC 2015.

Summary of my perl5 (revision 5 version 18 subversion 4) configuration:
   
  Platform:
    osname=linux, osvers=3.19.1-201.fc21.x86_64, archname=x86_64-linux-thread-multi
    uname='linux buildvm-23.phx2.fedoraproject.org 3.19.1-201.fc21.x86_64 #1 smp wed mar 18 04:29:24 utc 2015 x86_64 x86_64 x86_64 gnulinux '
    config_args='-des -Doptimize=-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -Dccdlflags=-Wl,--enable-new-dtags -Dlddlflags=-shared -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches  -m64 -mtune=generic -Wl,-z,relro  -Dshrpdir=/usr/lib64 -DDEBUGGING=-g -Dversion=5.18.4 -Dmyhostname=localhost -Dperladmin=root@localhost -Dcc=gcc -Dcf_by=Red Hat, Inc. -Dprefix=/usr -Dvendorprefix=/usr -Dsiteprefix=/usr/local -Dsitelib=/usr/local/share/perl5 -Dsitearch=/usr/local/lib64/perl5 -Dprivlib=/usr/share/perl5 -Dvendorlib=/usr/share/perl5/vendor_perl -Darchlib=/usr/lib64/perl5 -Dvendorarch=/usr/lib64/perl5/vendor_perl -Darchname=x86_64-linux-thread-multi -Dlibpth=/usr/local/lib64 /lib64 /usr/lib64 -Duseshrplib -Dusethreads -Duseithreads -Dusedtrace=/usr/bin/dtrace -Duselargefiles -Dd_semctl_semun -Di_db -Ui_ndbm -Di_gdbm -Di_shadow -Di_syslog -Dman3ext=3pm -Duseperlio -Dinstallusrbinperl=n -Ubincompat5005 -Uversiononly -Dpager=/usr/bin/less -isr -Dd_gethostent_r_proto -Ud_endhostent_r_proto -Ud_sethostent_r_proto -Ud_endprotoent_r_proto -Ud_setprotoent_r_proto -Ud_endservent_r_proto -Ud_setservent_r_proto -Dscriptdir=/usr/bin -Dusesitecustomize'
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=define, usemultiplicity=define
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=define, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='gcc', ccflags ='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic',
    cppflags='-D_REENTRANT -D_GNU_SOURCE -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.8.3 20140911 (Red Hat 4.8.3-7)', gccosandvers=''
    intsize=4, longsize=8, ptrsize=8, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=16
    ivtype='long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=8, prototype=define
  Linker and Libraries:
    ld='gcc', ldflags =' -fstack-protector'
    libpth=/usr/local/lib64 /lib64 /usr/lib64
    libs=-lresolv -lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lpthread -lc -lgdbm_compat
    perllibs=-lresolv -lnsl -ldl -lm -lcrypt -lutil -lpthread -lc
    libc=, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.18'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,--enable-new-dtags'
    cccdlflags='-fPIC', lddlflags='-shared -O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector-strong --param=ssp-buffer-size=4 -grecord-gcc-switches -m64 -mtune=generic -Wl,-z,relro '

Locally applied patches:
    Fedora Patch1: Removes date check, Fedora/RHEL specific
    Fedora Patch3: support for libdir64
    Fedora Patch4: use libresolv instead of libbind
    Fedora Patch5: USE_MM_LD_RUN_PATH
    Fedora Patch6: Skip hostname tests, due to builders not being network capable
    Fedora Patch7: Dont run one io test due to random builder failures
    Fedora Patch9: Fix find2perl to translate ? glob properly (RT#113054)
    Fedora Patch10: Update h2ph(1) documentation (RT#117647)
    Fedora Patch11: Update pod2html(1) documentation (RT#117623)
    Fedora Patch12: Disable ornaments on perl5db AutoTrace tests (RT#118817)
    Fedora Patch14: Do not use system Term::ReadLine::Gnu in tests (RT#118821)
    Fedora Patch15: Define SONAME for libperl.so
    Fedora Patch16: Install libperl.so to -Dshrpdir value
    Fedora Patch18: Fix crash with \\&$glob_copy (RT#119051)
    Fedora Patch19: Fix coreamp.t rand test (RT#118237)
    Fedora Patch20: Reap child in case where exception has been thrown (RT#114722)
    Fedora Patch21: Fix using regular expressions containing multiple code blocks (RT#117917)
    Fedora Patch22: Create site paths by cpan for the first time (CPAN RT#99905)
    Fedora Patch200: Link XS modules to libperl.so with EU::CBuilder on Linux
    Fedora Patch201: Link XS modules to libperl.so with EU::MM on Linux


@INC for perl 5.18.4:
    /home/eda/share/perl5
    /home/eda/lib/perl5/
    /home/eda/lib64/perl5/
    /usr/local/lib64/perl5
    /usr/local/share/perl5
    /usr/lib64/perl5/vendor_perl
    /usr/share/perl5/vendor_perl
    /usr/lib64/perl5
    /usr/share/perl5
    .


Environment for perl 5.18.4:
    HOME=/home/eda
    LANG=en_GB.UTF-8
    LANGUAGE (unset)
    LC_COLLATE=C
    LC_CTYPE=en_GB.UTF-8
    LC_MESSAGES=en_GB.UTF-8
    LC_MONETARY=en_GB.UTF-8
    LC_NUMERIC=en_GB.UTF-8
    LC_TIME=en_GB.UTF-8
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/eda/bin:/home/eda/bin:/usr/local/bin:/usr/bin:/sbin:/usr/sbin:/sbin:/usr/sbin
    PERL5LIB=/home/eda/share/perl5:/home/eda/lib/perl5/:/home/eda/lib64/perl5/
    PERL_BADLANG (unset)
    SHELL=/bin/bash

Please ignore autogenerated disclaimer below this point.
 

This email is intended only for the person to whom it is addressed and may contain confidential information. Any retransmission, copying, disclosure or other use of, this information by persons other than the intended recipient is prohibited. If you received this email in error, please contact the sender and delete the material. This email is for information only and is not intended as an offer or solicitation for the purchase or sale of any financial instrument. Wadhwani Asset Management LLP is a Limited Liability Partnership registered in England (OC303168) with registered office at 40 Berkeley Square, 3rd Floor, London, W1J 5AL. It is authorised and regulated by the Financial Conduct Authority.

[p5pRT]

From @rjbs

* Ed Avis <perlbug-followup@​perl.org> [2015-08-14T08​:50​:29]

So unless there is something I've overlooked, nesting one sub definition
inside another indicates some mistake or confusion by the programmer.
It should warn at compile time, and perhaps at some future point become a
hard error.

I have seen this mistake more times than seems reasonable, almost always as an
accident, rather than (I think) on purpose.

I find the suggestion tempting. Part of the issue is that it indicates a
confusion of timing. (This mistake is usually revealed by the "will not stay
shared" warning.)

I'd also think this error would apply to​:

  sub foo {
  BEGIN { ... }
  }

...but perhaps there is use for​:

  sub foo {
  my $finished;
  UNITCHECK { $finished = time; }
  ...
  }

...or the like.

Anyway, I think it's a useful warning. On the other hand, it's also a warning
that a linter can pick up. Still, I think I'd lean toward accepting a patch
that added this warning... other thoughts?

--
rjbs

[p5pRT]

The RT System itself - Status changed from 'new' to 'open'

[p5pRT]

From @Abigail

On Mon, Aug 17, 2015 at 06​:40​:06PM -0400, Ricardo Signes wrote​:

* Ed Avis <perlbug-followup@​perl.org> [2015-08-14T08​:50​:29]

So unless there is something I've overlooked, nesting one sub definition
inside another indicates some mistake or confusion by the programmer.
It should warn at compile time, and perhaps at some future point become a
hard error.

I have seen this mistake more times than seems reasonable, almost always as an
accident, rather than (I think) on purpose.

I find the suggestion tempting. Part of the issue is that it indicates a
confusion of timing. (This mistake is usually revealed by the "will not stay
shared" warning.)

I'd also think this error would apply to​:

sub foo {
BEGIN { ... }
}

...but perhaps there is use for​:

sub foo {
my $finished;
UNITCHECK { $finished = time; }
...
}

...or the like.

Anyway, I think it's a useful warning. On the other hand, it's also a warning
that a linter can pick up. Still, I think I'd lean toward accepting a patch
that added this warning... other thoughts?

Why not start with a warning in Perlcritic, and if it turns out to catch
thousands of bugs with almost no false positives, then it could be added
to perl.

Abigail

[p5pRT]

From @rjbs

* Abigail <abigail@​abigail.be> [2015-08-18T03​:00​:18]

Why not start with a warning in Perlcritic, and if it turns out to catch
thousands of bugs with almost no false positives, then it could be added
to perl.

It would be really great if we had some way to get feedback on which perlcritic
policies actually catch bugs and which just ensure matters of taste.

--
rjbs

[p5pRT]

From @rjbs

* Abigail <abigail@​abigail.be> [2015-08-18T03​:00​:18]

Anyway, I think it's a useful warning. On the other hand, it's also a warning
that a linter can pick up. Still, I think I'd lean toward accepting a patch
that added this warning... other thoughts?

Why not start with a warning in Perlcritic, and if it turns out to catch
thousands of bugs with almost no false positives, then it could be added
to perl.

It turns out there is one. And that I wrote it, in 2007!

https://metacpan.org/pod/Perl::Critic::Policy::Subroutines::ProhibitNestedSubs

I'll see what I can find out about who else benefitted from it. Or, possibly,
see about running it against CPAN...

--
rjbs

[p5pRT]

From @epa

Perhaps now that true lexically scoped subroutines exist, there is a case for revisiting 'sub' inside 'sub' and directing the programmer towards 'my sub' instead?