Relax termination_trait's error bound

[U007D]

As per this conversation with @withoutboats and @bkchr

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @shepmaster (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.

[bkchr]

@U007D could you add a test with the following main:
fn main() -> Result<(), Box<Error>>
I hope that this finally works with your changes.

[U007D]

Done, for the positive (Ok) case:

fn main() -> Result<(), Box<Error>> { Ok(()) }

I'd also like to test the negative (Err) case:

fn main() -> Result<(), Box<Error>> { 
    Err(Box::new(Error::new(ErrorKind::Other, "returned Box<Error> from main()")))
}

but after looking at spending some time with both Error Info commands and Header commands I've come to the initial conclusion that maybe returning errors from main isn't possible because it's never been supported before? :) I also tried to look for ways to expect a status code of libc::EXIT_FAILURE, but didn't find anything.

Let me know if you know a way to include the negative test--with just the positive case, at least we know it compiles... :)

[bkchr]

@U007D Please add the negative case also :) Add the test to src/test/run-fail and that should work.

[U007D]

@bkchr I tried, and it didn't. Near as I can tell, it was expecting a panic, not a non-zero status code. I also tried:

• //~ ERROR returned Box<Error> from main() in the run-fail folder
• setting //must-compile-successfully in the ui folder
• using .stderr in each of the run-fail and ui folders

All failed for me. :(

[bkchr]

@U007D Look at run-fail/tls-exit-status.rs, especially the // error-pattern:nonzero should be the required param :)

[U007D]

Sweet. I'll try that.

[bkchr]

Ahh, I think that is wrong :( Are you somewhere on irc or so?

[U007D]

// error-pattern:nonzero
#![feature(termination_trait)]

use std::io::{Error, ErrorKind};

fn main() -> Result<(), Box<Error>> {
    Err(Box::new(Error::new(ErrorKind::Other, "returned Box<Error> from main()")))
}

gives:

test [run-fail] run-fail/termination-trait-for-result-box-error_err.rs ... FAILED

Any ideas?

[U007D]

@bkchr I am--U007D on irc.mozilla.org. (#rust-beginners and #rust channels)

[U007D]

@bkchr Well, it's past midnight here--I think I'll call it a night. You can leave any ideas you may have here or /msg me on IRC (I tend to leave my IRC client on).

[bkchr]

@U007D I would propose the following patch for compiletest:

diff --git a/src/tools/compiletest/src/runtest.rs b/src/tools/compiletest/src/runtest.rs
index dbeee39e60..175a829807 100644
--- a/src/tools/compiletest/src/runtest.rs
+++ b/src/tools/compiletest/src/runtest.rs
@@ -212,11 +212,9 @@ impl<'test> TestCx<'test> {
     }
 
     fn check_correct_failure_status(&self, proc_res: &ProcRes) {
-        // The value the rust runtime returns on failure
-        const RUST_ERR: i32 = 101;
-        if proc_res.status.code() != Some(RUST_ERR) {
+        if proc_res.status.success() {
             self.fatal_proc_rec(
-                &format!("failure produced the wrong error: {}", proc_res.status),
+                "failure must not return success exit status!",
                 proc_res,
             );
         }

And as test just:

#![feature(termination_trait)]

use std::io::{Error, ErrorKind};

fn main() -> Result<(), Box<Error>> {
    Err(Box::new(Error::new(ErrorKind::Other, "returned Box<Error> from main()")))
}

[U007D]

Thanks, @bkchr, that was helpful. I ended up having to do a couple of hours of poking around to make it work: In run-fail, the test did need // must-compile-successfully; without it, or in run-pass (which didn't seem like the right folder anyway), it would error out before getting to check_correct_failure_status() function: the results of exec_compiled_test() would fail the test run first.

Once I figured out which way was was up ;), your patch worked like a charm and the solution ended up being simple--it was a good learning process for me :).

TL;DR: There is now a negative test and it passes. :)

[U007D]

I also have the change ready for Debug->Display:

index dc7fa53aab..f2ae7e99df 100644
--- a/src/libstd/termination.rs
+++ b/src/libstd/termination.rs
@@ -8,7 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use fmt::Debug;
+use fmt::Display;
 #[cfg(target_arch = "wasm32")]
 mod exit {
     pub const SUCCESS: i32 = 0;
@@ -45,12 +45,12 @@ impl Termination for () {
 }
 
 #[unstable(feature = "termination_trait", issue = "43301")]
-impl<T: Termination, E: Debug> Termination for Result<T, E> {
+impl<T: Termination, E: Display> Termination for Result<T, E> {
     fn report(self) -> i32 {
         match self {
             Ok(val) => val.report(),
             Err(err) => {
-                eprintln!("Error: {:?}", err);
+                eprintln!("Error: {}", err);
                 exit::FAILURE
             }
         }

But I don't know the process well enough (i.e. does feature gating let us try different implementations on for size to see how we like them, or for stability's sake, do we make the decision before pushing the change?, How much consensus is needed first? etc.) to push it until someone gives me the go-ahead.

[bkchr]

If the branch is taken, the error code is always 0, so printing it is not required ;)

[U007D]

Ha! That is an excellent point! :)

[withoutboats]

But I don't know the process well enough (i.e. does feature gating let us try different implementations on for size to see how we like them, or for stability's sake, do we make the decision before pushing the change?, How much consensus is needed first? etc.) to push it until someone gives me the go-ahead.

The PR won't be merged until we reach a preliminary decision, its not particularly important what is pushed to your branch (you just might have to revert it).

[U007D]

@withoutboats Ah, great. I'll just go ahead and push my preferred impl, and will wait to hear back from folks re: feedback. And thanks for that information!

[scottmcm]

Another thought, here -- if part of the goal here is to let people turn .unwrap() into ? in tests and doctests and such, then that also suggests Debug, not anything stricter.

[withoutboats]

Nominated to talk about Display or Debug.

[withoutboats]

Lang team met today and discussed this. We agreed that this should use the Debug bound and printing, aiming this feature more at prototyping than at finished products.

We're also open to PRs to improve the readability of Debug impls on error types in std :)

[U007D]

Ok, I'll roll back the last commit and will re-submit with Debug as the bound.

Haha--I'm not sure I'm up for a PR just yet! ;) But I'll definitely continue to look for areas I can contribute. Thanks for all your guidance!

[nikomatsakis]

@bors r+

[bors]

📌 Commit 7948afd has been approved by nikomatsakis

[bors]

⌛ Testing commit 7948afd with merge d5cf0de2663003c2b9e3a42b95c1e1f846fc7f69...

[bors]

💔 Test failed - status-travis

[kennytm]

@bors retry

[bors]

⌛ Testing commit 7948afd with merge fc30141e836b571eaca4976f2f771e6cf81543ad...

[bors]

💔 Test failed - status-appveyor

[kennytm]

@bors retry

3 hour timeout

[bors]

⌛ Testing commit 7948afd with merge e8f03b9...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing e8f03b9 to master...

[U007D]

Thanks, @kennytm, for getting this landed! :)

FYI, I tried using the feature on Playground but it panics. It works locally, though--is this an issue I should open somewhere?

Update: fixed link to demo panic.

Playground reports:

Compiling playground v0.0.1 (file:///playground)
thread 'rustc' panicked at 'assertion failed: !substs.has_erasable_regions()', librustc_trans_utils/symbol_names.rs:192:9
note: Run with `RUST_BACKTRACE=1` for a backtrace.

error: internal compiler error: unexpected panic

note: the compiler unexpectedly panicked. this is a bug.

note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports

note: rustc 1.25.0-nightly (27a046e93 2018-02-18) running on x86_64-unknown-linux-gnu

error: Could not compile `playground`.

To learn more, run the command again with --verbose.

The same code running on my machine compiles and runs successfully.

[shepmaster]

Seems to work for me. What kind of panic are you seeing?

[U007D]

Running the code from the run-pass test was enough to get it to panic for me. I've fixed the link to reproduce the panic.

[kennytm]

@U007D please file an issue

[U007D]

Took my best guess and filed it here.