[move-vm][closures] Refactor: move abilities and closure mask into core types

[wrwg]

Description

[PR 2/n vm closures]

The type AbilitySet is required in MoveTypeLayout, and the type ClosureMask in MoveValue. This PRs moves those both types from file_format into the core types crate.

How Has This Been Tested?

Existing tests

Type of Change

• New feature
• Bug fix
• Breaking change
• Performance improvement
• Refactoring
• Dependency update
• Documentation update
• Tests

Which Components or Systems Does This Change Impact?

• Validator Node
• Full Node (API, Indexer, etc.)
• Move/Aptos Virtual Machine
• Aptos Framework
• Aptos CLI/SDK
• Developer Infrastructure
• Move Compiler
• Other (specify)

[trunk-io]

⏱️ 50m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-move-tests	13m	🟩
rust-move-tests	13m	🟩
rust-move-tests	8m	🟥
rust-cargo-deny	7m	🟩 🟩 🟩 🟩
check-dynamic-deps	3m	🟩 🟩 🟩 🟩
rust-move-tests	2m	🟥
general-lints	2m	🟩 🟩 🟩 🟩
semgrep/ci	1m	🟩 🟩 🟩 🟩
file_change_determinator	46s	🟩 🟩 🟩 🟩
permission-check	12s	🟩 🟩 🟩 🟩
permission-check	12s	🟩 🟩 🟩 🟩
check-branch-prefix	1s	🟩

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

[wrwg]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• [move-vm][closures] Interpreter #15680
• [move-vm][closures] Type and Value representation and serialization #15670
• [move-cm][closures] Refactor: Move type conversions out of Loader into a trait #15669
• [move-vm][closures] Refactor: move abilities and closure mask into core types #15668 👈 (View in Graphite)
• [move-vm][closures] Types and opcodes for closures #15667
• main

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

[georgemitenkov]

This should be used somewhere? Please add tests to closure operations. They are quite easy to unit tests but also easy to get wrong

[wrwg]

This is an interesting methodological disagreement.

I believe unit tests on this atomic level are counter productive. Better one can read the code and verify/"see" its working, as it is now the case. If the code would be so complex you need unit tests, something wrong with it. (As it was with the version in the previous PR.)

Indeed, the feature will be extensively tested by higher-level tests as it is a basic mechanism used in many places.

[wrwg]

PS. Bzgl. MAX_ARGS, I expect it used by the compiler. In the stack machine of the VM it plays no role because we never check "number of arguments correct", this is implicit via stack balancing.

[georgemitenkov]

(2) Regarding number of arguments and MAX_ARGS: let's document that this is implicit via stack balancing. This helps reviews, security reviews, etc. and if someone else comes working on this code one does not need to spend searching through the codebase for all thus implicit checks.

[georgemitenkov]

(1) Regarding unit tests:

I disagree. Unit test exists exactly for this purpose to verify small components work as expected and test corner cases. Good tests act as a spec by itself describing behaviour (there is a reason why TDD was created). Saying "my code works, just read it and see" is not a great practice in my opinion: previous iteration of a mask had a panic on certain inputs - was it expected, unreachable code? These things should be documented and tested explicitly.

For example, since you derive Arbitrary and say that mask.compose(mask.extract(v, true), mask.extract(v, false)) == v should be preserved, a simple proptest doing this would be good. Actually, your extract right now skips elements > 64, so this property does not hold for all v.

[georgemitenkov]

But let's also hear what other folks think: @ziaptos @vgao1996 @runtian-zhou @vineethk

[wrwg]

You want me to actually write a test for this code:

    let mut mask = self.0;
        values
            .iter()
            .filter(|_| {
                let set = mask & 0x1 != 0;
                mask >>= 1;
                set && collect_captured || !set && !collect_captured
            })
            .collect()

Sorry, I won't. The code is already the spec.

Unnecessary unit tests are a kind of technical debt . You come back to change structure or semantics, the tests are a drag. This is a series of PRs where things evolve, and the basic concepts are revisited while solving each layer in the stack.

[wrwg]

(2) Regarding number of arguments and MAX_ARGS: let's document t

Done

[georgemitenkov]

I would strongly argue in favour of unit testing for the mask, but otherwise looks good