GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — GH-93143: Don't turn `LOAD_FAST` into `LOAD_FAST_CHECK` by brandtbucher · Pull Request #99075 · python/cpython

Conversation

@brandtbucher

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

Uh oh!

There was an error while loading. Please reload this page.

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.

  • Issue: Dataflow analysis in the compiler to avoid runtime NULL checks #93143

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
brandtbucher added 6 commits October 19, 2022 23:45
@brandtbucher
Just assign None instead of changing co_code
e0f2459
@brandtbucher
blurb add
6f68d32
@brandtbucher
Feedback from code review
dd52da5
@brandtbucher
fixup
da07230
@brandtbucher
Perform assignments *before* adjusting the stack
555d23e
@brandtbucher
Address code review
4e597b3
@brandtbucher brandtbucher added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Nov 3, 2022
@brandtbucher
brandtbucher requested a review from sweeneyde November 3, 2022 20:04
@brandtbucher brandtbucher self-assigned this Nov 3, 2022
@bedevere-bot bedevere-bot mentioned this pull request Nov 3, 2022
Dataflow analysis in the compiler to avoid runtime NULL checks #93143
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 3, 2022
@brandtbucher
brandtbucher marked this pull request as ready for review November 3, 2022 20:04
@brandtbucher
brandtbucher requested a review from markshannon as a code owner 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
sweeneyde
sweeneyde reviewed Nov 4, 2022
View reviewed changes
Comment thread Lib/test/test_peepholer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@sweeneyde sweeneyde mentioned this pull request Nov 4, 2022
Convert DELETE_FAST to LOAD_FAST_CHECK POP_TOP DELETE_FAST as a size optimization faster-cpython/ideas#490
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
markshannon
markshannon requested changes Nov 7, 2022
View reviewed changes

@markshannon markshannon left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_peepholer.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Core and Builtins/2022-10-19-23-54-43.gh-issue-93143.1wCYub.rst Outdated
@@ -0,0 +1,4 @@
Rather than changing :attr:`~types.CodeType.co_code`, the interpreter will
now display a :exc:`RuntimeWarning` and assign :const:`None` to any fast
locals that are incorrectly left unbound after jumps or :keyword:`del`

@markshannon markshannon Nov 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There aren't necessarily incorrect, so maybe just "locals that are unbound after ..."

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Objects/frameobject.c Outdated
}
value = Py_NewRef(Py_None);
}
Py_XINCREF(value);

@markshannon markshannon Nov 7, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
Py_XINCREF(value);
Py_INCREF(value);

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot removed the awaiting core review label Nov 7, 2022
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Nov 7, 2022
markshannon
markshannon approved these changes Nov 8, 2022
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Nov 8, 2022
@brandtbucher
brandtbucher merged commit c7065ce into python:main Nov 8, 2022
@bedevere-bot bedevere-bot removed the awaiting merge label Nov 8, 2022
@JelleZijlstra JelleZijlstra mentioned this pull request Sep 16, 2024
"RuntimeWarning: assigning None to unbound local": an inscrutable error #124118
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sweeneyde sweeneyde sweeneyde left review comments
@markshannon markshannon markshannon approved these changes

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@brandtbucher @sweeneyde @markshannon @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.