[PATCH universal.c] Remove sv_derived_from From Public API
Chromatic
Here's a patch to remove the documentation of sv_derived_from from the public
API. Calling this function on a blessed SV is almost always wrong, as it
ignores any overloaded isa() methods.
I'm not sure exactly what to recommend in its place -- but it should
definitely recommend calling the 'isa' method on the invocant and catching
any errors.
Changing this function to do that would be a mistake and could end up in a
circular loop.
-- c
Jan Dubois
> Here's a patch to remove the documentation of sv_derived_from from the
> public API. Calling this function on a blessed SV is almost always
> wrong, as it ignores any overloaded isa() methods.
>
> I'm not sure exactly what to recommend in its place -- but it should
> definitely recommend calling the 'isa' method on the invocant and
> catching any errors.
Well, if that is what needs to be done, then sv_derived_from() should just
do it. I disagree that this should be removed from the public API;
if you think it is broken, then please fix it!
But I'm also not convinced that calling this function "is almost always
wrong" either. I've been using it, and it does exactly what I expect
it to do: tell me that an object is derived from my base class, and that
I can expect that the magical things hanging of the object have the
semantics I expect for them. So how is this wrong?
Cheers,
-Jan
John Peacock
> Here's a patch to remove the documentation of sv_derived_from from the public
> API. Calling this function on a blessed SV is almost always wrong, as it
> ignores any overloaded isa() methods.
-1 from me on removing this from the public API (and I suspect Rafael will be
right behind me). There is a reason it is called "public" and you cannot
cavalierly delete something you don't approve of.
And you are also wrong about it being "almost always wrong" since the _only_
time it does the wrong thing is when a class overloads the isa() method, which
is far rarer than you want to acknowledge. Just because /you/ want to use stub
classes that overload isa() doesn't bind all of the existing classes that
correctly use this call.
Change the docstring if you must (as long as it is neutral and explains how this
may subtly break certain classes), but you simply cannot remove this from the
public Perl5 API.
John
--
John Peacock
Director of Information Research and Technology
Rowman & Littlefield Publishing Group
4720 Boston Way
Lanham, MD 20706
301-459-3366 x.5010
fax 301-429-5747
Jan Dubois
> And you are also wrong about it being "almost always wrong" since the
> _only_ time it does the wrong thing is when a class overloads the
> isa() method, which is far rarer than you want to acknowledge. Just
> because /you/ want to use stub classes that overload isa() doesn't
> bind all of the existing classes that correctly use this call.
Oh, I see, this is what it is about. In all the cases I use sv_derived_from()
it would be explicitly wrong to call an overloaded isa() in case it actually
lies and pretends to be derived from my base class if it isn't. The
sv_derived_from() code guards values that are stored as IVs in Perl data
structures, but are actually pointers to C structs. If the object is not
actually derived from the base class but just a mock object, then your
mock object is broken by calling my internal functions that expect a real
object.
Cheers,
-Jan
Chromatic
> Just because /you/ want to use stub classes that overload isa() doesn't bind
> all of the existing classes that correctly use this call.
Please show me a correct use of this call that respects polymorphism.
-- c
Chromatic
> Oh, I see, this is what it is about. In all the cases I use
> sv_derived_from() it would be explicitly wrong to call an overloaded isa()
> in case it actually lies and pretends to be derived from my base class if
> it isn't. The sv_derived_from() code guards values that are stored as IVs
> in Perl data structures, but are actually pointers to C structs. If the
> object is not actually derived from the base class but just a mock object,
> then your mock object is broken by calling my internal functions that
> expect a real object.
... and in the cases when my mock object does the right thing, then your code
is broken.
Something is broken here. I'd like to fix it so that if we both write
sensible code, things just work. Right now, they don't.
-- c
John Peacock
> Please show me a correct use of this call that respects polymorphism.
As someone wise already pointed out:
> I'm not sure exactly what to recommend in its place -- but it should
> definitely recommend calling the 'isa' method on the invocant and catching
> any errors.
;-)
You are ignoring my larger point that polymorphism is a very recent innovation
in Perl coding styles, and lots of OO code out there exists already that doesn't
rely on polymorphism and works fine with this call.
I really think you need to work on how to update the docstring to reflect a more
cautious approach that works with polymorphism. But the most you can hope for
is a deprecation cycle, not all out destruction. Code in the core and on CPAN
use this API and you _cannot_ delete it. Trust me on this one; I spent three
years trying to kill v-strings and all I managed to do was to defang them in the
more recent 5.8.x series. :)
Chromatic
> You are ignoring my larger point that polymorphism is a very recent
> innovation in Perl coding styles, and lots of OO code out there exists
> already that doesn't rely on polymorphism and works fine with this call.
We disagree about how "fine" it works. As more and more people use
Test::MockObject and Test::MockObject::Extends, they find more and more
places where calling UNIVERSAL methods as functions *doesn't* work.
> I really think you need to work on how to update the docstring to reflect a
> more cautious approach that works with polymorphism. But the most you can
> hope for is a deprecation cycle, not all out destruction. Code in the core
> and on CPAN use this API and you _cannot_ delete it. Trust me on this one;
> I spent three years trying to kill v-strings and all I managed to do was to
> defang them in the more recent 5.8.x series. :)
Here's another idea. The code in sv_derived_from() can stay as the
implementation of UNIVERSAL::isa in universal.c with a different name, but
the body of sv_derived_from() can change to call the isa() method on the
invocant (if it looks like a valid invocant) or the C function that actually
implements UNIVERSAL::isa if it doesn't look like a valid invocant.
This way the function stays public but has a chance of working more accurately
than it does now. This also avoids the infinite loop that changing
sv_derived_from() in place in universal.c might cause.
-- c
John Peacock
> We disagree about how "fine" it works. As more and more people use
> Test::MockObject and Test::MockObject::Extends, they find more and more
> places where calling UNIVERSAL methods as functions *doesn't* work.
_You_ broke it, so _you_ fix it... ;-)
> Here's another idea. The code in sv_derived_from() can stay as the
> implementation of UNIVERSAL::isa in universal.c with a different name, but
> the body of sv_derived_from() can change to call the isa() method on the
> invocant (if it looks like a valid invocant) or the C function that actually
> implements UNIVERSAL::isa if it doesn't look like a valid invocant.
Sounds like a plan. It will be a performance hit (since you have all those
nasty dispatches from C back into Perl code and out again), but it shouldn't be
significant since this functionality is rarely called (in my experience).
Patches welcome! Be sure and include some tests... <duck>
Steve Peters
A quick check of Google shows that the following modules depend on
Perl_sv_derived_from() in the public API.
mod_perl
Win32API::MIDI
SGI::FAM
BerkeleyDB
Digest::SHA
NDBM
...and the list goes on and on. Removing that function would make a
significant portion of CPAN unusable.
Steve Peters
st...@fisharerojo.org
Jan Dubois
> We disagree about how "fine" it works. As more and more people use
> Test::MockObject and Test::MockObject::Extends, they find more and more
> places where calling UNIVERSAL methods as functions *doesn't* work.
Now you are talking about UNIVERSAL::isa(). I can see that you prefer
that people should instead call $obj->isa(), but that is a separate
issue, and I would have less objections to changing the way UNIVERSAL::isa()
works to "fix" the function call mode.
> Here's another idea. The code in sv_derived_from() can stay as the
> implementation of UNIVERSAL::isa in universal.c with a different name, but
> the body of sv_derived_from() can change to call the isa() method on the
> invocant (if it looks like a valid invocant) or the C function that actually
> implements UNIVERSAL::isa if it doesn't look like a valid invocant.
Nope, I think sv_derived_from() should stay as is for backward compatibility
and general sanity. Just choose a new name for your new functionality,
and maybe even use that one for UNIVERSAL::isa(). But don't force us all
to talk about "sv_derived_from, new semantics, old name" vs "sv_derived_from,
old semantics". That is just painful.
I think if you want the $obj->isa() semantics, then chances are high
that you would write your code in Perl in the first place and not in XS.
If you care about derived classes in XS code, chances are very high that
mock objects will not work and you are looking for objects that share
the implementation. Your mock objects should simply not call XS functions
that they know nothing about.
> This way the function stays public but has a chance of working more
> accurately than it does now. This also avoids the infinite loop that
> changing sv_derived_from() in place in universal.c might cause.
Using a new name for the new functionality offers the same advantages without
introducing the backwards compatibility headache. How would ppport.h deal
with the change? How would XS code look that has to work with current
sv_derived_from() semantics with all of Perl 5.6, 5.8 and 5.10?
Cheers,
-Jan
Marvin Humphrey
On May 9, 2006, at 6:54 PM, John Peacock wrote:
> Sounds like a plan. It will be a performance hit (since you have
> all those
> nasty dispatches from C back into Perl code and out again), but it
> shouldn't be
> significant since this functionality is rarely called (in my
> experience).
I use it for all the C-struct classes in KinoSearch in the typemap.
That includes the I/O classes, so every I/O function called as a
method from Perl would cost two method calls if this change were made.
Perl's OO overhead is already quite an interesting challenge to work
with. :) OO overhead was killing Plucene, and I couldn't bring it
down without breaking the API -- that's why KinoSearch exists in its
current form. Many design decisions were guided by the need to
minimize the number of method calls.
I'm not feeling very argumentative about this, since I agree with
chromatic about what the desirable behavior is, but it would be nice
if someone could suggest a less expensive solution that allows my app
to check class membership and produce a meaningful error message
rather than segfault when the wrong SV shows up at the wrong place at
the wrong time. The only thing I've thought of so far is mildly
insane: list all the known subclasses in the typemap in an array of
char* and iterate over that before calling sv_derived_from().... Hmm....
To preview the kind of impact we're talking about, I built KinoSearch
with all the calls to sv_derived_from() stripped out of the typemap,
just taking it on faith that those objects were what they claimed to
be. Performance on an indexing benchmark inproved by a little more
than 2%. (3.10 secs vs 3.03 secs, truncated mean).
Marvin Humphrey
Rectangular Research
http://www.rectangular.com/
slothbear:~/Desktop/ks/t/benchmarks marvin$ perl -Mblib indexers/
kinosearch_indexer.plx --reps 12 --docs 1000
------------------------------------------------------------
1 Secs: 3.11 Docs: 1000
2 Secs: 3.12 Docs: 1000
3 Secs: 3.11 Docs: 1000
4 Secs: 3.09 Docs: 1000
5 Secs: 3.11 Docs: 1000
6 Secs: 3.12 Docs: 1000
7 Secs: 3.09 Docs: 1000
8 Secs: 3.11 Docs: 1000
9 Secs: 3.07 Docs: 1000
10 Secs: 3.08 Docs: 1000
11 Secs: 3.09 Docs: 1000
12 Secs: 3.11 Docs: 1000
------------------------------------------------------------
KinoSearch 0.10
Perl 5.8.6
Thread support: yes
Darwin 8.6.0 Power Macintosh
Mean: 3.10 secs
Truncated mean (6 kept, 6 discarded): 3.10 secs
------------------------------------------------------------
slothbear:~/Desktop/ks/t/benchmarks marvin$
slothbear:~/Desktop/ks/t/benchmarks marvin$ perl -Mblib indexers/
kinosearch_indexer.plx --reps 12 --docs 1000
------------------------------------------------------------
1 Secs: 3.02 Docs: 1000
2 Secs: 3.06 Docs: 1000
3 Secs: 3.05 Docs: 1000
4 Secs: 3.29 Docs: 1000
5 Secs: 3.01 Docs: 1000
6 Secs: 3.01 Docs: 1000
7 Secs: 3.04 Docs: 1000
8 Secs: 3.02 Docs: 1000
9 Secs: 3.02 Docs: 1000
10 Secs: 3.01 Docs: 1000
11 Secs: 3.28 Docs: 1000
12 Secs: 3.01 Docs: 1000
------------------------------------------------------------
KinoSearch 0.10
Perl 5.8.6
Thread support: yes
Darwin 8.6.0 Power Macintosh
Mean: 3.07 secs
Truncated mean (6 kept, 6 discarded): 3.03 secs
------------------------------------------------------------
slothbear:~/Desktop/ks/t/benchmarks marvin$
Jan Dubois
>
> sensible code, things just work. Right now, they don't.
After thinking some more about this I believe the real problem is this:
There are 2 types of "inheritance", implementation inheritance and
interface inheritance. We are trying to use the same APIs to determine
both, which obviously does not work. Claiming that only one of the 2
cases matters doesn't really help, as there have been arguments that
both types of information are useful in different situations.
So why don't we define a new API to determine interface inheritance:
UNIVERSAL::implements and sv_implements().
Then UNIVERSAL::isa and sv_derived_from() can be used strictly for
implementation inheritance purposes. The documentation for these should
encourage users to use implements() instead when they really only care
about the interface and not the implementation details.
On the other hand, authors of mock objects should be encouraged to add
an implements() method to their classes and leave isa() alone to allow
users to get the correct information about implementation details, if
they really need to know. (Leaving isa() alone may not be feasible for a
long while until all the other involved classes start to use
implements(), but that isn't really a problem).
To bootstrap this API it will need to fall back on isa() when a class
doesn't define an implements() method. With this fallback all existing
mock objects should continue to work.
This seems a much cleaner approach to me. Or is there anything it
doesn't cover?
Cheers,
-Jan
Rafael Garcia-Suarez
> Nope, I think sv_derived_from() should stay as is for backward compatibility
> and general sanity.
That's also what I think. Also, document the trap with custom isa() in
sv_derived_from.
> I think if you want the $obj->isa() semantics, then chances are high
> that you would write your code in Perl in the first place and not in XS.
> If you care about derived classes in XS code, chances are very high that
> mock objects will not work and you are looking for objects that share
> the implementation. Your mock objects should simply not call XS functions
> that they know nothing about.
Maybe not, scary XS code is possible. I don't see why some tasks will
be necessarily written in perl rather than to XS. (even if XS is
always going to be painful for this kind of things)
Rafael Garcia-Suarez
>
> There are 2 types of "inheritance", implementation inheritance and
> interface inheritance. We are trying to use the same APIs to determine
> both, which obviously does not work. Claiming that only one of the 2
> cases matters doesn't really help, as there have been arguments that
> both types of information are useful in different situations.
>
> So why don't we define a new API to determine interface inheritance:
> UNIVERSAL::implements and sv_implements().
Add duck typing paradigms to this and this begins to sound like can()
(but for classes).
So, if I understand correctly, method dispatch is done through
implementation inheritance, while object inheritance relationships can
be mocked through implementation inheritance ?
This is a nice idea, but needs to be phrased (specified, dare I say)
more clearly.
Graham Barr
On May 9, 2006, at 7:10 PM, chromatic wrote:
> Hi there,
>
> Here's a patch to remove the documentation of sv_derived_from from
> the public
> API. Calling this function on a blessed SV is almost always wrong,
> as it
> ignores any overloaded isa() methods.
But that is its purpose.
When implementing a XS extension where the object is just a pointer
to a C struct the the XS sub must make sure that the value in the IV
is actually what it expects and that the C code can use it as a
memory pointer. Mock objects do not work in this arena. Mock object
can work on a layer above this, but by the time the code gets to the
XS code it must pass the real object.
> I'm not sure exactly what to recommend in its place -- but it should
> definitely recommend calling the 'isa' method on the invocant and
> catching
> any errors.
Please show how the following, taken from typemap, would work with -
>isa.
T_PTROBJ
if (sv_derived_from($arg, \"${ntype}\")) {
IV tmp = SvIV((SV*)SvRV($arg));
$var = INT2PTR($type,tmp);
}
else
Perl_croak(aTHX_ \"$var is not of type ${ntype}\")
sv_derived_from says that the SV is an exact implementation so that C
code can peek inside directly and know what is there.
Graham.
Chromatic
> sv_derived_from says that the SV is an exact implementation so that C
> code can peek inside directly and know what is there.
Oh, that's structural typing. That's fine.
Here's a patch intended to clarify when to use sv_derived_from and when not to
use it.
I'll address Jan's suggestion in another patch.
-- c
Chromatic
> There are 2 types of "inheritance", implementation inheritance and
> interface inheritance. We are trying to use the same APIs to determine
> both, which obviously does not work. Claiming that only one of the 2
> cases matters doesn't really help, as there have been arguments that
> both types of information are useful in different situations.
I agree; I think it's an issue of structural versus nominal typing.
Structural typing makes sense from the C level, but doesn't (or at least
shouldn't) make much sense from the Perl level.
> So why don't we define a new API to determine interface inheritance:
> UNIVERSAL::implements and sv_implements().
>
> Then UNIVERSAL::isa and sv_derived_from() can be used strictly for
> implementation inheritance purposes. The documentation for these should
> encourage users to use implements() instead when they really only care
> about the interface and not the implementation details.
>
> On the other hand, authors of mock objects should be encouraged to add
> an implements() method to their classes and leave isa() alone to allow
> users to get the correct information about implementation details, if
> they really need to know. (Leaving isa() alone may not be feasible for a
> long while until all the other involved classes start to use
> implements(), but that isn't really a problem).
>
> To bootstrap this API it will need to fall back on isa() when a class
> doesn't define an implements() method. With this fallback all existing
> mock objects should continue to work.
>
> This seems a much cleaner approach to me. Or is there anything it
> doesn't cover?
Perl 6 uses the term does() instead. Here's a quick patch to universal.c and
t/op/universal.t that seems to do the right thing.
-- c
Fergal Daly
Adding a new method to UNIVERSAL could break existing code. Also
there's a chance that it conflicts with an existing method. Making it
DOES() would minimisze this risk. I know that's against the style of
isa and can but they've been there since the start,
F
Rafael Garcia-Suarez
>
> Here's a patch intended to clarify when to use sv_derived_from and when not to
> use it.
Thanks, applied as change #28195.
Jan Dubois
I think it would be good to expose the functionality to the XS layer too,
by providing a public Perl_sv_dos() function, with XS_UNIVERSAL_DOES
just being a thin XS wrapper around it.
Also, this part of looks suspect:
+ call_method("isa",G_DISCARD);
Shouldn't G_DISCARD be G_SCALAR?
+ if (POPi)
+ XSRETURN_YES;
+
+ XSRETURN_UNDEF;
+
+
+ XSRETURN(1);
The last line is unreachable.
Cheers,
-Jan
Chromatic
> I think it would be good to expose the functionality to the XS layer too,
> by providing a public Perl_sv_dos() function, with XS_UNIVERSAL_DOES
> just being a thin XS wrapper around it.
Here's a new version that does that and addresses your other concerns. I'm
not sure I've added Perl_sv_does() quite correctly (and I have to admit that
I still don't understand the proper use of all the XS-related macros), so
there might be some fine tweaking involved. I also admit that Fergal has a
point about possible name collisions.
Still, the tests pass.
-- c
Rafael Garcia-Suarez
Thanks, I applied that as change #28387, with those modifications :
- the new universal method is named DOES(), not does()
- I added the appropriate incantations to make sv_does() recognized as
part of the API.
What I'd like now is a doc patch to UNIVERSAL.pm. You might have clearer
ideas on the use cases than me about that.
Chromatic
> Thanks, I applied that as change #28387, with those modifications :
>
> - the new universal method is named DOES(), not does()
> - I added the appropriate incantations to make sv_does() recognized as
> part of the API.
>
> What I'd like now is a doc patch to UNIVERSAL.pm. You might have clearer
> ideas on the use cases than me about that.
Will do, hopefully by the weekend.
-- c
Nick Ing-Simmons
Just a note in passing as I catch up.
I have a lot of code where new is defined XS side and puts a C struct
or C++ object in a PV. I then use sv_derived_from ob the objects.
Derived classes can add methods (perhaps even by being proxies for
C++ classes derived from C++ class). But another class which
lies about its origins and hasn't created the object via the new
scheme will cause
static_cast<Myclass &>(SvPV(sv)).Method()
and we get SEGVs.
>
>Cheers,
>-Jan
Nick Ing-Simmons
It isn't painful of the XS code is auto-generated.
e.g. I have code which shadows public parts of a C++ class hierarchy
and provides perl method for all the C++ public methods.
(I still hope to make that open source one day.)