[compiler v2] Fix equalities, FreezeRef conversion #12162
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12162 +/- ##
=======================================
Coverage 64.1% 64.1%
=======================================
Files 792 792
Lines 175986 176059 +73
=======================================
+ Hits 112916 112994 +78
+ Misses 63070 63065 -5 ☔ View full report in Codecov by Sentry. |
vineethk
approved these changes
Feb 22, 2024
| @@ -2672,6 +2672,7 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo | |||
| } | |||
| let mut success = true; | |||
| for (i, arg_ty) in arg_types.iter().enumerate() { | |||
| /* | |||
There was a problem hiding this comment.
Delete commented out code?
brmataptos
reviewed
Feb 24, 2024
| error: cannot mutably borrow local `x` since other references exists | ||
| ┌─ tests/reference-safety/eq_ref.move:16:19 | ||
| │ | ||
| 16 │ &mut x == &mut x; // error expected, will be fixed with new reference safety |
There was a problem hiding this comment.
Do we have an issue to track this problem?
There was a problem hiding this comment.
Actually no error expected here since both args are frozen and then it is allowed.
..._party/move/move-compiler-v2/tests/reference-safety/v1-tests/call_mutual_borrows_invalid.exp
Show resolved
Hide resolved
wrwg
force-pushed
the
wg-freeze
branch
2 times, most recently
from
0324588 to
75f9429
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.
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
approved these changes
Feb 27, 2024
..._party/move/move-compiler-v2/tests/reference-safety/v1-tests/call_mutual_borrows_invalid.exp
Show resolved
Hide resolved
| fn check_borrow_safety(&mut self, temps: &BTreeSet<TempIndex>) { | ||
| fn check_borrow_safety(&mut self, temps_vec: &[TempIndex]) { | ||
| // First check direct duplicates | ||
| for (i, temp) in temps_vec.iter().enumerate() { |
There was a problem hiding this comment.
You could probably do something like:
let temps = BTreeSet<_>::new();
for temp in temps_vec.iter().rev() {
if temps.contains(temp) {
self.exclusive_access_direct_dup_error(temp.clone())
} else {
temps.insert(temp.clone());
}
}
and reduce this from O(N^2) to O(N log N) or so.
Or we can save that for later optimization if needed.
There was a problem hiding this comment.
Sorry I saw only after merge.
I'm certain there is a non-quadratic version, but I can't quite see what you mean, this seems to generate an error for every iteration?
There was a problem hiding this comment.
No, only if we've already seen temp does it yield an error. And we're processing iter().rev() so it's going backwards over the vec just as before. Though actually, now that I think about it, that might not matter unless we are showing the location.
This refactors instrumentation of copy and drop instructions, and checking of abilities in general, closing #11223, which lead us to generate unnecessary copies. Copy instrumentation is now moved after reference safety as borrow information is needed to compute it. The existing ability checking and explicit drop processors have been marged into a new `ability_processor` which deals with instrumenting copies and drops as well as checking ability conformance.
This enables the comparison of references with mixed mutability (`&x == &mut y`), and introduces generation of the `FreezeRef` operation when an argument is widned from mutable to immutable references. The later applies for any kind of function call, not only equalities. Without freeze, certain scenarios produce borrow errors in reference safety and/or the bytecode verifier. Note that this PR is intended to work together with the new reference safety analysis, so some tests do not yet work without it. Fixes #12151 Fixes #11738 Fixes #11434
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
✅ Forge suite
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This enables the comparison of references with mixed mutability (
&x == &mut y), and introduces generation of theFreezeRefoperation when an argument is widned from mutable to immutable references. The later applies for any kind of function call, not only equalities. Without freeze, certain scenarios produce borrow errors in reference safety and/or the bytecode verifier.Note that this PR is intended to work together with the new reference safety analysis, so some tests do not yet work without it.
Fixes #12151
Fixes #11738
Fixes #11434