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

Generated cached builtins from a fixed list rather than looking up in the interpreter #5591

Closed
da-woods opened this issue Aug 5, 2023 · 8 comments · Fixed by #6043
Closed

Comments

@da-woods
Copy link
Contributor

da-woods commented Aug 5, 2023

Just checking Python's builtins isn't right, though. They can change with new Python versions (e.g. when new exceptions are added, like the heap of detailed I/O exceptions in Py3.4), so you'd get different warnings when you run Cython in different Python installations.

We should check against a fixed set of builtin names, and update that regularly.

Originally posted by @scoder in #5589 (comment)

See https://github.com/cython/cython/blob/6776c96f66fc799c62be4d6d4022903a5e36cbb2/Cython/Compiler/Symtab.py#L1186C13-L1186C13 for where we do it.

This is more important in Symtab (where it affects code generation) than in the issue this was first discussed in (where it would effect warnings only)

@SauravChittal
Copy link

Hello, I wanted to help with this issue. I'm new to open source, where would be a good place to start @da-woods ? Thank you!

@da-woods
Copy link
Contributor Author

da-woods commented Aug 8, 2023

It's really just replacing one line of code in Symtab.py (linked in the question). You want to test against a list of class names (should probably live in Cython/Compiler/Builtin.py).

Take the list of names from the most recent version of Python you can

@rakeshcj
Copy link

Hi @da-woods is the issue still open ?. If its still open can you please assign it to me.

@scoder
Copy link
Contributor

scoder commented Jan 28, 2024 via email

@alexandrasp
Copy link
Contributor

hey @scoder / @da-woods

I tried to fix this issue and opened PR #6043 . I added the builtins set in Builtin.py as recommended, but this creates a circular import, so I let the list of class names in Symtab.py. I still need to write some tests or even test this change properly, could you give me some guidance? The default test suite was executed.

@scoder
Copy link
Contributor

scoder commented Feb 29, 2024

I just noticed one issue with this approach. When new builtins are added and users refer to them in their code, we will generate code to look them up at module init time. On Python versions that don't have them, this will fail. Even if users were referring to them conditionally, e.g. based on a Python version check, the module import will still fail because we are always looking up the builtins that we find, not just the ones that are actually (conditionally or not) used at runtime.

Maybe we should have a second list of builtins for which we ignore lookup failures at module init time, and then allow those globals to be NULL and check for that on use.

EDIT: Someone stole my time machine keys. Probably myself, in the future. There's already a uncachable_builtins list in Code.py. I've updated it for Py3.13.

@scoder
Copy link
Contributor

scoder commented Mar 6, 2024

I added the builtins set in Builtin.py as recommended, but this creates a circular import, so I let the list of class names in Symtab.py.

Given that the uncachable_builtins list is in Code.py, I'd say move the complete list of builtins just before that.

@scoder scoder linked a pull request Mar 8, 2024 that will close this issue
@scoder
Copy link
Contributor

scoder commented Mar 8, 2024

Closed by #6043
I moved the actual set from Symtab.py into Code.py in 4bc10b9

@scoder scoder closed this as completed Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants