search

rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
96.79k stars 12.5k forks source link

Tracking issue for RFC 1937: `?` in `main` #43301

Closed aturon closed 1 year ago

aturon commented 7 years ago

This is a tracking issue for the RFC "? in main" (rust-lang/rfcs#1937).

Steps:

Stabilizations:

Related issues:

Unresolved questions:

U007D commented 6 years ago

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

sanmai-NL commented 6 years ago

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

bkchr commented 6 years ago

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

alexcrichton commented 6 years ago

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

nikomatsakis commented 6 years ago

@bkchr how goes here?

bkchr commented 6 years ago

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

withoutboats commented 6 years ago

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
            }
        }
    }
}
sfackler commented 6 years ago

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.

withoutboats commented 6 years ago

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:

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

bkchr commented 6 years ago

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.

nikomatsakis commented 6 years ago

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

withoutboats commented 6 years ago

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.

nikomatsakis commented 6 years ago

@withoutboats I agree.

U007D commented 6 years ago

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

scottmcm commented 6 years ago

:+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

U007D commented 6 years ago

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?

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?

nikomatsakis commented 6 years ago

@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()

bkchr commented 6 years ago

He prefers option (b).

U007D commented 6 years ago

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

scottmcm commented 6 years ago

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?

Screwtapello commented 6 years ago

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.

U007D commented 6 years ago

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

glaebhoerl commented 6 years ago

(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\<Error> from main()" instead of... the actual contents of that Box<Error>?)

jdahlstrom commented 6 years ago

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.

U007D commented 6 years ago

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.

bkchr commented 6 years ago

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

eddyb commented 6 years ago

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

nikomatsakis commented 6 years ago

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

bkchr commented 6 years ago

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?

eddyb commented 6 years ago

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

nikomatsakis commented 6 years ago

@eddyb oh, good point.

nikomatsakis commented 6 years ago

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.

bkchr commented 6 years ago

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

eddyb commented 6 years ago

@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

ErichDonGubler commented 6 years ago

So, should this first checkbox...

Implement the RFC

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

kennytm commented 6 years ago

@ErichDonGubler Done :)

bkchr commented 6 years ago

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

kennytm commented 6 years ago

Separated into 3 checkboxes :)

nikomatsakis commented 6 years ago

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

U007D commented 6 years ago

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.

glaebhoerl commented 6 years ago

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

withoutboats commented 6 years ago

@glaebhoerl It's very straightforward:

  1. Create a new type.
  2. Implement From your old error type.
  3. Implement Debug (or Display).
  4. Replace the type in the signature of main.
glaebhoerl commented 6 years ago

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.

SimonSapin commented 6 years ago

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

Thomasdezeeuw commented 6 years ago

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.

zackw commented 6 years ago

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

nixpulvis commented 6 years ago

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

SimonSapin commented 6 years ago

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

bkchr commented 6 years ago

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

zackw commented 6 years ago

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.