Skip to content

[mono-move] String natives#20163

Open
georgemitenkov wants to merge 1 commit into
george/storage-slot-nativesfrom
george/string-natives
Open

[mono-move] String natives#20163
georgemitenkov wants to merge 1 commit into
george/storage-slot-nativesfrom
george/string-natives

Conversation

@georgemitenkov

@georgemitenkov georgemitenkov commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Description

Implements the remaining 0x1::string natives for mono-move (the v2 VM). Only internal_check_utf8 was implemented before; this adds the other three so Move code that calls string::sub_string and string::index_of can run:

  • internal_is_char_boundary(v: &vector<u8>, i: u64): bool
  • internal_sub_string(v: &vector<u8>, i: u64, j: u64): vector<u8>
  • internal_index_of(v: &vector<u8>, r: &vector<u8>): u64

Each native mirrors the legacy VM logic so both VMs produce identical results. internal_sub_string copies the result off the VM heap before allocating the return vector, since new_byte_vector can trigger a GC that relocates the source bytes.

How Has This Been Tested?

Renamed the existing check_utf8.move differential test to string.move and added cases for the new natives, run on both the legacy MoveVM and mono-move with their outputs compared:

  • index_of: match found, first of multiple occurrences, no match (returns length), empty needle, multibyte content.
  • sub_string: basic ranges, empty range, full string, out-of-range and reversed indices (abort), multibyte boundary success.
  • is_char_boundary has no public wrapper, so it is exercised through sub_string's boundary asserts: the multibyte success cases require it to return true, the mid-codepoint cases require false (abort).
cargo build -p mono-move-natives
RUST_MIN_STACK=1073741824 cargo test -p mono-move-testsuite --test differential

All 193 differential cases pass.

Key Areas to Review

  • Argument order in the natives matches the Move signatures (arg(0), arg(1), arg(2)).
  • GC safety: byte slices are consumed before any allocation, and the substring is copied to an owned Vec before new_byte_vector.

string::insert is not covered end-to-end: it calls vector::append, which needs the 0x1::vector::move_range native that mono-move does not implement yet. That is out of scope here, and is_char_boundary is fully covered through sub_string instead.

Type of Change

  • New feature

Which Components or Systems Does This Change Impact?

  • Move/Aptos Virtual Machine

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

Note

Medium Risk
Changes VM string execution and GC-sensitive native code; behavior is constrained by differential tests against the legacy VM but substring/index logic is user-visible on-chain.

Overview
Adds three mono-move natives for 0x1::string so string::index_of and string::sub_string work in the v2 VM alongside the existing internal_check_utf8. New implementations are internal_is_char_boundary, internal_sub_string, and internal_index_of, registered in make_all_string_natives.

Shared from_utf8_checked treats invalid UTF-8 on already-constructed strings as a VM invariant violation. internal_sub_string aborts with code 1 when j < i; out-of-range and non-boundary indices are still enforced by Move wrappers. Substring bytes are copied to an owned Vec before new_byte_vector to avoid GC relocating source slices.

Differential coverage moves from check_utf8.move to string.move, keeping utf8 cases and adding index_of / sub_string scenarios (including multibyte boundaries via sub_string asserts).

Reviewed by Cursor Bugbot for commit ed7b4ce. Bugbot is set up for automated code reviews on this repo. Configure here.

Copy link
Copy Markdown
Contributor Author

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

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

Implement the remaining string natives for mono-move:
internal_is_char_boundary, internal_sub_string, and internal_index_of,
mirroring the legacy VM so both produce identical results.

Add a differential test (renamed from check_utf8.move to string.move)
exercising the new natives through the public string wrappers index_of
and sub_string. is_char_boundary has no standalone wrapper, so it is
covered through sub_string's boundary asserts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown
Contributor

mono-move benchmark gate

2 regression(s) beyond ±3% noise band

3 ok · 2 improved · 0 new · 0 absent (threshold T = ±3%, criterion mean CI vs main)

Benchmark mean Δ 95% CI median (main → PR) Verdict
fib/mono -0.7% [-1.7%, +0.2%] 8.07ms → 8.07ms ok
nested_loop/mono +19.3% [+19.1%, +19.5%] 11.85ms → 14.10ms regression
merge_sort/mono +4.1% [+3.7%, +4.4%] 1.22ms → 1.27ms regression
bst/mono +3.0% [+2.8%, +3.2%] 3.63ms → 3.74ms ok
match_sum/mono -6.5% [-7.3%, -5.9%] 25.72ms → 24.33ms improved
int_arith_loop/mono_u64 -0.8% [-1.9%, +0.1%] 408.83µs → 405.35µs ok
int_arith_loop/mono_i64 -13.7% [-14.1%, -13.2%] 678.34µs → 585.60µs improved

Improvements are not failures. main rebaselines on merge, so the next PR compares against the faster code automatically.

@georgemitenkov georgemitenkov marked this pull request as ready for review July 2, 2026 16:02

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit ed7b4ce. Configure here.

//
// "héllo" is the six-byte sequence [0x68, 0xc3, 0xa9, 0x6c, 0x6c, 0x6f]: 'h',
// then 'é' (two bytes 0xc3 0xa9), then "llo". Byte indices 0, 1, 3, 4, 5, 6 are
// char boundaries; 2 (the 'é' continuation byte) is not.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verbose multi-line header block in test file

Low Severity

The 10-line header block narrates which natives are tested, how they're exercised (through wrappers), and provides byte-level encoding details for "héllo." Per the MonoMove review convention, verbose multi-line header blocks in test files narrating what tests exercise are flagged — brief per-function comments suffice. The per-function comments already present in the file (e.g., // Returns the index of the first occurrence.) are fine.

Fix in Cursor Fix in Web

Triggered by learned rule: MonoMove: flag verbose AI-generated comments in test and code files

Reviewed by Cursor Bugbot for commit ed7b4ce. Configure here.

// SAFETY: the bytes are consumed immediately, into an owned `Vec`,
// before any allocation.
let bytes = unsafe { v.as_bytes() };
from_utf8_checked(bytes)?[i..j].as_bytes().to_vec()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we safely checking if i and j are char boundaries here?


/// `0x1::string::internal_index_of(v: &vector<u8>, r: &vector<u8>): u64`
///
/// Returns the byte index of the first occurrence of `r` in `v`, or `v.len()`

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: or v.len() if absent

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants