8000 gh-110543: Fix CodeType.replace in presence of comprehensions by JelleZijlstra · Pull Request #110586 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-110543: Fix CodeType.replace in presence of comprehensions #110586

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 8, 2023
Merged

gh-110543: Fix CodeType.replace in presence of comprehensions #110586

merged 5 commits into from
Nov 8, 2023

Conversation

JelleZijlstra
Copy link
Member
@JelleZijlstra JelleZijlstra commented Oct 10, 2023 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Oct 10, 2023
Open
@CyberQin
Copy link

is it planned at 3.12.1? the only problem for pyinstaller + scipy user

@JelleZijlstra
Copy link
Member Author

I would like it to be but someone has to review it.

@JelleZijlstra JelleZijlstra requested a review from carljm November 3, 2023 03:52
@carljm
Copy link
Member
carljm commented Nov 8, 2023

Thanks @JelleZijlstra! I am back from vacation and looking at this issue/PR.

@JelleZijlstra
Copy link
Member Author

Thanks! This solution works but I'm not really happy with it; if you have a better idea, that'd be great.

@carljm
Copy link
Member
carljm commented Nov 8, 2023 •
edited
Loading

I think the solution that would be most in-line with the current design of code objects would be to add a co_fasthidden accessor on code objects, and corresponding new argument to both code_new and code_replace. If the new argument is not provided, it would fall back to marking the same names as "fast hidden" that were marked as such in the original code object (just like it currently does for co_freevars etc.) This is what I would have done in the first place in 3.12, if I'd realized the issue with code.replace().

I think this is better because it places full control into the hands of callers of code.replace() (and creators of new synthetic code objects), rather than implicitly tying CO_FAST_HIDDEN to LOAD_FAST_AND_CLEAR in the bytecode. This allows creation of broken code objects, of course, but there are already many ways to create broken code objects.

The main unpleasant consequence here would be the need to add a new PyUnstable_Code_NewWithFastHidden API (maybe technically we are allowed to modify the signature of an existing PyUnstable_*, but adding a new function seems much nicer than breaking users of the existing function, especially in a bugfix release).

But adding to the signature of code_new and code_replace in a bugfix release is not great either, even if it's backward compatible (new optional argument at the end), since it means that users of those APIs who use the new argument in their code would need to check for Python version >= 3.12.1.

So maybe we should take my suggested approach only for 3.13, and use your PR for 3.12? What do you think?

@JelleZijlstra
Copy link
Member Author

That does seem like a better long-term solution, but I'm also hesitant to make that sort of change in the 3.12 branch at this point. So I agree with your suggestion to apply this PR's solution to 3.12 and add new arguments to the constructor for 3.13.

carljm
carljm approved these changes Nov 8, 2023
View reviewed changes
Copy link
Member
@carljm carljm left a comment

Choose a reason for hiding this comment

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

I think the most straightforward way to implement the plan discussed above is to go ahead and land this PR in main, backport it to 3.12, and then I will prepare a separate PR for main to switch to a new constructor/replace argument.

Thank you for putting together this fix!

@carljm carljm added the needs backport to 3.12 only security fixes label Nov 8, 2023
@carljm carljm merged commit 0b718e6 into python:main Nov 8, 2023
@miss-islington-app
Copy link

Thanks @JelleZijlstra for the PR, and @carljm for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 8, 2023
@JelleZijlstra @miss-islington
pythongh-110543: Fix CodeType.replace in presence of comprehensions (p…
cc287b0 
…ythonGH-110586)

(cherry picked from commit 0b718e6)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-app
Copy link
bedevere-app bot commented Nov 8, 2023

GH-111866 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Nov 8, 2023
carljm pushed a commit that referenced this pull request Nov 8, 2023
@miss-islington @JelleZijlstra
[3.12] gh-110543: Fix CodeType.replace in presence of comprehensions (G…
4f976c3 
…H-110586) (#111866)

gh-110543: Fix CodeType.replace in presence of comprehensions (GH-110586)
(cherry picked from commit 0b718e6)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@JelleZijlstra JelleZijlstra deleted the cofasthidden branch November 9, 2023 12:41
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@JelleZijlstra @aisk
pythongh-110543: Fix CodeType.replace in presence of comprehensions (p…
42f7dc3 
…ython#110586)
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@JelleZijlstra @Glyphack
pythongh-110543: Fix CodeType.replace in presence of comprehensions (p…
609a8b1 
…ython#110586)
@JelleZijlstra JelleZijlstra restored the cofasthidden branch September 10, 2024 23:36
@JelleZijlstra JelleZijlstra deleted the cofasthidden branch September 17, 2024 04:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carljm carljm carljm approved these changes

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JelleZijlstra @CyberQin @carljm
0