Skip to content

[mypyc] fix: builder crashes on 3-arg range(x, y, z) if step isnt foldable #20098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
+29 −33
Open

[mypyc] fix: builder crashes on 3-arg range(x, y, z) if step isnt foldable #20098

Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
cf1470b
[mypyc] fix: builder crashes on 3-arg range if step isnt foldable
BobTheBuidler Oct 21, 2025
2b0407b
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 21, 2025
9efe377
Update for_helpers.py
BobTheBuidler Oct 21, 2025
1dbaa15
Update for_helpers.py
BobTheBuidler Oct 21, 2025
a266982
Update for_helpers.py
BobTheBuidler Oct 21, 2025
b5d519c
Update commandline.test
BobTheBuidler Oct 21, 2025
4fda932
Update commandline.test
BobTheBuidler Oct 21, 2025
3ad0bad
Update for_helpers.py
BobTheBuidler Oct 25, 2025
b1c9e3f
Merge branch 'master' into fix-3-arg-range
BobTheBuidler Oct 25, 2025
0bb8e0e
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Oct 25, 2025
57be7b8
Update for_helpers.py
BobTheBuidler Oct 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 24 additions & 32 deletions mypyc/irbuild/for_helpers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,39 +454,31 @@ def make_for_loop_generator(
return for_dict

if isinstance(expr, CallExpr) and isinstance(expr.callee, RefExpr):
if (
is_range_ref(expr.callee)
and (
len(expr.args) <= 2
or (len(expr.args) == 3 and builder.extract_int(expr.args[2]) is not None)
Copy link
Collaborator

@A5rocks A5rocks Oct 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh, why did this fail with an assertion error? (I'm not used to mypyc...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeh, I'm realizing while re-reading this that it doesn't

I discovered this assert when working on #20100 and made this PR as an intermediate step which could be merged in before the full feature.

In hindsight I suppose this is no longer necessary, but I'll leave it open so maintainers can decide if we want to merge it before #20100

)
and set(expr.arg_kinds) == {ARG_POS}
):
# Special case "for x in range(...)".
# We support the 3 arg form but only for int literals, since it doesn't
# seem worth the hassle of supporting dynamically determining which
# direction of comparison to do.
if len(expr.args) == 1:
start_reg: Value = Integer(0)
end_reg = builder.accept(expr.args[0])
else:
start_reg = builder.accept(expr.args[0])
end_reg = builder.accept(expr.args[1])
if len(expr.args) == 3:
step = builder.extract_int(expr.args[2])
assert step is not None
if step == 0:
Copy link
Contributor Author

@BobTheBuidler BobTheBuidler Oct 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we no longer need to check this because we validate that it isn't None or 0 in our new if-check. 0 now falls back to the standard implementation where it fails normally

builder.error("range() step can't be zero", expr.args[2].line)
else:
step = 1
num_args = len(expr.args)

for_range = ForRange(builder, index, body_block, loop_exit, line, nested)
for_range.init(start_reg, end_reg, step)
return for_range
if is_range_ref(expr.callee) and set(expr.arg_kinds) == {ARG_POS}:
# Special case "for x in range(...)".
# NOTE We support the 3 arg form but only when `step` is constant-
# foldable, since it doesn't seem worth the hassle of supporting
# dynamically determining which direction of comparison to do.
# If we cannot constant fold `step`, we just fallback to stdlib range.
if num_args <= 2 or (num_args == 3 and builder.extract_int(expr.args[2])):
if num_args == 1:
start_reg: Value = Integer(0)
end_reg = builder.accept(expr.args[0])
step = 1
else:
start_reg = builder.accept(expr.args[0])
end_reg = builder.accept(expr.args[1])
step = 1 if num_args == 2 else cast(int, builder.extract_int(expr.args[2]))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cast makes me feel really uneasy - it looks correct, but I'd be much more comfortable with an assert on a separate line despite the big readability penalty.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe just assert isinstance(step, int) after that - this way we'll know for sure that the cast was correct at that time

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed


for_range = ForRange(builder, index, body_block, loop_exit, line, nested)
for_range.init(start_reg, end_reg, step)
return for_range

elif (
expr.callee.fullname == "builtins.enumerate"
and len(expr.args) == 1
and num_args == 1
and expr.arg_kinds == [ARG_POS]
and isinstance(index, TupleExpr)
and len(index.items) == 2
Expand All @@ -500,10 +492,10 @@ def make_for_loop_generator(

elif (
expr.callee.fullname == "builtins.zip"
and len(expr.args) >= 2
and num_args >= 2
and set(expr.arg_kinds) == {ARG_POS}
and isinstance(index, TupleExpr)
and len(index.items) == len(expr.args)
and len(index.items) == num_args
):
# Special case "for x, y in zip(a, b)".
for_zip = ForZip(builder, index, body_block, loop_exit, line, nested)
Expand All @@ -512,7 +504,7 @@ def make_for_loop_generator(

if (
expr.callee.fullname == "builtins.reversed"
and len(expr.args) == 1
and num_args == 1
and expr.arg_kinds == [ARG_POS]
and is_sequence_rprimitive(builder.node_type(expr.args[0]))
):
Expand Down
Loading