use ast::Value equivalence, not enum equality, to do constant folding of constants #12518
Conversation
| if x.len() == y.len() { | ||
| iter::zip(x, y).all(|(value, addr)| { | ||
| if let Value::Address(addr1) = value { | ||
| addr1 == addr |
There was a problem hiding this comment.
Double check if this == is okay (i.e., could there be cases where Addresses are the same at runtime even though they are represented by different enums - that is, "numeric vs. symbolic" and "symbolic vs. symbolic where the symbols are different but resolved to the same address").
There was a problem hiding this comment.
Good idea. I'll change it to make that case incomparable and not constant.
|
I changed PTAL |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12518 +/- ##
========================================
Coverage 63.9% 63.9%
========================================
Files 816 816
Lines 180680 180914 +234
========================================
+ Hits 115548 115783 +235
+ Misses 65132 65131 -1 ☔ View full report in Codecov by Sentry. |
| /// implement the same runtime value, assuming that types match. | ||
| /// | ||
| /// If `Address` values are symbolic and differ, then no answer can be given. | ||
| pub fn equivalent(&self, other: &Value) -> Option<bool> { |
There was a problem hiding this comment.
Appreciate such cleanly written code, makes it easy to reason about.
| @@ -2993,3 +3091,183 @@ impl<'a> fmt::Display for EnvDisplay<'a, Spec> { | |||
| Ok(()) | |||
| } | |||
| } | |||
|
|
|||
| #[test] | |||
There was a problem hiding this comment.
nit: it is more idiomatic (and helpful in the presence of further changes and maintainance) to wrap all this with:
#[cfg(test)]
mod tests {
// code to be wrapped
}
Source: https://doc.rust-lang.org/book/ch11-01-writing-tests.html#the-anatomy-of-a-test-function
There was a problem hiding this comment.
I found I had to add several use statements as well, then I read that page and saw an example using use super::*;, but after I pushed my new code. I suppose explicit use statements provides guidance to users of the code.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
brmataptos
force-pushed
the
brm-issue-10850-fix
branch
from
d7e29ab to
ec22f03
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Symbolic Address inequality does not imply that Addresses are really different at runtime. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…ing about symbolic `Address` values. Do not constant-fold comparisons involving symbolic Addresses.
…re just like Vectors
brmataptos
force-pushed
the
brm-issue-10850-fix
branch
from
7d09e1b to
642f467
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Description
Fixes issue #12516 by using ast::Value equivalence
(newly exposed through function
Value::equivalent()),rather than struct
equality, when constant-folding equality operations.
(
Value::ByteArray(..)andValue::AddressArray(..)values caneach also be represented by
Value::Vector(..)instead).This fixes a failure that shows up in
0x1::ristretto:255test case
test_scalar_from_canonical_bytes.Fixes #12516.
Type of Change
Which Components or Systems Does This Change Impact?
V2 compiler
How Has This Been Tested?
Included thorough unit test for new function
Value::equivalent,and a targeted test case
constant_folding_ristretto.moveforcomparison with V1 and V2 without simplification.
Existing V2 CI tests should show the original problem is fixed.
Key Areas to Review
Consider redundancy and ambiguity of the AST representation, perhaps there are more cases.
Checklist