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
xis a cell inside the comprehension, after inlining it would appear inu_cellvarsforf, and thus the compilation offwould insert aMAKE_CELLforxat the start of the function. But we would still compile the non-comprehension part of the function treatingxas a fast-local (emittingLOAD_FASTfor it), sofwould return a cell object containing the value ofx, instead of returningxdirectly.Similar cases were already tested, but only with a write (compiled as
STORE_FAST) toxbefore 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_CELLfor those names (the inlined comprehension will itself emitMAKE_CELLwhere 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_DEREFandSTORE_DEREFfor 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