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

Borrowed<PyType>::name is not consistent #4055

Closed
wyfo opened this issue Apr 7, 2024 · 5 comments
Closed

Borrowed<PyType>::name is not consistent #4055

wyfo opened this issue Apr 7, 2024 · 5 comments

Comments

@wyfo
Copy link
Contributor

wyfo commented Apr 7, 2024

With #[cfg(not(any(Py_LIMITED_API, PyPy)))], Borrowed<'a, '_, PyType>::name implementation delegates to tp_name.
However, tp_name is not consistent.
For pure Python types Borrowed<'a, '_, PyType>::name returns different result if compiled with or without abi3 feature.

Solution

Make Borrowed<'a, '_, PyType>::name follow __name__ behavior, and expose Borrowed<'a, '_, PyType>::module.

I will open a PR.

@adamreichold
Copy link
Member

Make Borrowed<'a, ', PyType>::name follow name behavior, and expose Borrowed<'a, ', PyType>::module.

IIRC, we made the concicous decision to expose tp_name and not follow __name__. However, they should match when using the abi3 feature as we access __module__ and __name__ in that case to "emulate" tp_name.

@wyfo
Copy link
Contributor Author

wyfo commented Apr 7, 2024

The issue is that it's hard (if not impossible) to emulate tp_name, because it's not consistent across types: for list, it's "list", for datetime.date, it's "datetime.date", for asyncio.CancelledError, it's "CancelledError", etc.

With abi3 enabled, so without access to tp_name, I can't find a simple rule to determine how it behaves. That's the point of PEP 737.

That's why I think we cannot expose tp_name like that, as consistency with abi3 is way more important to me.
Do you have another solution?

@adamreichold
Copy link
Member

Do you have another solution?

Isn't using what you implemented as full_name for name basically a solution which fixes the inconsistencies between the builds? Why not just use that for name?

@wyfo
Copy link
Contributor Author

wyfo commented Apr 7, 2024

Isn't using what you implemented as full_name for name basically a solution which fixes the inconsistencies between the builds? Why not just use that for name?

I feel a little stupid 😅 However, we have to consider that most of the calls to this new PyType::name implementation may have to retrieve __module__, making it not so efficient.
That's an argument in favor of my proposal, with an efficient PyType::name implementation, and a less efficient PyType::full_name.

Anyway, I can modify my PR if you judge that it is worth it.

@davidhewitt
Copy link
Member

This got resolved in #4196

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 a pull request may close this issue.

3 participants