Bug #79383 for namespace-sweep: Sweep breaks all usage of package varables

Mon Sep 03 17:14:35 2012 chip [...] pobox.com - Ticket created

Subject:

Sweep breaks all usage of package varables

During compilation of code using package variables, Perl embeds a pointer to the glob. The sweep approach of always deleting the glob and replacing it with a new one, even if the new one is unchanged, ends up destroying the connection between the OP and the variable. The current logic that calls $sweep->($sym) unless it can find a reason not to should be reversed: only call $sweep->($sym) if it finds a reason to (i.e. a function that needs to be swept away).

Tue Sep 04 21:46:20 2012 FRIEDO [...] cpan.org - Correspondence added

Yep, this is a pretty damn serious bug. I'll try to get a fix for this out ASAP. Will _probably_ have time to work on it tomorrow.

Tue Sep 04 21:46:20 2012 The RT System itself - Status changed from 'new' to 'open'

Sat Sep 08 01:13:49 2012 FRIEDO [...] cpan.org - Correspondence added

I wrote the attached test, but it passes for me with namespace-sweep-0.004. Obviously I'm missing something here, can you show me a test which fails?

Subject:

pkg_symbol.t

#!/usr/bin/env perl use strict; use warnings; use Test::More tests => 11; package Blurns; { no strict; use namespace::sweep; use Scalar::Util 'reftype'; $ball = 'fun'; @loaded = ( 1, 2, 3 ); %infield_blurn = ( in_effect => 1 ); $reftype = 42; sub method { 1; } sub method2 { return 'the infield blurn rule is ' . ( $infield_blurn{in_effect} ? 'in effect' : 'not in effect' ); } } package main; my $o = bless { }, 'Blurns'; ok $o; isa_ok $o, 'Blurns'; ok $o->method; is $o->method2, 'the infield blurn rule is in effect'; is $Blurns::ball, 'fun'; ok @Blurns::loaded; is $Blurns::loaded[0], 1; ok %Blurns::infield_blurn; ok $Blurns::infield_blurn{in_effect}; ok !$o->can( 'reftype' ); is $Blurns::reftype, 42;

Sat Sep 08 01:32:15 2012 chip [...] pobox.com - Correspondence added

From:

chip [...] pobox.com

Repro seems to be tied to the use of "our" in the module: { package Foo; use namespace::sweep; use Exporter::Tidy default => [qw( $foo )]; our $foo = 1; BEGIN { $INC{'Foo.pm'}=1 } } package Bar; use Foo; print "foo=$foo\n";

Mon Sep 10 05:23:16 2012 perl [...] toby.ink - Correspondence added

This has nothing to do with our $foo. namespace::sweep is deleting the import sub that Exporter::Tidy creates. This is expected behaviour (namespace::sweep is supposed to delete any subs in your namespace installed by other modules) but in this particular example, a little annoying. It would be nice if namespace::sweep had an "except" flag like namespace::clean does: use namespace::sweep -except => [qw/ import /];

Mon Sep 10 05:34:06 2012 perl [...] toby.ink - Correspondence added

Here's a slightly hackish workaround: ``` { package Foo; use namespace::sweep; sub import { require Exporter::Tidy; Exporter::Tidy->import( default => [qw( $foo )] ); goto \&import; } our $foo = 1; BEGIN { $INC{'Foo.pm'}=1 } } package Bar; use Foo; print "foo=$foo\n"; ``` And another slightly less hackish... ``` { package Foo; use namespace::sweep; use Exporter::Tidy default => [qw( $foo )]; our $foo = 1; BEGIN { $INC{'Foo.pm'} = 1; require Sub::Name; Sub::Name::subname('Foo::import', \&Foo::import); } } package Bar; use Foo; print "foo=$foo\n"; ```

Mon Sep 10 10:42:16 2012 FRIEDO [...] cpan.org - Correspondence added

Thanks, Toby. I had a suspicion it might have to do with the exporter, but I didn't have time to work on it over the weekend. This is a good excuse to finally add a 'except' param for symbols that you want to protect. Then you could do: use namespace::sweep -except => [ 'import' ];

Mon Sep 10 10:44:02 2012 FRIEDO [...] cpan.org - Correspondence added

Looking back over the comments....there appears to be an echo in here. :)

Mon Sep 10 16:19:06 2012 chip [...] pobox.com - Correspondence added

From:

chip [...] pobox.com

Thanks for the analysis. Shirley, "import" should be excepted by default. Likewise "meta".

Mon Sep 10 20:06:46 2012 perl [...] toby.ink - Correspondence added

On 2012-09-10T21:19:06+01:00, CHIPS wrote: Show quoted text

> Shirley, "import" should be excepted by default.

Probably. I can't imagine any realistic situation where you wouldn't want it to be excepted. And if you really did want your import swept away, you could always explicitly: use namespace::sweep -also => 'import';

Fri Sep 13 16:35:14 2013 djerius [...] cpan.org - Correspondence added

On Mon Sep 10 05:34:06 2012, TOBYINK wrote: Show quoted text

> Here's a slightly hackish workaround: > > ``` > { > package Foo; > use namespace::sweep; > sub import { > require Exporter::Tidy; > Exporter::Tidy->import( > default => [qw( $foo )] > ); > goto \&import; > } > our $foo = 1; > BEGIN { $INC{'Foo.pm'}=1 } > } > package Bar; > use Foo; > print "foo=$foo\n"; > ``` > > And another slightly less hackish... > > ``` > { > package Foo; > use namespace::sweep; > use Exporter::Tidy > default => [qw( $foo )]; > our $foo = 1; > BEGIN { > $INC{'Foo.pm'} = 1; > require Sub::Name; > Sub::Name::subname('Foo::import', \&Foo::import); > } > } > package Bar; > use Foo; > print "foo=$foo\n"; > ```

Would this not also work (it seems to for me, but perhaps I'm missing something)? It seems a bit simpler. use Exporter(); sub import { goto \&Exporter::import }

Fri Sep 13 16:44:36 2013 djerius [...] cpan.org - Correspondence added

On Fri Sep 13 16:35:14 2013, DJERIUS wrote: Show quoted text

> On Mon Sep 10 05:34:06 2012, TOBYINK wrote:

> > Here's a slightly hackish workaround: > > > > ``` > > { > > package Foo; > > use namespace::sweep; > > sub import { > > require Exporter::Tidy; > > Exporter::Tidy->import( > > default => [qw( $foo )] > > ); > > goto \&import; > > } > > our $foo = 1; > > BEGIN { $INC{'Foo.pm'}=1 } > > } > > package Bar; > > use Foo; > > print "foo=$foo\n"; > > ``` > > > > And another slightly less hackish... > > > > ``` > > { > > package Foo; > > use namespace::sweep; > > use Exporter::Tidy > > default => [qw( $foo )]; > > our $foo = 1; > > BEGIN { > > $INC{'Foo.pm'} = 1; > > require Sub::Name; > > Sub::Name::subname('Foo::import', \&Foo::import); > > } > > } > > package Bar; > > use Foo; > > print "foo=$foo\n"; > > ```

> > > Would this not also work (it seems to for me, but perhaps I'm missing > something)? It seems a bit simpler. > > use Exporter(); > sub import { goto \&Exporter::import }

AARgh. and that's what you said, already, but with slightly different words. Please disregard...

Fri Sep 13 16:46:17 2013 djerius [...] cpan.org - Cc DJERIUS added

Sat Nov 23 22:14:54 2013 ether [...] cpan.org - Correspondence added

On 2012-09-10 02:23:16, TOBYINK wrote: Show quoted text

> This has nothing to do with our $foo. > > namespace::sweep is deleting the import sub that Exporter::Tidy creates.

Exporter::Tiny should be installing the import sub such that it is not swept, since it needs to be called as a class method at runtime.

Sat Nov 23 22:15:45 2013 ether [...] cpan.org - Correspondence added

On 2013-11-23 19:14:54, ETHER wrote: Show quoted text

> On 2012-09-10 02:23:16, TOBYINK wrote:

> > This has nothing to do with our $foo. > > > > namespace::sweep is deleting the import sub that Exporter::Tidy > > creates.

> > Exporter::Tiny should be installing the import sub such that it is not > swept, since it needs to be called as a class method at runtime.

er Tidy (I thought that was a typo!)

Mon Nov 25 07:06:48 2013 perl [...] toby.ink - Correspondence added

Show quoted text

> Exporter::Tiny should be installing the import > sub such that it is not swept, since it needs > to be called as a class method at runtime.

FWIW, this is one of many reasons why Exporter::Tiny doesn't install an import method. It uses inheritance.

Wed Nov 27 18:26:43 2013 chip [...] pobox.com - Correspondence added

CC:	djerius [...] cpan.org
Subject:	Re: [rt.cpan.org #79383] Sweep breaks all usage of package varables
Date:	Wed, 27 Nov 2013 15:26:18 -0800
To:	bug-namespace-sweep [...] rt.cpan.org
From:	Chip Salzenberg <chip [...] pobox.com>

On 11/25/2013 4:06 AM, Toby Inkster via RT wrote: Show quoted text

> <URL: https://rt.cpan.org/Ticket/Display.html?id=79383 > >

>> Exporter::Tiny should be installing the import >> sub such that it is not swept, since it needs >> to be called as a class method at runtime.

> FWIW, this is one of many reasons why Exporter::Tiny doesn't install an import method. It uses inheritance.

Awful solution. Makes every method call slower, forever.

Thu Nov 28 16:08:35 2013 perl [...] toby.ink - Correspondence added

On 2013-11-27T23:26:43Z, CHIPS wrote: Show quoted text

> Awful solution.

No, it's a good solution because it makes subclassing possible.