Skip to content

feat: accept _ digit separators in integer arguments #2685

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Galmanus wants to merge 2 commits into
base: main
Choose a base branch
from
Open

feat: accept _ digit separators in integer arguments #2685

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5a3b894
feat: accept `_` digit separators in integer arguments
Galmanus Aug 15, 2026
d31eb7a
test: fold the numeric-separator tests into a table macro
Galmanus Aug 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions cmd/crates/soroban-spec-tools/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,17 @@ impl Spec {
let v = value_type.as_ref().clone();
return self.from_string(s, &v);
}
// Integer types accept `_` as a digit separator, mirroring Rust numeric
// literals (e.g. `1_000_000`). Strip separators before parsing, gated to
// integer types only: `_` is a legal character in Symbol/String args and
// must be preserved there. See #2447.
let stripped;
let s = if is_integer_type(t) && s.contains('_') {
stripped = s.replace('_', "");
stripped.as_str()
} else {
s
};
Comment on lines +273 to +279

@Galmanus Galmanus Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Deliberate choice, documented in the PR description under design notes. At a type-committed CLI-arg position an underscore is a pure separator carrying no numeric information, so stripping can never produce a different number than the digit sequence typed — only the intended one; there's no aliasing of two distinct values. Worth noting two of the listed examples are actually valid Rust: the integer-literal grammar is DEC_DIGIT (DEC_DIGIT | _)*, so 1__2 (== 12) and 1_ (== 1) are legal literals with consecutive/trailing underscores. The genuinely non-Rust cases (__1, -_1, 1_x) either strip to an unambiguous digit sequence or still fail downstream with the existing clean type error — e.g. 1_x -> 1x -> the current parse error. Placement validation would reject inputs that have a single unambiguous meaning with no correctness or safety gain, so I'd keep strip-all as a deliberate superset of valid-Rust placements.

Comment on lines +273 to +279

@Galmanus Galmanus Aug 15, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Correct and fully compiler-enforced: stripped lives in the same scope as the shadowing let s, so the borrow is valid for the rest of the function, and a refactor that narrowed its scope would fail to compile (borrow of dropped value) rather than dangle — the borrow checker rules that out. The deferred-init idiom is deliberate: zero allocation on the common path (non-integer types, and integers without _) while keeping s: &str so none of the downstream consumers change. Moving to Cow<'_, str> ripples through all of those sites and changes s.to_owned() semantics (Cow::to_owned yields a Cow, not a String), so it's more churn for a stylistic gain. If readability is the concern I'm happy to extract a fn strip_int_separators<'a>(s: &'a str, t: &ScType) -> Cow<'a, str> helper that names the ownership explicitly while keeping downstream &str via .as_ref() — let me know if you'd prefer that.

// Parse as string and for special types assume Value::String
serde_json::from_str(s)
.map_or_else(
Expand Down Expand Up @@ -805,6 +816,21 @@ impl Spec {
/// # Errors
///
/// Might return an error
/// Integer contract-argument types that accept `_` as a digit separator.
fn is_integer_type(t: &ScType) -> bool {
matches!(
t,
ScType::U32
| ScType::I32
| ScType::U64
| ScType::I64
| ScType::U128
| ScType::I128
| ScType::U256
| ScType::I256
)
}

pub fn from_string_primitive(s: &str, t: &ScType) -> Result<ScVal, Error> {
Spec::from_string_primitive(s, t)
}
Expand Down Expand Up @@ -1853,6 +1879,45 @@ mod tests {
assert_eq!(to_string(&parsed).unwrap(), format!("\"{as_str}\""));
}

/// Assert that a value with `_` digit separators parses identically to the
/// same value without them, generating one named `#[test]` per row.
macro_rules! numeric_separator_tests {
($($name:ident: $ty:expr, $with:expr, $without:expr;)*) => {
$(
#[test]
fn $name() {
assert_eq!(
from_string_primitive($with, &$ty).unwrap(),
from_string_primitive($without, &$ty).unwrap()
);
}
)*
};
}

numeric_separator_tests! {
test_u32_numeric_separator: ScType::U32, "1_000", "1000";
test_i64_numeric_separator: ScType::I64, "1_000_000", "1000000";
// The exact #2447 example.
test_i128_numeric_separator: ScType::I128, "100_0000000", "1000000000";
test_u128_numeric_separator: ScType::U128, "1_000", "1000";
test_u256_numeric_separator: ScType::U256, "1_000", "1000";
test_i256_numeric_separator: ScType::I256, "1_000", "1000";
}

#[test]
fn test_symbol_underscore_preserved() {
// `_` is a legal Soroban symbol char and must NOT be stripped.
let parsed = from_string_primitive("hello_world", &ScType::Symbol).unwrap();
assert_eq!(parsed, ScVal::Symbol("hello_world".try_into().unwrap()));
}

#[test]
fn test_string_underscore_preserved() {
let parsed = from_string_primitive("a_b_c", &ScType::String).unwrap();
assert_eq!(parsed, sc_string("a_b_c"));
}

#[test]
fn test_symbol_conversion() {
let as_str = "hello";
Expand Down