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

[feature] an infix `isa` operator #17200

Closed
leonerd opened this issue Oct 21, 2019 · 39 comments
Closed

[feature] an infix `isa` operator #17200

leonerd opened this issue Oct 21, 2019 · 39 comments

Comments

@leonerd
Copy link
Contributor

@leonerd leonerd commented Oct 21, 2019

It is quite common in Perl code to want to know if a given object instance is derived from a given class. Such code as

if( $obj->isa( "Some::Class" ) ) ...

is however insufficient because it relies on invoking a method on what may not in fact be an object at all. Sufficiently-defensive code solves this by such constructs as:

use Scalar::Util 'blessed';

if( blessed $obj and $obj->isa( "Some::Class" ) ) ...

There are also CPAN-based solutions that try to make this nicer:

use Safe::Isa '$_isa';

if( $obj->$_isa( "Some::Class" ) ) ...

Such an omission in core Perl appears especially unfortunate in the wider context of thoughts around bringing in more of an OO system.

It would be nice if core Perl provided a basic infix operator to perform this test safely. Such an operator could additionally parse a bareword package name correctly in the common case, thus not requiring a "quoted" name:

use feature 'isa';

if( $obj isa Some::Class ) ...

In addition it may be useful to have a does operator that tests ->DOES instead, though I will admit to not fully understanding the subtle semantic difference is between the two.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 21, 2019

It is also for this feature that I suggest the use of extends rather than isa in any sort of proposed class syntax:

class DerivedClass extends BaseClass { ... }
@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 22, 2019

This would also solve the misguided use of UNIVERSAL::isa to accomplish the same "safe" check, while respecting custom isa methods.

@karenetheridge

This comment has been minimized.

Copy link
Member

@karenetheridge karenetheridge commented Oct 22, 2019

So presumably DerivedClass isa BaseClass would be 1. parsable, and 2. return true?

Similarly for: my @classes = qw(DerivedClass BaseClass); $classes[0] isa $classes[1]

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 22, 2019

@karenetheridge Yeah those both seem reasonable and DWIMy without being too magical.

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 23, 2019

Open question: should this have defined behavior for unblessed refs? The pro is this would obviate the other common use of UNIVERSAL::isa (working on both blessed and unblessed refs concisely). The con is that it would then have the same ambiguity as ref and it's a bit of feature creep.

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 23, 2019

One option is to define the behavior on unblessed refs as just "always false".

@haarg

This comment has been minimized.

Copy link
Contributor

@haarg haarg commented Oct 23, 2019

The issue isn't only unblessed refs, but also checking objects against a reftype. $object isa 'HASH'

@simcop2387

This comment has been minimized.

Copy link
Contributor

@simcop2387 simcop2387 commented Oct 23, 2019

I think it shouldn't ever be true with reftypes, simply because you can (though it's a terrible idea) actually have a class/package named 'HASH' and it'd mean a completely different thing than compared to checking for the reftype.

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 23, 2019

Always returning false for unblessed refs and not checking against reftypes, but not dying, would sufficiently replace the need for UNIVERSAL::isa or $_isa, IMO.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 23, 2019

@Grinnz @simcop2387 Indeed. I think $x isa Foo should always be false if $x is not a blessed object reference. That is, all non-refs and all unblessed refs are never isa anything - not even [] isa ARRAY. It must be blessed in order to be isa something.

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 23, 2019

Well, non-refs would be used as class names. undef should probably be specially false.

@xenu

This comment has been minimized.

Copy link
Contributor

@xenu xenu commented Oct 23, 2019

I don't understand why isa "deserves" to be an infix operator while ref does not.

@xenu

This comment has been minimized.

Copy link
Contributor

@xenu xenu commented Oct 23, 2019

On second thought, while ref($foo) eq 'something' is extremely common, it's not the only way how ref can be used, so unlike isa it doesn't translate into an infix operator that easily.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 23, 2019

I've had a go at implementing this. I've decided for now that $x isa ... should only ever be true if $x really is a blessed object; so for now it isn't true on any string - even one that gives the name of a class. That might need some thinking.

It's documented as:

=head2 Class Instance Operator
X<isa operator>

Binary C<isa> evaluates to true when left argument is an object instance of
the class (or a subclass derived from that class) given by the right argument.
If the left argument is not defined, not a blessed object instance, or does
not derive from the class given by the right argument, the operator evaluates
as false. The right argument may give the class either as a barename or a
scalar expression that yields a string class name:

    if( $obj isa Some::Class ) { ... }

    if( $obj isa "Different::Class" ) { ... }
    if( $obj isa $name_of_class ) { ... }

This feature is available from Perl 5.31.6.

Currently working in https://github.com/Perl/perl5/tree/leonerd/isa

Of particular note are the unit tests for the new op in https://github.com/Perl/perl5/blob/leonerd/isa/t/op/isa.t

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 24, 2019

A WIP PR is now open at #17212

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 24, 2019

A still-pending design question: under use strict are barewords on RHS checked at compiletime for being a real package, and rejected if not? Otherwise that's an easy typo waiting to happen

if($obj isa Some::Object::Klas) {  # an obvious typo
@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 24, 2019

I don't see a way to make that conceptually coherent. "a real package" is hard to define to begin with, and I will frequently use isa checks on packages that won't be loaded unless the caller loaded them to make the object.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 24, 2019

@Grinnz Oh well in that case you can always trivially just

if($obj isa "Some::Object::Klas") { ... }

While I can see a convenient parallel between isa and the class-arrow notation of Some::Class->method which is also only checked at runtime, the consequence of failure is a runtime exception rather than silently failing to ever match anything.

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 24, 2019

That makes sense, but honestly would just lead me to always quote them and ignore the bareword feature...

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 24, 2019

What I meant by hard to define, is that unlike a specific method that you expect the class to define, UNIVERSAL methods like isa will always work. So how do you decide if the class "exists"?

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 24, 2019

Mojo::Loader approaches this question by checking for the 'new' method, but that's obviously an assumption it's making.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 24, 2019

So how do you decide if the class "exists"?

Whether gv_stashsv() returns a value or not. It's something XS code can ask that probably pureperl can't.

@Grinnz

This comment has been minimized.

Copy link
Contributor

@Grinnz Grinnz commented Oct 24, 2019

Would that get fooled by e.g. Foo::Bar::Baz being loaded and creating the Foo::Bar:: stash?

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 24, 2019

Yes admittedly, but that's quite a minor typo-detection problem. Though it may be enough to feel odd and inconsistent.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 24, 2019

My PR #17212 is now ready for review/discussion.

@perlpunk perlpunk mentioned this issue Oct 24, 2019
0 of 1 task complete
@pali

This comment has been minimized.

Copy link
Contributor

@pali pali commented Oct 29, 2019

Anyway, why infix operator and not normal prefix-like function isa $obj, 'Some::Class'? I think this should be more readable as it would look like other parts written in perl.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Oct 29, 2019

@pali It's true, there isn't much to be gained from this infix notation if you look at the feature in isolation; it could easily be a function. But with a wider view to considering some sort of typed-dispatch mechanism on a try/catch syntax in core, or some of my thoughts on how to make a "dumb match" to replace the terrible design of smartmatch and given/when, and it starts to become quite useful to have an infix operator that can test if a value is an instance of a certain class type because syntactically it behaves the same as eq or == or =~ when doing so.

@pali

This comment has been minimized.

Copy link
Contributor

@pali pali commented Nov 1, 2019

Personally for me this infix isa operator looks suspicious. As it is word-like operator I would expect that it operates on strings (like eq, ne, cmp, lt, gt, le, ge) and not with references/objects. Function-like syntax looks more perl-lish way and I think more important: easily readable for existing perl developers who do not know that it is a new special infix operator. Also important is that isa is really a function (takes two arguments and returns one boolean). try/catch is something really different as it extends language, really not ordinary function, but rather new language construction.

@xsawyerx

This comment has been minimized.

Copy link
Member

@xsawyerx xsawyerx commented Nov 1, 2019

@iabyn has been reviewing the concept of an isa for signatures. I suggest pausing this to see how this is relevant to that work.

@Leont

This comment has been minimized.

Copy link
Contributor

@Leont Leont commented Nov 4, 2019

@iabyn has been reviewing the concept of an isa for signatures. I suggest pausing this to see how this is relevant to that work.

Holistic design sounds like a good idea.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Nov 11, 2019

I have written some thoughts on a match syntax in #17290 which notes why I would like isa as an infix operator; because it plays the same syntax role as eq, == and =~ which between them form the bulk of the sorts of comparisons people tend to want a match-like syntax to perform. If they all take syntacally identical infix notation, the notation for such a match condition becomes easier to design.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Nov 11, 2019

@pali It's true that isa is an operator whose name is composed of lowercase letters and yet doesn't behave on strings. But also and, or and not are lowercase-letter named and yet perform shortcut boolean semantics; whereas =~ is not named after lowercase letters and yet does care about stringy values - at least, of the LHS. The convention of operators named in lowercase letters correlating to whether they operate on stringy values is already a weak one, broken by several operators (and all of the infix numerical ones; such as sqrt and tan to pick just two).

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Nov 11, 2019

@iabyn has been reviewing the concept of an isa for signatures. I suggest pausing this to see how this is relevant to that work.

@iabyn : Is this available somewhere for me to look at?

@tobyink

This comment has been minimized.

Copy link
Contributor

@tobyink tobyink commented Nov 26, 2019

Add does too?

(Edit: oh, you already suggested that. Ignore me.)

@jkeenan

This comment has been minimized.

Copy link
Contributor

@jkeenan jkeenan commented Dec 10, 2019

Given this commit,

commit 813e85a03dc214f719dc8248bda36156897b0757 (HEAD -> blead, origin/blead, origin/HEAD)
Author:     Paul "LeoNerd" Evans <leonerd@leonerd.org.uk>
AuthorDate: Wed Oct 23 14:00:38 2019
Commit:     Paul "LeoNerd" Evans <leonerd@leonerd.org.uk>
CommitDate: Mon Dec 9 18:19:05 2019

    Add the `isa` operator

... can this ticket be closed?

Thank you very much.
Jim Keenan

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Dec 10, 2019

Ah absolutely it can.

@leonerd leonerd closed this Dec 10, 2019
@pali

This comment has been minimized.

Copy link
Contributor

@pali pali commented Dec 11, 2019

@leonerd Can you add into sv_isa_sv documentation information if sv_isa_sv calls mg_get on its arguments or not? Because currently from documentation added in commit 813e85a it cannot be deduced.

@leonerd

This comment has been minimized.

Copy link
Contributor Author

@leonerd leonerd commented Dec 11, 2019

@pali Ahyes, I forgot to mention that. Done in 11fcdeb

@pali

This comment has been minimized.

Copy link
Contributor

@pali pali commented Dec 11, 2019

@leonerd And what with namesv param? :-) Quick look into code and I think that it calls mg_get() two times.

@toddr toddr removed the Needs Triage label Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.