Repository navigation
feat: add IndexExpr support to constant_fold_expr [1/2] - #19982
BobTheBuidler wants to merge 50 commits into
Conversation
Sorry, something went wrong.
This comment has been minimized.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
This comment has been minimized.
JukkaL
left a comment
There was a problem hiding this comment.
Please add some test cases -- the logic is non-trivial so it would be good to have test coverage.
Sorry, something went wrong.
| return constant_fold_unary_op(expr.op, value) | ||
| elif isinstance(expr, IndexExpr): | ||
| base = constant_fold_expr(expr.base, cur_mod_id) | ||
| if base is not None: |
There was a problem hiding this comment.
I think it would be better to check if base is a Sequence -- you can index an int, for example.
Sorry, something went wrong.
|
If we can merge #19930 first I would be able to re-use the same run test, I think that's probably best for the sake of deduplication. Wdyt? |
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
for more information, see https://pre-commit.ci
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Jukka's comment about checking base is a sequence still applies, right?
Sorry, something went wrong.
|
Sequence / dict, yeah. It isn't just Sequence because #19864 enables quasi-folding over dicts |
Sorry, something went wrong.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Sorry, something went wrong.
|
@hauntsaninja did I perhaps misunderstand what you were asking about? Anything still blocking this one? |
Sorry, something went wrong.
|
Yeah, I understood Jukka's suggestion to be to use if isinstance(base, (Sequence, Mapping)) etc to avoid the type ignore comments on L102 and L109 #19982 (comment) |
Sorry, something went wrong.
This PR adds support for IndexExpr in constant_fold_expr and support for constant folding in transform_index_expr