Re: [5.8.12] Proposal for changing UNIVERSAL semantics

Front page | perl.perl5.porters | Postings from November 2003

Re: [5.8.12] Proposal for changing UNIVERSAL semantics

Thread Previous | Thread Next

From:

Tim Bunce

Date:

November 4, 2003 07:26

Subject:

Re: [5.8.12] Proposal for changing UNIVERSAL semantics

Message ID:

20031104152616.GA96036@dansat.data-plan.com

On Tue, Nov 04, 2003 at 02:34:36AM -0800, Michael G Schwern wrote:
> I think you're attacking this problem from the wrong end.
> 
> 
> On Mon, Nov 03, 2003 at 12:37:49PM -0800, Michael Jacob wrote:
> > 1.) Change can()'s semantics.
> > 
> > can() returns unexpected reults if any module changed something in the UNIVERSAL:: package. This can make other modules fail, we have seen this with DBI(!)... So:
> 
> It would seem DBI breaks because it did something to can().
> 
> DBI::disconnect_all(/usr/local/src/CPAN/DBI-1.38/blib/lib/DBI.pm:652):
> 652:        while ( my ($name, $drh) = each %DBI::installed_drh ) {
>   DB<2> n
> DBI::disconnect_all(/usr/local/src/CPAN/DBI-1.38/blib/lib/DBI.pm:653):
> 653:            $drh->disconnect_all() if ref $drh;
>   DB<2> x UNIVERSAL::can($drh, "disconnect_all")
> 0  CODE(0x514e94)
>    -> &CODE(0x514e94) in ???
>   DB<3> x $drh->can('disconnect_all')
>   empty array
>   DB<4> x $drh->can('can')
> 0  CODE(0x5142b4)
>    -> &DBI::dr::can in 0
>   DB<5> b DBI::dr::can
> Subroutine DBI::dr::can not found.
> 
> DBI appears to have overriden and busted can().

Overridden yes, but busted is a different matter:

	=item C<can>

	  $is_implemented = $h->can($method_name);

	Returns true if $method_name is implemented by the driver or a
	default method is provided by the DBI.
	It returns false where a driver hasn't implemented a method and the
	default method is provided by the DBI is just an empty stub.
	(That last sentance isn't in 1.38)

That's why $drh->can('disconnect_all') returned false above.
Also, when it returns true it returns a ref to the DBI dispatch
method and not to the underlying driver method. That's so
$code = $h->can($method_name); $code->(...); won't bypass the
important functionality provided by the dispatcher.

Note that the DBI does not alter UNIVERSAL::can, it only provides
a can() method for DBI handles.

> Running DBI with UNIVERSAL.pm
> doesn't cause any problems, so I think adding Attribute::Handlers into the 
> mix exposed some problem with DBI's can() hacks.

Since when is it a hack? There may be a problem in the implementation,
which I'll gladly fix if indentified, but there's no hack here.

> Here's t/01basic.t run this time with -MAttribute::Handlers
> 
>   DB<2> x $drh->can('can')
> 0  CODE(0x5417f0)
>    -> &CODE(0x5417f0) in ???
>   DB<3> x $drh->can('disconnect_all')
> 0  CODE(0x541820)
>    -> &DBI::dr::disconnect_all in 0
>   DB<4> 
> 
> DBI.xs is doing something really funky and intercepting can() method calls.
> It also appears to have changed the expected behavior of can().  It also 
> appears to have its own method dispatcher.  At this point I'm likely to
> say that Attribute::Handlers' screwing with @UNIVERSAL::ISA simply exposed a 
> bug in DBI. 

I'm not convinced. Here's the code for can() that's embedded into the DBI
dispatcher:

  if (*meth_name == 'c' && strEQ(meth_name,"can")) {
    char *can_meth = SvPV(st1,lna);
    SV *dbi_msv = Nullsv;
    SV  *imp_msv; /* handle implementors method (GV or CV) */

here it looks up the $h->can($can_meth) method just as it does when
it's going to call it:

    if ( (imp_msv = (SV*)gv_fetchmethod(DBIc_IMP_STASH(imp_xxh), can_meth)) ) {

if found it then looks up the same method name using the original handle
using gv_fetchmethod_autoload() just as UNIVERSAL::can does in universal.c:

	/* return DBI's CV, not the implementors CV (else we'd bypass dispatch) */
	/* and anyway, we may have hit a private method not part of the DBI     */
	GV *gv = gv_fetchmethod_autoload(SvSTASH(SvRV(orig_h)), can_meth, FALSE);
	if (gv && isGV(gv))
	    dbi_msv = (SV*)GvCV(gv);
    }
    if (dbi_msv) {
	ST(0) = sv_2mortal(newRV(dbi_msv));
	XSRETURN(1);
    }

else not found so return nothing:

  }
  XSRETURN(0);

Returning an empty list instead of undef difers from UNIVERSAL::can so I'll
change that for the next release. But I doubt that's the issue here.


> However, what you propose addresses a the valid problem of coordinating
> AUTOLOAD overrides.  But its likely to be a really sticky thing to get just
> and thus wrong for the core.  You should put this up on CPAN as 
> UNIVERSAL::AUTOLOAD.

I'm not sure it's relevent but I'll remind people here, in case
it's been forgotten, that AUTOLOAD and methods do not mix well unless
all the method have been pre-declared. That's why modules like AutoLoader
go to such lengths to generate and load declarations for all the subs
to be autoloaded.

> >  * useing UNIVERSAL breaking DBI, autouse, Net::SSH, ...
> 
> DBI is doing Dark Magick with can() and the method dispatcher which appears to
> be not 100% correct. Plus Attribute::Handlers is plowing *way* too much 
> stuff into the UNIVERSAL namespace.  Both will be fixed and the problem 
> should disappear.

Before I can "fix" the DBI someone needs to explain what it's doing
wrong and how that's causing the problem reported.

Tim.

Thread Previous | Thread Next