Carry index of InvalidDigit on IntErrorKind

[eopb]

@Lucretiel #22639 (comment)

Before stabilizing, would it be very complicated to additionally add the byte index where parsing failed? I can imagine that information being useful to an introspector (for example, to print a verbose rust-like arrow pointing to the exact location of the parse failure).

@ethanboxx #22639 (comment)

Should be easy to do. I will probably open a PR with that soon so people discuss it.

[rust-highfive]

r? @withoutboats

(rust-highfive has picked a reviewer for you, use r? to override)

[Dylan-DPC-zz]

r? @KodrAus

[KodrAus]

This seems reasonable to me. Maybe we could also update the Display implementation of ParseIntError to include the index and offending character encountered?

EDIT: Since we don’t have access to the original buffer just the index would be fine.

[KodrAus]

Thinking about this more, it might be better to try keep Kind a simple enum and optionally track an index on ParseIntError.

[eopb]

I agree that it would be nice to include the index in the Display implementation. I will add that soon.

I do not think placing the index as an Option on ParseIntError would be better. You lose the invariant that the index will only exist when the error is InvalidDigit and I feel that could add unnecessary API complexity.

I don't know what benefit keeping IntErrorKind as a simple enum would give.

[KodrAus]

@ethanboxx The way I see it, the benefits of carrying the index on ParseIntError rather than on IntErrorKind are:

• We don't paint ourselves into a semver corner, which enums with fields do
• We don't introduce a field that's only useful in niche cases (none of which seem to apply to the rustc codebase)
• We don't need to introduce the same field on any new variants that it might also apply to
• We have a way to introduce other pieces of information that may also be relevant, like the character itself that caused the error
• We can continue to use patterns like if kind() == IntErrorKind::InvalidDigit { .. }

[camelid]

Ping from triage: @ethanboxx could you address KodrAus's review? Thanks!

[Dylan-DPC-zz]

r? @dtolnay

[dtolnay]

I am not immediately sold on this. Can you provide more detail on the use case where you want to use this? Is there code you could show that you would want to be able to write?

[eopb]

@dtolnay

Use cases

An example use case would be to print an arrow pointing to the exact location of parsing failure as mentioned in #22639 (comment) where this feature was requested.

Another example could be writing a parser that ignores invalid characters.

Currently, the way of doing these things would be to rewrite the parsing logic in the local crate to plug in the index logic.

Why put this on IntErrorKind rather than ParseIntError?

Because I think that this code

if let Err(InvalidDigit(i)) = "1a0".parse::<i32>().map(ParseIntError::kind) {
     println!("Parsing failed at index: {}", i);
}

Is nicer than this code

if let Err(error) = "1a0".parse::<i32>() {
    if let Err(InvalidDigit) = error.kind() {
        let i = error.index().unwrap(); // Forced to unwrap
        println!("Parsing failed at index: {}", i);
    }
}

[dtolnay]

I think I'll go ahead and close this, since this API addition doesn't strike me as the most appropriate solution to either of those use cases.

An example use case would be to print an arrow pointing to the exact location of parsing failure as mentioned in #22639 (comment) where this feature was requested.

For parsing, ordinarily you need some variation of a lexer to decide what part of the input you are suppose to parse as a number in the first place. Consider input like {"id":987654321,"username":"ethanboxx"}. Some piece of logic needs to determine that the substring 987654321 is the correct thing to pass to u32::from_str, which it does by scanning ahead valid digits. Having from_str report an invalid digit position is only useful if you are planning to pass an entire suffix of the input to from_str i.e. 987654321,"username":"ethanboxx"}, then collect the invalid digit position, and run from_str a second time up to that position. This seems more confusing than just lexing the valid digits yourself up front.

The standard library already exposes the concept of "valid digit" by way of:

• u8::is_ascii_digit / char::is_ascii_digit (for base 10)
• u8::is_ascii_hexdigit / char::is_ascii_hexdigit (for base 16)
• char::is_digit (for arbitrary radix)

Another example could be writing a parser that ignores invalid characters.

Again I don't think an invalid digit position reported by from_str is really appropriate here. It sounds like you are planning to take an input containing a mix of valid and invalid digits, and run from_str on it in a loop, removing one invalid digit at a time. That seems needlessly complicated and confusing compared to filtering for valid digits up front (using the above set of methods) and running from_str a single time on only valid digits.

[camelid]

For parsing, ordinarily you need some variation of a lexer to decide what part of the input you are suppose to parse as a number in the first place. Consider input like {"id":987654321,"username":"ethanboxx"}. Some piece of logic needs to determine that the substring 987654321 is the correct thing to pass to u32::from_str, which it does by scanning ahead valid digits. Having from_str report an invalid digit position is only useful if you are planning to pass an entire suffix of the input to from_str i.e. 987654321,"username":"ethanboxx"}, then collect the invalid digit position, and run from_str a second time up to that position. This seems more confusing than just lexing the valid digits yourself up front.

For IntErrorKind this is true, but when lexing floats you may accept any number of .s and enforce that there's only one . when using from_str() (e.g., I do this in a compiler I'm working on). If f{32,64}::from_str() returned an error that included the location with an invalid character, you could easily show a more specific error location. So, I could see a feature like in this PR but for floats being more useful.