feat: add IndexExpr support to constant_fold_expr [1/1]
#19982
Conversation
BobTheBuidler
changed the title
constant_fold_exprconstant_fold_expr [1/1]
BobTheBuidler
changed the title
constant_fold_expr [1/1]constant_fold_expr [1/1]
This comment has been minimized.
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.
This comment has been minimized.
| 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.
| return constant_fold_unary_op(expr.op, value) | ||
| elif isinstance(expr, IndexExpr): | ||
| base = constant_fold_expr(builder, expr.base) | ||
| if base is not None: |
There was a problem hiding this comment.
Similar -- check if base is a Sequence?
There was a problem hiding this comment.
I left out the type check to make it maximally receptive to future constant folding changes
For example, while we can't constant fold a dict value as a real constant, we can constant fold it as an intermediate step of an existing constant fold
This isn't relevant at the moment, as Sequence would catch all existing cases, but it will be relevant in the coming weeks as I keep enhancing our folding capabilities
There was a problem hiding this comment.
In either case the program is still safe to run in its current state, but only with the None check do we get future extensibility built in
|
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? |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| return None | ||
|
|
||
|
|
||
| def folding_candidate( |
There was a problem hiding this comment.
This is a new decorator that lets us deduplicate existing folding code and also sets me up for cleaner implementation of folding transformations of cast and comparison when those PRs are ready
I think you'll likely agree that both of these functions are better suited here in the constant folding file
This comment has been minimized.
This comment has been minimized.
|
IR and run tests are ready to go |
This comment has been minimized.
This comment has been minimized.
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
This PR adds support for IndexExpr in
constant_fold_exprand support for constant folding intransform_index_expr