Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
aturon opened this issue Jul 17, 2017 · 183 comments
Open

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

aturon opened this issue Jul 17, 2017 · 183 comments

Comments

@aturon
Copy link
Member

@aturon aturon commented Jul 17, 2017

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

Steps:

Stabilizations:

  • Stabilize main with non-() return types (#48453) Merged in #49162
  • Stabilize unit tests with non-() return types (#48854)

Related issues:

  • Error message for unit tests is not great (#50291)

Unresolved questions:

  • The name of the trait being introduced
  • The precise initial implementations
    • this will be stabilized by #48453
  • exit codes (discussion) Moved to a separate tracking issue, #48711
    • this will be stabilized by #48453 no longer true after #48497
@ghost
Copy link

@ghost ghost commented Jul 17, 2017

How are exit statuses going to be dealt with?

@ketsuban
Copy link
Contributor

@ketsuban ketsuban commented Jul 18, 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?

@ghost
Copy link

@ghost ghost commented Jul 18, 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.

@Screwtapello
Copy link
Contributor

@Screwtapello Screwtapello commented Jul 18, 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".

@NoraCodes
Copy link
Contributor

@NoraCodes NoraCodes commented Jul 18, 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.

@ghost
Copy link

@ghost ghost commented Jul 18, 2017

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

@glaebhoerl
Copy link
Contributor

@glaebhoerl glaebhoerl commented Jul 18, 2017

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

@liigo
Copy link
Contributor

@liigo liigo commented Aug 11, 2017

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/

@nikomatsakis nikomatsakis added this to the impl period milestone Sep 15, 2017
@aturon aturon removed this from the impl period milestone Sep 15, 2017
@arielb1 arielb1 self-assigned this Sep 18, 2017
@bkchr
Copy link
Contributor

@bkchr bkchr commented Sep 19, 2017

@arielb1 are you going to implement this rfc?

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 19, 2017

@bkchr

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

@bkchr
Copy link
Contributor

@bkchr bkchr commented Sep 19, 2017

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

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 19, 2017

@bkchr

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

@bkchr
Copy link
Contributor

@bkchr bkchr commented Sep 19, 2017

Okay, then I'm waiting for your instructions.

@arielb1
Copy link
Contributor

@arielb1 arielb1 commented Sep 19, 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 0c3ac64).
That means:

  1. adding it to the lang-items list (in librustc/middle/lang_items.rs)
  2. 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:

fn check_main_fn_ty<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
main_id: ast::NodeId,
main_span: Span) {
let main_def_id = tcx.hir.local_def_id(main_id);
let main_t = tcx.type_of(main_def_id);
match main_t.sty {
ty::TyFnDef(..) => {
match tcx.hir.find(main_id) {
Some(hir_map::NodeItem(it)) => {
match it.node {
hir::ItemFn(.., ref generics, _) => {
if generics.is_parameterized() {
struct_span_err!(tcx.sess, generics.span, E0131,
"main function is not allowed to have type parameters")
.span_label(generics.span,
"main cannot have type parameters")
.emit();
return;
}
}
_ => ()
}
}
_ => ()
}
let se_ty = tcx.mk_fn_ptr(ty::Binder(
tcx.mk_fn_sig(
iter::empty(),
tcx.mk_nil(),
false,
hir::Unsafety::Normal,
Abi::Rust
)
));
require_same_types(
tcx,
&ObligationCause::new(main_span, main_id, ObligationCauseCode::MainFunctionType),
se_ty,
tcx.mk_fn_ptr(tcx.fn_sig(main_def_id)));
}
_ => {
span_bug!(main_span,
"main has a non-function type: found `{}`",
main_t);
}
}
}

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:

let mut actual_return_ty = coercion.complete(&fcx);
if actual_return_ty.is_never() {
actual_return_ty = fcx.next_diverging_ty_var(
TypeVariableOrigin::DivergingFn(span));
}
fcx.demand_suptype(span, ret_ty, actual_return_ty);
(fcx, gen_ty)
}

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:

fn lang_start(main: fn(), argc: isize, argv: *const *const u8) -> isize {

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.

match et {

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
Copy link
Contributor

@arielb1 arielb1 commented Sep 19, 2017

@bkchr

Can you at least join the IRC/gitter?

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Sep 25, 2017

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

@bkchr
Copy link
Contributor

@bkchr bkchr commented Sep 25, 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.

@xfix
Copy link
Contributor

@xfix xfix commented Sep 14, 2018

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

@jdahlstrom
Copy link

@jdahlstrom jdahlstrom commented Sep 21, 2018

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

@Lucretiel
Copy link
Contributor

@Lucretiel Lucretiel commented Oct 2, 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> ...
@shepmaster
Copy link
Member

@shepmaster shepmaster commented Oct 2, 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.

@Lucretiel
Copy link
Contributor

@Lucretiel Lucretiel commented Oct 2, 2018

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

@xfix
Copy link
Contributor

@xfix xfix commented Oct 3, 2018

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?

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Oct 4, 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().

@bstrie
Copy link
Contributor

@bstrie bstrie commented Oct 11, 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:

  1. A type that has Debug but not Display will have its Debug output printed by default
  2. A type that has both Debug and Display will have its Display output printed by default
  3. 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?).

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Oct 11, 2018

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.

@XAMPPRocky
Copy link
Member

@XAMPPRocky XAMPPRocky commented Oct 11, 2018

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

@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Oct 11, 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.

rust/src/libstd/process.rs

Lines 1527 to 1533 in cb6eedd

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

@withoutboats
Copy link
Contributor

@withoutboats withoutboats commented Oct 11, 2018

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
Copy link
Contributor

@withoutboats withoutboats commented Oct 11, 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.

@Kixunil
Copy link
Contributor

@Kixunil Kixunil commented Nov 15, 2018

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.

@Lucretiel
Copy link
Contributor

@Lucretiel Lucretiel commented Nov 15, 2018

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.

@sanmai-NL
Copy link

@sanmai-NL sanmai-NL commented Nov 17, 2018

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

@matthiasbeyer
Copy link

@matthiasbeyer matthiasbeyer commented Apr 25, 2019

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

@ghost
Copy link

@ghost ghost commented Nov 29, 2019

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()
    }
}
@Ayplow
Copy link

@Ayplow Ayplow commented Jan 13, 2020

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?

@nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jan 13, 2020

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.

@GrayJack
Copy link
Contributor

@GrayJack GrayJack commented Nov 3, 2020

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

@jhpratt
Copy link
Contributor

@jhpratt jhpratt commented Nov 3, 2020

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

@GrayJack
Copy link
Contributor

@GrayJack GrayJack commented Nov 3, 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?

@ehuss
Copy link
Contributor

@ehuss ehuss commented Nov 3, 2020

This issue should be closed

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.