gh-104357: fix inlined comprehensions that close over iteration var #104368
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.
This fixes the case where an inlined comprehension has an internal cellvar (i.e. it creates lambdas that close over the comprehension iteration variable), but the same name is also used a fast-local in the outer scope, and is read but never written within the body of the function (so e.g. an argument).
In code like this:
because
x
is a cell inside the comprehension, after inlining it would appear inu_cellvars
forf
, and thus the compilation off
would insert aMAKE_CELL
forx
at the start of the function. But we would still compile the non-comprehension part of the function treatingx
as a fast-local (emittingLOAD_FAST
for it), sof
would return a cell object containing the value ofx
, instead of returningx
directly.Similar cases were already tested, but only with a write (compiled as
STORE_FAST
) tox
before it was referenced, which stored the value directly and overwrote the wrongly-created cell.One approach to fix this would be to track which names are cells only inside an inlined comprehension, and prevent the compiler from emitting
MAKE_CELL
for those names (the inlined comprehension will itself emitMAKE_CELL
where it should, within the comprehension's isolated scope.) But this fix would add additional complexity to the compiler.A simpler fix (implemented here) is to ensure that we always treat such a variable as a cell throughout the outer function too (emitting
LOAD_DEREF
andSTORE_DEREF
for it). This maintains clearer semantics forco_cellvars
(if a name is in there, it is a cell in the main function) and keeps the compiler simpler. The downside is that cells are slightly slower than fast locals, but comprehensions creating lambdas that close over the iteration variable are rare in real code (they don't behave as people usually want them to, given that all the created lambda functions share the same final value in their closure), so I don't think performance is a concern in this edge case.Fixes #104357