PyFrame_LocalsToFast misbehaving when there is a comprehension with colliding local variable

[kycutler]

Bug report

Bug description:

The following fails in Python 3.12 (but not 3.11 or 3.13):

# Setup: in global scope, use a comprehension with colliding variable name
foo = None
[foo for foo in [0]]

# Repro: assigning to frame.f_locals and then merging to fast works only the first time
import inspect, ctypes
frame = inspect.currentframe()

frame.f_locals['a'] = 1
ctypes.pythonapi.PyFrame_LocalsToFast(ctypes.py_object(frame), ctypes.c_int(0))
print(a)  # 1

frame.f_locals['b'] = 2
ctypes.pythonapi.PyFrame_LocalsToFast(ctypes.py_object(frame), ctypes.c_int(0))
print(b)  # NameError: name 'b' is not defined

This impacts debuggers' ability to assign and evaluate local variables -- see microsoft/debugpy#1849 and microsoft/debugpy#1636.

Possibly related changes:

• gh-105340: include hidden fast-locals in locals() #105715
• gh-108732: include comprehension locals in frame.f_locals #109026

CPython versions tested on:

3.12

Operating systems tested on:

Windows

Linked PRs

• [3.12] gh-130809: Fix PyFrame_LocalsToFast copying the wrong value #130816

[ZeroIntensity]

Hi there! You've discovered the unfortunate truth about f_locals prior to PEP-667. In the reproducer, b isn't even getting set in f_locals:

foo = None
[foo for foo in [0]]

import inspect, ctypes
frame = inspect.currentframe()

frame.f_locals['a'] = 1
print(frame.f_locals['a'])
ctypes.pythonapi.PyFrame_LocalsToFast(ctypes.py_object(frame), ctypes.c_int(0))

frame.f_locals['b'] = 2
print(frame.f_locals['b'])  # KeyError :(

There's probably some way to fix it, but keep in mind that 3.12's final bugfix release is coming up in about a month. I'm not sure we can get this fixed by then, or if it's worth doing at all. (f_locals is notoriously unstable prior to 3.13--we'd probably rather not risk breaking all the existing cases in the last release.)

[gaogaotiantian]

PyFrame_LocalsToFast is an undocumented C API, and people using it should know well about how it works. For example, you are not supposed to call it twice in a frame. Actually, you are not supposed to call it at all, unless you really know what's going on. This is not a bug, the seemingly different behavior compared to 3.11 is due to PEP 709 - which could potentially introduce "local" variables in global scope. If you put the whole thing in local scope (in a function), the behavior would be consistent.

[gaogaotiantian]

Also, the original bug in debugpy is obviously a bug for their debugger. pdb does not have that issue - it can be done, but they are doing it wrong.

[ZeroIntensity]

For example, you are not supposed to call it twice in a frame. Actually, you are not supposed to call it at all, unless you really know what's going on

Ah! There's a reason we got rid of it :)

[kycutler]

Thank you both for the context and quick response!

@gaogaotiantian, I still believe the issue should be considered a bug in Python and not simply misuse by debugpy. Here is a repro that does not utilize the C APIs:

# Setup: in global scope, use a comprehension with colliding variable name
foo = 123
[foo for foo in [0]]

# Import * merges locals to fast internally, triggering the bug. The module here is arbitrary.
from abc import *

# Now modifications to locals (e.g. via exec) do not persist.
exec("b = 1")
print(b) # NameError: name 'b' is not defined

Please consider merging #130816 to fix the issue. I've updated it with a test and blurb as well.

[gaogaotiantian]

No, you can't expect print(b) to work after exec("b = 1") - that's just wrong. If you put it in a function scope, that just doesn't work.

[gaogaotiantian]

I'll take a closer look at the fundamental cause for this issue.

[kycutler]

Thank you. Another effect is that the comprehension variable ends up storing a reference to the outer variable, which can lead to unexpected ref counts / problems with garbage collection:

# Setup: in global scope, use a comprehension with colliding variable name
foo = {}
[foo for foo in [0]]

# Import * merges locals to fast internally, triggering the bug. The module here is arbitrary.
from abc import *

# The hidden `foo` variable now unexpectedly holds a reference to the same object as the outer `foo` variable.
import sys
print(sys.getrefcount(foo)) # 3 in Python 3.12, 2 elsewhere

del foo
# Even after deletion there is still a reference via the hidden variable, so the object can't be cleaned up

[devdanzin]

No, you can't expect print(b) to work after exec("b = 1")

But it does work in 3.13, only gives an error in 3.12, right?

[gaogaotiantian]

But it does work in 3.13, only gives an error in 3.12, right?

That's because 3.13 has PEP 667 which solves everything. We can't backport a PEP to 3.12. Also that won't work in function scope either in 3.13. Overall that's a very bad thing to do.

[gaogaotiantian]

Okay I spent some time investigating this. I think it is a bug, but the fundamental issue is complicated.

PyFrame_LocalsToFast only reads data from frame->f_locals (C), which could be different than frame.f_locals (Python). If there's hidden fast, frame.f_locals returns a new dict which is the combination of frame->f_locals and the hidden fasts. This dict, will not affect PyFrame_LocalsToFast (because it's not frame->f_locals) so changing values on it does not have any impacts.

Therefore, if you get the frame.f_locals inside the inline comprehension somehow, that dict is simply useless.

The second issue, which is what the linked PR fixed, made it worse. With a closer look at the problem, I believe that is an issue, but not the issue. The PR fixed an issue where PyFrame_LocalsToFast tries to merge local value back to hidden fasts from frame->f_locals, and that's just wrong, because frame->f_locals never contains hidden fasts. Then the hidden fast always holds a reference, which caused frame.f_locals always return a useless dict, then the exec() symptom.

This system is just beyond repairing, but the good news is we don't have to - we already replaced it in 3.13. The linked PR actually seemed okay to me. It does not solve the big issue, but does make the current situation better. I think it's pretty safe and explanable. I'll get a second opinion about whether to merge it, but I'm +1 on it.

[picnixz]

I'm removing the pending label, not because I'm +1, but whether we decide to close as planned/not planned or not, we had an argument in favor. I don't have enough insight on that matter to say +1. I would however say: is there some possibility to actually trigger the issue in a production code? while there are issues with poorly written code, is it possible to 1) avoid from X import * to trigger the bug and 2) find a code in the wild that would be broken by this? (and/or partially fixed by the PR)

[gaogaotiantian]

I believe the debugpy PR linked is something out there that's broken - that's why I'm +1 on this. If it's just hypothetical I would probably be +0.

[picnixz]

I see. If that helps then it's probably better to have that fix. Note that I'm leaving for 10 days so I won't be able to interact fast with this issue in the upcoming days.

[gaogaotiantian]

I think I got the blessing from Carl so the fix is good. I'll help @kycutler to finish the PR.

[gaogaotiantian]

Thank you for the PR!