[compiler-v2] Revise ability checking for type parameters #15006

wrwg · 2024-10-18T16:44:56Z

Description

Move has some unusual semantics for ability checking of type parameters

In most contexts, a type parameter is assumed to have all abilities during checking. Then when the parameter is instantiated, checking takes place.
However, there is one exemption, namely when a type parameter is used to instantiate another type parameter which has an abilitie constraint, as in f<T:drop>. If we instantiate f<R> then R needs to be checked for drop.

This PR solves this by adding a field type_param_exempted to HasAbilities(abilities, type_param_exempted) to the ability constraint.

How Has This Been Tested?

New baseline test, existing tests

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io · 2024-10-18T16:44:59Z

⏱️ 1h 30m total CI duration on this PR

Slowest 15 Jobs	Cumulative Duration	Recent Runs
execution-performance / single-node-performance	21m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-move-tests	9m	🟩
rust-cargo-deny	7m	🟩 🟩 🟩 ⬜ 🟩
check-dynamic-deps	6m	🟩 🟩 🟩 🟩 🟩
rust-doc-tests	5m	🟩
execution-performance / test-target-determinator	4m	🟩
test-target-determinator	4m	🟩
check	4m	🟩
rust-move-tests	3m	⬜
semgrep/ci	2m	🟩 🟩 🟩 🟩 🟩
general-lints	2m	🟩 🟩 🟩 ⬜ 🟩
fetch-last-released-docker-image-tag	2m	🟩
file_change_determinator	55s	🟩 🟩 🟩 🟩 🟩

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

wrwg · 2024-10-18T16:45:16Z

[compiler-v2] Revise ability checking for type parameters #15006 👈
main

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

Join @wrwg and the rest of your teammates on Graphite

brmataptos

Maybe an example where params_exempted is false would be useful to understand this better.

brmataptos · 2024-10-18T17:09:38Z

third_party/move/move-compiler-v2/tests/checking/abilities/bug_14490_field_abilities.exp

Can we also add this test to v1 so we can compare what V1 does with the example?

Argh no. I do not add any tests to the deprecated code. I tested this manually.

So you are asking the reviewers to each go and manually test it to verify that your demonstrated behavior matches V1? That's pretty unhelpful. Please try to keep your emotions out of your work.

brmataptos · 2024-10-18T17:18:02Z

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

-    HasAbilities(AbilitySet),
+    /// The type must have the given set of abilities. The boolean indicates whether
+    /// type parameters are exempted.
+    HasAbilities(AbilitySet, bool),


This needs more explanation, something like

(E.g., for type `S<T>` to have `drop`, ordinarily parameter `T` must also have `drop`, but sometimes (???) we don't require that.)

But I hope you can fill in sometimes (???) because I don't quite get it.

Ok, I made an enum out of this, for clarity.

Much nicer, thanks!

rahxephon89 · 2024-10-18T18:12:40Z

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

@@ -519,7 +532,10 @@ impl Constraint {
            (Constraint::NoReference, Constraint::NoReference) => Ok(true),
            (Constraint::NoTuple, Constraint::NoTuple) => Ok(true),
            (Constraint::NoPhantom, Constraint::NoPhantom) => Ok(true),
-            (Constraint::HasAbilities(a1), Constraint::HasAbilities(a2)) => {
+            (
+                Constraint::HasAbilities(a1, params_exempted1),


nit: maybe change the name params_exempted to params_scope?

Done already

rahxephon89 · 2024-10-18T18:35:50Z

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

@@ -573,23 +592,22 @@ impl Constraint {

    /// Returns the constraints which need to be satisfied for a field type,
    /// given a struct with declared abilities.
-    pub fn for_field(struct_abilities: AbilitySet, field_ty: &Type) -> Vec<Constraint> {
+    pub fn for_field(struct_abilities: AbilitySet, _field_ty: &Type) -> Vec<Constraint> {


Do we still need _field_ty here?

We may in the future, so keeping it.

Closes #14490 Move has some unusual semantics for ability checking of type parameters - In most contexts, a type parameter is assumed to have all abilities during checking. Then when the parameter is instantiated, checking takes place. - However, there is one exemption, namely when a type parameter is used to instantiate another type parameter which has an abilitie constraint, as in `f<T:drop>`. If we instantiate `f<R>` then `R` needs to be checked for `drop`. This PR solves this by adding a field `type_param_exempted` to `HasAbilities(abilities, type_param_exempted)` to the ability constraint.

github-actions · 2024-10-18T19:29:50Z

✅ Forge suite `realistic_env_max_load` success on `f732c04c870658cbea246d07343f797123432ffc`

two traffics test: inner traffic : committed: 11584.24 txn/s, submitted: 11585.66 txn/s, expired: 1.42 txn/s, latency: 3418.83 ms, (p50: 3000 ms, p70: 3300, p90: 4200 ms, p99: 12300 ms), latency samples: 4404600
two traffics test : committed: 100.02 txn/s, latency: 2406.71 ms, (p50: 2300 ms, p70: 2500, p90: 2800 ms, p99: 10100 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.240, avg: 0.215", "QsPosToProposal: max: 0.825, avg: 0.655", "ConsensusProposalToOrdered: max: 0.340, avg: 0.326", "ConsensusOrderedToCommit: max: 0.461, avg: 0.441", "ConsensusProposalToCommit: max: 0.793, avg: 0.767"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 0.69s no progress at version 2603705 (avg 0.24s) [limit 15].
Max epoch-change gap was: 1 rounds at version 2242419 (avg 1.00) [limit 4], 8.33s no progress at version 2242419 (avg 8.33s) [limit 15].
Test Ok

github-actions · 2024-10-18T19:30:21Z

✅ Forge suite `compat` success on `b29f09f57e898d8d211c8bc3e303f6e50bba2266` ==> `f732c04c870658cbea246d07343f797123432ffc`

Compatibility test results for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> f732c04c870658cbea246d07343f797123432ffc (PR)
1. Check liveness of validators at old version: b29f09f57e898d8d211c8bc3e303f6e50bba2266
compatibility::simple-validator-upgrade::liveness-check : committed: 13172.79 txn/s, latency: 2314.04 ms, (p50: 1800 ms, p70: 2000, p90: 2400 ms, p99: 24400 ms), latency samples: 522180
2. Upgrading first Validator to new version: f732c04c870658cbea246d07343f797123432ffc
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 4333.02 txn/s, latency: 6461.89 ms, (p50: 4100 ms, p70: 9500, p90: 11800 ms, p99: 12600 ms), latency samples: 83160
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6678.15 txn/s, latency: 4798.53 ms, (p50: 4900 ms, p70: 5100, p90: 6700 ms, p99: 7200 ms), latency samples: 223060
3. Upgrading rest of first batch to new version: f732c04c870658cbea246d07343f797123432ffc
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6589.67 txn/s, latency: 4240.36 ms, (p50: 4600 ms, p70: 4800, p90: 5700 ms, p99: 6000 ms), latency samples: 131720
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 6940.96 txn/s, latency: 4642.57 ms, (p50: 4800 ms, p70: 5200, p90: 6500 ms, p99: 7000 ms), latency samples: 233160
4. upgrading second batch to new version: f732c04c870658cbea246d07343f797123432ffc
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 11006.85 txn/s, latency: 2490.42 ms, (p50: 2700 ms, p70: 2900, p90: 3000 ms, p99: 3400 ms), latency samples: 193560
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 11478.02 txn/s, latency: 2740.55 ms, (p50: 2600 ms, p70: 2700, p90: 3200 ms, p99: 4700 ms), latency samples: 375300
5. check swarm health
Compatibility test for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> f732c04c870658cbea246d07343f797123432ffc passed
Test Ok

github-actions · 2024-10-18T19:31:51Z

✅ Forge suite `framework_upgrade` success on `b29f09f57e898d8d211c8bc3e303f6e50bba2266` ==> `f732c04c870658cbea246d07343f797123432ffc`

Compatibility test results for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> f732c04c870658cbea246d07343f797123432ffc (PR)
Upgrade the nodes to version: f732c04c870658cbea246d07343f797123432ffc
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1328.27 txn/s, submitted: 1330.73 txn/s, failed submission: 2.46 txn/s, expired: 2.46 txn/s, latency: 2309.20 ms, (p50: 2100 ms, p70: 2400, p90: 3600 ms, p99: 5300 ms), latency samples: 118780
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1156.67 txn/s, submitted: 1160.63 txn/s, failed submission: 3.96 txn/s, expired: 3.96 txn/s, latency: 2557.61 ms, (p50: 2400 ms, p70: 2700, p90: 4200 ms, p99: 6100 ms), latency samples: 105140
5. check swarm health
Compatibility test for b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> f732c04c870658cbea246d07343f797123432ffc passed
Upgrade the remaining nodes to version: f732c04c870658cbea246d07343f797123432ffc
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1252.46 txn/s, submitted: 1253.62 txn/s, failed submission: 1.16 txn/s, expired: 1.16 txn/s, latency: 2489.35 ms, (p50: 2100 ms, p70: 2600, p90: 3900 ms, p99: 6300 ms), latency samples: 107940
Test Ok

wrwg requested review from brmataptos, fEst1ck, rahxephon89 and vineethk October 18, 2024 16:45

brmataptos reviewed Oct 18, 2024

View reviewed changes

wrwg force-pushed the wrwg/field_abilities branch from dfa8f11 to 2b45e76 Compare October 18, 2024 18:07

wrwg requested a review from brmataptos October 18, 2024 18:07

brmataptos approved these changes Oct 18, 2024

View reviewed changes

rahxephon89 approved these changes Oct 18, 2024

View reviewed changes

wrwg added 2 commits October 18, 2024 11:57

Adding enum AbilityCheckingScope to make the change clearer.

f732c04

wrwg force-pushed the wrwg/field_abilities branch from 2b45e76 to f732c04 Compare October 18, 2024 19:00

wrwg enabled auto-merge (squash) October 18, 2024 19:00

This comment has been minimized.

Sign in to view

wrwg merged commit 87348fc into main Oct 18, 2024
86 of 92 checks passed

wrwg deleted the wrwg/field_abilities branch October 18, 2024 19:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[compiler-v2] Revise ability checking for type parameters #15006

[compiler-v2] Revise ability checking for type parameters #15006

wrwg commented Oct 18, 2024

trunk-io bot commented Oct 18, 2024 •

edited

Loading

wrwg commented Oct 18, 2024

brmataptos left a comment

brmataptos Oct 18, 2024

wrwg Oct 18, 2024

brmataptos Oct 18, 2024

brmataptos Oct 18, 2024

wrwg Oct 18, 2024

brmataptos Oct 18, 2024

rahxephon89 Oct 18, 2024

wrwg Oct 18, 2024

rahxephon89 Oct 18, 2024

wrwg Oct 18, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

github-actions bot commented Oct 18, 2024

[compiler-v2] Revise ability checking for type parameters #15006

[compiler-v2] Revise ability checking for type parameters #15006

Conversation

wrwg commented Oct 18, 2024

Description

How Has This Been Tested?

Type of Change

Which Components or Systems Does This Change Impact?

trunk-io bot commented Oct 18, 2024 • edited Loading

wrwg commented Oct 18, 2024

brmataptos 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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Oct 18, 2024

✅ Forge suite realistic_env_max_load success on f732c04c870658cbea246d07343f797123432ffc

github-actions bot commented Oct 18, 2024

✅ Forge suite compat success on b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> f732c04c870658cbea246d07343f797123432ffc

github-actions bot commented Oct 18, 2024

✅ Forge suite framework_upgrade success on b29f09f57e898d8d211c8bc3e303f6e50bba2266 ==> f732c04c870658cbea246d07343f797123432ffc

trunk-io bot commented Oct 18, 2024 •

edited

Loading

✅ Forge suite `realistic_env_max_load` success on `f732c04c870658cbea246d07343f797123432ffc`

✅ Forge suite `compat` success on `b29f09f57e898d8d211c8bc3e303f6e50bba2266` ==> `f732c04c870658cbea246d07343f797123432ffc`

✅ Forge suite `framework_upgrade` success on `b29f09f57e898d8d211c8bc3e303f6e50bba2266` ==> `f732c04c870658cbea246d07343f797123432ffc`