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 types for None, Ellipsis, and NotImplemented #3408

Merged
merged 1 commit into from
Aug 26, 2023

Conversation

rytheo
Copy link
Contributor

@rytheo rytheo commented Aug 19, 2023

Fixes #3162

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.

Thank you! Looks like a merge conflict and also needs a newsfragment 3408.added.md please

@rytheo
Copy link
Contributor Author

rytheo commented Aug 19, 2023

The singletons don't expose their type objects in the C API. Is it necessary for these types to implement PyTypeInfo?

@davidhewitt
Copy link
Member

davidhewitt commented Aug 19, 2023

Ah yes, looks like they are not part of the stable API and will even be removed in Python 3.13 python/cpython#107030

So the way to implement for None and NotImplemented should be like we implement PySequence and PyMapping. (You're right, we can't implement PyTypeInfo for them.)

@rytheo rytheo force-pushed the builtin-singletons branch 3 times, most recently from 188067a to 4d4769b Compare August 19, 2023 20:32
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.

Thanks, this implementation looks correct to me. Would you be willing to add some tests please?

I think it should be sufficient to check that some arbitrary value like empty dict doesn't downcast to these types (e.g. PyDict::new(py).downcast::<PyEllipsis>().is_err()) and also that downcasting the singletons succeeds (e.g. py.Ellipsis().as_ref(py).downcast().unwrap().is(&py.Ellipsis())).

@rytheo rytheo force-pushed the builtin-singletons branch 2 times, most recently from d9411f6 to 24c6db3 Compare August 20, 2023 13:11
@mejrs
Copy link
Member

mejrs commented Aug 20, 2023

Should we deprecate/remove Py::None in favour of this?

@davidhewitt
Copy link
Member

That's a good question; I'd prefer to leave the py.None() etc APIs unchanged for 0.20 because they'll probably all change anyway in 0.21. Probably we do want to remove py.None() as it's untyped.

So for now, this PR looks good to me, thanks @rytheo!

@davidhewitt davidhewitt added this pull request to the merge queue Aug 23, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 23, 2023
@davidhewitt
Copy link
Member

Ah that's unfortunate, it looks like PyPy doesn't expose PyEllipsis_Type. Probably the most practical solution is to change the PyEllipsis implementation here to expose PyTryFrom instead of PyTypeInfo as well, then the three implementations work the same. (An alternative would be to use #[cfg] to have different CPython and PyPy implementations, but I don't think that's as nice for either maintainers or users.)

@rytheo rytheo force-pushed the builtin-singletons branch 2 times, most recently from 4314f4b to 565ba62 Compare August 23, 2023 23:23
@rytheo
Copy link
Contributor Author

rytheo commented Aug 24, 2023

@davidhewitt the coverage-ubuntu CI failure was caused by some unterminated character literal error. It doesn't seem to be related to my changes. Any ideas?

@davidhewitt
Copy link
Member

Looks like a possible bug in cargo llvm-cov, let's see if rerunning fixes

@davidhewitt
Copy link
Member

Please rebase and CI should be working again.

auto-merge was automatically disabled August 26, 2023 13:51

Head branch was pushed to by a user without write access

Merged via the queue into PyO3:main with commit abc942a Aug 26, 2023
28 checks passed
@rytheo rytheo deleted the builtin-singletons branch August 26, 2023 14:34
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.

Add types for None, Ellipsis and NotImplemented
3 participants