rust 🚀 - Tracking issue for RFC 1937: `?` in `main`

How are exit statuses going to be dealt with?

ghost on 18 Jul 2017

This comment by @Screwtapello seems to have been made too close to the end of FCP for any alterations to be made to the RFC in response to it.

In short: the RFC proposes returning 2 on failure on grounds which, while well-founded, are obscure and produce a slightly unusual result; the least surprising thing is to return 1 when the program has no indication it wants any more detail than just success or failure. Is this sufficiently bikesheddy that it can be discussed without it feeling like we're perverting the RFC process, or are we now locked into this specific implementation detail?

ketsuban on 18 Jul 2017

It's not an implementation detail though, is it?

Some scripts use exit codes as a way to get information from a sub-process.

ghost on 18 Jul 2017

This is specifically about the case when a sub-process (implemented in Rust) has no information to give, beyond a binary "everything's fine"/"something went wrong".

Screwtapello on 18 Jul 2017

Some scripts use exit codes as a way to get information from a sub-process.

That behavior is always extremely dependent on the program being called _except_ in that non-zero means failure. Given that std::process::exit with a main-function wrapper and a lookup table is going to remain the best option for those who want a more articulate exit status no matter what is done, this seems like a mostly insignificant detail.

NoraCodes on 18 Jul 2017

I don't think SemVer has a "mostly insignificant detail" exception though.

ghost on 18 Jul 2017

I think the exit code should be added to the unresolved questions list. @zackw also opened a related internals thread.

glaebhoerl on 18 Jul 2017

👍1

Many people agree that the exit code should be 1 on failure (instead of 2):
https://www.reddit.com/r/rust/comments/6nxg6t/the_rfc_using_in_main_just_got_merged/

liigo on 11 Aug 2017

👍14

@arielb1 are you going to implement this rfc?

bkchr on 19 Sep 2017

@bkchr

No, just to mentor it. I assigned so I won't forget to write the mentoring notes.

arielb1 on 19 Sep 2017

Ahh nice, I would be interested in doing it :)
But, I don't have any idea where to start :D

bkchr on 19 Sep 2017

@bkchr

That's why I'm here :-). I should write the mentoring instructions soon enough.

arielb1 on 19 Sep 2017

👍1

Okay, then I'm waiting for your instructions.

bkchr on 19 Sep 2017

Mentoring Instructions

This is a [WG-compiler-middle] issue. If you want to seek help, you can join #rustc on irc.mozilla.org (I'm arielby) or https://gitter.im/rust-impl-period/WG-compiler-middle (I'm @arielb1 there).

There's a WIP compiler readme at #44505 - it describes some things in the compiler.

Work plan for this RFC:

[ ] - add the Termination lang-item to libcore
[ ] - allow using Termination in main
[ ] - allow using Termination in doctests
[ ] - allow using Termination in #[test]

add the `Termination` lang-item to libcore

First, you need to add the Termination trait to libcore/ops/termination.rs, along with some documentation. You'll also need to mark it as unstable with an #[unstable(feature = "termination_trait", issue = "0")] attribute - this will prevent people from using it before it is stabilized.

Then, you need to mark it as a lang-item in src/librustc/middle/lang_items.rs. This means the compiler can find it out when type-checking main (e.g. see 0c3ac648f85cca1e8dd89dfff727a422bc1897a6).
That means:

adding it to the lang-items list (in librustc/middle/lang_items.rs)
adding a #[cfg_attr(not(stage0), lang = "termination")] to the Termination trait. The reason you can't just add a #[lang = "termination"] attribute is because the "stage0" compiler (during bootstrapping) won't know termination is something that exists, so it won't be able to compile libstd. We'll manually remove the cfg_attr when we update the stage0 compiler.
See bootstrapping docs at XXX for details.

allow using `Termination` in main

This is the interesting part I know how to deal with. This means making a main that returns () not type-check (currently you get a main function has wrong type error) and work.

To make it type-check, you first need to remove the existing error in:
https://github.com/rust-lang/rust/blob/9a00f3cc306f2f79bfbd54f1986d8ca7a74f6661/src/librustc_typeck/lib.rs#L171-L218

Then, you need to add a check that the return type implements the Termination trait in (you add a trait obligation using register_predicate_obligation - search for uses of that). That can be done here:
https://github.com/rust-lang/rust/blob/9a00f3cc306f2f79bfbd54f1986d8ca7a74f6661/src/librustc_typeck/check/mod.rs#L1100-L1108

The other part is making it work. That should be rather easy. As the RFC says, you want to make lang_start generic over the return type.

lang_start is currently defined here:
https://github.com/rust-lang/rust/blob/9a00f3cc306f2f79bfbd54f1986d8ca7a74f6661/src/libstd/rt.rs#L32

So you'll need to change it to be generic and match the RFC:

#[lang = "start"]
fn lang_start<T: Termination>
    (main: fn() -> T, argc: isize, argv: *const *const u8) -> !
{
    use panic;
    use sys;
    use sys_common;
    use sys_common::thread_info;
    use thread::Thread;

    sys::init();

    sys::process::exit(unsafe {
        let main_guard = sys::thread::guard::init();
        sys::stack_overflow::init();

        // Next, set up the current Thread with the guard information we just
        // created. Note that this isn't necessary in general for new threads,
        // but we just do this to name the main thread and to give it correct
        // info about the stack bounds.
        let thread = Thread::new(Some("main".to_owned()));
        thread_info::set(main_guard, thread);

        // Store our args if necessary in a squirreled away location
        sys::args::init(argc, argv);

        // Let's run some code!
        let exitcode = panic::catch_unwind(|| main().report())
            .unwrap_or(101);

        sys_common::cleanup();
        exitcode
    });
}

And then you'll need to call it from create_entry_fn. Currently, it instantiates a monomorphic lang_start using Instance::mono, and you'll need to change it to use monomorphize::resolve with the right substs.

https://github.com/rust-lang/rust/blob/9a00f3cc306f2f79bfbd54f1986d8ca7a74f6661/src/librustc_trans/base.rs#L697

allow using `Termination` in doctests

I don't really understand how doctests work. Maybe ask @alexcrichton (that's what I would do)?

allow using `Termination` in `#[test]`

I don't really understand how libtest works. Maybe ask @alexcrichton (that's what I would do)? Unit tests are basically generated by a macro, so you need to change that macro, or its caller, to handle return types that are not ().

arielb1 on 19 Sep 2017

@bkchr

Can you at least join the IRC/gitter?

arielb1 on 19 Sep 2017

@bkchr just checking in -- I saw you and @arielb1 were conversing on gitter some time back, any progress? Get suck somewhere?

nikomatsakis on 25 Sep 2017

No sorry, no progress up to now. Currently I have a lot of things to do, but I hope that I will find some time this week to start on this.

bkchr on 25 Sep 2017

👍2

@bkchr If you need some help let me know!

Freyskeyd on 7 Oct 2017

I'm currently a little bit stuck, I want to create the Obligation. To create the Obligation I need a TraifRef, for a TraitRef I need a DefId. Can someone point me to some code on how to create a DefId from the Termination Trait?

bkchr on 8 Oct 2017

@bkchr The trait should be added to the lang item list, e.g.: https://github.com/rust-lang/rust/blob/ade0b01ebf18550e41d24c6e36f91afaccd7f389/src/librustc/middle/lang_items.rs#L312
and get marked with #[termination_trait], e.g.: https://github.com/rust-lang/rust/blob/ade0b01ebf18550e41d24c6e36f91afaccd7f389/src/libcore/fmt/mod.rs#L525-L526

eddyb on 8 Oct 2017

Yeah that is not the problem, I already done that. I need to check for the termination trait in the check_fn function. I want to use register_predicate_obligation and for that I need the defid of the termination trait.

bkchr on 8 Oct 2017

Oh, then all you need is tcx.require_lang_item(TerminationTraitLangItem).

eddyb on 8 Oct 2017

👍1

@bkchr how goes? Just checking in again. =) No worries if you are busy, just want to make sure you're getting all the help you need.

nikomatsakis on 16 Oct 2017

Sorry, busy at the moment :/ Up to now, I got all the help I needed :)

bkchr on 17 Oct 2017

This is the code to check for the TerminationTrait: https://github.com/bkchr/rust/blob/f185e355d8970c3350269ddbc6dfe3b8f678dc44/src/librustc_typeck/check/mod.rs#L1108

I think that I not checking on the return type of the function? I get the following error:

error[E0277]: the trait bound `Self: std::ops::Termination` is not satisfied
  --> src/rustc/rustc.rs:15:11
   |
15 | fn main() { rustc_driver::main() }
   |           ^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::ops::Termination` is not implemented for `Self`
   |
   = help: consider adding a `where Self: std::ops::Termination` bound

What does I need to change, to check on the return type of the function?

bkchr on 18 Oct 2017

@bkchr I'd recommend joining the compiler-middle working group gitter at https://gitter.im/rust-impl-period/WG-compiler-middle for feedback, as well as trying the #rust-internals IRC channel at https://chat.mibbit.com/?server=irc.mozilla.org%3A%2B6697&channel=%23rust-internals . :)

bstrie on 20 Oct 2017

@bstrie yeah thanks, I'm already part of the gitter chat and could solve my problem. :)

bkchr on 20 Oct 2017

@bkchr your problem is on this line. The trait reference you want to build there is something like R: Termination where R is the return type of the function. This is specified by building up an appropriate "substs", which is the set of values to substitute for the trait's type parameters (in this case, Self).

However, you are invoking the Substs::identity_for_item method on the trait. This will give you back the substitutions one would use inside the trait definition itself. i.e., in this case you are mapping the Self parameter declared on the Termination trait to Self. This would be appropriate if you were checking the definition of some function inside the Terminator trait, but not so much here.

What you want instead is to get the return type of the entry function. This is just one of the variables ret_ty or actual_ret_ty. Either is fine, but I guess ret_ty is better -- that corresponds to the return type that the user declared (whereas actual_ret_ty is the type the actual code returned).

You can make the substs you want by just calling the mk_substs() method from the tcx. In this case, there is only one parameter, the type, so something like let substs = fcx.tcx.mk_substs(&[ret_ty]); would work, I think.

nikomatsakis on 30 Oct 2017

I believe the thing to use is tcx.mk_substs_trait(ret_ty, &[]).

eddyb on 31 Oct 2017

@bkchr just checking in -- had a chance to put that advice to use? (Also, for faster responses, it may be wise to ask on gitter.)

nikomatsakis on 1 Nov 2017

Yeah, I could solve the problem with gitter :)

bkchr on 2 Nov 2017

@bkchr how goes? Just checking in.

nikomatsakis on 7 Nov 2017

Everything okay, I will probably get some time this week to look into the code.

bkchr on 7 Nov 2017

❤2

Is there room for one more person to help with this? I would like to begin contributing to the Rust community before year's end and would love to help with this feature. Hopefully it wouldn't be too confusing to have two people collaborating on this.

U007D on 26 Nov 2017

@U007D

This is a small feature and @bkchr is almost done with it.

arielb1 on 26 Nov 2017

Ah, ok--that's good to know, thanks. I'll keep an eye out for something else I can help with.

U007D on 26 Nov 2017

@U007D Have you seen https://www.rustaceans.org/findwork ?

lnicola on 26 Nov 2017

@lnicola Yes, I have! I'm trying to find something at the intersection of something I feel confident about being able to work on (ie. be a net positive) and I'm passionate about. To be honest, even though I've been learning Rust for about a year, it's still a little intimidating to step up to volunteer for something. FWIW, that is by no means the fault of the Rust community--the Rust community has bent over backwards to make this an open, welcoming and inclusive culture--the best I've had the pleasure of experiencing. (I suspect it has more to do with old battle scars from years and years of experience in the tech industry where teams tend to be competitive rather than collaborative.)

Anyway, it's my goal to pick something this year and to at least begin making a positive contribution. It's time for me to get involved! :)

Thanks for the suggestion, @lnicola. That is a good resource.

U007D on 26 Nov 2017

❤2

@bkchr any updates?

Manishearth on 16 Dec 2017

I'm on it (https://github.com/rust-lang/rust/pull/46479). Now I have holidays and time to work in the comments in the pull request. Sorry for all the delays :/

bkchr on 17 Dec 2017

Oh, sorry, didn't notice you had a pull request up. Cross-linked it.

Manishearth on 17 Dec 2017

Hello, uhm. So I thought I'd start my potential Rust contributor career by bikeshedding, as is tradition. Specifically, about this one:

[ ] The name of the trait being introduced

How about Exit? It's short and to the point, fitting existing Rust vocabulary. Exit-as-a-noun is a natural counterpart to exit-as-a-verb which, to most, is the familiar word for ending a process "from inside" in a controlled manner.

To a C++ programmer specifically, "termination" brings to mind std::terminate which defaults to abnormal termination (calling abort) and is basically the C++ equivalent to a panic (but unlike a panic, never unwinds the stack).

jdahlstrom on 24 Dec 2017

👍2

Wait, ignore that comment, looks like the RFC left that explicitly open to discussion.

I do like Exit as a trait name.

Manishearth on 24 Dec 2017

I'm figuring that the feature will get stabilized far before the trait does, like happened with Carrier.

scottmcm on 27 Dec 2017

FWIW that's another case where I'm really glad that the provisional name was changed before stabilization :D

jdahlstrom on 27 Dec 2017

As the author of the RFC I have no objection to changing the name of the trait to Exit, or anything else really. I'm not particularly good at naming things and happy for someone else to have a better idea.

zackw on 27 Dec 2017

👍5

https://github.com/rust-lang/rust/blob/5f7aeaf6e2b90e247a2d194d7bc0b642b287fc16/src/libstd/lib.rs#L507

Is the trait supposed to be

placed in libstd instead of libcore, and
just called std::Termination, not std::ops::Termination?

kennytm on 28 Dec 2017

The trait could not be placed into libcore, because the implementation for Result requires to print to stderr and that can not be done in libcore.

bkchr on 28 Dec 2017

@bkchr The impl being in libstd doesn't mean the trait needs to be in libstd as well.

kennytm on 28 Dec 2017

@kennytm I know, but Result is also defined in libcore, so Termination can not be implemented for Result in libstd.

bkchr on 28 Dec 2017

👍1

@zackw +1 more vote for Exit as the trait name.

U007D on 29 Dec 2017

@U007D: Could you please use the reactions button (e.g., 👍) instead of posting such a message? That’d let you avoid annoying issue subscribers by pinging them needlessly.

sanmai-NL on 29 Dec 2017

👍5

Can I check in libtest/libsyntax, if a language_feature is activated(in a crate)? @arielb1 @nikomatsakis @alexcrichton

bkchr on 7 Jan 2018

@bkchr in libsyntax you may have to pass it in but it's in theory possible, but in libtest itself at runtime I don't believe you can check.

alexcrichton on 8 Jan 2018

@bkchr how goes here?

nikomatsakis on 11 Jan 2018

I'm still working on it, but currently I don't have more questions :)

bkchr on 12 Jan 2018

I think this impl is too strict:

#[unstable(feature = "termination_trait", issue = "43301")]
impl<T: Termination, E: Error> Termination for Result<T, E> {
    fn report(self) -> i32 {
        match self {
            Ok(val) => val.report(),
            Err(err) => {
                print_error(err);
                exit::FAILURE
            }
        }
    }
}


#[unstable(feature = "termination_trait", issue = "43301")]
fn print_error<E: Error>(err: E) {
    eprintln!("Error: {}", err.description());

    if let Some(ref err) = err.cause() {
        eprintln!("Caused by: {}", err.description());
    }
}

There are several commonly used errors that do not implement Error, most importantly Box<::std::error::Error> and failure::Error. I also think it is a mistake to use the description method instead of the display impl of this error.

I'd propose to replace this impl with this broader impl:

#[unstable(feature = "termination_trait", issue = "43301")]
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)
                exit::FAILURE
            }
        }
    }
}

withoutboats on 16 Jan 2018

👍2

That does lose the cause chain which is a bummer.

Using the display impl rather than description is definitely the better thing to do though.

sfackler on 16 Jan 2018

The cause chain is an interesting problem. In particular, this implementation only prints the first two members of the cause chain.

failure's had to deal with how to handle the cause chain and settled on this behavior by default (e.g. if you just build up errors using .context:

{} prints only this error
{:?} prints this error as well as its cause (recursively)

We could decide to use :? here and bound it Debug instead of Display. Unsure.

withoutboats on 16 Jan 2018

Yeah, I already know that I need to improve the impl to support. I'm open on what we could do. Binding to Debug might be a good idea.

bkchr on 16 Jan 2018

Hmm, this is a tricky one. I guess it depends on whether we think that a "polished" program will make use of this trait impl. I tend to think it's ok to say that "no, they will not" -- basically, a polished program will either (a) catch the output and handle it in some other way or (b) use some newtype or something that implements Debug the right way. This would mean we can optimize the impl for dumping useful information but necessarily in the prettiest form (which seems like the role of Debug).

nikomatsakis on 17 Jan 2018

👍2 👎1

It might be the right choice to make this very clearly targeted at prototyping by using Debug, since I don't think we could ever automatically handle errors in a way that is correct for most production use cases.

withoutboats on 17 Jan 2018

👎1 👍1

@withoutboats I agree.

nikomatsakis on 18 Jan 2018

👎1

@nikomatsakis I assume you meant "not necessarily in the prettiest form"? If so, yes, I agree.

Update: after working on this for a couple of days, I've flipped on this. See below.

U007D on 18 Jan 2018

:+1: on Debug, here; I like @nikomatsakis's "kind of analogous to an uncaught exception" from https://github.com/rust-lang/rfcs/pull/1937#issuecomment-284509933. A comment from Diggsey also suggesting Debug: https://github.com/rust-lang/rfcs/pull/1937#issuecomment-289248751

scottmcm on 19 Jan 2018

FYI, I've flipped on the "more complete" vs "more user-friendly" default (ie. Debug vs Display trait bound) issue.

The TL;DR is I now believe we should set the bound to be on Display (as per @withoutboats' original post) to provide cleaner, summary output in the 'do nothing' case.

Here's my rationale:

On the termination trait's RFC issue @zackw makes the compelling point that Rust has the dual panic/Result system because panics are for bugs and Results are for errors. From this, I think a compelling case can be made for evaluating default error presentation independently of default panic presentation.

Of course there is no one default that will satisfy everyone, so applying the principle of least surprise, I ask myself which default is more appropriate?

An error is often not handled by design, in that it's intended to be communicated to the user that something (possibly fixable) went wrong (file not found, etc.). As such, the use case exists and could reasonably be considered common that the user is the intended audience.
As @nikomatsakis pointed out, regardless of the default we choose, any developer wishing to change the behavior can either use the newtype pattern or develop a custom implementation in main().

And finally on the more subjective side, In working with this feature over the past couple of days, I've found the Debug output left me feeling that my "Rust app" felt more unpolished:

$ foo
Error: Custom { kind: Other, error: StringError("returned Box<Error> from main()") }
$

vs

$ foo
Error: returned Box<Error> from main()
$

The Dispay trait just seems to be a much more civilized default for an error (as opposed to a bug), does it not?

U007D on 19 Jan 2018

👍3

@U007D wait, which of those two outputs do you prefer?

(a) Error: Custom { kind: Other, error: StringError("returned Box<Error> from main()") }

or

(b) Error: returned Box<Error> from main()

nikomatsakis on 19 Jan 2018

He prefers option (b).

bkchr on 19 Jan 2018

@nikomatsakis Originally, I was fine with a) Debug as a concept in my head, but after working with it for a couple of days actually seeing the output, I now prefer b) Display as a default. I think my preference for b) would become even more pronounced if I were modeling a chained error.

U007D on 19 Jan 2018

I don't think "polished" or "civilized" is the goal of this, though, since I understood the thread to already have accepted this as being mostly for examples, with people fully expected to add custom handling as the program matures.

In those cases, to me "least surprise" is a developer-oriented output just like unwrap.

Would it be worth discussing {:#?} here, if there's concern about a long error?

scottmcm on 20 Jan 2018

Error-reporting for end-users is going to be different for every tool and every use-case, but error-reporting for developers should resemble what Rust does in other situations like .unwrap(). Since there can only be one default, and polished software will need to override the output anyway, I vote for the default being developer-oriented with Debug.

Screwtapello on 20 Jan 2018

I think the heart of this discussion is really "who is the target audience for the default message?"

Let's say for a moment that we all agreed that the default target audience was developers. I think the Debug default bound would be a straightforward choice.

Now let's say for a moment that we agreed that the default target audience was the user, then this is where, respecfully, I disagree with some others and feel that subjective qualities like "polish" and "civilized" output do have an important part to play. For some, end-user presentation "polish" may be the best reason of all for avoiding Display. (I don't happen to share that view, but I understand and respect it.)

So yes, I can certainly see reasonable arguments for either group as the default target. I think if strong consensus develops around which audience should be the default target, then the choice for the trait bound will be clear (er)... :)

U007D on 20 Jan 2018

(I'm not fully versed in this whole topic, but) is it not conceivable that there could be small utilities for which the default error output with Termination would be perfectly adequate, provided it's in some user-presentable format like Display? In that case, the only reason authors would have to reach for "custom handling" is if we make them.

Can someone provide examples of what the output looks like in each case (I'm assuming it also depends on the particular E type used?), and what steps authors actually need to take if they want "custom handling" instead? I'm just going on hypotheticals above.

(Does the output literally look like what @U007D pasted above? Why would it print "returned Box\Box<Error>?)

glaebhoerl on 20 Jan 2018

How often is even the Display of the error message user-friendly enough? For instance, the following program:

fn main() {
    if let Err(e) = std::fs::File::open("foo") {
        println!("{}", e)
    }
}

emits the following message:

No such file or directory (os error 2)

Which, I'd say, is not great UX, especially with the file name not mentioned. At least not unless the program literally takes a single filename as input. On the other hand, it's not great developer experience either, missing the source file/line number/stack trace. The Debug output is, obviously, even worse user experience, and adds no useful information for the deceloper, either.

So I guess what I'm trying to say is that without improving the information content of the library errors themselves, neither Debug nor Display is great.

jdahlstrom on 20 Jan 2018

👍1

Does the output literally look like what @U007D pasted above? Why would it print "returned Box from main()" instead of... the actual contents of that Box?

@glaebhoerl You are correct--in this case, the "actual contents of that Box<Error>" was a custom message field I'd created to test the termination_trait, displayed verbatim. I could have written "foo bar baz" or anything else in there instead (but that might not have been as useful to a user running the compiler tests).

Using @jdahlstrom's example, here is the actual output for a Boxed standard library "file not found" Error (note, as you correctly point out, no mention of boxing anywhere):
Debug:

$ foo
Error { repr: Os { code: 2, message: "No such file or directory" } }
$

and Display:

$ foo
No such file or directory (os error 2)
$

@jdahlstrom I think you make a good point. I agree that while both messages may underserve their target audience and I want to highlight that providing the wrong one is even worse (as I think you alluded to):

Providing Display to a developer has all the downsides of Debug plus misses the specificity of what error type is even being displayed.

Providing Debug to a user has all the downsides of Display plus adds even more technical information the user does not need and may not be able to understand.

So yes, I agree the messages are often not targeted enough, to either audience. I think this highlights another important reason for us to be clear about who we are targeting so we provide the best experience we can (flaws notwithstanding) for that group.

U007D on 20 Jan 2018

👍2

I require some help for implementing the support for ? in #[test]. My current implementation can be found here: https://github.com/rust-lang/rust/compare/master...bkchr:termination_trait_in_tests

Compiling a test with my changes result in the following error:

error: use of unstable library feature 'test' (see issue #27812)
  |
  = help: add #![feature(test)] to the crate attributes to enable

@eddyb said I should not use quote_item!/expr! anymore, because they are legacy.
What should I do now, switch to the new quote! macro or rework everything to the manual ast building?

I appreciate any help :)

bkchr on 21 Jan 2018

I think that generating a macro invocation to some macro defined in libtest could work very well.

eddyb on 21 Jan 2018

@eddyb I'm not sure I understand your suggestion here:

I think that generating a macro invocation to some macro defined in libtest could work very well.

Oh, I guess maybe I do. You're saying -- define a macro in libtest and then generate code that invokes it? Interesting idea. Wouldn't that macro name kind of "leak" out though? (i.e., it becomes part of the public interface of libtest?)

@bkchr

Compiling a test with my changes result in the following error:

Do you have any idea why that error is getting generated? Just from reading the diff, I do not, but I can try to build locally and figure it out.

I should not use quote_item!/expr! anymore, because they are legacy.

I don't have a strong opinion here. I agree with @eddyb they are legacy, but I'm not sure if they're the sort of legacy where adding a few more uses will make them harder to remove -- i.e., once we get a recent replacement, would it be easy @eddyb to move from one to the other?

Manual AST building is certainly a pain, though I guess we have some helpers for that.

Mostly we just need to make a tiny edit, right? i.e., change from invoking the function to testing the result of report()?

PS, we probably want to generate something like Termination::report(...) rather than using a .report() notation, to avoid relying on the Termination trait being in scope?

nikomatsakis on 22 Jan 2018

No, I don't have any idea where that error is coming from :(

The RFC proposed to generate a wrapper function that calls the original test function. That is also my current way of doing it.
I think we could also drop the wrapper function, but then we would require boxing the function pointer, as each test function can return a different type.

Hmm, as the test already imports other stuff, it is not that complicated to also import the Termination trait.

@alexcrichton do you maybe have an idea where this error is coming from?

bkchr on 22 Jan 2018

@nikomatsakis libtest is unstable and can't we also mark macros as unstable even if it weren't?

eddyb on 23 Jan 2018

@eddyb oh, good point.

nikomatsakis on 23 Jan 2018

The RFC proposed to generate a wrapper function that calls the original test function. That is also my current way of doing it.

A wrapper function seems fine to me.

nikomatsakis on 23 Jan 2018

@eddyb with the macro you mean something like create_test that is defined in libtest? But what I don't understand is "mark macro as unstable". What do you intend by that? Could you give me an example?

bkchr on 23 Jan 2018

@bkchr Putting an #[unstable(...)] attribute on the macro definition, e.g.: https://github.com/rust-lang/rust/blob/3a39b2aa5a68dd07aacab2106db3927f666a485a/src/libstd/thread/local.rs#L159-L165

eddyb on 23 Jan 2018

So, should this first checkbox...

Implement the RFC

...be checked now that the linked PR has been merged?

ErichDonGubler on 25 Jan 2018

@ErichDonGubler Done :)

kennytm on 25 Jan 2018

Hmm, it is currently only the half rfc that is implemented with the merged pr ^^

bkchr on 25 Jan 2018

Separated into 3 checkboxes :)

kennytm on 25 Jan 2018

I've been trying to use this feature in main and I've found it pretty frustrating. The existing impls of the termination trait just don't allow me to conveniently "accumulate" multiple kinds of errors -- for example, I can't use failure::Fail, because it doesn't implement Error; I can't use Box<Error>, same reason. I think we should prioritize changing to Debug. =)

nikomatsakis on 31 Jan 2018

👍4

Hi, @nikomatsakis,

I felt exactly the same frustration as you did when I tried to use termination_trait.

That, combined with your posts on hacking on the compiler inspired me to take a crack at this problem earlier this month. I've posted the impl for Display (and for Debug in the previous commit) along with tests here: https://github.com/rust-lang/rust/pull/47544. (It's very minor, but still, my first Rust compiler PR! :tada:) :)

My understanding is the lang team will make a call on which trait to go with, but either way, the implementation is ready to go.

U007D on 31 Jan 2018

❤3

A question I'm still interested in: suppose you don't want to rely on the default error message output (whether Debug or Display), and want your own instead, how do you do that? (Apologies if this was already written down somewhere and I missed it.) You wouldn't have to stop using ?-in-main entirely, would you? It's something like writing your own result and/or error type and/or impl? (It seems unfortunate to me if ?-in-main were only a toy, and as soon as you wanted to "get serious" you'd have to revert to less ergonomic ways.)

glaebhoerl on 1 Feb 2018

@glaebhoerl It's very straightforward:

Create a new type.
Implement From your old error type.
Implement Debug (or Display).
Replace the type in the signature of main.

withoutboats on 1 Feb 2018

Thanks!

It seems slightly odd to me to write custom implementations of Debug which aren't debugging-oriented, but I guess it's not the end of the world.

glaebhoerl on 1 Feb 2018

👍3

@glaebhoerl That’s why the Result impl should use Display rather than Debug IMO.

SimonSapin on 1 Feb 2018

👍6

Can't the Termination trait have an extra method called message, or error_message or something like that, that has a default implementation that uses the Debug/Display to show the message? Then you just have to implement a single method rather then creating a new type.

Thomasdezeeuw on 1 Feb 2018

👍1

@glaebhoerl @Thomasdezeeuw Something like an error_message method was in the original draft of the RFC, but it got dropped for lack of consensus. My feeling at the time was that it would be best to get the basic feature landed (not necessarily stabilized) and then iterate.

zackw on 1 Feb 2018

@zackw Agreed, I'd personally be ok with no messaging, just a number, lets say 0 and 1 for error codes. But if we want to get the messaging in the first iteration I think I'd be more in favor of a Termination::message than anything on Debug or Display.

nixpulvis on 2 Feb 2018

Would that message method return a String? Wouldn’t that be incompatible with Termination being in libcore?

SimonSapin on 2 Feb 2018

@SimonSapin Termination is currently defined in libstd.

Hmm, but I don't think that adding a method message to the trait would be the best implementation. What would the method return for types like i32? How would decide when to print this message? Currently the implementation of Termination for Result, prints the error in the report function. That works, because Result knows, that it is an Err. We could integrate somewhere a check report() != 0 and then print, but that does not feel right.
The next problem would be, that we want to provide a standard implementation for Result, but what does the Error type needs to implement to be printed probably? This would bring us back to the current question Debug or Display.

bkchr on 2 Feb 2018

Another headache that comes up if you want to use ? in main in a "serious" program is that, under some circumstances, command-line programs want to exit with a nonzero status but without printing _anything_ (consider grep -q). So now you need a Termination impl for something that _isn't_ an Error, that prints nothing, that lets you control the exit status ... and you need to decide whether or not you're returning that thing _after_ parsing the command line arguments.

zackw on 2 Feb 2018

This is what I think:

Returning Result<T, E> should use the debug impl for E. This is the most convenient trait -- it's widely implemented, and satisfies the "quick and dirty output" use case as well as the unit testing use case. I'd prefer not to use Display both because it is less widely implemented and because it gives the impression that this is polished output, which I think is highly unlikely.

But there should also be a way to get professional looking output. Fortunately, there are already two such ways. First, as @withoutboats said, people can make a "display-from-debug" bridge if they want to use E: Display or otherwise give professional style output. But if this feels too hacky, then you can also just define your own type to replace result. e.g., you might do:

fn main() -> ProfessionalLookingResult {
    ...
}

and then implement Try for ProfessionalLookingResult. Then you can implement Terminate as well, whatever:

impl Terminate for ProfessionalLookingResult {
    fn report(self) -> i32 {
        ...
        eprintln!("Something very professional here.");
        return 1;
        ...
    }
}

nikomatsakis on 2 Feb 2018

👍8 😄2

I agree with @nikomatsakis that this should use Debug.

I also think that for polished output, writing some code in main is probably better than creating a new type to implement Try and Terminate. The point of Terminate seems to me to be for things libraries can easily make a good default for, which is just never the case for a sitution in which how the program terminates is significant to end users (e.g. professional CLIs).

Of course other people may have a different opinion, and there are multiple ways to use the traits involved to inject code instead of writing it directly in main. What's great is that we have multiple choices and we don't have to come up with a single blessed way to handle errors always.

withoutboats on 2 Feb 2018

👍1

Let me just jot down some thoughts, though there are a few problems with them.

I'd love to see the following

fn main() -> i32 {
    1
}

Which could be written more generally as:

fn main() -> impl Display {
    1
}

Both of these main functions should return a 0 exit code and println! the Display of 1.

This should be as simple as the following (I'd think).

impl<T> Termination for T where T: Display {
    fn report(self) -> i32 {
        println!("{}", self);
        EXIT_SUCCESS
    }
}

Then for errors we can have:

impl<T: Termination, E: Debug> Termination for Result<T, E> { ... }

where the implementation is the same as in the RFC just with "{:?}" to use
a Debug format.

As mentioned before, people who need more control over the output can simply
write:

fn main() -> Result<i32, MyError> { ... }
impl Termination for Result<i32, MyError> { ... }

Though this would be undecidable with our current compiler I guess, since it
would see conflicting implementations... So we do what @nikomatsakis suggests
and write:

fn main() -> MyResult { ... }
impl Termination for MyResult { ... }
or, if you want something more general.
impl<T, E> Termination for MyResult<T, E> { ... }

I know this is partly restating things that have been said, but I thought I'd present my vision altogether, showing that a more general solution for displaying return values, not just results. Seems like a lot of this comment is arguing what implementations of Termination we ship by default. Also this comment is at odds with implementation like impl Termination for bool as described in the RFC. I personally think that non-zero exit codes should be handled exclusively by Results or custom types that implement Termination.

It's an interesting question of how to then handle ? on Option types in main since they don't have an implementation for Display.

nixpulvis on 2 Feb 2018

TL;DR: I am OK with Debug.

More detailed explanation:
I spent some time thinking about this yesterday and today with an aim to uncover implicit assumptions I have or am making to help arrive at the best possible answer.

Key Assumption: Of all the various types of applications that one can write in Rust, I think the console app stands to benefit the most/be most impacted by this decision. My thinking is that when writing a library, a AAA game title, an IDE or a proprietary control system, one would probably not be expecting a default main termination trait to meet one's need out of the box (indeed, one might not even have a main). So my bias toward Display as default comes from what we expect to see when using a small command-line application--for example:

$ cd foo
bash: cd: foo: No such file or directory

Most of us do not expect any sort of debugging aids, just a succinct indicator of what went wrong. I am simply advocating for this as a default position.

When I think of writing a Terminate impl to get simple output like this, I realize that the ? from main feature isn't that different from stable rust today (in terms of amount of code written), where a Result-aware "inner_main()" is often created in order to handle E.

With this assumption in mind, as a thought exercise, I tried to determine whether I felt strongly that the preponderance of "inner_main()"-style implementations in existence today were of a more-casual Display flavor (over a more-technical Debug flavor). My thinking was that this would be an indication of how the feature is likely to actually be used.

I was unable to convince myself that this is be the case. (Meaning, I don't think there currently exists a strong bias toward Display in existing implementations). Indeed, when looking through my own repositories that I've written over the past 16 months, I also found enough of both cases that I can't say that implementing Display by default would have amounted to a net savings.

Holding to the "primary beneficiary is the cli application" assumption, there are a great number of console apps which provide help and usage information. For example:

$ git foo
git: 'foo' is not a git command. See 'git --help'.

The most similar command is
    log

So even in for console apps, it's hard for me to identify an "injured group" by going with Debug.

And finally, I'd be happier with a Debug impl than with the feature held up for another 6 months, too, so, selfishly, there's that :).

So there's my thought process laid out in public. To summarize, I think Debug will no better and no worse than Display, and as such, should be fine as the default implementation.

Like many of you, I'm sure, I do wish there was an implementation I felt more excited about--like, "YES, THAT'S IT!!!", TBH. But maybe that's just my expectations being unrealistic... Maybe once we have a solution that works with failure cutting down boilerplate in my projects, it'll grow on me. :)

U007D on 2 Feb 2018

Note I opened a PR to support the terminatio trait in tests: #48143 (building on @bkchr's work).

I took the liberty of extending the Termination trait with a method for processing the result of the test. This simplified the implementation, but it also makes sense, since test failures may want more verbose output than executable failures.

nikomatsakis on 11 Feb 2018

Termination should be renamed Terminate after our general preference for verbs for traits in libstd.

withoutboats on 22 Feb 2018

@withoutboats I think at some point there was a discussion that verb traits are mostly those that have a single method with the same name as the trait. Anyhow, might I float again my own bikeshed suggestion, Exit?

jdahlstrom on 22 Feb 2018

👍1

Gratuitous bikeshedding: this is a single-method trait. If we want to give them the same name, maybe ToExitCode / to_exit_code?

SimonSapin on 22 Feb 2018

Stabilizing returning Result can be done independently from stabilizing the trait, right?

To me, this feels much like ? where most of the value comes from the language feature, and we can delay on figuring out the trait. The RFC discussion even made me wonder if the trait _ever_ needs to be stabilized, since putting the code in an inner_main seemed easier than a trait impl for this...

scottmcm on 22 Feb 2018

Yes, we don't need to stabilize the trait - though it is useful on stable for things like frameworks, which can't necessarily rely so much on inner_main.

@SimonSapin I think of To as refering to a type conversion, and this doesn't. But we could name the method terminate (also I don't think this restriction on when to name traits verbs holds up. Try is an obvious counter example.)

withoutboats on 23 Feb 2018

👍1

I have proposed that we stabilize fn main() -> T where T is not unit. This leaves many details -- particularly the name/location/details of the trait -- unstabilized, but some things are fixed. Details here:

https://github.com/rust-lang/rust/issues/48453

Please give your feedback!

nikomatsakis on 23 Feb 2018

terminate seems to be more descriptive than report. We always terminate, but may omit reporting.

WiSaGaN on 23 Feb 2018

But unlike for example std::process::exit this method does not terminate anything. It only converts the return value of main() into an exit code (after optionally printing Result::Err to stderr).

SimonSapin on 23 Feb 2018

Another vote for Exit. I like that it is brief, quite descriptive and consistent with traditional concept of exit codes/exit status.

U007D on 23 Feb 2018

I'd definitely prefer Exit over Terminate since we're gracefully exiting by returning from main, rather than suddenly hard terminating where we are because something went really wrong.

retep998 on 24 Feb 2018

👍9

I added https://github.com/rust-lang/rust/issues/48854 to propose stabilizing unit tests that return results.

nikomatsakis on 8 Mar 2018

Oh hey, i found the proper place to talk about this.

Using `?` in doctests

The way doctests currently work is something like this:

rustdoc scans the doctest for whether it declares a fn main
- (currently it just does a line-by-line text search for "fn main" that's not after a //)
If fn main was found, it won't touch what's already there
If fn main was not found, it will wrap most* of the doctest in a basic fn main() { }
- *Full version: it will extract #![inner_attributes] and extern crate declarations and put them outside the generated main function, but everything else goes inside.
If the doctest doesn't include any extern crate statements (and the crate being documented isn't called std) then rustdoc will also insert an extern crate my_crate; statement right before the generated main function.
rustdoc then compiles and runs the final result as a standalone binary, as part of the test harness.

(I left a couple details out, but i conveniently made a full writeup here.)

So, to use ? seamlessly in a doctest, the bit that needs to change is the part where it adds fn main() { your_code_here(); } Declaring your own fn main() -> Result<(), Error> will work as soon as you can do that in regular code - rustdoc doesn't even need to be modified there. However, to make it work without declaring main manually will require a small tweak. Having not closely followed this feature, i'm not sure if there's a one-size-fits-all solution. Is fn main() -> impl Termination possible?

QuietMisdreavus on 21 Mar 2018

❤5

Is fn main() -> impl Termination possible?

In a shallow sense, yes: https://play.rust-lang.org/?gist=8e353379f77a546d152c9113414a88f7&version=nightly

Unfortunately, I think that -> impl Trait is fundamentally troublesome with ? due to the inbuilt error-conversion, which needs the inference context to tell it what type to use: https://play.rust-lang.org/?gist=23410fa4fa684710bc75e16f0714ec4b&version=nightly

Personally I was imagining ?-in-doctests working via something like https://github.com/rust-lang/rfcs/pull/2107 as fn main() -> Result<(), Box<Debug>> catch { your_code_here(); } (using syntax from https://github.com/rust-lang/rust/issues/41414#issuecomment-373985777).

The impl Trait version is cool, though, and might work if there was some sort of "prefer the same type if uncontrained" support that rustc could use internally in the ? desugar, but my recollection is that the idea of a feature like that tends to make the people who understand how things work recoil in horror :sweat_smile: But maybe it being an internal thing that only applies if the output is impl Trait would be feasible...

scottmcm on 22 Mar 2018

❤2

Ooh, those are very real concerns. I'd forgotten about how that would mess up type inference. If the catch block started Ok-wrapping like that linked issue, then that seems like a much easier (and much quicker, stabilization-wise) path forward.

The only thing i wonder is how that will be affected by the edition transition. Isn't the catch syntax changing in the 2018 epoch? Rustdoc will likely want to compile doctests in the same edition as the library it's running, so it would need to differentiate between the syntaxes based on the epoch flag it's been handed.

QuietMisdreavus on 22 Mar 2018

I'm concerned that this is now stabilized but simple cases apparently still ICE: https://github.com/rust-lang/rust/issues/48890#issuecomment-375952342

fn main() -> Result<(), &'static str> {
    Err("An error message for you")
}

assertion failed: !substs.has_erasable_regions(), librustc_trans_utils/symbol_names.rs:169:9

https://play.rust-lang.org/?gist=fe6ae28c67e7d3195a3731839d4aac84&version=nightly

scottmcm on 25 Mar 2018

👍5 😄2 ❤1

At what point do we say "this is too buggy and we should unstabilize"? Seems like if this hit the stable channel in its current form, it would cause a lot of confusion.

frewsxcv on 4 Apr 2018

👍2

@frewsxcv I think the problems are fixed now, right?

nikomatsakis on 27 Apr 2018

@nikomatsakis the issue I raised in https://github.com/rust-lang/rust/issues/48389 is resolved in 1.26-beta, so yes from my perspective.

U007D on 27 Apr 2018

Yep, the ICE I was worried about is fixed now!

frewsxcv on 28 Apr 2018

👍1

Apologies for chiming in at a point in which it's probably too late to do anything about this, but I wanted to leave my feedback here in case there was. I did read most of this thread, so I speak with that context in mind. However, this thread is long, so if it looks like I've overlooked something, then I probably did and I would appreciate having it pointed out to me. :-)

TL;DR - I think showing the Debug message of an error was a mistake, and that a better choice would be to use the Display message of an error.

At the core of my belief is that, as someone who _routinely builds CLI programs in Rust_, I can't ever remember caring much about what the Debug message of an Error is. Namely, the Debug of an error is, by design, for developers, not for end users. When you build a CLI program, its interface is fundamentally intended to be read by end users, so a Debug message has very little utility here. That is, if any CLI program I shipped to end users showed the debug representation of a Rust value in normal operation, then I would consider that a bug to be fixed. I generally think this should be true of every CLI program written in Rust, although I understand it may be a point on which reasonable people can disagree. With that said, a somewhat startling implication of my opinion is that we've effectively stabilized a feature where its default mode of operation starts you off with a bug (again, IMO) that should be fixed.

By showing the Debug representation of an error by default, I also think we are encouraging poor practice. In particular, it is very common in the course of writing a Rust CLI program to observe that even the Display impl of an error is not good enough to be consumed by end users, and that real work must be done to fix it. A concrete example of this is io::Error. Showing an io::Error without a corresponding file path (assuming it came from reading/writing/opening/creating a file) is basically a bug, because it is difficult for an end user to do anything with it. By electing to show the Debug representation of an error by default, we've made it harder for those sorts of bugs to be uncovered by folks creating CLI programs. (On top of that, the Debug of an io::Error is much less useful than its Display, but that on its own isn't a huge pain point in my experience.)

Finally, to round out my argument, I also have a hard time envisioning the circumstances under which I would use ?-in-main even in examples. Namely, I've been trying to write examples that match real world programs as closely as possible, and this generally has involved writing things like this:

use std::error::Error;
use std::process;

fn try_main() -> Result<(), Box<Error>> {
    // do stuff with `?`
}

fn main() {
    if let Err(err) = try_main() {
        eprintln!("{}", err);
        process::exit(1);
    }
}

On its face, it would be _lovely_ to replace this with ?-in-main, but I can't, because it won't show the Display of an error. That is, when writing a real CLI program, I will in fact use the above approach, so if I want my examples to reflect reality, then I think I should show what I do in real programs and not take shortcuts (to a reasonable extent). I actually think this sort of thing is really important, and one side effect of this historically was that it showed folks how to write idiomatic Rust code without sprinkling unwrap everywhere. But if I revert to using ?-in-main in my examples, then I've just reneged on my goal: I'm now setting up folks who may not know any better to just write programs that, by default, emit very unhelpful error messages.

The pattern of "emit an error message and exit with an appropriate error code" is actually used in polished programs. For example, if ?-in-main used Display, then I could merge the main and run functions in ripgrep today:

https://github.com/BurntSushi/ripgrep/blob/64317bda9f497d66bbeffa71ae6328601167a5bd/src/main.rs#L56-L86

Of course, I could use ?-in-main in the future by providing my own impl for the Termination trait once that stabilizes, but why would I bother doing that if I could just write out the main function I have? And even so, this doesn't help with my conundrum when writing examples. I'd need to include that impl in the examples in order to make them match reality, and at that point, I might as well stick with the examples I have today (using a main and a try_main).

From the looks of it, it appears that fixing this would be a breaking change. That is, this code compiles on Rust stable today:

#[derive(Debug)]
struct OnlyDebug;

fn main() -> Result<(), OnlyDebug> {
    Err(OnlyDebug)
}

I think switching to Display would break this code. But I don't know for sure! If this is truly a ship-has-sailed issue, then I understand and there's not too much use in belaboring the point, but I do feel strongly enough about this to at least say something and see if I can't convince others and see if there's anything that can be done to fix it. (It's also quite possible that I am overreacting here, but I've so far had a couple people ask me "why aren't you using ?-in-main?" in my CSV examples, and my answer has basically been, "I don't see how it would ever be feasible to do that." Maybe that wasn't a problem that was intended to be solved by ?-in-main, but some folks certainly had that impression. With its current implementation, I could see it being useful in doc tests and unit tests, but I have a hard time thinking of other situations in which I would use it.)

BurntSushi on 12 May 2018

👍35 ❤9

I agree that displaying the Display over the Debug is a better for CLI applications. I don't agree that that it should be Display instead of Debug, because that drastically limits what errors can actually be ?-ed and defeats the purpose of ?-in-main. As far I can tell(Could totally be wrong as I don't have the time to compile stdlib and I don't know if specialisation covers this) there's no reason we can't add the following impl to Termination. This would provide a non-breaking way to use Display when that is available and fall back to Debug when it doesn't.

#[unstable(feature = "termination_trait_lib", issue = "43301")]
impl<E: fmt::Display> Termination for Result<!, E> {
    fn report(self) -> i32 {
        let Err(err) = self;
        eprintln!("Error: {}", err);
        ExitCode::FAILURE.report()
    }
}

XAMPPRocky on 12 May 2018

👍2

Finally, to round out my argument, I also have a hard time envisioning the circumstances under which I would use ?-in-main even in examples.

I have to agree with @BurntSushi in general for real CLI programs, but for random scripts and internal tools that only I plan to use, this is really convenient. Also, it's really convenient for prototypes and toys. We could always discourage actually using it in production code, right?

mark-i-m on 12 May 2018

Part of the goal with ?-in-main is to avoid unidiomatic code in code examples, namely unwraps. But if ?-in-main itself becomes unidiomatic, then that undermines the entire feature. I would strongly like to avoid any outcome that would cause us to have to discourage using it, in production code or otherwise.

bstrie on 13 May 2018

👍7

I've been trying to use this feature in main and I've found it pretty frustrating. The existing impls of the termination trait just don't allow me to conveniently "accumulate" multiple kinds of errors -- for example, I can't use failure::Fail, because it doesn't implement Error; I can't use Box, same reason. I think we should prioritize changing to Debug. =)

If we use Display, we can introduce a fast and dirty type like MainError to accumulate multiple kinds of errors:

pub struct MainError {
    s: String,
}

impl std::fmt::Display for MainError {
    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        self.s.fmt(f)
    }
}

impl<T> From<T> for MainError where T: std::error::Error {
    fn from(t: T) -> Self {
        MainError {
            s: t.to_string(),
        }
    }
}

This will allow something like below similar to Box<Error>:

fn main() -> Result<(), MainError> {
    let _ = std::fs::File::open("foo")?;
    Ok(())
}

WiSaGaN on 13 May 2018

Previous discussion of Display vs Debug is in the hidden part here, starting around https://github.com/rust-lang/rust/issues/43301#issuecomment-362020946.

scottmcm on 13 May 2018

👍2

@BurntSushi I agree with you. If this can't be fixed, there could be a workaround:

use std::fmt;
struct DisplayAsDebug<T: fmt::Display>(pub T);

impl<T: fmt::Display> Debug for DisplayAsDebug {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        fmt::Display::fmt(&self.0, f)
    }
}

impl<T: fmt::Display> From<T> for DisplayAsDebug {
    fn from(val: T) -> Self {
        DisplayAsDebug(val)
    }
}

If this was in std::fmt, we could use something like this:

use std::{fmt, io};

fn main() -> Result<(), fmt::DisplayAsDebug<io::Error>> {
    let mut file = File::open("/some/file")?;
    // do something with file
}

Kixunil on 13 May 2018

I still don't have time to contribute any more to this for the foreseeable future, but I also agree that ? in main ought to be usable in serious CLI programs, and there were features in the original draft of the RFC that were intended to facilitate that. They got dropped in the name of incrementalism but maybe they should be revisited.

zackw on 13 May 2018

I think it's fine to fix this in a backwards incompatible way if it's done soon, before much code out there is using it on stable.

Boscop on 13 May 2018

👍3

As somebody who writes a lot of CLI programs in Rust, and who's in charge of Rust style at work, I pretty much agree with @burntsushi here. I would have happily used the version of this feature specified in the RFC.

But I consider showing a Debug implementation to a user to be a bug, and not much better than just calling unwrap everywhere in a CLI program. So even in example code, I can't imagine ever using this version of the feature. This is too bad, because removing boilerplate from main would have simplified the learning for new Rust developers at work, and we wouldn't have needed to explain quick_main! or the equivalent any more.

Pretty much the only circumstances where I can imagine using this would be to eliminate unwrap from doctests. But I don't know if that's supported yet.

If we use Display, we can introduce a fast and dirty type like MainError to accumulate multiple kinds of errors:

Personally, this sort of workaround would add sufficient complexity that it's just easier to avoid ?-in-main completely.

@Aaronepower:

As far I can tell(Could totally be wrong as I don't have the time to compile stdlib and I don't know if specialisation covers this) there's no reason we can't add the following impl to Termination. This would provide a non-breaking way to use Display when that is available and fall back to Debug when it doesn't.

If this would enable me to write fn main() -> Result<(), failure::Error> and get a nice, human-readable error using Display, it would definitely satisfy my major concerns. However, I suppose that this would still leave the question of optionally displaying the backtrace in failure::Error when RUST_BACKTRACE=1 is set—but that might be out of scope, anyways, or at least an issue which should be taken up with failure.

emk on 15 May 2018

This was stabilized some time ago, and I don't think we can really change the behavior of Result. I personally remain pretty happy with the current behavior for the use cases that I tend to do — quick and dirty scripts and the like — but I agree that more complex scripts won't want it. However, at the time of the discussion, we did also discuss a number of ways that you could control that behavior (I can't seem to find those comments now because github is hiding things from me).

For example, you could define your own "wrapper" for the error type, and implement From for it:

struct PrettyPrintedError { ... }
impl<E: Display> From<E> for PrettyPrintedError { }

impl Debug { /* .. invoke Display .. */ }

Now you can write something like this, which means you can use ? in main:

fn main() -> Result<(), PrettyPrintedError> { ... }

Perhaps such a type should be part of quick-cli or something?

nikomatsakis on 15 May 2018

@nikomatsakis Yeah, I totally get that workaround, but I do feel like that defeats the purpose of the succinctness of using ?-in-main. I feel like "it's possible to use ?-in-main using this work-around" undermines ?-in-main itself unfortunately. For instance, I'm not going to write out your work-around in succinct examples and I'm not going to impose a dependency on quicli for every example I write either. I'm certainly not going to use it for quick programs either, because I want the Display output of an error in basically every CLI program I write. My opinion is that the debug output of an error is a bug in CLI programs that you put in front of users.

The alternative to ?-in-main is an additional ~4-line function. So if the workaround to fixing ?-in-main to use Display is much more than that, then I personally don't see much reason to use it at all (outside of doc tests or unit tests, as mentioned above).

BurntSushi on 15 May 2018

👍6

Is this something that could be changed in the edition?

mark-i-m on 15 May 2018

@BurntSushi How would you feel about the workaround being in std, so you'd only have to write a bit longer return type declaration on main()?

Kixunil on 20 May 2018

👍1

@Kixunil My initial feeling is that it might be palatable. Haven't given it too much thought though.

BurntSushi on 20 May 2018

@BurntSushi

The alternative to ?-in-main is an additional ~4-line function.

Ultimately, the behavior is stable, and I don't think we can realistically change it at this point. (I mean it's not a soundness violation or something.)

That said, I still find the current setup pretty nice and preferable to Display, but I guess it's a matter of what you want the "default" to be — that is, either setup can be made to work like the other via various newtypes. So either we default to favoring "quick-n-dirty" scripts, which want Debug, or polished end products. I tend to think a polished end product is where I can afford an additional import readily enough, and also where I'd like to pick from many different possible formats (e.g., do I want just the error, or do I want to include argv[0], etc).

To be more concrete, I imagine it would look like this:

use failure::format::JustError;

fn main() -> Result<(), JustError> { .. }

or, to get a different format, perhaps I do this:

use failure::format::ProgramNameAndError;

fn main() -> Result<(), ProgramNameAndError> { .. }

Both of these feel reasonably nice compared to the 4-line function you had to write before:

use std::sys;

fn main() {
  match inner_main() {
    Ok(()) => { }
    Err(error) => {
      println!("{}", error);
      sys::exit(1);
    }
}

fn inner_main() -> Result<(), Error> {
  ...
}

nikomatsakis on 24 May 2018

👍7

I'm coming around on this topic since, for production quality, adding what amouts to essensially a custom error type for the output seems like a good direction to push people anyway. This seems to paralell how panic!, for example, gives debug quality messages by default.

nixpulvis on 24 May 2018

👍1

@nikomatsakis That is perhaps a better long term view, and I do find your end result appetizing. It would be nice if those error types ended up in std some day so that they can be used in examples with as little overhead as possible. :-)

Process note: I tried to frame my initial feedback here with "is there anything that can be done" rather than "let's change something that has already stabilized," in an effort to focus on the high level goal rather than ask for something that we certainly cannot do. :-)

BurntSushi on 24 May 2018

👍3

Well that didn't take long: https://crates.io/crates/exitfailure 😆

scottmcm on 25 May 2018

😄4

I am wondering how many people are surprised by using Debug trait versus Display trait. Both the initial discussion draft and the final RFC is using Display. This is similar to the universal impl trait incident, where people thought they are getting one thing, but only know they are getting another after it is getting stabalized. Also considering a lot of people who are surprised won't be using this feature, I feel there won't be big backslash if we change it in the next edition.

WiSaGaN on 25 May 2018

👍7 😕1 👎1

@WiSaGaN The details of an RFC can and will change from time to time. This is expected and healthy. RFCs are design documents and not a perfect representation of what is and always will be. Information discovery is hard, and it's my fault for not paying more attention. I'm hoping that we can avoid relitigating the stability of this feature and instead focus on the higher level goals that we can feasibly act upon.

BurntSushi on 25 May 2018

❤3 👍1

@nikomatsakis The main issue I see with favoring quick-n-dirty cases is that there already is a way to address them: .unwrap(). So from that perspective, ? is just another way to write quick-n-dirty things, yet there's no similarly easy way to write polished things.

Of course the ship has sailed, so we're mostly screwed. I'd love to see this changed in next edition, if possible.

Kixunil on 26 May 2018

👍2

@Kixunil, unwrap is really not a good way to address things, as the title of this thread states we would like to be able to use the syntactic sugar ? in main. I can understand where you are coming from (in fact it was part of my initial hesitation with the use of ? for option handling) but with the inclusion of ? in the language this issue is a pretty nice usability win IMHO. If you need nicer output, then there are options for you. You don't release things to users without testing them first, and the discovery that you need a custom type for the output of main will be a pretty quick realization.

As for "why" we'd like ? in main, well consider how weird it is now. You can use it practically everywhere else (since you have control of the return type). The main function then ends up feeling kinda special, which it shouldn't.

nixpulvis on 27 May 2018

❤2

.unwrap() is a quick-and-dirty solution that doesn't compose, so you can't factor code in and out of main() while you're sketching out a program.

By comparison, ? is a quick-and-dirty solution that does compose, and making it not-quick-and-dirty is a matter of putting the correct return-type on main(), not of modifying the code itself.

Screwtapello on 27 May 2018

👍1

@Screwtapello ah, now it makes sense to me. Thanks!

Kixunil on 2 Jun 2018

I just want to express that I thought the aim of this was to bring ? in main for all applications without having to resort to additional wrappers... If it's just for testing I don't see much benefit in it and will keep sticking to .unwrap(), sadly.

oblitum on 2 Jun 2018

@oblitum It's not just for testing. It's just not (by default) going to use the Display format. This may or may not be the output you are looking for, but will almost definitely be better than the output of .unwrap. With that said, it might make your code "nicer" to use ?. In any case it's still possible to customize the output of ? errors in main as @nikomatsakis has detailed above.

nixpulvis on 2 Jun 2018

Would it be possible to introduce a newtype in the stdlib for the Display case? Something like:

#[derive(Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Debug, Hash)]
#[must_use]
struct DisplayResult<E: fmt::Display>(Result<(), E>)

impl<E> Termination for DisplayResult<E> {
    // ... same as Result, but use "{}" instead of "{:?}"
}

impl<E> From<Result<(), E>> for DisplayResult<E> {}
impl<E> Deref<Result<(), E>> for DisplayResult<E> {}
impl<E> Try for DisplayResult<E> {}
// ...

This should allow users to write:

fn main() -> DisplayResult<MyError> {
    // Ordinary code; conversions happen automatically via From, Try, etc.
}

The main motivations here are:

Display is used when an error is returned
Users don't have to do any extra wrapping of their error or result types; they can just use ? in their main function.

Lucretiel on 12 Jun 2018

👍4

Follow-up: It occurs to me that the semantics of ? and From may not allow this to work as implicitly as I'd like. I know that ? will convert between Error types via From, but I don't know if it does the same thing for different implementators of Try. That is, it will convert from Result<?, io::Error> to Result<?, FromIoError>, but not necessarily from Result<?, Err> to DisplayResult<Result<?, Err>>. Basically, I'm looking for something like this to work:

fn main() -> DisplayResult<io::Error> {
    let f = io::File::open("path_to_file")?;
    let result = String::new();
    f.read_to_string(result)?
}

Assuming that this doesn't work, perhaps instead this could be made a simple conditional compilation flag in Cargo.toml?

Lucretiel on 24 Jul 2018

@Lucretiel: you seem to assume programmers mainly just want to print an exit message to the console. This does not hold for non-technical desktop applications, daemons with logging plumbing, etc. I prefer us not to add ‘commonly useful’ stuff to the standard library without compelling evidence (e.g. numbers) it should be in there.

sanmai-NL on 30 Jul 2018

I am assuming that programmers who are returning a Result<(), E> where E: Error from main are interested in printing an error message to the console, yes. Even the current behavior, which is print via Debug, is "print an exit message to the console".

There seems to be wide agreement in this thread that something should be printed to stderr; the key decision to be made are "Should it print with "{}" or "{:?}", or something else, and how configurable should it be, if at all".

Lucretiel on 30 Jul 2018

@Lucretiel, for me the value of returning a Result is controlling the exit status nicely. Would whether to print something not best be left to the panic handler? There’s not just the question of whether to print something to stderr, also what to print (backtrace, error message, etc?). See https://github.com/rust-lang/rust/issues/43301#issuecomment-389099936, esp. the last part.

sanmai-NL on 31 Jul 2018

I used this recently and wished that it called Display. Pretty sure we made the wrong call here.

A process failure I notice is that the decision was made by lang team (mainly Niko and I) rather than libs team, but the code in question lives entirely in std. Possibly libs would've made a better decision.

withoutboats on 31 Jul 2018

❤18 👍9

@withoutboats is it too late to change? I notice that the feature is still tagged as nightly-only / experimental in the docs, but it's possible I don't understand some of the granularity behind the stabilization process.

Lucretiel on 26 Aug 2018

👍4

I too would like to see this change and regret not being a stronger advocate for it when the team asked me to change the implementation from Display to Debug.

U007D on 26 Aug 2018

@Lucretiel Termination trait is nightly, but the feature itself (returning Termination implementations from main/tests) is stable already.

xfix on 14 Sep 2018

Is there a ticket or a timeline for stabilizing the name of the trait?

jdahlstrom on 21 Sep 2018

@xfix That would imply that it's possible to change the implementation, right? The behavior doesn't change (Termination returned from main), but the trait itself is would change from:

impl<E: Debug> Termination for Result<(), E> ...

to:

impl<E: Display> Termination for Result<(), E> ...

Lucretiel on 2 Oct 2018

it's possible to change the implementation, right?

Not without breaking backwards compatibility. This code compiles in today's stable Rust:

#[derive(Debug)]
struct X;

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

With the proposed change to require Display, it would stop compiling.

As has already been mentioned, it's possible something like specialization might help, but my current understanding of specialization is that would only be useful for those types which implement both Display and Debug (i.e. such that there's one implementation that's more special).

trait ResultTerm {
    fn which(&self);
}
impl<T: Debug> ResultTerm for T {
    default fn which(&self) {
        println!("{:?}", self)
    }
}
impl<T: Debug + Display> ResultTerm for T {
    fn which(&self) {
        println!("{}", self)
    }
}

enum MyResult<T, E> {
    Ok(T),
    Err(E),
}

impl<T, E> Termination for MyResult<T, E>
where
    E: ResultTerm,
{
    fn report(self) -> i32 {
        match self {
            MyResult::Err(e) => {
                e.which();
                1
            }
            _ => 0,
        }
    }
}

playground

This may be enough to cover all the common cases, but it does publicly expose specialization.

shepmaster on 2 Oct 2018

👍2

Ah, I see what you mean about stable rust. For some reason I wasn't aware that this was available in stable.

Is this something that could be switched at compile time with a Cargo flag? In the same way that other things are switchable, like panic = "abort".

Lucretiel on 2 Oct 2018

👍2

Maybe? I could see it being feasible to have a different behaviour for ? in main in another Rust edition. Probably not 2018 however, maybe 2021?

xfix on 3 Oct 2018

Different crates in the same program can be in different editions but use the same std, so the traits available in std and their impls need to be the same across editions. What we could do in theory (I’m not advocating this) is have two traits, Termination and Termination2, and require the return type of main() to implement one or the other depending on the edition of the crate that defines main().

SimonSapin on 4 Oct 2018

I don't think we need to deprecate anything. I'll echo @Aaronepower 's sentiment that limiting to only Display types could serve to just dissatisfy quick-script users (deriving Debug is trivial, implementing Display isn't) in the same way that the current situation dissatisfies production users. Even if we could, I don't think it would ultimately be a benefit to. I think the specialization approach laid out by @shepmaster has promise, in that it would create the following behavior:

A type that has Debug but not Display will have its Debug output printed by default
A type that has both Debug and Display will have its Display output printed by default
A type that has Display but not Debug results in a compiler error

Someone might object to point 2, in the case where you have a type with Display but you want it to print Debug for whatever reason; I think this case would be addressed by Niko's proposal of various predesigned output format Error types (which are probably worth exploring even if we go with the specialization route).

As for point 3, it's a little janky (maybe we should have done trait Display: Debug back in 1.0?), but if someone's gone to the trouble of writing a Display impl then it's not much to ask to have them slap on a single derive (which, I suspect, they probably have anyway... is there anyone out there who objects on principle to having Debug on their Display type?).

bstrie on 11 Oct 2018

👍3 ❤1

limiting to only Display types could serve to just dissatisfy quick-script users (deriving Debug is trivial, implementing Display isn't)

A case where Display is preferable over Debug in quick scripts is using &'static str or String as the error type. If you have a custom error type at all to slap #[derive(Debug)] onto you’re already spending some non trivial energy on error handling.

SimonSapin on 11 Oct 2018

👍1

@SimonSapin I wouldn't say it is preferable in a quick scripting environment, or rather not preferable enough that I would be in a position where I wanted the String to be printed as Display format without wanting to make the effort to have properly formatted errors, To me Debug is preferred because the error format when it is String is not well formed so having Err("…") around it lets me easily visually pick out the error from any other other output that may have been emitted.

XAMPPRocky on 11 Oct 2018

FWIW what is formatted is not the Result<_, E> value, only the E value inside. So you won’t see Err( in the output. However the current code adds an Error: prefix, which presumably would stay if Display is used. In addition to not printing quotes, Display would also not backslash-escape the contents of the string which is desirable IMO when it comes to showing an (error) message to users.

https://github.com/rust-lang/rust/blob/cb6eeddd4dcefa4b71bb4b6bb087d05ad8e82145/src/libstd/process.rs#L1527-L1533

SimonSapin on 11 Oct 2018

👍1

I think given the situation the best thing we can do is specialization for types that implement Display + Debug to print the Display impl. Its unfortunate that types that don't implement Debug can't be used as errors (until we support intersection impls), but its the best we could do.

The main question is whether or not that impl falls under our current policy for specialized impls in std. We generally have a rule that we only use specialization in std where we don't provide a stable guarantee that the behavior won't change later (because specialization itself is an unstable feature). Are we comfortable providing this impl now in the awareness that its possible that someday these types will revert to printing their Debug output if we eventually remove specialization as a feature?

withoutboats on 11 Oct 2018

Actually, I think we cannot allow this specialization right now, because it could be used to introduce unsoundness. This impl is actually exactly the kind of specialization that causes us so many problems!

use std::cell::Cell;
use std::fmt::*;

struct Foo<'a, 'b> {
     a: &'a Cell<&'a i32>,
     b: &'b i32,
}

impl<'a, 'b> Debug for Foo<'a, 'b> {
    fn fmt(&self, _: &mut Formatter) -> Result {
        Ok(())
    }
}

impl<'a> Display for Foo<'a, 'a> {
    fn fmt(&self, _: &mut Formatter) -> Result {
        self.a.set(self.b);
        Ok(())
    }
}

Because of the way specialization currently works, I could call report on a Foo<'a, 'b> where 'b does not outlive 'a, and its currently entirely possible that the compiler will select the impl that calls into the Display instance even though the lifetime requirements are not met, allowing the user to extend the lifetime of a reference invalidly.

withoutboats on 11 Oct 2018

🎉2 ❤1 😄1

I don't know about you guys, but when I write quick "scripts", I just unwrap() the shit out of everything. It's 100 times better, because it has backtraces which provide me contextual information. Without it, in case I have let ... = File::open() more than once in my code, I already can't identify which one failed.

Kixunil on 15 Nov 2018

👍4

I don't know about you guys, but when I write quick "scripts", I just unwrap() the shit out of everything. It's 100 times better, because it has backtraces which provide me contextual information. Without it, in case I have let ... = File::open() more than once in my code, I already can't identify which one failed.

This is actually why I feel so strongly about Display, since generally what I do in this case is do a map_err that attaches the filename and other relevant contextual information. Generally the debug output is less helpful for tracking down what actually went wrong, in my experience.

Lucretiel on 15 Nov 2018

👍2

@Kixunil: Alternatively, I would prefer a more elaborate error handling strategy and/or more extensive logging and/or debugging.

sanmai-NL on 17 Nov 2018

👍2

I guess "Implement the RFC" can be ticked off, right? At least according to this! :smile:

matthiasbeyer on 25 Apr 2019

👍1

Pardon me if this is completely clueless, but couldn't you use specialization to report the error chain when possible:

use std::error::Error;

impl<E: fmt::Debug> Termination for Result<!, E> {
    default fn report(self) -> i32 {
        let Err(err) = self;
        eprintln!("Error: {:?}", err);
        ExitCode::FAILURE.report()
    }
}

impl<E: fmt::Debug + Error> Termination for Result<!, E> {
    fn report(self) -> i32 {
        let Err(err) = self;
        eprintln!("Error: {:?}", err);

        for cause in Error::chain(&err).skip(1) {
            eprintln!("Caused by: {:?}", cause);
        }
        ExitCode::FAILURE.report()
    }
}

ghost on 29 Nov 2019

https://github.com/rust-lang/rfcs/blob/f4b8b61a414298ba0f76d9b786d58ccdc34a44bb/text/1937-ques-in-main.md#L260-L270

impl<T: Termination, E: Display> Termination for Result<T, E> {
    fn report(self) -> i32 {
        match self {
            Ok(val) => val.report(),
            Err(ref err) => {
                print_diagnostics_for_error(err);
                EXIT_FAILURE
            }
        }
    }
}

Is there a reason this part of the RFC wasn't implemented?

Ayplow on 13 Jan 2020

👍2

We did make some changes to the precise sets of traits and impls to be used, I don't recall the details at this point though -- and I believe there remain a few details left to be stabilized. You might want to check out the stabilization reports linked to from the top issue for more details.

nikomatsakis on 13 Jan 2020

What is the status of this feature? Is there a proposal to stabilization?

GrayJack on 3 Nov 2020

@GrayJack This issue should be closed, as this landed quite a while ago.

jhpratt on 3 Nov 2020

True, and I got confused a bit, I got here from the link in the Termination trait and I was asking about that, is there a proper issue for termination_trait_lib?

GrayJack on 3 Nov 2020

This issue should be closed

This issue is still open to track the stabilization of Termination.

ehuss on 3 Nov 2020

👍2

Rust: Tracking issue for RFC 1937: `?` in `main`

Most helpful comment

All 183 comments

Mentoring Instructions

add the `Termination` lang-item to libcore

allow using `Termination` in main

allow using `Termination` in doctests

allow using `Termination` in `#[test]`

Using `?` in doctests

Related issues

Rust: Tracking issue for RFC 1937: `?` in `main`

Most helpful comment

All 183 comments

Mentoring Instructions

add the Termination lang-item to libcore

allow using Termination in main

allow using Termination in doctests

allow using Termination in #[test]

Using ? in doctests

Related issues

add the `Termination` lang-item to libcore

allow using `Termination` in main

allow using `Termination` in doctests

allow using `Termination` in `#[test]`

Using `?` in doctests