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 PyType::is_subclass_of and PyAny::is_instance_of #1985

Merged
merged 2 commits into from
Nov 20, 2021
Merged

Conversation

birkenfeld
Copy link
Member

@birkenfeld birkenfeld commented Nov 11, 2021

which get the type to check against as an arguments, as opposed to a compile-time generic type.

Also added some tests for the instance/subclass methods.

Question: is it better to name the new methods like this with _of, or to move the current methods to _of and name the new methods with the conventional arguments more like in Python?

This would also fix the current (slight) inconsistency between PyType::is_instance and PyAny::is_instance_of.

Fixes #1984

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay! Thank you!

If we temporarily disregard backwards compatibility, I wonder if there's still value in keeping is_instance? It's only for boilerplate reduction, right? If so, I wonder how common it is to know the types at compile time.

src/types/typeobject.rs Outdated Show resolved Hide resolved
/// Check whether `obj` is an instance of `self`.
///
/// Equivalent to Python's `isinstance` function.
/// Equivalent to the Python expression `isinstance(obj, self)`.
pub fn is_instance<T: AsPyPointer>(&self, obj: &T) -> PyResult<bool> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also have is_instance_of? Ack that PyAny already has that

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we kept the names consistent, this would be is_instance_of. is_instance is also possible, it would be a static method.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just throw this method away entirely? obj.is_instance(other) and type.is_subclass(base) work in the same order isinstance(obj, other), whereas this type.is_instance(obj) method is the reversed isinstance(obj, type).

Seems like a great way to introduce confusion and bugs, so I'd be up for deprecating (can point users to PyAny::is_instance).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, that's a good point...In Python, you're meant to use issubclass() for types, isinstance() for objects. So I agree with deprecating this method on PyType - it's too confusing of sugar for something you already can achieve.

@birkenfeld
Copy link
Member Author

If we temporarily disregard backwards compatibility, I wonder if there's still value in keeping is_instance? It's only for boilerplate reduction, right? If so, I wonder how common it is to know the types at compile time.

It's quite convenient if you're checking against your own pyclass types. Otherwise you need the PyClass::type_object(py) which is a bit clumsy.

But since we can still break compat, it would be also fine (IMO) to exchange the names.

@Eric-Arellano
Copy link
Contributor

But since we can still break compat, it would be also fine (IMO) to exchange the names.

Imo, that makes sense. I was surprised that is_subclass() did not take any args because it conflicted with my mapping to Python.

This reads naturally to me:

typ1.is_subclass(typ2)
type.is_subclass_of::<MyType>()

While I don't love breaking backwards compatibility, I agree it's less offensive to do so now than to have an unclear API for all future developers, given how much PyO3 seems to be picking up steam.

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 this is definitely a necessary addtion, thanks! In particular actually for PEP 489 (#1982) I suspect we may need to remove the static type objects, so having runtime arguments may become necessary eventually.

Regarding the names - I don't feel strongly either way with which way round is_instance_of and is_instance should be for generic vs dynamic form. I'm inclined to call the new ones is_instance_of and as_subclass_of as already proposed in this PR and avoid making a breaking change given there's not a clear advantage of making the breaking change IMO.

Needs a CHANGELOG entry 😉

/// Check whether `obj` is an instance of `self`.
///
/// Equivalent to Python's `isinstance` function.
/// Equivalent to the Python expression `isinstance(obj, self)`.
pub fn is_instance<T: AsPyPointer>(&self, obj: &T) -> PyResult<bool> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should just throw this method away entirely? obj.is_instance(other) and type.is_subclass(base) work in the same order isinstance(obj, other), whereas this type.is_instance(obj) method is the reversed isinstance(obj, type).

Seems like a great way to introduce confusion and bugs, so I'd be up for deprecating (can point users to PyAny::is_instance).

@Eric-Arellano
Copy link
Contributor

In particular actually for PEP 489 (#1982) I suspect we may need to remove the static type objects, so having runtime arguments may become necessary eventually.

Oh cool!!

I'm inclined to call the new ones is_instance_of and as_subclass_of as already proposed in this PR and avoid making a breaking change given there's not a clear advantage of making the breaking change IMO.

Given that we may need to remove the static implementations for PEP 489, my two cents is to name these methods what you want them to be after that removal happens in the future. I imagine you'd prefer is_instance over is_instance_of - the _of would be unnecessary because there is no ambiguity what it refers to anymore.

That way, you're not going to be adding a ticking deprecation time bomb for users who use the more correct implementation (this PR) from the start.

--

One thing I don't know though is PyO3s approach to deprecations, if you are willing to make completely breaking changes in a release or if you have to have a deprecation cycle first. The latter would complicate this all.

@birkenfeld
Copy link
Member Author

+1 this is definitely a necessary addtion, thanks! In particular actually for PEP 489 (#1982) I suspect we may need to remove the static type objects, so having runtime arguments may become necessary eventually.

Hm, I suspect you'll still need access to it, so either type_object() will need to do some thread-local magic to figure out its current interpreter, or the interpreter info needs to be in the py token?

@birkenfeld
Copy link
Member Author

Ok, review comments addressed, changelog entry added and PyType::is_instance removed. I'm still slightly in favor of switching the names.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still slightly in favor of switching the names.

Looks great! I agree with you here though.

@davidhewitt
Copy link
Member

👍 Ok go for it, let's switch the names. (This can go as a part of 0.16 I guess.)

Hm, I suspect you'll still need access to it, so either type_object() will need to do some thread-local magic to figure out its current interpreter, or the interpreter info needs to be in the py token?

Yes that's definitely possible. It's a bit complex so I've buried my head in the sand about it so far! 😂

One thing I don't know though is PyO3s approach to deprecations, if you are willing to make completely breaking changes in a release or if you have to have a deprecation cycle first. The latter would complicate this all.

We don't have any hard restrictions; I like it if we can give users a helpful migration pathway. Deprecations and compile errors are both ways to achieve that. Here it isn't too hard to figure out what to do to update especially if we write a helpful CHANGELOG message.

@davidhewitt davidhewitt added this to the 0.16 milestone Nov 12, 2021
@birkenfeld
Copy link
Member Author

So this is updated; I also missed PyErr::is_instance at first which is now handled as well.

@@ -123,7 +121,7 @@ To check the type of an exception, you can similarly do:
# use pyo3::prelude::*;
# Python::with_gil(|py| {
# let err = PyTypeError::new_err(());
err.is_instance::<PyTypeError>(py);
Copy link
Member

@davidhewitt davidhewitt Nov 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, just been reminded from this that we probably need to make equivalent changes to PyErr?

Either that or we could deprecate Err::is_instance() too (and advise just using err.pvalue(py).is_instance_of::<PyTypeError>()).

EDIT: Sorry just seen you have actually fixed this! Was worried the above diff was from a blanket rename.

Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@davidhewitt
Copy link
Member

I'll rebase this and then merge it. Thanks!

which get the type to check against as an arguments,
as opposed to a compile-time generic type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PyType.is_subclass does not work with two dynamic PyTypes
3 participants