[Compiler-v2] Add ability check on number constraint #15346

rahxephon89 · 2024-11-21T00:21:02Z

Description

This PR:

use specialize_with_defaults instead of specialize when instantiating types for all all struct patterns in the block expression so that the default type can be inferred from constraints.
When adding constraints for a variable type, do not allow the case that it has key ability and be a number. To achieve this goal, a new variant MissingAbilitiesForConstraints is added to TypeUnificationError.

Close #14629

How Has This Been Tested?

existing tests;
Added two new test cases.

Key Areas to Review

Whether it is OK to add the check between two different types of constraints.

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

I have read and followed the CONTRIBUTING doc
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I identified and added all stakeholders and component owners affected by this change as reviewers
I tested both happy and unhappy path of the functionality
I have made corresponding changes to the documentation

trunk-io · 2024-11-21T00:21:05Z

⏱️ 2h 58m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
rust-move-tests	29m	🟥 🟥 🟩
execution-performance / single-node-performance	23m	🟩
rust-targeted-unit-tests	18m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-move-tests	12m	🟩
rust-cargo-deny	11m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	8m	🟩 🟩 🟩 🟩 🟩 (+2 more)
rust-doc-tests	5m	🟩
execution-performance / test-target-determinator	4m	🟩
test-target-determinator	4m	🟩
check	4m	🟩
general-lints	3m	🟩 🟩 🟩 🟩 🟩 (+1 more)

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

rahxephon89 · 2024-11-21T00:45:30Z

...ove/move-compiler-v2/tests/ability-check/v1-typing/module_call_constraints_not_satisfied.exp

@@ -11,16 +11,11 @@ error: type `S` is missing required ability `key` (type was inferred)
   │
   = required by instantiating type parameter `R:key` of function `both`

-error: type `Coin` is missing required ability `copy` (type was inferred)
-   ┌─ tests/ability-check/v1-typing/module_call_constraints_not_satisfied.move:29:9
+error: constraint `integer` does not have required ability `key`


The original error is shadowed by the new error.

Can you add a variant of this test that shows the original error is still there if this one is fixed?

Added a test in the same file

rahxephon89 · 2024-11-21T01:00:49Z

..._party/move/move-compiler-v2/tests/ability-check/v1-typing/pack_constraint_not_satisfied.exp

@@ -11,6 +17,17 @@ error: type `Coin` is missing required ability `drop` (type was inferred)
  │
  = required by instantiating type parameter `T:drop` of struct `S`

+error: type `R<*error*>` is missing required ability `key` (type was inferred)


This is a duplicated error

rahxephon89 · 2024-11-21T05:59:09Z

..._party/move/move-compiler-v2/tests/ability-check/v1-typing/pack_constraint_not_satisfied.exp

+error: constraint `integer` does not have required ability `key`
+  ┌─ tests/ability-check/v1-typing/pack_constraint_not_satisfied.move:7:27
+  │
+7 │         R {r:_ } = R { r: 0 };


V1 has this error

rahxephon89 · 2024-11-21T05:59:18Z

..._party/move/move-compiler-v2/tests/ability-check/v1-typing/pack_constraint_not_satisfied.exp

+error: constraint `integer` does not have required ability `key`
+   ┌─ tests/ability-check/v1-typing/pack_constraint_not_satisfied.move:12:44
+   │
+12 │         R {r: R { r: _ } } = R { r: R { r: 0 }};


V1 has this error

rahxephon89 · 2024-11-21T06:24:54Z

[Compiler-v2] Add ability check on number constraint #15346 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

vineethk

some minor comments

vineethk · 2024-11-22T17:34:18Z

third_party/move/move-model/src/ty.rs

@@ -459,6 +459,7 @@ impl Constraint {
        subs: &mut Substitution,
        loc: &Loc,
        other: &Constraint,
+        ctx_opt: Option<ConstraintContext>,


Please add brief info to the function doc string on how this additional parameter is only used for additional error info.

vineethk · 2024-11-22T17:34:36Z

third_party/move/move-model/src/ty.rs

@@ -543,6 +544,32 @@ impl Constraint {
                *a1 = a1.union(*a2);
                Ok(true)
            },
+            // After the above checks on same type of contraint


Suggested change

// After the above checks on same type of contraint

// After the above checks on same type of constraint

vineethk · 2024-11-22T17:41:24Z

third_party/move/move-model/src/ty.rs

@@ -543,6 +544,32 @@ impl Constraint {
                *a1 = a1.union(*a2);
                Ok(true)
            },
+            // After the above checks on same type of contraint
+            // Check compatibility between ability and number


Are there other combinations to consider, e.g., references etc?

Could you add a comment about why this particular combination requires the special check?

Added the comment

brmataptos · 2024-11-22T19:02:32Z

third_party/move/move-model/src/builder/exp_builder.rs

@@ -2089,7 +2089,8 @@ impl<'env, 'translator, 'module_translator> ExpTranslator<'env, 'translator, 'mo
                Pattern::Struct(sid, std, variant, patterns) => {
                    let mut new_inst = vec![];
                    for ty in &std.inst {
-                        let nty = subs.specialize(ty);
+                        // use `specialize_with_defaults` to get type info from constraints
+                        let nty: Type = subs.specialize_with_defaults(ty);


I'm a bit concerned about using this here, as _with_defaults it will turn is_integer into u64, though coming up with a bad test case is hard. Please add a test case to pack_constraint_not_satisfied2.move where, instead of 0, we have a literal=MAX_U64+1?

Added a new test

vineethk · 2024-11-22T20:39:58Z

third_party/move/move-model/src/ty.rs

+            // This check is needed because sometime the concrete integer type is not available
+            // TODO: check other combination of constraints may be necessary as well.
+            (Constraint::HasAbilities(a1, _), Constraint::SomeNumber(_)) => {
+                let unsupported_abilities = a1.setminus(AbilitySet::PRIMITIVES);


nit: would be nice if the common duplicate code in these two arms could be refactored out.

I was trying to merge these two arms but could not do it because one requires mutable reference and the other one does not. Once we decide we want to generalize this check to different types of constraints, I will refactor them out.

Rust doesn't allow polymorphism over mutability. :-(

brmataptos · 2024-11-24T19:30:50Z

third_party/move/move-compiler-v2/tests/bytecode-generator/bug_14629.move

+    }
+
+    fun t0_u128() {
+        let y = R { r: X { r: MAX_U64 + 1} };


I'm sorry I was unclear. Inline the number here:

let y = R { r: X { r: 18446744073709551616 } };

so we can see what happens. I would expect that the u128 type is constraining the value in that situation. But maybe a less direct situation is needed.

And now that I ask for this I suddenly realize I'm not sure what Move specifies it will do with a big constant.

This will lead to the error

+Diagnostics: +error: unable to infer instantiation of type `store + u128|u256` (consider providing type arguments or annotating the type) + ┌─ tests/bytecode-generator/bug_14629.move:14:31 + │ +14 │ let y = R { r: X { r: 18446744073709551616 } }; + │ ^^^^^^^^^^^^^^^^^^^^

,which I think should be fine? @brmataptos

Another example:

module 0x8675309::M { const MAX_U64: u64 = 18446744073709551615; fun t0_u128() { let y = R { r: X { r: MAX_U64 + 1 } }; let R { r: _r } = y; }

In general, should the compiler infer the type as u64, which will lead to overflow? I think it should be fine as well because it can only be detected during runtime?

These are both ok. I just wanted to make sure it didn't do something crazy like inferring u64 while looking for abilities and then complaining about it to the user.

brmataptos

LGTM

github-actions · 2024-11-25T06:13:03Z

✅ Forge suite `realistic_env_max_load` success on `795ffeb8b87b69a3cd0f628ab12f1e92396e784b`

two traffics test: inner traffic : committed: 14137.47 txn/s, latency: 2811.54 ms, (p50: 2700 ms, p70: 2700, p90: 3000 ms, p99: 3300 ms), latency samples: 5375360
two traffics test : committed: 99.96 txn/s, latency: 1889.12 ms, (p50: 1400 ms, p70: 1800, p90: 2200 ms, p99: 13500 ms), latency samples: 1720
Latency breakdown for phase 0: ["MempoolToBlockCreation: max: 2.419, avg: 1.492", "ConsensusProposalToOrdered: max: 0.322, avg: 0.294", "ConsensusOrderedToCommit: max: 0.378, avg: 0.363", "ConsensusProposalToCommit: max: 0.669, avg: 0.657"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.55s no progress at version 33976 (avg 0.20s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 15.44s no progress at version 2814217 (avg 15.44s) [limit 16].
Test Ok

github-actions · 2024-11-25T06:15:33Z

✅ Forge suite `compat` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `795ffeb8b87b69a3cd0f628ab12f1e92396e784b`

Compatibility test results for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 795ffeb8b87b69a3cd0f628ab12f1e92396e784b (PR)
1. Check liveness of validators at old version: 94976266c0e4edd1a31a3783be4c4184f8892ff5
compatibility::simple-validator-upgrade::liveness-check : committed: 16733.97 txn/s, latency: 2008.69 ms, (p50: 1900 ms, p70: 2000, p90: 2400 ms, p99: 4200 ms), latency samples: 547680
2. Upgrading first Validator to new version: 795ffeb8b87b69a3cd0f628ab12f1e92396e784b
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 7068.23 txn/s, latency: 4016.41 ms, (p50: 4500 ms, p70: 4800, p90: 5200 ms, p99: 5500 ms), latency samples: 129340
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 7208.69 txn/s, latency: 4494.00 ms, (p50: 4800 ms, p70: 4900, p90: 6000 ms, p99: 6600 ms), latency samples: 239040
3. Upgrading rest of first batch to new version: 795ffeb8b87b69a3cd0f628ab12f1e92396e784b
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 7923.31 txn/s, latency: 3598.23 ms, (p50: 4000 ms, p70: 4200, p90: 4300 ms, p99: 4400 ms), latency samples: 146160
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 8170.11 txn/s, latency: 3975.19 ms, (p50: 4200 ms, p70: 4300, p90: 4400 ms, p99: 4500 ms), latency samples: 271040
4. upgrading second batch to new version: 795ffeb8b87b69a3cd0f628ab12f1e92396e784b
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 12305.98 txn/s, latency: 2250.53 ms, (p50: 2500 ms, p70: 2600, p90: 2700 ms, p99: 2900 ms), latency samples: 214860
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 5082.79 txn/s, submitted: 5082.94 txn/s, expired: 0.14 txn/s, latency: 2632.17 ms, (p50: 2600 ms, p70: 2700, p90: 2800 ms, p99: 4400 ms), latency samples: 390809
5. check swarm health
Compatibility test for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 795ffeb8b87b69a3cd0f628ab12f1e92396e784b passed
Test Ok

github-actions · 2024-11-25T06:16:23Z

✅ Forge suite `framework_upgrade` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `795ffeb8b87b69a3cd0f628ab12f1e92396e784b`

Compatibility test results for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 795ffeb8b87b69a3cd0f628ab12f1e92396e784b (PR)
Upgrade the nodes to version: 795ffeb8b87b69a3cd0f628ab12f1e92396e784b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1424.59 txn/s, submitted: 1428.42 txn/s, failed submission: 3.84 txn/s, expired: 3.84 txn/s, latency: 2141.85 ms, (p50: 1800 ms, p70: 2100, p90: 3300 ms, p99: 4500 ms), latency samples: 126160
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1475.50 txn/s, submitted: 1479.58 txn/s, failed submission: 4.09 txn/s, expired: 4.09 txn/s, latency: 2009.51 ms, (p50: 1800 ms, p70: 2100, p90: 2700 ms, p99: 4800 ms), latency samples: 129940
5. check swarm health
Compatibility test for 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 795ffeb8b87b69a3cd0f628ab12f1e92396e784b passed
Upgrade the remaining nodes to version: 795ffeb8b87b69a3cd0f628ab12f1e92396e784b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1329.13 txn/s, submitted: 1330.91 txn/s, failed submission: 1.78 txn/s, expired: 1.78 txn/s, latency: 2412.80 ms, (p50: 2100 ms, p70: 2500, p90: 4200 ms, p99: 5400 ms), latency samples: 119640
Test Ok

rahxephon89 force-pushed the teng/fix-14629 branch from 15c8e2c to b9355c7 Compare November 21, 2024 00:34

rahxephon89 commented Nov 21, 2024

View reviewed changes

rahxephon89 changed the title ~~[WIP][Compiler-v2] Add ability check on number constraint~~ [Compiler-v2] Add ability check on number constraint Nov 21, 2024

rahxephon89 marked this pull request as ready for review November 21, 2024 00:58

rahxephon89 commented Nov 21, 2024

View reviewed changes

rahxephon89 force-pushed the teng/fix-14629 branch from b9355c7 to ea86c8d Compare November 21, 2024 06:24

rahxephon89 requested review from vineethk, brmataptos, fEst1ck and wrwg November 21, 2024 06:26

rahxephon89 force-pushed the teng/fix-14629 branch from ea86c8d to fd0bc1a Compare November 21, 2024 18:07

vineethk approved these changes Nov 22, 2024

View reviewed changes

brmataptos reviewed Nov 22, 2024

View reviewed changes

rahxephon89 added 2 commits November 22, 2024 12:23

add ability check on number constraints

d33a561

add tests and comments

9671634

rahxephon89 force-pushed the teng/fix-14629 branch from fd0bc1a to 9671634 Compare November 22, 2024 20:23

rahxephon89 requested review from brmataptos and vineethk November 22, 2024 20:24

vineethk approved these changes Nov 22, 2024

View reviewed changes

brmataptos reviewed Nov 24, 2024

View reviewed changes

rahxephon89 requested a review from brmataptos November 24, 2024 23:04

brmataptos approved these changes Nov 25, 2024

View reviewed changes

Merge branch 'main' into teng/fix-14629

795ffeb

rahxephon89 enabled auto-merge (squash) November 25, 2024 05:45

This comment has been minimized.

Sign in to view

rahxephon89 merged commit 04109e7 into main Nov 25, 2024
79 of 92 checks passed

rahxephon89 deleted the teng/fix-14629 branch November 25, 2024 07:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Compiler-v2] Add ability check on number constraint #15346

[Compiler-v2] Add ability check on number constraint #15346

rahxephon89 commented Nov 21, 2024 •

edited

Loading

trunk-io bot commented Nov 21, 2024 •

edited

Loading

rahxephon89 Nov 21, 2024

brmataptos Nov 21, 2024

rahxephon89 Nov 21, 2024

rahxephon89 Nov 21, 2024

rahxephon89 Nov 21, 2024

rahxephon89 Nov 21, 2024

rahxephon89 commented Nov 21, 2024

vineethk left a comment

vineethk Nov 22, 2024

rahxephon89 Nov 22, 2024

vineethk Nov 22, 2024

rahxephon89 Nov 22, 2024

vineethk Nov 22, 2024

rahxephon89 Nov 22, 2024

brmataptos Nov 22, 2024

rahxephon89 Nov 22, 2024

vineethk Nov 22, 2024

rahxephon89 Nov 22, 2024

brmataptos Nov 24, 2024

brmataptos Nov 24, 2024

rahxephon89 Nov 24, 2024

rahxephon89 Nov 24, 2024

brmataptos Nov 25, 2024

brmataptos left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

github-actions bot commented Nov 25, 2024

	// After the above checks on same type of contraint
	// After the above checks on same type of constraint

[Compiler-v2] Add ability check on number constraint #15346

[Compiler-v2] Add ability check on number constraint #15346

Conversation

rahxephon89 commented Nov 21, 2024 • edited Loading

Description

How Has This Been Tested?

Key Areas to Review

Type of Change

Which Components or Systems Does This Change Impact?

Checklist

trunk-io bot commented Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahxephon89 commented Nov 21, 2024

vineethk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brmataptos left a comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Nov 25, 2024

✅ Forge suite realistic_env_max_load success on 795ffeb8b87b69a3cd0f628ab12f1e92396e784b

github-actions bot commented Nov 25, 2024

✅ Forge suite compat success on 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 795ffeb8b87b69a3cd0f628ab12f1e92396e784b

github-actions bot commented Nov 25, 2024

✅ Forge suite framework_upgrade success on 94976266c0e4edd1a31a3783be4c4184f8892ff5 ==> 795ffeb8b87b69a3cd0f628ab12f1e92396e784b

rahxephon89 commented Nov 21, 2024 •

edited

Loading

trunk-io bot commented Nov 21, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `795ffeb8b87b69a3cd0f628ab12f1e92396e784b`

✅ Forge suite `compat` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `795ffeb8b87b69a3cd0f628ab12f1e92396e784b`

✅ Forge suite `framework_upgrade` success on `94976266c0e4edd1a31a3783be4c4184f8892ff5` ==> `795ffeb8b87b69a3cd0f628ab12f1e92396e784b`