Repository navigation
fix: replace a comprehension variable only where it is a whole identifier - #66
Merged
ylluminate merged 1 commit intoNov 13, 2026
Conversation
…fier
A list, set or generator comprehension is lowered to
`.filter(...)`/`.map(...)` (unless a filter holds a walrus), and its
loop variable is turned into V's `it` by replacing the variable's name
in the emitted text. The replacement took every occurrence of those
letters, so with `r` as the variable
`[max([r.turns, r.pred]) for r in rows]` came out as
`rows.map(aititays.max([it.tuitns, it.pited]) oit { panic('!') })`,
`[r + r_tail for r in rows]` as `rows.map(it + it_tail)`, and
`[r for r in rows if r > len("r")]` as
`rows.filter(it > 'it'.len).map(it)`.
replace_comp_identifier() replaces the variable only where it is a
whole identifier, and leaves the text of string literals alone. Inside
a `${...}` interpolation the name is replaced in the expression, but
the type letter of a static format such as `:05d` is left alone. A
target that is not emitted as a single identifier (a tuple or starred
target) is replaced as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
A list, set or generator comprehension is lowered to
.filter(...)/.map(...)(unless a filter holds a walrus, which takes another path), and its loop variable is turned into V'sitby replacing the variable's name in the emitted text. The replacement took every occurrence of those letters. Withras the variable,[max([r.turns, r.pred]) for r in data]came out asdata.map(aititays.max([it.tuitns, it.pited]) oit { panic('!') }),[r + r_tail for r in rows]asrows.map(it + it_tail), and[r for r in rows if r > len("r")]asrows.filter(it > 'it'.len).map(it). The first two don't compile, and the third compiles and prints[3]where Python prints[2, 3].The two replacements now go through
replace_comp_identifier(), which replaces the name only where it is a whole identifier and leaves the text of string literals alone. Inside a${...}interpolation the name is replaced in the expression ([f"r={r}" for r in rows]isrows.map('r=${it}')), but the type letter of a static format such as:dor:05.2fis left alone (withdas the variable,'${7:d}'keeps itsd). The three above becomedata.map(arrays.max([it.turns, it.pred]) or { panic('!') }),rows.map(it + r_tail)androws.filter(it > 'r'.len).map(it).It is still a replacement after the element has been emitted, on purpose. py2v keys several tables on a name as it is emitted (a function's defaults, its
mutparameters, Path variables), so emittingitfor the variable while the element is visited changes what they see:[f() for f in fs]losesf's default argument, for one.What it leaves as it was:
[x.r + r for r in rows]isrows.map(x.it + it)). py2v lowersgetattr(x, r)tox.r, so a name after a dot can be the variable itself, and the text doesn't say which.forclause is still dropped, and a nested comprehension that uses the outer variable still reads the innerit([[r + s for s in [1, 2]] for r in rows]isrows.map([1, 2].map(it + it))).s,case r if r.startswith("a")comes out ass.stasts_with('a')), which is a separate fix.This needed #57 first. Without it,
["{0[r]}".format({"r": r}) for r in ["it"]]would go from right to wrong here: it was only right because the replacement also rewrote the quoted key.Test plan
v run tests/run_tests.vsh: 179 passed, 0 failed, 0 skipped (V 0.5.2 ec510e2, macOS), with the new casecomprehension_identifier_replace. No existing expected file changes.v test py2v_test.vpasses with the new tests ofreplace_comp_identifier().V_MACOS_V3_NO_FALLBACK=1) andv -W run. Of 686 comprehension and lambda programs, 84 that failed to compile or printed the wrong thing before now print what CPython prints (43 and 41), the 202 that matched still match, and 400 match on neither side. Eight of those 400 didn't compile before and now compile and print the wrong thing, for reasons this leaves alone: an enclosing variable namedit(three), the dropped secondfor(two), the nestedit, a set emitted as an array, and a walrus result typedAny. Five other sets (445, 188, 316, 280 and 489 programs) end the same way with and without this change.