◐ Shell
reader mode source ↗
Skip to content

GH-93143: Don't turn LOAD_FAST into LOAD_FAST_CHECK#99075

Merged
brandtbucher merged 6 commits into
python:mainfrom
brandtbucher:maybe-bind-fast-local
Nov 8, 2022
Merged

GH-93143: Don't turn LOAD_FAST into LOAD_FAST_CHECK#99075
brandtbucher merged 6 commits into
python:mainfrom
brandtbucher:maybe-bind-fast-local

Conversation

@brandtbucher

@brandtbucher brandtbucher commented Nov 3, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

The compiler's dataflow analysis for fast locals can be invalidated by jumps or deletions while tracing. When this happens, rather than changing co_code (which users assume to be constant) to use the less-efficient LOAD_FAST_CHECK everywhere, just display a RuntimeWarning and set any problematic locals to None instead.

@brandtbucher brandtbucher added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 3, 2022
@brandtbucher brandtbucher self-assigned this Nov 3, 2022
@bedevere-bot bedevere-bot mentioned this pull request Nov 3, 2022
Closed
@brandtbucher brandtbucher marked this pull request as ready for review November 3, 2022 20:04
@sweeneyde

sweeneyde commented Nov 3, 2022

Copy link
Copy Markdown
Member

I can look closer this evening, but mutating co_code is not ideal, so I think this is probably a good idea.

@markshannon

markshannon commented Nov 4, 2022

Copy link
Copy Markdown
Member

There are two cases when we need to set locals to None.

  1. Deletion of locals
  2. Jumping to arbitrary code

In the first case we can set the local to None and issue the warning cheaply and probably correctly. That's fine.

However when jumping, trying to decide which locals need to be set to None is going to be complex, expensive and error prone.
So I think we should go with @sweeneyde suggestion of setting all the undefined local variables to None.
Either don't issue any warning (as it might be confusing), or maybe issue a generic warning only if any locals were set to None.

@sweeneyde sweeneyde mentioned this pull request Nov 4, 2022
Open
@brandtbucher

brandtbucher commented Nov 4, 2022

Copy link
Copy Markdown
Member Author

I've updated the code to skip scanning the bytecode for LOAD_FAST. Any unbound locals will be assigned None on jumps, and (if so) a single RuntimeWarning will be issued for all of them.

markshannon
markshannon requested changes Nov 7, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

👍
This feels safer and more robust than what we have now.

@bedevere-bot

bedevere-bot commented Nov 7, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@brandtbucher brandtbucher merged commit c7065ce into python:main Nov 8, 2022
@JelleZijlstra JelleZijlstra mentioned this pull request Sep 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sweeneyde sweeneyde

@markshannon markshannon markshannon approved these changes

Assignees

@brandtbucher brandtbucher

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@brandtbucher @sweeneyde @markshannon @bedevere-bot