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

[kycutler]

Don't update hidden fast locals in PyFrame_LocalsToFast, to make sure we don't write the wrong value if there is a name collision.

• Issue: PyFrame_LocalsToFast misbehaving when there is a comprehension with colliding local variable #130809

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[kycutler]

Not sure about the best place to test this. But validated the issue no longer reproduces, and existing tests pass.

Edit: well I thought they passed... will take a look at the peephole test.

[ZeroIntensity]

See @gaogaotiantian's comment in the issue.

[bedevere-app]

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

[gaogaotiantian]

I took a second look at the fix and I think it's reasonable. However, the test itself is actually a bit misleading because that's just a symptom fixed with this PR - there are a few reason why that worked that way.

As we already removed this system after 3.13, we don't need any regression test in the future. This would be the last bug fix version of 3.12, so even for 3.12 the test would not be helpful. Based on the misleading nature of the test, I would suggest that we simply remove the test. If others have different opinions, we can replace it with a more direct test.

[picnixz]

If others have different opinions, we can replace it with a more direct test.

Is it possible to have a small but direct test? even if 3.12 has its last bug fix, it can still accept security patches and if, for some reasons (agreed I'm playing the Devil's advocate here), those security patches affected in some way or another the test, it would still be useful to have that test. I'm really stressing here that the test should be small, namely few lines. It doesn't hurt to test more unless testing more ends up with a larger and convoluted test.

[gaogaotiantian]

I think a test like

import sys
frame = sys._getframe()
f_locals = frame.f_locals
foo = None
[foo for foo in [0]]
from abc import *
assert frame.f_locals is f_locals

could work - it confirms frame.f_locals is not returning a new dict, which means no hidden fast is discovered.

[gaogaotiantian]

Maybe @carljm can take a quick look at this? I know Carl has been busy recently but hopefully the explanation in #130809 (comment) helps. The code fix itself is also trivial.

[carljm]

Looks fine to me; thank you!

[gaogaotiantian]

Please take a look at the comment about this test :)

[bedevere-app]

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Stale

[kycutler]

Updated, thank you!

[gaogaotiantian]

Hmm, you can't simply assert this in the test case (my code was more like a gist). One thing you could do is same_f_locals = frame.f_locals is f_locals and for the argument of _check_in_scopes, add {"same_f_locals", True}.

Also this definitely needs some comment to explain what happened. You could follow the patterns on the other test cases in the same file. I would suggest add GH-130809 in the comment to refer to the original issue.

For comments put something like

The existence of a hidden fast from list comprehension should not cause frame.f_locals on module level to return a new dict every time it is accessed.

[ZeroIntensity]

Obligatory ping @Yhg1s. The final 3.12 bugfix is coming up, and this is a somewhat risky change to include.

[gaogaotiantian]

Yeah I agree we should probably involve the release manager of 3.12.