Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add an `isa` operator #17212

Closed
wants to merge 19 commits into from
Closed

Add an `isa` operator #17212

wants to merge 19 commits into from

Conversation

@leonerd
Copy link
Contributor

leonerd commented Oct 24, 2019

An implementation of the isa operator as per #17200

Outstanding issues:

  • Consider whether barename package names should be looked up into HV*s at compiletime or fail
@leonerd leonerd force-pushed the leonerd/isa branch from d6aaa9a to 59e8baf Oct 24, 2019
@leonerd leonerd force-pushed the leonerd/isa branch 2 times, most recently from a6ac5e5 to 37d6c4a Oct 24, 2019
@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Oct 24, 2019

As a first draft it now stands ready for review and consideration, though there is one outstanding issue of semantics left.

We may want to consider whether barename package names should be looked up into HV*s at compiletime and if not fail to compile, at least under use strict. That might feel like what the user would expect, otherwise typoes in package names go undetected. If they really wanted to specify a late-bound name that is dynamically resolved at runtime they can still just use a quoted string:

if($x isa "Late::Loaded::Package") { ... }

Though on the reverse hand, named package dispatch is only resolved late at runtime and typoes aren't detected at compiletime there either:

Nonexistent::Package->new()

though at least in that latter case the code would die at runtime, rather than just silently evaluating to false.

I'm open to ideas in either direction. Discussion?

@leonerd leonerd changed the title [WIP] Add an `isa` operator Add an `isa` operator Oct 24, 2019
@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Oct 24, 2019

PS. also open to discussion on whether I should squash more of these commits together before merging.

@leonerd leonerd removed the do not merge label Oct 24, 2019
@perlpunk

This comment has been minimized.

Copy link
Member

perlpunk commented Oct 24, 2019

Maybe add a link to #17200 in the PR description so we know what it is about?

@Corion

This comment has been minimized.

Copy link

Corion commented Oct 24, 2019

I don't see the approach of checking the package hash for existence at compile time breaking much existing code, but I can see how it will make things harder if you're trying to make a duck-typing class fit some imaginary class hierarchy.

As an imaginary example, consider using a WWW::Mechanize::Chrome object where WWW::Mechanize is expected. The approach above will enforce WWW::Mechanize being loaded (or faked), so all code replacing WWW::Mechanize::Chrome must come in a BEGIN block before the package with that isa check is even loaded. Otherwise, that replacement can take place at runtime.

One thing against compile-time checking is if an object is optional (consider a cookie jar). Then the module implementing this (HTTP::Cookies) is loaded by the module user and not (necessarily) by the module author. The approach above enforces that the module is always loaded by the author. This brings me into "against" territory.

@toddr toddr added the do not merge label Oct 24, 2019
pp.c Outdated

/* TODO: Support right being an HV ref to the stash directly */

SETs(boolSV(sv_derived_from_sv(left, right, 0)));

This comment has been minimized.

Copy link
@tonycoz

tonycoz Oct 24, 2019

Contributor

Is this correct? Shouldn't it call isa() on the class/object so isa() overrides work?

This comment has been minimized.

Copy link
@leonerd

leonerd Oct 25, 2019

Author Contributor

Ahyes, in the general case almost certainly - and definitely for the DOES version that will be necessary. I was hoping to avoid that for this initial implementation as it's quite a rare case.

I wonder if it would be possible to optimise it a bit. Attempt to resolve the "isa" method and i the likely event that it doesn't exist continue with this optimised path, but if it does exist then invoke that instead via call_sv(). I was hoping to avoid a nested code invocation in the common case that nobody overrides sub isa though.

This comment has been minimized.

Copy link
@leonerd

leonerd Oct 26, 2019

Author Contributor

I've had an attempt at this, but I can't seem to get the nested call_sv() call quite right. It mostly runs but then crashes:

http://paste.scsys.co.uk/586753

if I put various warn() calls in there I can see it mostly gets to the right places, or even the presence of warn() makes it work entirely but then if I remove the warn it stops again :( Can anyone suggest what I'm doing wrong here?

This comment has been minimized.

Copy link
@leonerd

leonerd Oct 26, 2019

Author Contributor

The answer, for posterity, is that the SvTRUE() macro might evaluate its argument more than once, so doing SvTRUE(POPs) is always a bug. It should be written SvTRUEx(POPs) or maybe SvTRUE(TOPs); POPs;. Doing that it works fine.

This comment has been minimized.

Copy link
@leonerd

leonerd Oct 26, 2019

Author Contributor

@tonycoz Now added in f0e6ff8

This comment has been minimized.

Copy link
@xenu

xenu Oct 30, 2019

Contributor

Are there any valid reasons to override the isa method?

This comment has been minimized.

Copy link
@leonerd

leonerd Oct 30, 2019

Author Contributor

@xenu I think it is surely easily possible to find examples on CPAN of people overriding ->isa to do all sorts of odd things. Whether any of those are "legitimate" is open to question. I'm simply trying to provide a more efficient (notation/implementation) of the same behaviour as is usually achieved by just calling that now.

Besides, there's precedent in the overload module to allow all sorts of unary, and binary infix operators between types to be overridden. To not allow this isa in similar fashion would seem strange.

This comment has been minimized.

Copy link
@tonycoz

tonycoz Oct 30, 2019

Contributor

Are there any valid reasons to override the isa method?

Apparently Test::MockObject uses an overridden isa.

This comment has been minimized.

Copy link
@karenetheridge

karenetheridge Dec 1, 2019

Member

I override isa in Object::ForkAware also.

This comment has been minimized.

Copy link
@haarg

haarg Dec 2, 2019

Contributor

Both Moo and Type::Tiny override isa in some places to allow interoperability with Moose.

@haarg

This comment has been minimized.

Copy link
Contributor

haarg commented Oct 25, 2019

Somewhat related, using:

my $f = Nonexistent::Package::;

Will trigger a warning:

Bareword "Nonexistent::Package::" refers to nonexistent package

And return the value "Nonexistent::Package". This op should probably match that behavior at least for packages in that form.

@leonerd leonerd mentioned this pull request Oct 26, 2019
pp.c Outdated
* TODO: Consider if we want a NOUNIVERSAL flag for requesting this in a
* more obvious way
*/
isagv = gv_fetchmeth_pv(SvSTASH(SvRV(left)), "isa", 1, 0);

This comment has been minimized.

Copy link
@dur-randir

dur-randir Oct 29, 2019

Member

Can you use gv_fetchmeth_pvn instead of gv_fetchmeth_pv for a lesser run-time cost?

This comment has been minimized.

Copy link
@leonerd

leonerd Oct 30, 2019

Author Contributor

It seems so. 04987d4

@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Oct 30, 2019

I won't merge this yet as it will collide with #17229 and I'd prefer to see that one in first so I can take advantage of it yet, but aside from that I think this is basically ready now. Any further objections?

@xsawyerx

This comment has been minimized.

Copy link
Member

xsawyerx commented Nov 1, 2019

#17229 has been merged, but as I commented on the original feature request ticket, I want to hear @iabyn's opinion on this.

@iabyn

This comment has been minimized.

Copy link
Contributor

iabyn commented Nov 28, 2019

I am generally happy with this proposal. I am ok with it being a word infix op, if that helps elsewhere. I think that the class name should behave exactly as it does in Foo::Bar->new() for consistency for all permutations of quoted or bare or with trailing '::'. I think it should respect 'isa' method overriding and any dereferencing overloading. I think that the new test file should pass 't/TEST -deparse t/op/isa.t' (this is where the test file is first fed through Deparse then run).

Finally, i would ideally like the main body of pp_isa() to be separated out into a separate function that is part of the API, and which I can then call from signature-handling code to guarantee that it provides exactly the same semantics.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Nov 28, 2019

I am generally happy with this proposal. I am ok with it being a word infix op, if that helps elsewhere. I think that the class name should behave exactly as it does in Foo::Bar->new() for consistency for all permutations of quoted or bare or with trailing '::'. I think it should respect 'isa' method overriding and any dereferencing overloading.

I believe it already does those things; happy to adjust and fix them if not as those sound sensible.

I think that the new test file should pass 't/TEST -deparse t/op/isa.t' (this is where the test file is first fed through Deparse then run).

Ah I can try testing that too.

Finally, i would ideally like the main body of pp_isa() to be separated out into a separate function that is part of the API, and which I can then call from signature-handling code to guarantee that it provides exactly the same semantics.

Yes that sounds a good idea. Do you have a suggestion on what I should name it?

@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Nov 30, 2019

I think that the new test file should pass 't/TEST -deparse t/op/isa.t' (this is where the test file is first fed through Deparse then run).

Looking happy:

$ (cd t; ./TEST -deparse op/isa.t)
------------------------------------------------------------------------------
TESTING DEPARSER
------------------------------------------------------------------------------
t/op/isa ... ok
All tests successful.
Elapsed: 0 sec
u=0.02  s=0.00  cu=0.28  cs=0.00  scripts=1  tests=11
@leonerd leonerd force-pushed the leonerd/isa branch from 4016e2f to 6a02a25 Nov 30, 2019
@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Nov 30, 2019

@iabyn I've added a new function documented thus:

NOTE: this function is experimental and may change or be
removed without notice.


Returns a boolean indicating whether the SV is an object reference and is
derived from the specified class, respecting any C<isa()> method overloading
it may have. Returns false if C<sv> is not a reference to an object, or is
not derived from the specified class.

This is the function used to implement the behaviour of the C<isa> operator.

Not to be confused with the older C<sv_isa> function, which does not use an
overloaded C<isa()> method, nor will check subclassing.

	bool	sv_isa_sv(SV* sv, SV *namesv)

The pp_isa operator is now a trivial wrapper around this.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Nov 30, 2019

Annoyingly there was already an API function called sv_isa without the suffix, whose behaviour doesn't check for derived classes or respects the ->isa method; in fact it is just a C implementation of ref($sv) eq $classname. I've expanded the docs on both these functions a little.

I've also opened #17334 to suggest we look further into that and try to clean up the names somewhat.

@leonerd leonerd removed the do not merge label Nov 30, 2019
@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Nov 30, 2019

I think we're ready for review.

@dmcbride

This comment has been minimized.

Copy link
Contributor

dmcbride commented Dec 1, 2019

bool sv_isa_sv(SV* sv, SV *namesv)

Just a minor comment, but since my job is now 60% code reviews ... inconsistent spacing on the * :)

@leonerd leonerd force-pushed the leonerd/isa branch from 5ea0120 to 7eaa78f Dec 1, 2019
@leonerd

This comment has been minimized.

Copy link
Contributor Author

leonerd commented Dec 1, 2019

bool sv_isa_sv(SV* sv, SV *namesv)

Just a minor comment, but since my job is now 60% code reviews ... inconsistent spacing on the * :)

Huh.. Yeah; embed.fnc is just totally full of such inconsistencies. Some even within the same line. I'll at least settle on something self-consistent with mine, but there's no larger consistency on the file itself unfortunately :(

@leonerd leonerd force-pushed the leonerd/isa branch from c4219f6 to ffa6413 Dec 9, 2019
@jkeenan

This comment has been minimized.

Copy link
Contributor

jkeenan commented Dec 11, 2019

Commit 813e85a, so this p.r. can be closed.

@jkeenan jkeenan closed this Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.