refactor(strconv): raise structured StrConvError from internal/strconv - #3997
Closed
bobzhang wants to merge 1 commit into
Closed
refactor(strconv): raise structured StrConvError from internal/strconv#3997bobzhang wants to merge 1 commit into
bobzhang wants to merge 1 commit into
Conversation
internal/strconv previously raised bare Failure with magic message
strings ("value out of range", "invalid syntax", "invalid base"), so
callers could only catch-all or compare strings. Introduce
pub(all) suberror StrConvError { RangeError; SyntaxError; InvalidBase }
and declare it on every parse_* signature (public and private chain).
Consumers:
- json/lex_number.mbt now catches RangeError precisely for the
infinity+repr sentinel and turns SyntaxError/InvalidBase into
abort("unreachable"), encoding "the lexer already validated the
grammar" as a checked invariant instead of silently laundering a
hypothetical strconv disagreement into Number(Infinity).
- json/from_json.mbt Int64/UInt64 decoding interpolates err.message(),
preserving the exact historical JsonDecodeError texts.
- string replaces its `pub using` re-export with thin wrappers that
translate StrConvError back into Failure with the historical message
strings, so @string.parse_* keeps its public error contract and
string/pkg.generated.mbti is byte-identical (existing snapshot tests
pin the Failure messages and pass unchanged).
No external mbti changes: only internal/strconv/pkg.generated.mbti
(not publicly exposed) differs.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors internal/strconv to raise a structured StrConvError (instead of Failure with magic strings) and threads that typed error through in-module consumers, while preserving the public @string.parse_* error contract by translating back to Failure("<historical message>").
Changes:
- Introduces
StrConvError(RangeError | SyntaxError | InvalidBase) ininternal/strconvand updates all parse helpers toraise StrConvError. - Updates
jsonnumber lexing/decoding to handle out-of-range vs. syntax/base errors precisely (range → infinity sentinel; others treated as unreachable given lexer validation). - Replaces
string’s directpub using @internal/strconv { parse_* }re-exports with thin wrappers that mapStrConvErrorback to legacyFailuremessages.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| string/strconv.mbt | Adds wrapper functions that catch @internal/strconv.StrConvError and re-raise legacy Failure messages to keep the public contract stable. |
| json/lex_number.mbt | Narrows error handling from catch-all to RangeError vs. other StrConvError variants when falling back to @internal/strconv.parse_double. |
| json/from_json.mbt | Updates Int64/UInt64 decoding to interpolate StrConvError::message() to preserve historical error text. |
| internal/strconv/strconv_errors.mbt | Defines StrConvError and helper raisers (range_err, syntax_err, base_err) plus message() for legacy strings. |
| internal/strconv/strconv_int.mbt | Updates integer parsing helpers/signatures/tests to raise and assert StrConvError variants. |
| internal/strconv/strconv_uint.mbt | Updates unsigned parsing signatures/tests to raise and assert StrConvError variants. |
| internal/strconv/strconv_double.mbt | Updates double parsing signature/tests to raise and assert StrConvError variants, especially overflow as RangeError. |
| internal/strconv/strconv_decimal.mbt | Adjusts decimal parsing signatures and updates tests to expect StrConvError (including debug inspection of RangeError). |
| internal/strconv/strconv_number.mbt | Updates internal number parsing helper signatures/docs to raise StrConvError. |
| internal/strconv/strconv_bool.mbt | Updates boolean parsing signature/tests to raise and assert StrConvError variants. |
| internal/strconv/strconv_coverage_test.mbt | Updates coverage tests to treat StrConvError variants as the expected failure modes without string matching. |
| internal/strconv/moon.pkg | Switches dependencies from error to debug to support derived @debug.Debug for StrConvError. |
| internal/strconv/pkg.generated.mbti | Regenerates the internal package interface to reflect raise StrConvError signatures and exported error/methods. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to the discussion on #3996:
internal/strconvraised bareFailurewith magic message strings ("value out of range","invalid syntax","invalid base"), so callers could only catch-all or string-compare. This introduces a structured error and threads it through every consumer, with zero externally visible changes.All
parse_*signatures (public and the private call chain) now declareraise StrConvError, and amessage()method carries the historical strings.Consumers
lex_number.mbt): both sentinel fallbacks now catchRangeErrorprecisely;SyntaxError | InvalidBasebecomeabort("unreachable")— the lexer has already validated the number grammar, so a strconv disagreement is a bug that should surface loudly instead of being silently laundered intoNumber(Infinity, repr=...).from_json.mbt):Int64/UInt64decoding interpolateserr.message(), keeping the exact historicalJsonDecodeErrortexts.pub using @internal/strconv { parse_* }re-export becomes thin wrappers that translateStrConvErrorback intoFailurewith the historical messages. External users of@string.parse_*see identical behavior — the existing snapshot tests pinningFailure("value out of range")pass unchanged.Why not let the typed error escape through
@string.parse_*?The type lives in an internal package, so external users could never name its constructors to match on it — they'd actually lose the ability to match
Failure(msg)they have today. Translation at the facade keeps the public contract intact; giving external users a typed error would mean unifying with the publicstrconvpackage's (separate, also stringly)StrConvError, which is a bigger, breaking discussion.Validation
internal/strconv+json: 228/228;string: all passingmoon info:string/pkg.generated.mbtiandjson/pkg.generated.mbtibyte-identical; onlyinternal/strconv/pkg.generated.mbti(not publicly exposed) changedmoon fmtclean🤖 Generated with Claude Code