[do-not-merge] apply the updated movefmt tool to framework code #13342

rahxephon89 · 2024-05-20T03:11:30Z

Description

This PR shows the result after applying the newest movefmt to the framework code.

trunk-io · 2024-05-20T03:11:34Z

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

Job	Cumulative Duration	Recent Runs
rust-move-unit-coverage	33m	⬜ ⬜ 🟩
rust-targeted-unit-tests	31m	⬜ ⬜ 🟩
rust-move-tests	21m	⬜ ⬜ 🟩
rust-lints	14m	⬜ 🟩 🟩
run-tests-main-branch	12m	🟩 🟩 🟩
general-lints	6m	🟩 🟩 🟩
check-dynamic-deps	4m	🟩 🟩 🟩
semgrep/ci	1m	🟩 🟩 🟩
file_change_determinator	32s	🟩 🟩 🟩
file_change_determinator	31s	🟩 🟩 🟩
permission-check	9s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩
permission-check	8s	🟩 🟩 🟩

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

codecov · 2024-05-20T03:45:28Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 57.5%. Comparing base (247d2e1) to head (2d746ed).
Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #13342   +/-   ##
=======================================
  Coverage    57.5%    57.5%           
=======================================
  Files         833      833           
  Lines      198661   198662    +1     
=======================================
+ Hits       114281   114293   +12     
+ Misses      84380    84369   -11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rahxephon89 · 2024-05-22T01:27:09Z

aptos-move/framework/move-stdlib/sources/bit_vector.move

@@ -90,8 +95,8 @@ module std::bit_vector {
    public fun shift_left(bitvector: &mut BitVector, amount: u64) {
        if (amount >= bitvector.length) {
            vector::for_each_mut(&mut bitvector.bit_field, |elem| {
-                *elem = false;
-            });
+                    *elem = false;


This extra indent looks strange to me.

This is because it is nested within two layers of calls, one is lambda and the other is for_each_mut

I see your point but I still think the original code looks better. cc @vineethk

Might be worth checking what rustfmt does in a similar situation.

It is a bit counter-intuitive that there is extra indentation here compared to the original. It would be okay if for some reason the lambda would have to be pushed to the next line, but as is, it does look weird. In general, having 8 spaces (or rather double or more of the default indentation) looks out of place IMHO.

rahxephon89 · 2024-05-22T01:28:11Z

aptos-move/framework/move-stdlib/sources/bit_vector.move

-        if (bit_index >= length(bitvector)) {
-            false
-        } else {
+        if (bit_index >= length(bitvector)) { false }


This does not look correct to me.

rahxephon89 · 2024-05-22T01:29:35Z

aptos-move/framework/move-stdlib/sources/bit_vector.move

+                    invariant index >= start_index;
+                    invariant index == start_index || is_index_set(bitvector, index - 1);
+                    invariant index == start_index || index - 1 < vector::length(bitvector
+                        .bit_field);


This new line does not look correct to me.

rahxephon89 · 2024-05-22T01:31:22Z

aptos-move/framework/move-stdlib/sources/bit_vector.move

        ensures amount < bitvector.length ==>
            (forall i in bitvector.length - amount..bitvector.length: !bitvector.bit_field[i]);
        ensures amount < bitvector.length ==>
-            (forall i in 0..bitvector.length - amount: bitvector.bit_field[i] == old(bitvector).bit_field[i + amount]);
+            (forall i in 0..bitvector.length - amount: bitvector.bit_field[i] == old(
+                    bitvector).bit_field[i + amount]);


This new line does not look correct to me.

rahxephon89 · 2024-05-22T01:34:12Z

aptos-move/framework/move-stdlib/sources/configs/features.move

-        while (vector::length(features) <= byte_index) {
-            vector::push_back(features, 0)
-        };
+        while (vector::length(features) <= byte_index) { vector::push_back(features, 0) };


Squeezing into one line does not look correct to me.

rahxephon89 · 2024-05-22T02:13:12Z

aptos-move/framework/aptos-framework/sources/account.move

            ed25519::signature_to_bytes(&alice_signer_capability_offer_sig),
            0,
            alice_pk_bytes,
-            bob_addr
-        );
+            bob_addr);


Bracket should be in a new line?

rahxephon89 · 2024-05-22T02:15:08Z

aptos-move/framework/aptos-framework/sources/account.spec.move


        let address_map = global<OriginatingAddress>(@aptos_framework).address_map;
        let new_auth_key = from_bcs::deserialize<address>(new_auth_key_vector);

        aborts_if !exists<OriginatingAddress>(@aptos_framework);
        aborts_if !from_bcs::deserializable<address>(account_resource.authentication_key);
-        aborts_if table::spec_contains(address_map, curr_auth_key) &&
-            table::spec_get(address_map, curr_auth_key) != originating_addr;
+        aborts_if table::spec_contains(address_map, curr_auth_key) && table::spec_get(


The original code looks better.

In general, breaking to newline after an operator is preferable to breaking to newline in the middle of a call (like in the original).

rahxephon89 · 2024-05-22T02:15:33Z

aptos-move/framework/aptos-framework/sources/account.spec.move


        let post auth_key = global<Account>(addr).authentication_key;
        ensures auth_key == new_auth_key_vector;

    }

-    spec rotate_authentication_key_with_rotation_capability(
-        delegate_signer: &signer,
+    spec rotate_authentication_key_with_rotation_capability(delegate_signer: &signer,


The original code looks better

rahxephon89 · 2024-05-22T02:16:23Z

aptos-move/framework/aptos-framework/sources/account.spec.move

-        addr != @0x9 &&
-        addr != @0xa
+    spec fun spec_is_framework_address(addr: address): bool {
+        addr != @0x1 && addr != @0x2 && addr != @0x3 && addr != @0x4 && addr != @0x5 && addr


Not sure whether we should format this.

In general, it seems like having a way for the user to specify parts of code that should be left as-is without formatting is likely very important, because a user might have spent a lot of time hand-formatting certain parts of code to read a particular way.

There are two ways to handle this:

Comments above and below the block of code that should not be formatted (// move-format off and // move-format on). This is what is done in a lot of C++ tools. The positives are that it could be applied to an arbitrary portion of the code (eg., for three functions out of several, or a subset of statements within a function). The negative is that it is not really first class to the language.

Have an attribute like #[move-format::skip] on blocks of code that the user wants formatting to be skipped. This is what rustfmt does, however, it has the drawback of requiring lots of annotations or workarounds (see https://stackoverflow.com/questions/67288537/how-can-i-switch-off-rustfmt-for-a-region-of-code-instead-of-a-single-item), and also, we would need to support these attributes (but probably only for compiler v2).

@robinlzw , since option 2 requires support of the attribute, we may want to go with option 1

rahxephon89 · 2024-05-22T02:17:19Z

aptos-move/framework/aptos-framework/sources/aptos_account.move

@@ -184,8 +193,12 @@ module aptos_framework::aptos_account {

    #[test(alice = @0xa11ce, core = @0x1)]
    public fun test_transfer(alice: &signer, core: &signer) {
-        let bob = from_bcs::to_address(x"0000000000000000000000000000000000000000000000000000000000000b0b");
-        let carol = from_bcs::to_address(x"00000000000000000000000000000000000000000000000000000000000ca501");
+        let bob =


The original code looks better.

vineethk · 2024-05-23T00:57:26Z

aptos-move/framework/aptos-framework/sources/account.move

+
+        let curr_auth_key =
+            from_bcs::to_address(offerer_account_resource.authentication_key);
+        let challenge =


The original formatting does not exceed column limit, not sure why stuff after = got moved to the new line.

robinlzw · 2024-05-23T01:47:58Z

For issues such as complex code indentation, which also is not handled flexibly and make sense enough in rustfmt. Our plan is to carry out specialized efforts in the future. I think it shouldn’t affect the overall situation, you can refer to it https://github.com/movebit/movefmt/blob/develop/doc/ChangeLog.md For the code under aptos-core, most formatted versions respect the source code.

vineethk · 2024-05-23T15:59:37Z

aptos-move/framework/aptos-framework/sources/aptos_governance.move

                    min_voting_threshold,
                    required_proposer_stake,
                    voting_duration_secs
-                },
-            )
+                },)


I feel like the symmetry is lost here because ) was moved up. I am pretty sure rustfmt e.g., would put this on the next line.

vineethk · 2024-05-23T16:04:48Z

aptos-move/framework/aptos-framework/sources/aptos_governance.spec.move

-        should_pass: bool,
-    ) {
+    spec vote_internal(voter: &signer, stake_pool: address, proposal_id: u64, voting_power: u64,
+        should_pass: bool,) {


minor nit: ,) looks a bit weird.

rahxephon89 force-pushed the teng/fmt-0519 branch from cc9aa7c to 8def7f9 Compare May 20, 2024 03:14

format

2d746ed

rahxephon89 force-pushed the teng/fmt-0519 branch from 8def7f9 to 2d746ed Compare May 20, 2024 03:21

rahxephon89 requested a review from vineethk May 20, 2024 14:46

rahxephon89 commented May 22, 2024

View reviewed changes

vineethk reviewed May 23, 2024

View reviewed changes

rahxephon89 closed this Jun 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[do-not-merge] apply the updated movefmt tool to framework code #13342

[do-not-merge] apply the updated movefmt tool to framework code #13342

rahxephon89 commented May 20, 2024

trunk-io bot commented May 20, 2024 •

edited

codecov bot commented May 20, 2024

rahxephon89 May 22, 2024

robinlzw May 23, 2024

rahxephon89 May 23, 2024

vineethk May 23, 2024

rahxephon89 May 22, 2024

rahxephon89 May 22, 2024

rahxephon89 May 22, 2024

rahxephon89 May 22, 2024

rahxephon89 May 22, 2024

rahxephon89 May 22, 2024

vineethk May 23, 2024

rahxephon89 May 22, 2024

rahxephon89 May 22, 2024

vineethk May 23, 2024

rahxephon89 May 23, 2024

rahxephon89 May 22, 2024

vineethk May 23, 2024

robinlzw commented May 23, 2024

vineethk May 23, 2024

vineethk May 23, 2024

[do-not-merge] apply the updated movefmt tool to framework code #13342

[do-not-merge] apply the updated movefmt tool to framework code #13342

Conversation

rahxephon89 commented May 20, 2024

Description

trunk-io bot commented May 20, 2024 • edited

codecov bot commented May 20, 2024

Codecov Report

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

Choose a reason for hiding this comment

robinlzw commented May 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trunk-io bot commented May 20, 2024 •

edited