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.

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

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

Merged
merged 12 commits into from Nov 26, 2018

Conversation

Projects
None yet
@ethanboxx
Contributor

ethanboxx commented Nov 5, 2018

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

This comment has been minimized.

Collaborator

rust-highfive commented Nov 5, 2018

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.

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 5, 2018

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Nov 5, 2018

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:0c299c43:start=1541442553312489885,finish=1541442554430759872,duration=1118269987
$ 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:27] 
[00:04:27] error: This node does not have a stability attribute
[00:04:27]     --> libcore/num/mod.rs:4777:5
[00:04:27]      |
[00:04:27] 4777 |     InvalidDigit,
[00:04:27] 
[00:04:27] error: This node does not have a stability attribute
[00:04:27]     --> libcore/num/mod.rs:4778:5
[00:04:27]      |
---
[00:04:27]      |
[00:04:27] 4779 |     Underflow,
[00:04:27]      |     ^^^^^^^^^
[00:04:27] 
[00:04:27] error: This node does not have arive(Debug, Clone, PartialEq, Eq)]
[00:04:27] 
[00:04:28] error: missing documentation for a struct field
[00:04:28]     --> libcore/num/mod.rs:4771:5
[00:04:28]      |
---
[00:04:28] 
[00:04:28] error: missing documentation for an enum
[00:04:28]     --> libcore/num/mod.rs:4775:1
[00:04:28]      |
[00:04:28] 4775 | pub enum IntErrorKind {
[00:04:28] 
[00:04:28] error: missing documentation for a variant
[00:04:28]     --> libcore/num/mod.rs:4776:5
[00:04:[00:04:28] error: aborting due to 16 previous errors
[00:04:[00:04:28] error: aborting due to 16 previous errors
[00:04:28] 
[00:04:28] error: Could not compile `core`.
[00:04:28] 
[00:04:28] To learn more, run the command again with --verbose.
[00:04:28] 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:28] expected success, got: exit code: 101
[00:04:28] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
[00:04:28] travis_fold:end:stage0-std

[00:04:28] travis_time:end:stage0-std:start=1541442812513021635,finish=1541442832870690834,duration=20357669199


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

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:01ff4fb6
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:06a5fc34:start=1541442833541286653,finish=1541442833547343721,duration=6057068
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:041371b9
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:353eb540
travis_time:start:353eb540
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:23017a18
$ dmesg | grep -i kil

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)

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 5, 2018

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

@eddyb

This comment has been minimized.

Member

eddyb commented Nov 5, 2018

@frewsxcv frewsxcv added the T-libs label Nov 5, 2018

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Nov 5, 2018

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:09609df8:start=1541453069867097299,finish=1541453070878809379,duration=1011712080
$ 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:01] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:04:01] tidy error: /checkout/src/libcore/num/mod.rs:4775: line longer than 100 chars
[00:04:03] some tidy checks failed
[00:04:03] 
[00:04:03] 
[00:04:03] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:04:03] 
[00:04:03] 
[00:04:03] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:04:03] Build completed unsuccessfully in 0:00:50
[00:04:03] Build completed unsuccessfully in 0:00:50
[00:04:03] make: *** [tidy] Error 1
[00:04:03] Makefile:79: recipe for target 'tidy' failed

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:31d45bf8
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:04ff68fc:start=1541453326008673957,finish=1541453326012825227,duration=4151270
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00fc5463
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:0c2e5d35
travis_time:start:0c2e5d35
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0db2d7ae
$ dmesg | grep -i kill

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)

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Nov 5, 2018

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:00848b06:start=1541453930283672534,finish=1541453932994622969,duration=2710950435
$ 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:56]    Compiling unwind v0.0.0 (/checkout/src/libunwind)
[00:05:01] error[E0547]: missing 'issue'
[00:05:01]     --> libcore/num/mod.rs:4774:1
[00:05:01]      |
[00:05:01] 4774 | / #[unstable(feature = "int_error_matching",
[00:05:01] 4775 | |            reason = "it can be useful to match errors when making error messages \
[00:05:01] 4776 | |                      for integer parsing")]
[00:05:01] 
[00:05:02]    Compiling compiler_builtins v0.0.0 (/checkout/src/rustc/compiler_builtins_shim)
[00:05:02]    Compiling cmake v0.1.33
[00:05:02]    Compiling std v0.0.0 (/checkout/src/libstd)
---
[00:05:13] 
[00:05:13] To learn more, run the command again with --verbose.
[00:05:13] 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:05:13] expected success, got: exit code: 101
[00:05:13] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
[00:05:13] travis_fold:end:stage0-std

[00:05:13] travis_time:end:stage0-std:start=1541454238612260335,finish=1541454255852601034,duration=17240340699


[00:05:13] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:05:13] Build completed unsuccessfully in 0:00:18
[00:05:13] make: *** [all] Error 1
[00:05:13] Makefile:28: recipe for target 'all' failed
find: `/home/travis/Library/Logs/DiagnosticReports': No such file or directory
travis_time:end:1719c4a6:start=1541454256521174131,finish=1541454256526206890,duration=5032759
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_fold:start:after_failure.4
travis_time:start:15059780
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:049e07ca
travis_time:start:049e07ca
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:28289880
$ dmesg | grep -i kil

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)

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Nov 5, 2018

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:0697c323:start=1541455300905558340,finish=1541455301982113055,duration=1076554715
$ 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:29] 
[00:04:29] error: missing documentation for an enum
[00:04:29]     --> libcore/num/mod.rs:4780:1
[00:04:29]      |
[00:04:29] 4780 | pubm|     Underflow,
[00:04:29] 
[00:04:29] error: aborting due to 7 previous errors
[00:04:29] 
[00:04:29] error: Could not compile `core`.
[00:04:29] error: Could not compile `core`.
[00:04:29] 
[00:04:29] To learn more, run the command again with --verbose.
[00:04:29] 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:29] expected success, got: exit code: 101
[00:04:29] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
[00:04:29] travis_fold:end:stage0-std

[00:04:29] travis_time:end:stage0-std:start=1541455560642288556,finish=1541455581554374197,duration=20912085641


[00:04:29] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:04:29] Build completed unsuccessfully in 0:00:22
[00:04:29] Makefile:28: recipe for target 'all' failed
[00:04:29] make: *** [all] Error 1
56436 ./src/llvm/test/MC
51388 ./obj/build/x86_64-unknown-linux-gnu/stage0/bin
50128 ./src/llvm/test/CodeGen/X86
43808 ./src/libcompiler_builtins

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)

ethanboxx added some commits Nov 6, 2018

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Nov 6, 2018

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:1583ee00:start=1541493552633335169,finish=1541493628668214801,duration=76034879632
$ 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:21] 
[00:04:21] To learn more, run the command again with --verbose.
[00:04:21] 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:21] expected success, got: exit code: 101
[00:04:21] thread 'main' panicked at 'cargo must succeed', bootstrap/compile.rs:1101:9
[00:04:21] travis_fold:end:stage0-std

[00:04:21] travis_time:end:stage0-std:start=1541493879326450331,finish=1541493900183991471,duration=20857541140


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

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:179eefaf
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
---
travis_time:end:03955f36:start=1541493900866188328,finish=1541493900871916833,duration=5728505
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:16e3f5a0
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:072d9118
travis_time:start:072d9118
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:1788d25b
$ dmesg | grep -i kill

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)

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 6, 2018

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

This comment has been minimized.

Contributor

TimNN commented Nov 6, 2018

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 6, 2018

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)

@rkruppe

This comment has been minimized.

Contributor

rkruppe commented Nov 6, 2018

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.

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 6, 2018

@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

This comment has been minimized.

Contributor

SimonSapin commented Nov 6, 2018

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.

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 6, 2018

@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

This comment has been minimized.

Member

eddyb commented Nov 6, 2018

@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.

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 6, 2018

@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

This comment has been minimized.

Contributor

SimonSapin commented Nov 6, 2018

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.

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 6, 2018

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

This comment has been minimized.

Contributor

SimonSapin commented Nov 6, 2018

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?

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 6, 2018

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

This comment has been minimized.

Contributor

eddyp commented Nov 6, 2018

@ethanboxx I think you meant @eddyb

@sfackler

This comment has been minimized.

Member

sfackler commented Nov 6, 2018

Why would it be better than this method?

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

ethanboxx added some commits Nov 7, 2018

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 7, 2018

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

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 7, 2018

@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

This comment has been minimized.

Collaborator

rust-highfive commented Nov 7, 2018

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)

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 14, 2018

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.

@Aaronepower

This comment has been minimized.

Contributor

Aaronepower commented Nov 25, 2018

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

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 25, 2018

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

@ethanboxx

This comment has been minimized.

Contributor

ethanboxx commented Nov 25, 2018

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

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 25, 2018

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)

#[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)]

This comment has been minimized.

@frewsxcv

frewsxcv Nov 25, 2018

Member

Tangential, but this could also derive Copy

This comment has been minimized.

@SimonSapin

SimonSapin Nov 25, 2018

Contributor

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).

This comment has been minimized.

@frewsxcv

frewsxcv Nov 25, 2018

Member

Oh I missed the non_exhaustive attribute, nevermind then!

@SimonSapin

This comment has been minimized.

Contributor

SimonSapin commented Nov 25, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 2018

📌 Commit 121e5e8 has been approved by SimonSapin

@bors

This comment has been minimized.

Contributor

bors commented Nov 26, 2018

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

bors added a commit that referenced this pull request Nov 26, 2018

Auto merge of #55705 - ethanboxx:master, r=SimonSapin
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

This comment has been minimized.

Contributor

bors commented Nov 26, 2018

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

@bors bors merged commit 121e5e8 into rust-lang:master Nov 26, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment