Unintentional base.pm behavior change

The "push onto ISA once" patch to base.pm had an
unintended change.
http://public.activestate.com/cgi-bin/perlbrowse/p/31851


The "don't push to ISA if it already is
one" logic is now ignored... mostly.

For example...

package Parent;
sub foo {}

package Middle;
use base qw(Parent);

package Child;
use base qw(Middle Parent);

print join " ", Child::ISA;

Using 5.8's base that produces just Middle.  Parent is not
pushed onto ISA
because base.pm detects Child is already a Parent via
Middle.

Using 5.10's base you get Middle and Parent.  The isa()
check is still in the
code, but because Child::ISA isn't altered until the very
end of import() it
isn't yet a Middle.

Because the now almost useless isa() check is still there, I
believe this
change was unintentional.  It produces weird inconsistencies
like...

package Parent;
sub foo {}

package Middle;
use base qw(Parent);

package Child;
use base qw(Middle);
use base qw(Parent);

print join " ", Child::ISA;

That should be equivalent to the code above, but its not. 
5.10's base
produces just "Middle".  It's effectively
emulating 5.8's behavior.  This is
because the isa() check is still in there, but now ISA has
been changed
before it checks.

So we should fix this one way or another.  I lean towards
removing the isa
check entirely, it means the docs that say "use
base" is just like "require
and push ISA" are a white lie and adds yet another bit of
invisible
complexity that really doesn't much matter.  Also keeping it
in makes altering
ISA
all in one go difficult.

OTOH this feature has been in there since 2000.
http://public.activestate.com/cgi-bin/perlbrowse/p/4835
and it does break some code.  Jesse caught this whole thing
when it broke
Jifty's ISA rewriting.

However, if we're going to introduce minor incompatibilities
to undocumented
features, 5.10.0 is the time to do it.  Jesse just wants it
to be stated
somewhere in fifty foot high flaming letters.


OTGH the original intention of this feature appears to have
been to prevent
one from adding the same module to ISA twice, that is, if you
reload a module.
http://www.xray.mpe.mpg.de/mailing-l
ists/perl5-porters/1999-12/msg00446.html


So I think what should happen is this either...

A1) We roll back 31851 and leave things as they were.
A2) Some unknown genius figures out how to make base do just
one ISA push
    while retaining the isa() check without totally blowing
performance which
    was the original point of this exercise.

or

B1) The isa check code is removed.
B2) In it's place is code to check if the module is being
reloaded.

    if( I'm already loaded AND
        the new ISA is the same as the old one )
    {
         don't add to ISA
    }
B3) 5.10's base.pm is put on CPAN so people can depend on it
and the new
    behavior.


PS  No, this is not a 5.10 glitch.  I'm using 5.10's base.pm
with 5.8.


-- 
Ahh email, my old friend.  Do you know that revenge is a
dish that is best
served cold?  And it is very cold on the Internet!

On Sep 17, 2007, at 5:30 PM, Michael G Schwern wrote:

> The "push onto ISA once" patch to
base.pm had an unintended change.
> http://public.activestate.com/cgi-bin/perlbrowse/p/31851

>
> The "don't push to ISA if it already is
one" logic is now  
> ignored... mostly.
>
...

> OTOH this feature has been in there since 2000.
> http://public.activestate.com/cgi-bin/perlbrowse/p/4835
> and it does break some code.  Jesse caught this whole
thing when it  
> broke
> Jifty's ISA rewriting.
>
> However, if we're going to introduce minor
incompatibilities to  
> undocumented
> features, 5.10.0 is the time to do it.  Jesse just
wants it to be  
> stated
> somewhere in fifty foot high flaming letters.

Since you can't ship 50 foot high flaming letters, I'd argue
for not  
making this construct "work" differently than it
already "worked."
If the previous behaviour was undocumented, wrong and should
be  
abolished (thus breaking existing user code), I'd rather it
die with  
an error that you can no longer use multiple classes in the
same base  
statement.

If you want the same syntax to have a different semantic,
put that  
new semantic in a new module. I hear there's a lot of
interest in new  
'base' workalikes.

-jesse

On Sep 17, 2007, at 4:30 PM, Michael G Schwern wrote:
> So I think what should happen is this either...
>
> A1) We roll back 31851 and leave things as they were.
> A2) Some unknown genius figures out how to make base do
just one  
> ISA push
>     while retaining the isa() check without totally
blowing  
> performance which
>     was the original point of this exercise.

Well you have to do the isa check against all the packages
that have  
already been accepted to be pushed onto ISA.
patch attached.

However I question what performance is being
"really" gained by doing  
only one push. What percentage of uses of C<use base>
actually give  
more than one parent anyway.

Graham.

On Sep 17 2007, Michael G Schwern wrote:
> Because the now almost useless isa() check is still
there, I believe this
> change was unintentional.

For the record, it was definitely unintentional when I wrote
it but
there was some discussion after that because Brandon wasn't
fooled.
Hugo provided a patch similar to Graham's but it was not
applied.

> So we should fix this one way or another.  I lean
towards removing the isa
> check entirely,

Me too.  If someone says

    use base qw(Dog Animal);

then they shouldn't be surprised when ISA = qw(Dog Animal).

Unfortunately...

> OTOH this feature has been in there since 2000.
> http://public.activestate.com/cgi-bin/perlbrowse/p/4835
> and it does break some code.
...
> OTGH the original intention of this feature appears to
have been to prevent
> one from adding the same module to ISA
twice, that is, if you reload a module.
> http://www.xray.mpe.mpg.de/mailing-l
ists/perl5-porters/1999-12/msg00446.html

The change looks like it was only meant for fields.pm.  We
really
should decouple base and fields and let fields take care of
its own
special inheritance needs.  But its such a big, ugly mess
that I don't
think I'll get to it soon, even if Rafael was inclined to
accept such a
big change this close to 5.10.

So maybe backing out the change for 5.10.0 would be best.  I
think users
of Class::C3 (Catalyst) will be very pleased with the
performance
enhancements they're getting so it doesn't really matter if
there is
still room for improvement.

-- 
Rick Delaney
rickbort.ca

Graham Barr wrote:
> On Sep 17, 2007, at 4:30 PM, Michael G Schwern wrote:
>> So I think what should happen is this either...
>>
>> A1) We roll back 31851 and leave things as they
were.
>> A2) Some unknown genius figures out how to make
base do just one ISA
>> push
>>     while retaining the isa() check without totally
blowing
>> performance which
>>     was the original point of this exercise.
> 
> Well you have to do the isa check against all the
packages that have
> already been accepted to be pushed onto ISA.
patch attached.

I've attached a slightly modified version of that patch to
tighten it up a
little, with a test.

Regardless of whether the code changes, that test should go
in.


> However I question what performance is being
"really" gained by doing
> only one push. What percentage of uses of C<use
base> actually give more
> than one parent anyway.

That is the magical question.


-- 
The interface should be as clean as newly fallen snow and
its behavior
as explicit as Japanese eel porn.

Michael G Schwern wrote:
> I've attached a slightly modified version of that patch
to tighten it up a
> little, with a test.

Now with 521% more patch!


-- 
Robrt:   People can't win
Schwern: No, but they can riot after the game.

On 19/09/2007, Michael G Schwern <schwernpobox.com> wrote:
> Michael G Schwern wrote:
> > I've attached a slightly modified version of that
patch to tighten it up a
> > little, with a test.
>
> Now with 521% more patch!

Thanks, applied.

* Michael G Schwern <schwernpobox.com> [2007-09-17
23:35]:
> OTOH this feature has been in there since 2000.
> http://public.activestate.com/cgi-bin/perlbrowse/p/4835 and it
> does break some code. Jesse caught this whole thing
when it
> broke Jifty's ISA rewriting.
> 
> However, if we're going to introduce minor
incompatibilities to
> undocumented features, 5.10.0 is the time to do it.
Jesse just
> wants it to be stated somewhere in fifty foot high
flaming
> letters.

Leave `base` alone. We gain nothing substantial from this
change; it remains unfixably magical in a bad way and
infested
with `fields` crap. The fix was to put `parent` in 5.10
instead.
Let’s agree that `base` is backcompat legacy, that
existing API
clients should not be broken, and that its behaviour should
therefore not change.

Anyone who doesn’t like its quirks can use `parent` and
get
ultra-obvious behaviour.

Regards,
-- 
Aristotle Pagaltzis // <http://plasmasturm.org/&g
t;