RUST-2028 Fix Decimal128 panic when parsing strings w/o a char boundary at idx 34 #496
Conversation
arthurprs
force-pushed
the
decimal128-parse-panic
branch
from
d50873b to
3e0dd2f
Compare
abr-egn
changed the title
src/decimal128.rs
Outdated
| if !s.is_char_boundary(precision) { | ||
| // a non-digit (all single byte utf8) would trigger a ParseIntError::InvalidDigit, | ||
| // so here we generate a ParseIntError::InvalidDigit kind of error too. | ||
| return Err(ParseError::InvalidCoefficient( |
There was a problem hiding this comment.
I think I'd rather have a new enum variant for ParseError (something like ParseError::Unparseable) than synthesize an error this way; that way it could also carry the source string to make debugging easier.
There was a problem hiding this comment.
I'm not saying I disagree, but no other error variants carry more information than this one.
InvalidDigit is arguably correct, as that would be the error if the split was character-based (instead of byte-based). The only reason I didn't do that is that it's more and slower code (linear vs. constant time).
There was a problem hiding this comment.
If we could directly construct an InvalidDigit I'd have less concern. My worry is that by synthesizing it this way it'll only probably continue to be InvalidDigit; the internals of ParseIntError are private so there are no guarantees of stability. If the error changes in the future to include the offending text, for example, it'll be very confusing for downstream users of this crate to see "❤" that doesn't actually appear in their data.
Adding a new variant to ParseError avoids that issue, and if we're doing that it may as well carry the text.
There was a problem hiding this comment.
I'll add a variant. But also, I'd like to point out that InvalidDigit (and any variant of ParseIntError for that matter) do not carry information about the input, such as the offending character.
Fix Decimal128 panic when parsing strings w/o a char boundary at idx 34 (Coefficient::MAX_DIGITS).