feat: compiler v2 #206
feat: compiler v2 #206
Conversation
WalkthroughThis update introduces dynamic version handling for the Move compiler and language, adds support for external lint checks during package builds, and refines experimental flag handling in the compiler configuration. Numerous native functions across the codebase now suppress Clippy warnings for large error types. Minor comment and formatting improvements, as well as reference equality and borrowing adjustments in Move modules, are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuiltPackage
participant CompilerConfig
participant ExternalChecks
participant Compiler
User->>BuiltPackage: build(package_path, config, docgen_options)
BuiltPackage->>BuiltPackage: build_with_external_checks(..., external_checks=[])
BuiltPackage->>CompilerConfig: Set compiler/language version (latest_stable)
BuiltPackage->>BuiltPackage: check_versions()
BuiltPackage->>BuiltPackage: inferred_bytecode_version()
BuiltPackage->>Compiler: compile_package_no_exit(..., external_checks)
Compiler-->>BuiltPackage: Compilation result
BuiltPackage->>BuiltPackage: Handle experimental flags & warnings
BuiltPackage-->>User: BuiltPackage result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (1)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
crates/e2e-move-tests/src/tests/move_unit.rs (1)
30-41: 🛠️ Refactor suggestion
static mutaccess is still data-race-prone – wrap inUnsafeCell/MutexorOnceLock
The unchangedBLANK_TABLE_RESOLVERis mutated throughaddr_of_mut!(BLANK_TABLE_RESOLVER)insideunit_test_extensions_hook. If these tests are ever executed in parallel, the raw mutable reference may cause UB. Consider guarding the value behind aMutexorOnceLock, or usingLazy<Mutex<_>>.crates/compiler/src/built_package.rs (1)
163-167:⚠️ Potential issueStore the mutated
new_config, not the original
Selfcurrently retains the unmodifiedconfig, so helper methods that serialize byte-code will use the wrong version. Replace it with the updatednew_config.- Ok(Self { - config, + Ok(Self { + config: new_config, package_path, package, })This ensures consistency between compilation settings and any subsequent operations performed through the
BuiltPackageAPI.
♻️ Duplicate comments (4)
crates/natives/src/query.rs (4)
143-143: Duplicate: lint suppression suggestion
Refer to the suggestion at lines 64–64 for consolidatingclippy::result_large_errsuppression.
277-277: Duplicate: lint suppression suggestion
Refer to the suggestion at lines 64–64 for consolidatingclippy::result_large_errsuppression.
298-298: Duplicate: lint suppression suggestion
Refer to the suggestion at lines 64–64 for consolidatingclippy::result_large_errsuppression.
355-355: Duplicate: lint suppression suggestion
Refer to the suggestion at lines 64–64 for consolidatingclippy::result_large_errsuppression.
🧹 Nitpick comments (25)
crates/natives/src/from_bcs.rs (1)
28-28: Suppress large error type lint on native_from_bytes
The added#[allow(clippy::result_large_err)]correctly suppresses the Clippy warning about a largeResulterror type and aligns with the crate-wide pattern for native functions.Optionally, if multiple functions in this module require the same suppression, you could move this to a module-level attribute (
#![allow(clippy::result_large_err)]at the top) for cleaner code.crates/natives/src/move_stdlib/signer.rs (1)
1-27: Optional: Consolidate lint suppression at module level
To reduce repetitive annotations and apply the suppression globally within this file, consider moving the attribute to the top of the module:--- a/crates/natives/src/move_stdlib/signer.rs +++ b/crates/natives/src/move_stdlib/signer.rs @@ // SPDX-License-Identifier: Apache-2.0 +#![allow(clippy::result_large_err)]You could then remove individual
#[allow(clippy::result_large_err)]annotations on each native function.crates/natives/src/cosmos.rs (2)
42-43: Consistent suppression of Clippyresult_large_errlint
The added#[allow(clippy::result_large_err)]aligns with similar suppressions in other native modules to avoid warnings about large error types. Approving this change. Consider moving this to a module‐level attribute (#![allow(clippy::result_large_err)]) if multiple functions require it, to reduce repetition.
85-86: Suppress Clippy lint for testing function
Adding#[allow(clippy::result_large_err)]onnative_requested_messagesensures consistency withnative_stargate. If future test-only natives also need this, grouping the suppression at the top of the module can keep the code DRY.crates/natives/src/object.rs (1)
19-19: Suppress large error type lint consistently.
Adding#[allow(clippy::result_large_err)]onnative_exists_atmatches the pattern used across other native functions to silence warnings about returningResultwith a large error type without altering logic. Consider DRY’ing this by applying a module‐level allowance (#![allow(clippy::result_large_err)]) if every function in this file requires it.crates/natives/src/crypto/secp256k1.rs (1)
89-90: Suppressing Clippy’sresult_large_errlint fornative_recover_public_key
This mirrors the pattern used fornative_verifyto silence warnings on large error types. As more native functions accumulate this attribute, consider moving the suppression to the module level (e.g.,#![allow(clippy::result_large_err)]at the top) to reduce repetition.crates/natives/src/address.rs (2)
19-19: Clippy lint suppression for large error variants
Suppressingclippy::result_large_erronnative_to_stringis consistent with the crate’s pattern of returningSafeNativeResultacross native functions. To reduce repetition, consider moving this to a module-level allow (e.g.,#![allow(clippy::result_large_err)]at the top) or a crate-level configuration.
38-38: Apply consistent lint suppression tonative_from_string
The addition of#[allow(clippy::result_large_err)]here mirrors the suppression on other native functions and avoids noise from large-result warnings. As above, you may lift this attribute to the module or crate root to DRY up the code.crates/natives/src/account.rs (1)
103-104: Approve Clippy suppression and consider module-level consolidation.
Adding#[allow(clippy::result_large_err)]here aligns with the project's pattern to suppress large‐error‐type warnings. For maintainability, you may consolidate these repeated attributes by placing a single#![allow(clippy::result_large_err)]at the top of the module instead of on each function.crates/natives/src/debug.rs (1)
53-56: Suggest more idiomatic handling of the unusedargsparameter.Rather than using
#[allow(unused_variables)], you can renameargsto_argsand then group Clippy allows in one attribute for clarity. This avoids an explicit allow and follows Rust’s convention for unused parameters. For example:- #[inline] - #[allow(unused_variables)] - #[allow(clippy::result_large_err)] + #[inline] + #[allow(clippy::result_large_err)] fn native_stack_trace( context: &mut SafeNativeContext, ty_args: Vec<Type>, - args: VecDeque<Value>, + _args: VecDeque<Value>, ) -> SafeNativeResult<SmallVec<[Value; 1]>> {crates/natives/src/keccak.rs (1)
26-26: Optional: Module-level lint suppression
If multiple functions in this module require the same Clippy allowance, consider moving it to the top of the file as a module-level attribute (#![allow(clippy::result_large_err)]) to reduce repetition and improve maintainability.crates/natives/src/move_stdlib/hash.rs (3)
26-28: Consider consolidating Clippy allows at module level
The addition of#[allow(clippy::result_large_err)]onnative_sha2_256silences warnings about large error types, which is consistent with other natives. To reduce repetition and improve maintainability, you could apply#![allow(clippy::result_large_err)]at the top of this file instead of annotating each function.
55-57: Consider consolidating Clippy allows at module level
You've added#[allow(clippy::result_large_err)]tonative_sha3_256. If most native functions in this file require the same suppression, a file-level attribute would improve readability and reduce boilerplate.
84-86: Consider consolidating Clippy allows at module level
The#[allow(clippy::result_large_err)]onnative_ripemd160matches the pattern above. For consistency and to avoid duplication, consider moving this suppression to a module-level#![allow(clippy::result_large_err)].crates/natives/src/interface/context.rs (1)
48-48: Consolidate repeated Clippy suppressions
The added#[allow(clippy::result_large_err)]matches other native functions, but since this lint is suppressed in many places, consider moving it out of individual methods and into the module or crate root (e.g.,#![allow(clippy::result_large_err)]at the top ofcontext.rsor inlib.rs) to DRY up the code and improve maintainability.crates/natives/src/oracle.rs (1)
65-66: Consider module-level lint suppression
The#[allow(clippy::result_large_err)]attribute correctly silences warnings here, but since multiple native functions in this module require it, consider applying a file-level attribute:#![allow(clippy::result_large_err)]at the top of the file and removing the per-function
#[allow(...)]annotations to reduce duplication.crates/natives/src/biguint.rs (1)
31-31: Consider grouping Clippy lint suppression at module level
Since all native functions in this file return a largeResulterror type, you can reduce repetition by using a module-level attribute:#![allow(clippy::result_large_err)]at the top of the file instead of annotating each function.
crates/natives/src/transaction_context.rs (1)
45-46: Suppressclippy::result_large_errfornative_get_transaction_hash
This addition correctly silences the large error type warning for this native function. Since multiple functions in this file require the same suppression, consider moving to a module-level attribute (#![allow(clippy::result_large_err)]) to reduce repetition and improve maintainability.crates/natives/src/query.rs (1)
64-64: Consolidate Clippy suppression
Suppressingclippy::result_large_erron each native function is acceptable, but you can reduce repetition by applying it at the module level with a top-of-file#![allow(clippy::result_large_err)].crates/e2e-move-tests/src/tests/move_unit.rs (1)
73-75: Consider pinning the compiler & language versions for deterministic test runs
UsingCompilerVersion::latest_stable()/LanguageVersion::latest_stable()keeps tests on the bleeding-edge, but it also means that a new tool-chain release can suddenly break CI (or give different results on local machines vs. CI caches). Re-running the exact same commit in the future may start failing for reasons unrelated to the code under test.
You may want to:
- Hard-code a specific stable version (e.g. pull the current value into a constant) or
- Read the target version from an env-var / cargo feature so that updates are explicit.
- build_config.compiler_config.compiler_version = Some(CompilerVersion::latest_stable()); - build_config.compiler_config.language_version = Some(LanguageVersion::latest_stable()); + // Keep tests reproducible. Bump deliberately when you really want the upgrade. + const COMPILER_V: CompilerVersion = CompilerVersion::V2_1; // ← pick current stable + const LANGUAGE_V: LanguageVersion = LanguageVersion::V2_1; // ← idem + build_config.compiler_config.compiler_version = Some(COMPILER_V); + build_config.compiler_config.language_version = Some(LANGUAGE_V);crates/compiler/src/test_package.rs (3)
42-50: Same reproducibility concern for test packages
For the same reasons mentioned inmove_unit.rs, dynamically pulling the current stable version can lead to irreproducible CI. Pin or gate the upgrade behind an explicit flag to avoid surprise failures.
52-57:check_versionsreturns ananyhow::Result<()>– capture and surface warnings
Today any mismatch aborts the build, butcheck_versionsmight also emit non-fatal warnings (e.g. about deprecated versions). You’re discarding that context. Consider logging the warnings (if the API supports it) so users know why a version combination was rejected.
58-64: Avoid double option moves – use references when callinginferred_bytecode_version
inferred_bytecode_versiononly needs read-only access; passing the ownedOptions consumes them and forces the extraletbinding. Passing as references keeps the original values intact and is a tad clearer:-let bytecode_version = inferred_bytecode_version( - new_build_config.compiler_config.language_version, - new_build_config.compiler_config.bytecode_version, -); -new_build_config.compiler_config.bytecode_version = bytecode_version; +new_build_config.compiler_config.bytecode_version = + inferred_bytecode_version( + &new_build_config.compiler_config.language_version, + &new_build_config.compiler_config.bytecode_version, + );(Adjust the callee signature accordingly if feasible.)
crates/compiler/src/extended_checks.rs (1)
224-253:#[view]still allows&mut signerparameters
The new check blockssignerand immutable&signerbut intentionally skips&mut signer. However,check_transaction_args(called earlier) already rejects both&signerand&mut signer.
Because of that, the currentifguard in lines 236-243 can be simplified:- Type::Reference(mutability, inner) => { - if let Type::Primitive(inner) = inner.as_ref() { - if inner == &PrimitiveType::Signer - && mutability == &ReferenceKind::Immutable - { - ... - } - } - } + Type::Reference(_, inner) if matches!(inner.as_ref(), Type::Primitive(PrimitiveType::Signer)) => { + // `&signer` or `&mut signer` + self.env.error( + param_loc, + "`#[view]` function cannot use a reference to `signer` parameter", + ) + }This keeps the rule simple and avoids diverging behaviour if
check_transaction_argsis ever relaxed.crates/compiler/src/built_package.rs (1)
51-58: Consider accepting external checks by slice or iterator
external_checksis taken by value as aVec<Arc<dyn ExternalChecks>>.
Becausebuild_with_external_checksforwards the vector unchanged tocompile_package_no_exit, the current signature forces callers to allocate a freshVeceven when they already keep the checks in some other collection (e.g. a slice or&[Arc<_>]). Acceptingimpl IntoIterator<Item = Arc<dyn ExternalChecks>>(or&[Arc<dyn ExternalChecks>]) would make the API more flexible and avoid an extra allocation.This is optional and can be revisited later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
Cargo.toml(1 hunks)crates/compiler/Cargo.toml(1 hunks)crates/compiler/src/built_package.rs(7 hunks)crates/compiler/src/extended_checks.rs(5 hunks)crates/compiler/src/test_package.rs(2 hunks)crates/e2e-move-tests/src/test_utils/mock_chain.rs(1 hunks)crates/e2e-move-tests/src/tests/move_unit.rs(1 hunks)crates/natives/src/account.rs(5 hunks)crates/natives/src/address.rs(2 hunks)crates/natives/src/base64.rs(2 hunks)crates/natives/src/bech32.rs(2 hunks)crates/natives/src/biguint.rs(10 hunks)crates/natives/src/block.rs(2 hunks)crates/natives/src/code.rs(1 hunks)crates/natives/src/cosmos.rs(2 hunks)crates/natives/src/crypto/ed25519.rs(5 hunks)crates/natives/src/crypto/secp256k1.rs(4 hunks)crates/natives/src/debug.rs(2 hunks)crates/natives/src/dispatchable_fungible_asset.rs(1 hunks)crates/natives/src/event.rs(2 hunks)crates/natives/src/from_bcs.rs(1 hunks)crates/natives/src/function_info.rs(5 hunks)crates/natives/src/interface/context.rs(1 hunks)crates/natives/src/json.rs(4 hunks)crates/natives/src/keccak.rs(1 hunks)crates/natives/src/move_stdlib/bcs.rs(2 hunks)crates/natives/src/move_stdlib/hash.rs(3 hunks)crates/natives/src/move_stdlib/signer.rs(1 hunks)crates/natives/src/move_stdlib/string.rs(5 hunks)crates/natives/src/move_stdlib/unit_test.rs(1 hunks)crates/natives/src/object.rs(1 hunks)crates/natives/src/oracle.rs(2 hunks)crates/natives/src/query.rs(6 hunks)crates/natives/src/staking.rs(8 hunks)crates/natives/src/string_utils.rs(5 hunks)crates/natives/src/table.rs(11 hunks)crates/natives/src/transaction_context.rs(4 hunks)crates/natives/src/type_info.rs(2 hunks)crates/types/src/compiler.rs(1 hunks)crates/vm/src/verifier/transaction_arg_validation.rs(2 hunks)libmovevm/src/vm.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: rust lint
- GitHub Check: Rust libmovevm
🔇 Additional comments (72)
libmovevm/src/vm.rs (1)
116-118: Approve cosmetic comment block simplification
The new shorter slash-style header before the storage operation functions is clear and consistent with surrounding code. No functional impact.crates/natives/src/type_info.rs (2)
51-52: Properly suppresses Clippy’sresult_large_errlint fornative_type_of.
The#[allow(clippy::result_large_err)]attribute is correctly placed immediately above the function signature, matching the pattern used in other native modules.
85-86: Properly suppresses Clippy’sresult_large_errlint fornative_type_name.
The attribute placement is consistent and aligns with the newly adopted convention across native functions.crates/natives/src/bech32.rs (2)
39-44: Suppress Clippy’sresult_large_errlint fornative_encode
Adding#[allow(clippy::result_large_err)]here aligns with the pattern across other native functions and prevents noisy warnings without altering behavior.
81-85: Suppress Clippy’sresult_large_errlint fornative_decode
Similarly, this attribute is consistent with the rest of the native implementations and has no side effects on the function logic.crates/natives/src/move_stdlib/unit_test.rs (1)
30-30: Code enhancement to suppress Clippy warning.Adding the
#[allow(clippy::result_large_err)]attribute is consistent with similar changes across other native functions in the codebase. This suppresses Clippy warnings about large error types in the Result return value without changing the function's behavior or signature.crates/natives/src/base64.rs (2)
27-27: Suppress large error type lint as intendedAdding the
#[allow(clippy::result_large_err)]attribute here aligns with the broader pattern of suppressing this lint for native functions returning aSafeNativeResultwith potentially large error variants.
54-54: Consistent lint suppression fornative_decodeThe
#[allow(clippy::result_large_err)]attribute is appropriately applied tonative_decode, matching the suppression innative_encodeand other native functions.crates/natives/src/code.rs (1)
120-120: Suppress Clippy warning for large error types.
Adding#[allow(clippy::result_large_err)]to thenative_request_publishfunction appropriately silences Clippy’s complaints about returning aResultwith a sizeable error type. This matches the existing pattern across other native functions and keeps lint output clean without altering behavior.crates/natives/src/event.rs (2)
62-62: Suppress Clippy lint for large error types in native_emit_event
The addition of#[allow(clippy::result_large_err)]here is consistent with the pattern applied across other native functions returning potentially largeResulterror types. This change only affects linting and does not modify behavior.
161-161: Suppress Clippy lint for large error types in native_emitted_events
Adding#[allow(clippy::result_large_err)]matches the approach used innative_emit_eventand other natives in this directory, ensuring consistent lint suppression without impacting functionality.crates/natives/src/move_stdlib/signer.rs (1)
27-27: Consistent Clippy lint suppression for large error types
Adding#[allow(clippy::result_large_err)]tonative_borrow_addressaligns with the pattern used across native modules to suppress lints about large error types without altering function logic.crates/natives/src/crypto/secp256k1.rs (3)
42-43: Suppressing Clippy’sresult_large_errlint fornative_verify
Adding#[allow(clippy::result_large_err)]here is consistent with other native functions and prevents noisy warnings due to the largeResulterror type. Ensure the error types remain correct and document the rationale if not already captured.
162-163: Suppressing Clippy’sresult_large_errlint fornative_test_only_generate_keys
Test-only functions can also yield large errors; this suppression keeps the lint output focused on real issues.
187-188: Suppressing Clippy’sresult_large_errlint fornative_test_only_sign
Consistent with other testing natives, this attribute keeps Clippy warnings at bay for large error returns.crates/natives/src/block.rs (2)
33-33: LGTM: Appropriate lint suppression for native function.This change appropriately suppresses the Clippy warning about large error types in the Result return value. This is consistent with the pattern applied throughout the codebase for native functions and helps reduce noise in the build output without compromising code quality.
50-50: LGTM: Consistent lint suppression for test function.Adding the same Clippy suppression to the test-only function maintains consistency across the codebase. This is good practice as it standardizes how these warnings are handled throughout the native functions.
crates/natives/src/account.rs (4)
165-166: Approve Clippy suppression fornative_create_account.
The lint suppression is consistent with other native implementations and prevents noise around large error types.
209-210: Approve Clippy suppression fornative_create_address.
This matches the convention used across thenativescrate to keep Clippy happy when returning largeResulttypes.
240-241: Approve Clippy suppression fornative_create_signer.
Suppressingresult_large_errhere is appropriate to avoid spurious lint failures on native functions.
282-283: Approve Clippy suppression for the testing helper.
Adding the lint allow onnative_test_only_set_account_infoensures consistent treatment ofResultsizes in test‐only functions.crates/natives/src/json.rs (1)
30-30: Clippy warning suppression looks appropriate.The addition of
#[allow(clippy::result_large_err)]attributes to all JSON native functions consistently addresses Clippy warnings about large error types in these functions' return values. This approach aligns with the PR objective of fixing linting issues.While suppressing warnings is appropriate here for consistency with other native functions, a future refactoring could consider redesigning the error types to be more compact (perhaps using
Box<dyn Error>or similar techniques) if the size of error types becomes a performance concern.Also applies to: 72-72, 82-82, 101-101
crates/natives/src/debug.rs (1)
26-28: Approve suppression of large error lints onnative_print.Adding
#[allow(clippy::result_large_err)]tonative_printis consistent with other native functions in this crate and prevents noisy lints without altering behavior.crates/natives/src/keccak.rs (1)
26-26: Suppress Clippy lint for large Result error types
Adding#[allow(clippy::result_large_err)]silences warnings about large error types in the function’sResultreturn, matching patterns applied across other native functions. This does not alter logic or error handling.crates/natives/src/crypto/ed25519.rs (5)
39-39: Suppress Clippy large-error lint fornative_verify.Adding
#[allow(clippy::result_large_err)]is appropriate here to avoid spurious warnings on the inherently largeResulttype returned bynative_verify, which wrapsPartialVMError.
91-91: Suppress Clippy large-error lint forpop_vec_of_vec_u8.Silencing
clippy::result_large_erris reasonable in this helper since it returns aSafeNativeResultthat can wrap sizeable error types (PartialVMError,TryFromSliceError).
158-158: Suppress Clippy large-error lint fornative_batch_verify.The batch verification function also returns a large
Resulttype. Allowingclippy::result_large_errhere aligns it with other native functions.
227-227: Suppress Clippy large-error lint fornative_test_only_generate_keys.This test-only native function returns a
SafeNativeResultwith potentially large error variants. The lint suppression is valid.
242-242: Suppress Clippy large-error lint fornative_test_only_sign.Adding
#[allow(clippy::result_large_err)]here correctly silences warnings on the large error type returned by this test-only signing function.crates/natives/src/table.rs (1)
314-878: LGTM: Consistent application of Clippy lint suppression across native functions.The addition of
#[allow(clippy::result_large_err)]to all native functions that returnResulttypes with potentially large error variants is appropriate. This approach is consistent with the PR objective of fixing linting issues, and applies the same pattern across all relevant functions without altering their behavior.crates/natives/src/staking.rs (3)
151-151: Appropriate suppression of Clippy warnings for native staking functions.The added
#[allow(clippy::result_large_err)]attributes are appropriate here since these functions interact with external staking APIs and need to return detailed error information. The large error types are necessary for proper error handling and debugging in this context.Also applies to: 207-207, 279-279, 323-323
389-389: Consistent lint suppression applied to test-only function.Good to see the same lint suppression applied consistently to the test-only function, maintaining coding standards across both production and test code.
416-416: Appropriate lint suppression for helper functions.The helper functions that handle BigDecimal conversions and error handling also need detailed error types. These suppressions are consistent with the approach taken for the main native functions and improve the overall lint compliance of the codebase.
Also applies to: 426-426, 443-443
crates/natives/src/oracle.rs (1)
104-105: Approve Clippy lint suppression for test function
Adding#[allow(clippy::result_large_err)]tonative_test_only_set_pricealigns with the pattern adopted across native modules to suppress large error-type warnings in test-only code. This change is consistent and necessary.crates/natives/src/biguint.rs (9)
63-63: Suppress Clippyresult_large_errlint fornative_sub
The attribute is correctly applied to silence warnings about large error variants returned bynative_sub.
101-101: Suppress Clippyresult_large_errlint fornative_mul
The added attribute appropriately suppresses theresult_large_errlint for this multiplication native.
133-133: Suppress Clippyresult_large_errlint fornative_div
Correctly added to avoid clippy warnings on large error types when dividing.
165-165: Suppress Clippyresult_large_errlint fornative_new
The annotation properly silences clippy’s large error lint for the constructor native.
215-215: Suppress Clippyresult_large_errlint fornative_cast
Appropriate addition to silence lint warnings on casting errors.
317-317: Suppress Clippyresult_large_errlint fornative_lt
Attribute correctly added to suppress large error type warnings on the less-than check.
342-342: Suppress Clippyresult_large_errlint fornative_le
The lint suppression is correctly applied for the less-or-equal native function.
367-367: Suppress Clippyresult_large_errlint fornative_gt
Correct use of the attribute to silence warnings on the greater-than native.
392-392: Suppress Clippyresult_large_errlint fornative_ge
Appropriately suppresses the large error lint for the greater-or-equal native.crates/natives/src/transaction_context.rs (3)
67-68: Silenceclippy::result_large_errfornative_generate_unique_address
Appropriately suppresses the large error type lint for this function’sResultreturn.
99-100: Suppress large error type lint innative_test_only_get_session_id
Correctly prevents Clippy from warning about the potentially large error type in this test-only function under thetestingfeature.
113-114: Silenceclippy::result_large_errfornative_test_only_set_transaction_hash
This allow-attribute is valid to avoid Clippy warnings on the function’s error type.crates/natives/src/string_utils.rs (4)
97-97: Suppress Clippy warning for large error types onformat_vector
Addition of#[allow(clippy::result_large_err)]here aligns with the broader pattern across native functions to suppress this lint.
131-131: Suppress Clippy warning for large error types onnative_format_impl
Consistent with other native functions; the attribute is appropriate.
396-396: Suppress Clippy warning for large error types onnative_format
Adding#[allow(clippy::result_large_err)]is consistent and justified given the large error type ofSafeNativeResult.
440-440: Suppress Clippy warning for large error types onnative_format_list
Attribute inclusion here follows the crate-wide convention. Approved.crates/natives/src/move_stdlib/string.rs (4)
39-39: LGTM: Adding Clippy suppression for large error typeThis is a reasonable lint suppression to avoid warnings about large error types in the
Resultreturn value. It's consistent with the PR's goal of fixing linting issues.
68-68: LGTM: Adding Clippy suppression for large error typeThis is a reasonable lint suppression to avoid warnings about large error types in the
Resultreturn value. It's consistent with the PR's goal of fixing linting issues.
97-97: LGTM: Adding Clippy suppression for large error typeThis is a reasonable lint suppression to avoid warnings about large error types in the
Resultreturn value. It's consistent with the PR's goal of fixing linting issues.
136-136: LGTM: Adding Clippy suppression for large error typeThis is a reasonable lint suppression to avoid warnings about large error types in the
Resultreturn value. It's consistent with the PR's goal of fixing linting issues.crates/compiler/Cargo.toml (1)
46-46: LGTM - Dependency addition looks good.The addition of the
move-compiler-v2dependency is properly configured with workspace-level management, which aligns with the PR objective of adding support for compiler version 2.crates/natives/src/dispatchable_fungible_asset.rs (1)
18-18: LGTM - Lint suppression appropriate for this function.The added attribute suppresses Clippy warnings about large error types in the Result return value, which is consistent with the PR's objective of fixing linting issues. This is a reasonable approach as the error type is part of the core design of the native function implementation.
crates/natives/src/move_stdlib/bcs.rs (2)
44-44: LGTM - Appropriate lint suppression for native_to_bytes.This lint suppression for large error types is necessary and consistent with the pattern applied to other native functions across the codebase.
103-103: LGTM - Appropriate lint suppression for native_serialized_size.This lint suppression for large error types is necessary and consistent with the pattern applied to other native functions across the codebase.
crates/types/src/compiler.rs (2)
93-95: LGTM - Enhanced compiler version handling.The addition of support for symbolic compiler versions (
latestandlatest_stable) improves flexibility by allowing users to target the most recent or stable compiler without specifying exact version numbers.
101-105: LGTM - Extended language version support.The implementation correctly adds support for newer language versions (2.2, 2.3) and symbolic references (
latest,latest_stable), which enhances the compiler's ability to work with different Move language specifications.crates/vm/src/verifier/transaction_arg_validation.rs (2)
467-470: Logic change usingis_none_orinstead of negatedis_some_andThe condition now uses
is_none_orwith an inverted predicate instead of what was likely a negatedis_some_and. While functionally equivalent, this improves readability by avoiding double negation.
566-566: Consistent use ofis_none_orpatternSimilar to the previous change, this uses the same
is_none_orpattern for boundary checking, maintaining consistency in the codebase.crates/natives/src/function_info.rs (5)
23-23: Clippy suppression for large error typeAdded
#[allow(clippy::result_large_err)]to suppress warnings about large error types in function return values, which is appropriate for this native function that needs to return detailed error information.
35-35: Consistent suppression of Clippy warningThis annotation maintains consistency with other native functions that return complex error types.
75-75: Clippy warning suppression for native functionAdded suppression annotation for the large error type returned by this dispatch compatibility check function.
136-136: Suppression for identifier validation functionConsistently applied the same Clippy suppression pattern to this native function.
170-170: Final Clippy suppression in the fileCompleted the pattern of suppressing large error type warnings across all relevant native functions in this file.
Cargo.toml (1)
129-129:✅ Verification successful
Added dependency on Move compiler v2
Added the
move-compiler-v2dependency using the same Git repository and revision as other Move dependencies, which is essential for supporting the compiler version 2 features mentioned in the PR title.
🏁 Script executed:
#!/bin/bash # Verify that this dependency is correctly referenced in the compiler crate grep -r "move-compiler-v2" --include="Cargo.toml" crates/Length of output: 122
Dependency correctly configured
Confirmed that:
- The root
Cargo.tomlpinsmove-compiler-v2to the intended Git revision.crates/compiler/Cargo.tomlreferences it withworkspace = true.No further changes needed.
crates/e2e-move-tests/src/test_utils/mock_chain.rs (1)
484-485: Minor comment style improvementChanged the comment style for the section divider to be more consistent, using a line of forward slashes and a regular comment instead of a doc comment.
crates/compiler/src/built_package.rs (3)
47-50: Delegation pattern looks goodForwarding the public
buildAPI to the more generalbuild_with_external_checksavoids code duplication and keeps the default path clean.
No further action required.
71-78: 👍 Sensible default-to-latest-stable behaviourFalling back to
CompilerVersion::latest_stable()andLanguageVersion::latest_stable()when the caller didn’t specify explicit versions is a safer default than pinning to a hard-coded constant.
275-305: Minor ergonomics & verification request forcheck_versions
unwrap_or_default()is invoked on&Option<CompilerVersion>/&Option<LanguageVersion>.
This compiles only ifCompilerVersionandLanguageVersionimplementCopy.
If they are (or later become) non-Copy, this will fail. Consider cloning explicitly or passing theOptionby value.After emitting the warnings,
error_writer.flush()?;could be added to guarantee the messages appear before subsequent output (especially useful when piping logs).Please confirm that both
CompilerVersionandLanguageVersionare indeedCopy; otherwise the current implementation will not compile.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
types/bcs.go (1)
3-6: 💡 Verification agent🧩 Analysis chain
Warning about modifying auto-generated file
This file has a prominent warning indicating it should not be modified directly, but changes have been made to it. Consider whether these changes should be part of the code generation process instead of direct modifications.
🏁 Script executed:
#!/bin/bash # Look for the code generation script mentioned in the file header echo "Searching for code generation script..." fd -t f "generate-bcs-go" --exec grep -l "generate.*bcs.*go" {} \;Length of output: 158
Update code generation process for
types/bcs.gomodificationsIt looks like you’ve manually edited an auto-generated file without updating the generator itself. I couldn’t find a
generate-bcs-goscript in the repo, so these changes will be lost the next time codegen runs. Please:
- Add or locate the
generate-bcs-gotool in your repo (e.g. under atools/orscripts/directory).- Revert manual edits in
types/bcs.go.- Incorporate your intended changes into the generator so that running it regenerates the correct output.
♻️ Duplicate comments (1)
crates/compiler/src/built_package.rs (1)
69-81:⚠️ Potential issueAdded version validation and bytecode version inference
This improvement:
- Validates compiler and language versions
- Warns about unstable versions
- Infers the appropriate bytecode version based on language version
- Sets the inferred version in the compiler configuration
However, there's an issue that needs fixing.
The inferred bytecode version is updated in
new_configbut not in the originalconfigthat's stored inSelf(line 154). This could cause inconsistencies if code elsewhere accessesself.config.compiler_config.bytecode_version.Fix this by updating the original config:
new_config.compiler_config.bytecode_version = bytecode_version; +config.compiler_config.bytecode_version = bytecode_version;
🧹 Nitpick comments (1)
crates/compiler/src/built_package.rs (1)
103-109: Added experimental flags for warnings handling and early exitTwo more experimental flags are now supported:
FAIL_ON_WARNING: Fails with an error if warnings are foundSTOP_AFTER_EXTENDED_CHECKS: Exits after extended checksThe same concern about using
std::process::exitapplies here.Consider refactoring to return a result instead of calling
std::process::exitdirectly, which would give library users more control:if model_options.experiment_on(Experiment::STOP_AFTER_EXTENDED_CHECKS) { - std::process::exit(if model.has_warnings() { 1 } else { 0 }) + return Ok(Self { + config, + package_path, + package, + early_exit: Some(if model.has_warnings() { 1 } else { 0 }), + }); }You would need to add an
early_exitfield toBuiltPackageand have consumers check this field.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
Cargo.toml(2 hunks)crates/compiler/Cargo.toml(1 hunks)crates/compiler/src/built_package.rs(7 hunks)crates/types/Cargo.toml(1 hunks)crates/types/src/compiler.rs(3 hunks)precompile/modules/initia_stdlib/sources/code.move(1 hunks)precompile/modules/initia_stdlib/sources/fa/fungible_asset.move(1 hunks)precompile/modules/initia_stdlib/sources/minitswap.move(15 hunks)precompile/modules/initia_stdlib/sources/multisig_v2.move(1 hunks)precompile/modules/initia_stdlib/sources/simple_map.move(1 hunks)precompile/modules/initia_stdlib/sources/stableswap.move(4 hunks)precompile/modules/initia_stdlib/sources/staking.move(5 hunks)precompile/modules/initia_stdlib/sources/token/collection.move(1 hunks)precompile/modules/minitia_stdlib/sources/code.move(1 hunks)precompile/modules/minitia_stdlib/sources/fa/fungible_asset.move(2 hunks)precompile/modules/minitia_stdlib/sources/managed_coin.move(1 hunks)precompile/modules/minitia_stdlib/sources/multisig_v2.move(1 hunks)precompile/modules/minitia_stdlib/sources/simple_map.move(1 hunks)precompile/modules/minitia_stdlib/sources/token/collection.move(1 hunks)types/bcs.go(3 hunks)types/compiler/build/options.go(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- precompile/modules/initia_stdlib/sources/simple_map.move
- precompile/modules/minitia_stdlib/sources/simple_map.move
- crates/types/Cargo.toml
- precompile/modules/minitia_stdlib/sources/managed_coin.move
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/compiler/Cargo.toml
- Cargo.toml
- crates/types/src/compiler.rs
🧰 Additional context used
🧬 Code Graph Analysis (2)
types/compiler/build/options.go (1)
types/bcs.go (1)
CompilerBuildConfig(188-202)
types/bcs.go (1)
types/serialize.go (2)
SerializeBool(45-52)DeserializeBool(55-58)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Rust libmovevm
🔇 Additional comments (43)
precompile/modules/minitia_stdlib/sources/multisig_v2.move (1)
720-722: Optimization: Changed string comparison to reference comparison.The comparison in the predicate has been updated from value comparison (
tt.name == tier_name) to reference comparison (&tt.name == &tier_name), which is more efficient forStringtypes. This change aligns with the broader pattern of optimizations in the PR.precompile/modules/initia_stdlib/sources/multisig_v2.move (1)
721-722: Good optimization: Using reference comparison for stringsThe change to compare string references instead of string values is a performance improvement. In Move, strings are resource types (vectors of bytes), so comparing by reference avoids unnecessary copying and is more efficient.
precompile/modules/minitia_stdlib/sources/code.move (1)
254-255: Good improvement to borrowing patternChanged from mutable to immutable borrow since the function only reads from
ModuleStorewithout modifying it. This follows Move best practices of only requesting the minimum necessary access level.precompile/modules/minitia_stdlib/sources/token/collection.move (2)
362-362: Appropriate immutable borrowChanged from mutable to immutable borrow for
FixedSupplysince the function only reads thecurrent_supplyfield without modifying the resource.
365-365: Appropriate immutable borrowChanged from mutable to immutable borrow for
UnlimitedSupplysince the function only reads thecurrent_supplyfield without modifying the resource.precompile/modules/minitia_stdlib/sources/fa/fungible_asset.move (2)
871-871: Improved reference comparisonChanged from value comparison to reference comparison, which is more efficient for complex types as it avoids copying the entire metadata value.
962-962: Appropriate immutable borrowChanged from mutable to immutable borrow for
Metadatasince the function only reads from it to initialize theExtraMetadataresource without modifying the original resource.precompile/modules/initia_stdlib/sources/staking.move (5)
1018-1018: Improved resource access pattern with immutable borrowChanged from mutable to immutable borrow of ModuleStore in claim_reward since no modification is needed.
1243-1243: Improved resource access pattern with immutable borrowChanged from mutable to immutable borrow of ModuleStore in destroy_delegation_and_extract_reward since no modification is needed.
1278-1278: Improved resource access pattern with immutable borrowChanged from mutable to immutable borrow of ModuleStore in unbonding_share_from_amount since no modification is needed.
1181-1181: Updated BigDecimal comparison to use referencesChanged equality check to compare references rather than values for BigDecimal types, which is the recommended pattern for custom types with potentially expensive equality operations.
1456-1456: Updated BigDecimal comparison to use referencesChanged equality check to compare references rather than values for BigDecimal types, maintaining consistency with the pattern used elsewhere in the codebase.
precompile/modules/initia_stdlib/sources/stableswap.move (5)
499-499: Improved vector access with immutable borrowChanged from mutable to immutable borrow when accessing coin_amounts vector since no modification is needed here.
990-990: Simplified object address accessRemoved unnecessary dereferencing when accessing the object address.
1380-1382: Updated metadata comparison to use referencesChanged equality check to compare references rather than values for Object types, which aligns with the comparison pattern used elsewhere in the codebase.
1392-1393: Updated metadata comparison to use referencesChanged equality check to compare references rather than values for Object types, maintaining consistency in comparison operations.
1395-1396: Updated metadata comparison to use referencesChanged equality check to compare references rather than values for Object types, maintaining consistency in comparison operations.
types/compiler/build/options.go (1)
103-107: Added function to enable lint checks in compiler configurationNew function
WithLintChecks()follows the established pattern of other option functions and enables the new lint checking functionality in the compiler build process.This change aligns with the PR objective of "adding missing checks for compiler version 2" and is consistent with the pattern used for other compiler build options.
precompile/modules/initia_stdlib/sources/fa/fungible_asset.move (1)
817-819: Improved reference equality check for metadata comparison.The change to compare references (
&ref.metadata == &metadata) instead of direct values is more appropriate for this type of comparison, ensuring consistent behavior when dealing withObject<Metadata>types.precompile/modules/initia_stdlib/sources/code.move (1)
254-256: Good practice: Changed mutable borrow to immutable borrow.Replaced
borrow_global_mutwithborrow_globalsince this function only reads from themodule_storeand doesn't modify it. This follows the principle of least privilege and improves code safety.precompile/modules/initia_stdlib/sources/token/collection.move (2)
361-363: Good practice: Changed mutable borrow to immutable borrow.Using
borrow_globalinstead ofborrow_global_mutis more appropriate since this view function only reads thecurrent_supplywithout modifying theFixedSupplyresource.
364-366: Good practice: Changed mutable borrow to immutable borrow.Using
borrow_globalinstead ofborrow_global_mutis more appropriate since this view function only reads thecurrent_supplywithout modifying theUnlimitedSupplyresource.types/bcs.go (3)
193-193: New field added to CompilerBuildConfig structThe addition of the
EnableLintChecksfield to theCompilerBuildConfigstruct provides support for integrating external lint checks during package builds, which aligns with the compiler v2 feature.
210-210: Field properly included in serialization methodThe new
EnableLintChecksfield is correctly incorporated into the struct's serialization method, maintaining consistency with the other configuration options.
239-239: Field properly included in deserialization methodThe new
EnableLintChecksfield is correctly incorporated into the struct's deserialization method, ensuring it can be properly restored from serialized data.precompile/modules/initia_stdlib/sources/minitswap.move (13)
354-359: Code optimized by replacing mutable borrow with immutable borrowThe code has been optimized by changing
borrow_global_muttoborrow_globalandtable::borrow_muttotable::borrowsince theModuleStoreand pools are only being read in this function, not modified.
553-554: Immutable borrow for read-only accessCorrectly changed to immutable borrow since the
VirtualPoolis only being read here, not modified.
697-699: Simplified object address dereferencingThe code now directly passes the result of
option::borrowtoobject::object_addresswithout first dereferencing the option. This is cleaner and more idiomatic.
764-766: Consistent immutable borrow patternThis change follows the same pattern as the previous modifications, using immutable borrow to access the
VirtualPoolobject address.
1105-1109: Kept mutable borrow where neededMutable borrow is correctly maintained here because the
deactivatefunction needs to modifypool.active.
1118-1123: Mixed borrowing approach aligns with function requirementsImmutable borrows are used for reading the
ModuleStoreand pools, while a mutable borrow is correctly used for theVirtualPoolthat will be modified by settingpool.active = true.
1137-1141: Consistent pattern for accessing VirtualPoolThis change follows the same pattern as previous modifications, using immutable borrows for read-only access while maintaining mutable borrow of the VirtualPool that will be modified.
1337-1342: Module and pools accessed with immutable borrowsThe
update_pool_paramsfunction correctly uses immutable borrows forModuleStoreand pools while maintaining a mutable borrow for theVirtualPoolthat will be updated.
1857-1864: Immutable borrows for callback handlersCallback handlers like
ibc_ackappropriately use immutable borrows since they're only reading the object state before calling other functions that handle the mutations.
1879-1886: Consistent immutable borrowing in timeout handlerSimilarly, the
ibc_timeouthandler uses immutable borrows in a consistent manner with the other callbacks.
1935-1939: Optimized borrow chainThe borrowing chain has been simplified by directly passing the result of
option::borrowtoobject::object_addresswithout intermediate dereferencing.
1950-1954: Consistent immutable borrowing patternThis follows the same pattern established throughout the file, using immutable borrows where possible and simplifying the object address resolution chain.
2261-2264: Changed parameter type to immutable referenceThe
send_ibc_messagefunction now correctly takes an immutable reference topoolsince it doesn't modify the pool state directly.crates/compiler/src/built_package.rs (5)
50-50: Added lint checks to compilation processThe code now includes
MoveLintChecks::make()in the external checks vector, integrating lint checking into the compilation process.
61-67: Dynamic version handling for better maintainabilityInstead of using fixed versions, the code now dynamically uses the latest stable versions for compiler and language. This is more maintainable and automatically adapts to new stable releases.
84-84: Updated compilation to include external checksThe compilation call now includes the external checks (including lint checks) and a stderr handler.
264-295: Added version validation function with helpful warningsThe new
check_versionsfunction validates compiler and language versions, providing colored warnings for unstable versions and checking compatibility.
297-306: Added bytecode version inference functionThe new
inferred_bytecode_versionfunction determines the appropriate bytecode version based on language version and any explicitly set bytecode version.
Description
A new linter feature will be added after this PR.
Example usage:
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Chores