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

Stabilize termination_trait, split out termination_trait_test #49162

Merged
merged 11 commits into from Mar 24, 2018

Conversation

@tmandry
Copy link
Contributor

tmandry commented Mar 19, 2018

For #48453.

First time contribution, so I'd really appreciate any feedback on how this PR can be better.

Not sure exactly what kind of documentation update is needed. If there is no PR to update the reference, I can try doing that this week as I have time.

tmandry added 2 commits Mar 19, 2018
This stabilizes `main` with non-() return types; see #48453.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 19, 2018

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@tmandry

This comment has been minimized.

Copy link
Contributor Author

tmandry commented Mar 19, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 19, 2018

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 19, 2018

@tmandry

First time contribution, so I'd really appreciate any feedback on how this PR can be better.

Woohoo, welcome! <3

@nikomatsakis nikomatsakis mentioned this pull request Mar 19, 2018
0 of 2 tasks complete
Copy link
Contributor

nikomatsakis left a comment

This looks great! My one concern is whether we can improve that error message in the case of a bad return type for main. I fear it is going to be confuse people as is.

@@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

fn main() -> i32 { //~ ERROR main function has wrong type [E0580]
fn main() -> i32 {
//~^ ERROR the trait bound `i32: std::process::Termination` is not satisfied [E0277]

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

Hmm, this is not your fault, but this error message seems less clear than before.

@estebank -- can we improve this readily enough with a #[rustc_on_unimplemented] annotation perhaps?

This comment has been minimized.

Copy link
@estebank

estebank Mar 19, 2018

Contributor

Yes, you could annotate std::process::Termination using #[rustc_on_unimplemented] to replace the error message to something appropriate. The only limitation is not knowing wether the requirement came from a return type or anywhere else, so the message will have to be more generic than it is now.

This comment has been minimized.

Copy link
@tmandry

tmandry Mar 20, 2018

Author Contributor

It looks like there is already a #[rustc_on_unimplemented] annotation, but it is not used in the error message for some reason.

This comment has been minimized.

Copy link
@tmandry

tmandry Mar 20, 2018

Author Contributor

Nevermind, once I cleaned my rustc build the note showed up. But since it's a label, not the primary error message, I had to use the error-message: directive to match against it.

This comment has been minimized.

Copy link
@estebank

estebank Mar 20, 2018

Contributor

For what it's worth, you have further control on the rustc_on_unimplemented output by being able to set a separate message and label, as well as customize either one for specific types.

That way, after including Niko's span change you could have the following output:

error[E0277]: `main` can only return types that implement `std::process::Termination`, not `i32`
  --> src/bin/termination-trait-main-i32.rs:11:18
   |
11 | fn main() -> i32 {
   |              ^^^ can only return types that implement `std::process::Termination`
   |
   = help: the trait `std::process::Termination` is not implemented for `i32`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

This comment has been minimized.

Copy link
@estebank

estebank Mar 20, 2018

Contributor

since it's a label, not the primary error message, I had to use the error-message: directive to match against it.

You didn't need to use error-message, you could use

//~^ NOTE `main` can only return types that implement std::process::Termination, not `i32`

instead for labels.

@tmandry

This comment has been minimized.

Copy link
Contributor Author

tmandry commented Mar 20, 2018

I updated the tests to reflect the error label that's already there from rustc_on_unimplemented. Here's what the full output looks like:

error[E0277]: the trait bound `i32: std::process::Termination` is not satisfied
  --> src/bin/termination-trait-main-i32.rs:11:18
   |
11 |   fn main() -> i32 {
   |  __________________^
12 | | //~^ ERROR the trait bound `i32: std::process::Termination` is not satisfied [E0277]
13 | |     0
14 | | }
   | |_^ `main` can only return types that implement std::process::Termination, not `i32`
   |
   = help: the trait `std::process::Termination` is not implemented for `i32`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2018

@tmandry ok, so, that is looking better! the main problem is that the span we are giving for the error (i.e., the region in the source code that we highlight) is the function body, which makes it cross multiple lines and makes my eyes glaze over (I imagine others have the same reaction). Let me see if I can give any tips about how to get a better span.

let substs = fcx.tcx.mk_substs(iter::once(Kind::from(ret_ty)));
let trait_ref = ty::TraitRef::new(term_id, substs);
let cause = traits::ObligationCause::new(
span, fn_id, ObligationCauseCode::MainFunctionType);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 20, 2018

Contributor

the span is specified here; if you look up to line 1028 you can see it comes from the body:

let span = body.value.span;

In general, to get a good span, you want to look to the HIR, which corresponds pretty closely to the input program. In this case, we want to look at the argument decl:

decl: &'gcx hir::FnDecl,

The FnDecl struct is declared here:

rust/src/librustc/hir/mod.rs

Lines 1718 to 1727 in 5ccf3ff

/// Represents the header (not the body) of a function declaration
#[derive(Clone, PartialEq, Eq, RustcEncodable, RustcDecodable, Hash, Debug)]
pub struct FnDecl {
pub inputs: HirVec<P<Ty>>,
pub output: FunctionRetTy,
pub variadic: bool,
/// True if this function has an `self`, `&self` or `&mut self` receiver
/// (but not a `self: Xxx` one).
pub has_implicit_self: bool,
}

We want to get the span from the return type, for which there is a convenient method:

rust/src/librustc/hir/mod.rs

Lines 1815 to 1820 in 5ccf3ff

pub fn span(&self) -> Span {
match *self {
DefaultReturn(span) => span,
Return(ref ty) => ty.span,
}
}

So basically the span should be something like:

let return_ty_span = decl.output.span();

and use that instead of span.

@tmandry

This comment has been minimized.

Copy link
Contributor Author

tmandry commented Mar 20, 2018

Thanks for your help! I agree, the error message is much better this way. Updated and converted one of the tests to a UI test.

@tmandry tmandry changed the title [WIP] Stabilize termination_trait, split out termination_trait_test Stabilize termination_trait, split out termination_trait_test Mar 20, 2018
@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2018

@tmandry 🏂

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2018

cc @rust-lang/docs -- I want to r+ this PR! It stabilizes fn main() -> T { } where T: Termination (though not the termination trait itself). What kind of documentation is needed and where?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2018

According to @aturon, for edition-related docs we're going to handle them out of band. Therefore, I'm allowed to r+.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 20, 2018

📌 Commit 72334fe has been approved by nikomatsakis

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 20, 2018

@tmandry nice work

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 22, 2018

@bors r-

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 22, 2018

So, it does look great, but I did just want to make two comments:

It does seem like it'd be marginally better to use the E0580 code, so that we can have a --explain output that is tailored to this case. I can help @tmandry with the answer to this question:

since I'm not sure how to go from here to librustc::infer::error_reporting where E0580 is used (or vice versa)

in one second.

Regarding what @scottmcm wrote:

One thought: is there a canonical "I don't really care I just want to use ?" Result type we could point people to?

Interesting point. I don't think though that there is :( -- I wish that Result<_, Box<Error>> would work but for Reasons I don't think it will really will, and I don't think that Box<dyn Debug> would work because there needed Into clauses don't exist.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Mar 22, 2018

@bors r+

Ugh. OK, digging into the code I see what @tmandry was tempted to leave it the way they did. =) It's a bit annoying to use E0580. I'm sort of inclined to land this now, and leave that for possible follow-up. So r+ again.

One interesting thing is that E0580 is also issued for things unrelated to the return type -- e.g., supplying arguments, or making main unsafe or the wrong ABI. So in a sense maybe using E0580 isn't the best option. If we did want to do it, we would create a refactored helper function somewhere that both the error-reporting modules can call into. But this code is in dire need of some refactoring.

Anyway, thanks @tmandry for all the effort you've put into this.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 22, 2018

📌 Commit 2b13d95 has been approved by nikomatsakis

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Mar 22, 2018

@nikomatsakis I just realized that it could make sense to allow rustc_on_unimplemented to set an error code but I'd be worried about over/mis-use...

@scottmcm

This comment has been minimized.

Copy link
Member

scottmcm commented Mar 22, 2018

Filed rust-lang/rfcs#2367 to track coming up with a good return type to suggest.

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 23, 2018
…, r=nikomatsakis

Stabilize termination_trait, split out termination_trait_test

For rust-lang#48453.

First time contribution, so I'd really appreciate any feedback on how this PR can be better.

Not sure exactly what kind of documentation update is needed. If there is no PR to update the reference, I can try doing that this week as I have time.
frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 23, 2018
…, r=nikomatsakis

Stabilize termination_trait, split out termination_trait_test

For rust-lang#48453.

First time contribution, so I'd really appreciate any feedback on how this PR can be better.

Not sure exactly what kind of documentation update is needed. If there is no PR to update the reference, I can try doing that this week as I have time.
bors added a commit that referenced this pull request Mar 23, 2018
Rollup of 13 pull requests

- Successful merges: #48265, #48575, #49028, #49035, #49046, #49064, #49102, #49160, #49162, #49169, #49235, #49262, #49272
- Failed merges:
bors added a commit that referenced this pull request Mar 24, 2018
Rollup of 20 pull requests

- Successful merges: #48482, #49046, #49089, #49101, #49121, #49122, #49141, #49193, #49194, #49203, #49235, #49249, #49254, #49273, #49274, #49284, #49290, #49311, #49312, #49314
- Failed merges: #49162
bors added a commit that referenced this pull request Mar 24, 2018
Rollup of 20 pull requests

- Successful merges: #48482, #49046, #49089, #49101, #49121, #49122, #49141, #49193, #49194, #49203, #49235, #49249, #49254, #49273, #49274, #49284, #49290, #49311, #49312, #49314
- Failed merges: #49162
kennytm added a commit to kennytm/rust that referenced this pull request Mar 24, 2018
…, r=nikomatsakis

Stabilize termination_trait, split out termination_trait_test

For rust-lang#48453.

First time contribution, so I'd really appreciate any feedback on how this PR can be better.

Not sure exactly what kind of documentation update is needed. If there is no PR to update the reference, I can try doing that this week as I have time.
kennytm added a commit to kennytm/rust that referenced this pull request Mar 24, 2018
…, r=nikomatsakis

Stabilize termination_trait, split out termination_trait_test

For rust-lang#48453.

First time contribution, so I'd really appreciate any feedback on how this PR can be better.

Not sure exactly what kind of documentation update is needed. If there is no PR to update the reference, I can try doing that this week as I have time.
bors added a commit that referenced this pull request Mar 24, 2018
Rollup of 21 pull requests

- Successful merges: #49046, #49076, #49089, #49120, #49121, #49122, #49162, #49193, #49194, #49203, #49229, #49235, #49254, #49268, #49273, #49274, #49290, #49312, #49314, #49318, #49299
@bors bors merged commit 2b13d95 into rust-lang:master Mar 24, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tmandry tmandry deleted the tmandry:stabilize-termination-trait branch Mar 24, 2018
@aturon aturon mentioned this pull request Mar 25, 2018
9 of 12 tasks complete
@SimonSapin SimonSapin added the relnotes label Mar 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.