-
Notifications
You must be signed in to change notification settings - Fork 41
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
allow partially const tuples #206
base: master
Are you sure you want to change the base?
Conversation
Great PR. Strengthening the constant-folder for tuples is a strict win. Once the stronger constant-folder has a test to prevent regressions, this PR is good to go. |
I actually found a small edge case that this change doesn't cover. Specifically, the following:
The above causes an error because The culprit is that |
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.
Good catch!
I left a suggestion inline.
let term = get(0).cs()[*n].clone(); | ||
Some(term.as_value_opt().cloned().map(const_).unwrap_or(term)) | ||
} | ||
_ => 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.
Another simple solution: just match on Op::Const(Value::Tuple(..))
too.
Some(new_tuple(children)) | ||
|
||
} | ||
_ => 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.
ditto
Currently, the following program fails to compile:
Even though
x.b
is conceptually a const value, the way constant folding works at present makes it so the entire tuple is const or it isn't and sincex.a
is not const,x
is not const. This makes a very useful paradigm of creating values that are "tagged" with e.g. their bit width for the purpose of range checking infeasible. This PR aims to change that.I do want to flag that the fundamental difficulty here (and why it wasn't implemented this way in the first place) is that the constant folding optimization relies on turning a Term into a Value and then calling
eval_op
to do the evaluation. For "partially-const" tuples, there is no such correspondingValue
, so the constant folding code ends up duplicating some logic that is ineval_op
(adapting it to work onTerm
s instead). For tuples this logic is pretty minimal, but for arrays it was less so which is why I did not implement an analogous fix for arrays, even though it would be nice to have that as well.I'm not sure what the "cleanest" way to unify these are (maybe one could introduce a temporary
Value::Phantom
used only in the constant folding optimization to represent the non-const children of aValue::Tuple
? or maybe this is overengineering and I should just duplicate the array evaluation code onTerm
s).