-
-
Notifications
You must be signed in to change notification settings - Fork 3k
[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.
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
base: master
Are you sure you want to change the base?
Conversation
range(x, y, z)
if step isnt foldable
if len(expr.args) == 3: | ||
step = builder.extract_int(expr.args[2]) | ||
assert step is not None | ||
if step == 0: |
There was a problem hiding this comment.
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
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) |
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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
3-arg call to
builtins.range
currently crashes mypyc if the 3rd argument's Expression isn't constant-foldable, withAssertionError
with no message.This PR fixes it.
I also got rid of some builder error which doesn't seem necessary but can put it back if it still serves a purpose even with the new generic fallback.