Make `ParseIntError` and `IntErrorKind` fully public #55705

eopb · 2018-11-05T18:26:28Z

Why would you write nice error types if I can't read them?

Why

It can be useful to use match with errors produced when parsing strings to int. This would be useful for the .err_match() function in my new crate.

I could also do this for ParseFloatError if people think it is a good idea.
I am new around hear so please tell me if I am getting anything wrong.

rust-highfive · 2018-11-05T18:26:32Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @bluss (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

eopb · 2018-11-05T18:27:43Z

#22639

eopb · 2018-11-05T18:35:31Z

oops, I think I am going to need more help than I thought with this one.

eddyb · 2018-11-05T18:46:00Z

cc @rust-lang/libs

eopb · 2018-11-06T08:52:50Z

I can't work out what the problem is. Is this just travis falling over or is it a problem with my code? @TimNN

TimNN · 2018-11-06T09:04:11Z

Looking at the raw log, it looks like pub kind: IntErrorKind!is missing a doc comment.

src/libcore/num/mod.rs

rust-highfive · 2018-11-06T09:36:30Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:0cf0de34:start=1541496665903864765,finish=1541496728415448820,duration=62511584055
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
151412 ./src/tools/clang
150256 ./obj/build/bootstrap/debug/incremental
149124 ./src/llvm-emscripten/test
134668 ./obj/build/bootstrap/debug/incremental/bootstrap-zemjd6kcyh2u
134664 ./obj/build/bootstrap/debug/incremental/bootstrap-zemjd6kcyh2u/s-f6exvxmw7d-11lq9t-22tmsi8iacpi9
107892 ./obj/build/x86_64-unknown-linux-gnu/stage0/lib/rustlib/x86_64-unknown-linux-gnu/codegen-backends
104700 ./src/tools/lldb
93748 ./src/tools/clang/test
72532 ./src/llvm/lib

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

hanna-kruppe · 2018-11-06T09:38:56Z

Before doing more work, maybe wait for someone from the libs team to respond? As far as I know, these sorts of types are generally very intentionally not fully public, to allow adding more information later. That's particularly important for ParseFloatError whose ErrorKind enum is far, far less helpful than it could be.

#22639 has a point, but could be solved by just making ErrorKind public and #[non_exhaustive], which forbids people from matching on it exhaustively and therefore makes it possible to add more variants or more information later.

eopb · 2018-11-06T09:51:21Z

@rkruppe Thanks for the information. It does sound like #[non_exhaustive] it the way to go. I will wait for someone from the libs team before continuing.

SimonSapin · 2018-11-06T12:09:11Z

Why is this more useful than impl Display for ParseIntError? As far as I can tell read_input::InputBuilder::err_match is all about formatting an error message as a string.

eopb · 2018-11-06T12:23:09Z

@SimonSapin The messages made by impl Display for ParseIntError may not be fit for input. For instance if IntErrorKind::Empty is passed into impl Display for ParseIntError it will output "cannot parse integer from empty string". This is useful for debug purposes but is not fit as an error for an end user. A better error could be "I can not see any input. Please input again.".

Having ParseIntError as public will stop ugly patterns like this example @amosbatto gave in #22639

match my_str.parse::<i32>() {
	Ok(x) => {my_int = x;},
	Err(e) => {
		if e.to_string() == "invalid digit found in string" {
			println!("Only positive integers are allowed!");
		}
		else if e.to_string() == "number too large to fit in target type" {
			println!("Number is too large!");
		}
		else if e.to_string() == "number too small to fit in target type" {
			println!("Number is too small!");
		}
		else if e.to_string() == "cannot parse integer from empty string" {
			println!("Number is empty!");
		}
	}
}

eddyb · 2018-11-06T14:17:53Z

@ethanboxx Quick nit: e.to_string() shouldn't be repeatedly like that, since it allocates. I don't know if there's a .description() method you could use instead, but if you use .to_string(), you should cache the output.

eopb · 2018-11-06T15:53:13Z

@eddyp At the end of the day, that code example would never pass a review even if .description() was used.

We need it to be able to match the error with enum matching for this to be nice clean code.

SimonSapin · 2018-11-06T17:07:42Z

Exposing the enum sounds ok (especially with non_exhaustive) but it should probably be through an accessor method rather than a public field. This would allow adding more fields to the error type in the future.

eopb · 2018-11-06T19:09:56Z

That sounds all right. Before I try it can you please give me a reason why a new field may be added to ParseIntError. Shouldn't new errors end up in the enum making the struct a little redundant? @SimonSapin

SimonSapin · 2018-11-06T19:42:59Z

This is being very conservative, quite possibly we’ll never change this type again. But one example could be providing further details, for example in the InvalidDigit case: what character was found that is not a digit? At what position?

eopb · 2018-11-06T20:42:32Z

Would it not be better to store things like "what digit was invalid" in the enum.

#[non_exhaustive]
pub enum ParseIntError {
    Empty,
    InvalidDigit,
    Overflow,
    Underflow,
}

would become

#[non_exhaustive]
pub enum ParseIntError {
    Empty,
    InvalidDigit(char),
    Overflow,
    Underflow,
}

or to preserve backward compatibility

#[non_exhaustive]
pub enum ParseIntError {
    Empty,
    SomeInvalidDigit(char),
    /// `InvalidDigit` is no longer ever constructed
    InvalidDigit,
    Overflow,
    Underflow,
}

A new struct field sounds confusing to me. Why would it be better than this method? @SimonSapin

eddyp · 2018-11-06T21:59:05Z

@ethanboxx I think you meant @eddyb

sfackler · 2018-11-06T22:06:18Z

Why would it be better than this method?

It wouldn't break literally all code that matches on ParseIntError::InvalidDigit for one.

eopb · 2018-11-07T08:41:30Z

@eddyp Thank you for taking your time correcting my mistake.

eopb · 2018-11-07T08:45:12Z

@sfackler @SimonSapin I didn't think of that. Thank you for the suggestion. I have now implemented the accessor method. What do you think now?

rust-highfive · 2018-11-07T08:45:49Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:1eaa3ed8:start=1541580016565814781,finish=1541580078217353319,duration=61651538538
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---
[00:04:07]    Compiling rustc_tsan v0.0.0 (/checkout/src/librustc_tsan)
[00:04:15] error: This node does not have a stability attribute
[00:04:15]     --> libcore/num/mod.rs:4799:5
[00:04:15]      |
[00:04:15] 4799 | /     pub fn kind(self) -> IntErrorKind {
[00:04:15] 4800 | |         self.kind
[00:04:15]      | |_____^
[00:04:15] 
[00:04:15] error: aborting due to previous error
[00:04:15] 
[00:04:15] 
[00:04:16] error: Could not compile `core`.
[00:04:16] 
[00:04:16] To learn more, run the command again with --verbose.
[00:04:16] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:04:16] expected success, got: exit code: 101
[00:04:16] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
[00:04:16] travis_fold:end:stage0-std

[00:04:16] travis_time:end:stage0-std:start=1541580323734497013,finish=1541580345307811196,duration=21573314183


[00:04:16] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:16] Build completed unsuccessfully in 0:00:22
[00:04:16] Makefile:28: recipe for target 'all' failed
[00:04:16] make: *** [all] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:22abf800
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

eopb · 2018-11-14T10:06:43Z

Is there a reason why activity on this thread has stopped? I would like to know if my code has any ways in which it can be improved. It is fine if this is a low priority, but I would like to know if it is.

XAMPPRocky · 2018-11-25T15:40:54Z

Triage; @SimonSapin Hello, have been able to get back to this PR?

SimonSapin · 2018-11-25T18:14:23Z

Sorry for the delay. Since neither type is Copy, maybe the signature should be fn kind(&self) -> &IntErrorKind?

eopb · 2018-11-25T19:34:55Z

@SimonSapin I think that is the correct thing to do. I have updated the code to do what you suggested.

rust-highfive · 2018-11-25T19:38:46Z

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

travis_time:end:058b5265:start=1543174394559408517,finish=1543174447482399712,duration=52922991195
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

frewsxcv · 2018-11-25T21:00:55Z

src/libcore/num/mod.rs

+#[unstable(feature = "int_error_matching",
+           reason = "it can be useful to match errors when making error messages \
+                     for integer parsing",
+           issue = "22639")]
 #[derive(Debug, Clone, PartialEq, Eq)]


Tangential, but this could also derive Copy

As always, this is a trade-off between being slightly more convenient to users today v.s. being more flexible to library maintainers in the future (in the kind of variants we can add to this non-exhaustive enum).

Oh I missed the non_exhaustive attribute, nevermind then!

SimonSapin · 2018-11-25T21:33:34Z

@bors r+

bors · 2018-11-25T21:33:35Z

📌 Commit 121e5e8 has been approved by SimonSapin

bors · 2018-11-26T01:46:25Z

⌛ Testing commit 121e5e8 with merge 423291f...

Make `ParseIntError` and `IntErrorKind` fully public Why would you write nice error types if I can't read them? # Why It can be useful to use `match` with errors produced when parsing strings to int. This would be useful for the `.err_match()` function in my [new crate](https://crates.io/crates/read_input). --- I could also do this for `ParseFloatError` if people think it is a good idea. I am new around hear so please tell me if I am getting anything wrong.

bors · 2018-11-26T04:43:40Z

☀️ Test successful - status-appveyor, status-travis
Approved by: SimonSapin
Pushing 423291f to master...

Make ParseIntError and IntErrorKind fully public

2d1cd9a

rust-highfive assigned bluss Nov 5, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 5, 2018