-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
[Compiler-v2] porting more V1 unit tests to V2 #12085
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #12085 +/- ##
=======================================
Coverage 70.2% 70.3%
=======================================
Files 850 850
Lines 189074 189122 +48
=======================================
+ Hits 132901 132975 +74
+ Misses 56173 56147 -26 ☔ View full report in Codecov by Sentry. |
ab2eb95
to
cd36a41
Compare
@@ -0,0 +1,15 @@ | |||
module 0x8675309::A { |
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.
This test is derived from pop_weird to reveal the complete error message from V1.
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.
As the error message shows this is actually a bug, not an error message. Can we check to fix it?
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.
Sure, filed an issue for it: #12099
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.
A fix is added to this PR: aecf7ad
@@ -0,0 +1,7 @@ | |||
module 0x8675309::M { |
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.
This test is derived from pack_reference to reveal the complete error message from V1.
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.
This is actually some kind of a bug. Only checks which happen on stackless bytecode we do not expect to see here. Could you check why this 2nd error is omitted?
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.
2nd error is omitted because each exp_translator has a boolean flag had_errors
to check whether an error is already generated for it. If so, no further error is generated. The corresponding checking is here.
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.
Ideally, we would keep going and also produce errors for unrelated types/expressions. This seems nontrivial to do, at least for someone (me) with little experience with the type checker, but we should file a bug to fix it eventually, since it's definitely better user experience.
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.
A bug is filed: #12117
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.
After removing had_errors or putting it into finalize_types
, duplicated errors are generated in some tests. I suggest we keep this bug open and visit it later.
@@ -0,0 +1,32 @@ | |||
module 0x42::Test { |
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.
This test is derived from vector_with_non_base_type_inferred to reveal the complete error message from V1.
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.
Again the same bug.
cd36a41
to
ce34cac
Compare
@@ -0,0 +1,22 @@ | |||
module 0x42::Test { |
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.
This test is derived from vector_with_non_base_type to reveal the complete error message from V1.
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.
This seems to be the same bug as above, this errors should be produced
ce34cac
to
2f32088
Compare
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.
Great work. None of the cases where errors aren't reported by v2 seem to be related to later phases but actually are bugs. Can we take a closer look whats wrong?
@@ -0,0 +1,7 @@ | |||
module 0x8675309::M { |
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.
This is actually some kind of a bug. Only checks which happen on stackless bytecode we do not expect to see here. Could you check why this 2nd error is omitted?
@@ -18,23 +18,23 @@ module 0x42::m { | |||
|
|||
Diagnostics: | |||
error: Expected a struct type. Global storage operations are restricted to struct types declared in the current module. Found: '#0' | |||
┌─ tests/bytecode-generator/global_invalid.move:4:17 | |||
┌─ tests/bytecode-generator/v1-typing/global_invalid.move:4:17 | |||
│ | |||
4 │ assert!(exists<T>(addr), 0); | |||
│ ^^^^^^^^^^^^^^^ | |||
│ │ | |||
│ Invalid call to exists<#0>. |
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.
The #0
is a bug which we should fix. Lets open one?
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.
I think it is because T
is a generic type that is not instantiated with a concrete type here. Or you mean we should use "T" instead of "#0" in the error message?
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.
Yes, #0
is a bad message. Better to show T
.
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.
A bug is filed: #12116
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.
Fixed in A fix is added to this PR: 1be81cc
@@ -0,0 +1,15 @@ | |||
module 0x8675309::A { |
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.
As the error message shows this is actually a bug, not an error message. Can we check to fix it?
@@ -0,0 +1,22 @@ | |||
module 0x42::Test { |
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.
This seems to be the same bug as above, this errors should be produced
@@ -0,0 +1,32 @@ | |||
module 0x42::Test { |
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.
Again the same bug.
15671c9
to
aecf7ad
Compare
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.
Mostly looks good. We need to file some related bugs for outstanding issues before submitting this.
@@ -0,0 +1,7 @@ | |||
module 0x8675309::M { |
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.
Ideally, we would keep going and also produce errors for unrelated types/expressions. This seems nontrivial to do, at least for someone (me) with little experience with the type checker, but we should file a bug to fix it eventually, since it's definitely better user experience.
┌─ tests/checking/typing/v1-commands/assign_wrong_type.move:6:5 | ||
│ | ||
6 │ x = false | ||
│ ^ |
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.
Please file a bug (low priority) to add context to bugs like this to show why we expected u64
. In this case the declaration is right above, but in other cases it may be far away. We should do better.
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.
filed a bug: #12118
6 │ y = 0; | ||
│ ^ | ||
|
||
error: undeclared `<SELF>_0::y` |
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.
Please file a (low priority) bug to investigate why this message isn't consistent with the above.
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.
filed a bug: #12121
11 │ let T { i, x, b: flag } = t; | ||
│ ^ | ||
|
||
error: undeclared `Test::i` |
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.
We shouldn't be having these follow-on errors. In other cases we would have fewer errors; in this case, we have the following 3 errors that V1 doesn't have. File a bug to investigate.
It also appears in unpack_missing_binding.exp
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.
filed a bug: #12122
third_party/move/move-compiler-v2/tests/checking/typing/vector_with_non_base_type.exp
Show resolved
Hide resolved
error: invalid type instantiation `&u64`: only structs, vectors, and primitive types allowed | ||
┌─ tests/checking/typing/vector_with_non_base_type_inferred.move:4:9 | ||
│ | ||
4 │ vector[&0]; |
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.
This test case, too.
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.
Same as #12117
@@ -1,3 +0,0 @@ | |||
// ---- Model Dump |
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.
What happened to this file? I'm concerned about this. ***
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.
I just moved it so that it is in the same folder with the corresponding move file
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.
Ok, I now see that (in current main, so before your PR): move-compiler-v2/tests/folding/
has
constant_supported_exps.exp
and constant_supported_exps.move
.
while move-compiler-v2/tests/checking/typing/
has just
constant_supported_exps.exp
(is missing the .move
file)
So you are moving everything from /folding/
to /checking/typing/
. At least nothing is gone.
aecf7ad
to
92a5753
Compare
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.
Looks good.
@@ -1,3 +0,0 @@ | |||
// ---- Model Dump |
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.
Ok, I now see that (in current main, so before your PR): move-compiler-v2/tests/folding/
has
constant_supported_exps.exp
and constant_supported_exps.move
.
while move-compiler-v2/tests/checking/typing/
has just
constant_supported_exps.exp
(is missing the .move
file)
So you are moving everything from /folding/
to /checking/typing/
. At least nothing is gone.
a7d67e1
to
d870287
Compare
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.
Have some comments to address (either in this PR, or file as issues to fix later). LGTM otherwise.
@@ -593,7 +593,7 @@ impl<'env> Generator<'env> { | |||
let err_msg = format!( | |||
"Expected a struct type. Global storage operations are restricted to struct types declared in the current module. \ | |||
Found: '{}'", | |||
self.env().get_node_instantiation(id)[0].display(&self.env().get_type_display_ctx()) | |||
self.env().get_node_instantiation(id)[0].display(&self.func_env.get_type_display_ctx()) |
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.
Please also check other calls to get_type_display_ctx()
in this file?
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.
done
@@ -0,0 +1,13 @@ | |||
|
|||
Diagnostics: | |||
error: wrong number of type arguments |
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.
I believe for non-vector generic structs, we additionally provide (expected, got). Ideally we should do the same here. Low priority bug? (it is a minor fix).
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.
fixed
move_to<R>(s, R { f: false }, a); | ||
} | ||
} | ||
// check: POSITIVE_STACK_SIZE_AT_BLOCK_END |
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.
Just curious: were these comments used for anything automated in v1?
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.
yeah, it seems to be related to bytecode verification but I could not find the code to parse and check it.
15 │ 0x2::X::foo(); | ||
│ ------------- called here | ||
|
||
error: function `0x2::X::baz` cannot be called from a scriptbecause it is not public |
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.
Looks like we are missing a space in "scriptbecause".
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.
fixed
c9210a9
to
0103119
Compare
@@ -17,23 +17,23 @@ module 0x42::m { | |||
|
|||
|
|||
Diagnostics: | |||
error: Expected a struct type. Global storage operations are restricted to struct types declared in the current module. Found: '#0' | |||
error: Expected a struct type. Global storage operations are restricted to struct types declared in the current module. Found: 'T' | |||
┌─ tests/bytecode-generator/v1-typing/global_invalid.move:4:17 | |||
│ | |||
4 │ assert!(exists<T>(addr), 0); | |||
│ ^^^^^^^^^^^^^^^ | |||
│ │ | |||
│ Invalid call to exists<#0>. |
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.
There is still #0
used on line 26?
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.
Thanks for catching this. Updated the fix in this commit: 17f8886
third_party/move/move-compiler-v2/tests/checking/typing/vector_with_non_base_type.exp
Show resolved
Hide resolved
error: invalid type instantiation `&u64`: only structs, vectors, and primitive types allowed | ||
┌─ tests/checking/typing/vector_with_non_base_type_inferred.move:4:9 | ||
│ | ||
4 │ vector[&0]; |
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.
Same as #12117
@@ -0,0 +1,111 @@ | |||
============ initial bytecode ================ |
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.
Can you leave all this live-var tests out? This is refactored in the not yet reviewed (sigh) PRs and the tests have to go somewhere else. if we land yours there is a risk we forget.
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.
done
5ba6f41
to
1f9d681
Compare
5442230
to
fa97b8b
Compare
third_party/move/move-compiler-v2/tests/checking/typing/vector_with_non_base_type.exp
Outdated
Show resolved
Hide resolved
fa97b8b
to
780b45e
Compare
} | ||
} | ||
} | ||
|
||
/// Finalize the the given type, producing an error if it is not complete, or if | ||
/// invalid type instantiations are found. | ||
fn finalize_type(&mut self, node_id: NodeId, ty: &Type) -> Type { | ||
fn finalize_type(&mut self, node_id: NodeId, ty: &Type, vector_type: &mut Vec<Type>) -> Type { |
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.
BTreeSet, also call it already_reported
or something
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.
done
Var(_) => { | ||
self.error( | ||
&loc, | ||
&format!( |
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.
You'd report ty
multiple times now if you have more than one type var. Perhaps bring back the incomplete
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.
done
ty.display(&self.type_display_context()) | ||
), | ||
); | ||
let loc = self.parent.parent.env.get_node_loc(node_id); |
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.
Not seeing where you actually assign free type vars with error types to avoid followup errors.
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.
I used another set to store all reported variable types
}, | ||
Struct(_, _, inst) => { | ||
for i in inst { | ||
self.check_valid_instantiation(&loc, i) |
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.
This is not just vectors here too.
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.
done
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.
✅ Forge suite
|
This comment has been minimized.
This comment has been minimized.
✅ Forge suite
|
* Fix `iss`-related bug in Groth16 path & refactor (#12017) Co-authored-by: Oliver <[email protected]> * [aptosvm] Simplify VM flows (#11888) * Duplicated logic for creating the gas meter for view functions has been removed. * Duplicated logic for calculating gas used for view functions has been removed. * There was unreachable code in failure transaction cleanup, where the discarded status has been returned immediately, but then re-checked again. The first check is shifted inside. * No more default transaction metadata. * Scripts are now validated consistently. * Simplifies transaction execution function signature to avoid `Option<String>`. * Removes duplicated features from `AptosVM` and keeps them in `MoveVMExt`. * Fixes a bug when script hash was not computed for `RunOnAbort`. Related tests are moved to `move-e2e-tests`. * [Compiler V2] Critical edge elimination (#11894) Implement a pass to eliminate critical edges by splitting them with empty blocks * [consensus configs] reduce sending block size from 2500 to 1900 (#12091) ### Description The block output limit is no longer hit with p2p txns. ### Test Plan Forge `realistic_env_max_load` TPS improves. * [Indexer-grpc] Add profiling support. (#12034) * Minor aggregator cleanup (#12013) * Minor aggregator cleanup * Addressing PR comments * [move] rotate_authentication_key_call should not modify OriginatingAddress (#12108) Co-authored-by: Alin Tomescu <[email protected]> * [Data Streaming Service] Add dynamic prefetching support * [Data Streaming Service] Add dynamic prefetching unit tests. * [Data Streaming Service] Update existing integration tests. * [State Sync] Add backpressure to fast sync receiver. * Update perf baseline for gas charging coverage improvements (reducing throughput) (#12124) * Reduce latency of cloning network sender using Arc pointers (#12103) * Avoid cloning network sender using Arc pointers * Removing a clone * 100 node sweep test * Removing a few clone operations * reset forge test * Removing some clones * Removing clones * adopt AIP-61 terminology for consistency (#12123) adopt AIP-61 terminology for consistency * [Consensus] Remove non-decoupled execution and refactor for cleaner interfaces (#12104) * fix jwk key logging (#12090) * remove spurious error lines (#12137) * randomness #1: types update from randomnet (#12106) * types update from randomnet * update * lint * lint * All validators broadcast commit vote messages (#12059) * All validators broadcast commit messages * Forge testing * Increase timeout for forge * test forge realistic_env_workload_sweep_test * run realistic_env_workload_sweep_test * run realistic_env_workload_sweep_test * run sweep test * increase forge runner duration * forge testing * Letting the proposer also broadcast commit decision for backward compatibility * removing forge changes * Added a TODO * [vm] Resource access control: runtime engine (#10544) * [vm] Resource access control: runtime engine Implements the runtime engine for resource access control: - a representation of access control specifiers in `loaded_data::runtime_access_specifiers`. - a loader for access specifiers in `runtime::loader::access_specifier_loader`. - a new stateful object representing the access control logic in `runtime::access_control`. - finally the use of the `AccessControlState` in `runtime::interpreter`. * Addressing reviewer comments. * Addressing reviewer comments. * typo: PTLA -> PTAL * Rebasing: adjusting to upstream changes * Rebasing * ObjectCodeDeployment API cleanup update (#12133) * ObjectCodeDeployment API cleanup update (#12141) * [Compiler-v2] porting more V1 unit tests to V2 (#12085) * update tests * fix bug * fix-12116 * fix missing space * add expected got * remove live-var tests * fix had_erros * fix * Enable the max object nesting check (#12129) * Resolved the warning for unused variable (#12157) * update * update * update * update * update * update * update * update * update * update * update * update * update * Squashed commit of the following: commit a50ffec Author: Zhoujun Ma <[email protected]> Date: Thu Feb 22 21:10:12 2024 +0000 lint commit 388350f Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 13:04:28 2024 -0800 update commit 76f7eca Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:56:04 2024 -0800 update commit a663542 Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:54:18 2024 -0800 update commit b439449 Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:34:14 2024 -0800 update commit 3378ceb Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:17:06 2024 -0800 update commit 6cd6685 Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:15:05 2024 -0800 update commit 6d89f37 Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:13:51 2024 -0800 update commit 980f257 Author: zhoujun.ma <[email protected]> Date: Thu Feb 22 12:12:04 2024 -0800 update commit 16e9349 Author: Zhoujun Ma <[email protected]> Date: Thu Feb 22 18:25:08 2024 +0000 lint --------- Co-authored-by: Alin Tomescu <[email protected]> Co-authored-by: Oliver <[email protected]> Co-authored-by: George Mitenkov <[email protected]> Co-authored-by: Zekun Wang <[email protected]> Co-authored-by: Brian (Sunghoon) Cho <[email protected]> Co-authored-by: Guoteng Rao <[email protected]> Co-authored-by: Satya Vusirikala <[email protected]> Co-authored-by: David Wolinsky <[email protected]> Co-authored-by: Josh Lind <[email protected]> Co-authored-by: igor-aptos <[email protected]> Co-authored-by: Sital Kedia <[email protected]> Co-authored-by: Wolfgang Grieskamp <[email protected]> Co-authored-by: Teng Zhang <[email protected]> Co-authored-by: Junkil Park <[email protected]>
Description
This PR ports 34 tests from v1 to v2. To achieve this, remapping of v1-v2 test paths is updated as well. It also closes #12099 and closes #12116
Test Plan
Check updated tests in v2.