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
6 changes: 3 additions & 3 deletions src/librustc/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1764,12 +1764,12 @@ The `main` function was incorrectly declared.
Erroneous code example:

```compile_fail,E0580
fn main() -> i32 { // error: main function has wrong type
0
fn main(x: i32) { // error: main function has wrong type
println!("{}", x);
}
```

The `main` function prototype should never take arguments or return type.
The `main` function prototype should never take arguments.
Example:

```
Expand Down
35 changes: 16 additions & 19 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1106,25 +1106,22 @@ fn check_fn<'a, 'gcx, 'tcx>(inherited: &'a Inherited<'a, 'gcx, 'tcx>,
}
fcx.demand_suptype(span, ret_ty, actual_return_ty);

if fcx.tcx.features().termination_trait {
// If the termination trait language item is activated, check that the main return type
// implements the termination trait.
if let Some(term_id) = fcx.tcx.lang_items().termination() {
if let Some((id, _)) = *fcx.tcx.sess.entry_fn.borrow() {
if id == fn_id {
match fcx.sess().entry_type.get() {
Some(config::EntryMain) => {
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);

inherited.register_predicate(
traits::Obligation::new(
cause, param_env, trait_ref.to_predicate()));
},
_ => {},
}
// Check that the main return type implements the termination trait.
if let Some(term_id) = fcx.tcx.lang_items().termination() {
if let Some((id, _)) = *fcx.tcx.sess.entry_fn.borrow() {
if id == fn_id {
match fcx.sess().entry_type.get() {
Some(config::EntryMain) => {
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


inherited.register_predicate(
traits::Obligation::new(
cause, param_env, trait_ref.to_predicate()));
},
_ => {},
}
}
}
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_typeck/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@ fn check_main_fn_ty<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
}

let actual = tcx.fn_sig(main_def_id);
let expected_return_type = if tcx.lang_items().termination().is_some()
&& tcx.features().termination_trait {
let expected_return_type = if tcx.lang_items().termination().is_some() {
// we take the return type of the given main function, the real check is done
// in `check_fn`
actual.output().skip_binder()
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,6 @@
#![feature(str_char)]
#![feature(str_internals)]
#![feature(str_utf16)]
#![feature(termination_trait)]
#![feature(test, rustc_private)]
#![feature(thread_local)]
#![feature(toowned_clone_into)]
Expand All @@ -325,6 +324,7 @@
#![cfg_attr(test, feature(update_panic_count))]
#![cfg_attr(windows, feature(used))]
#![cfg_attr(stage0, feature(never_type))]
#![cfg_attr(stage0, feature(termination_trait))]

#![default_lib_allocator]

Expand Down
6 changes: 4 additions & 2 deletions src/libsyntax/feature_gate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,8 +429,8 @@ declare_features! (
// `foo.rs` as an alternative to `foo/mod.rs`
(active, non_modrs_mods, "1.24.0", Some(44660), None),

// Termination trait in main (RFC 1937)
(active, termination_trait, "1.24.0", Some(43301), None),
// Termination trait in tests (RFC 1937)
(active, termination_trait_test, "1.24.0", Some(48854), None),

// Allows use of the :lifetime macro fragment specifier
(active, macro_lifetime_matcher, "1.24.0", Some(46895), None),
Expand Down Expand Up @@ -551,6 +551,8 @@ declare_features! (
(accepted, match_beginning_vert, "1.25.0", Some(44101), None),
// Nested groups in `use` (RFC 2128)
(accepted, use_nested_groups, "1.25.0", Some(44494), None),
// Termination trait in main (RFC 1937)
(accepted, termination_trait, "1.25.0", Some(43301), None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build for accepted is when it was accepted, right? Which I think will now be 1.26.

Also, you could consider making this feature be termination_trait_main, which would let all the remaining checks (and attributes in the minimal nightly code) stay unchanged.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure about this. The tracking issue (#48453) has milestone 1.25.

re: termination_trait_main, it makes a little more sense since the trait itself is still not stable. However, it makes just as much sense to me to keep the trait for tests as it is now in the PR. Then we'd have termination_trait_main, termination_trait_lib, and termination_trait_test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After re-reading the Forge guide, it should definitely be 1.26. Updated.

For the feature name, I see where you're coming from more now: pull out the thing being stabilized, and leave the unstable stuff in the "main feature." I have no strong preference here at all, and went with the initial recommendation. If two people agree on what it should be, or one person feels more strongly and no one objects, I'll change it to that!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good by me! (I try to say "consider" for things I'm not that concerned about.)

// a..=b and ..=b
(accepted, inclusive_range_syntax, "1.26.0", Some(28237), None),
// allow `..=` in patterns (RFC 1192)
Expand Down
8 changes: 4 additions & 4 deletions src/libsyntax/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ fn is_test_fn(cx: &TestCtxt, i: &ast::Item) -> bool {
ast::ItemKind::Fn(ref decl, _, _, _, ref generics, _) => {
// If the termination trait is active, the compiler will check that the output
// type implements the `Termination` trait as `libtest` enforces that.
let output_matches = if cx.features.termination_trait {
let output_matches = if cx.features.termination_trait_test {
true
} else {
let no_output = match decl.output {
Expand All @@ -359,7 +359,7 @@ fn is_test_fn(cx: &TestCtxt, i: &ast::Item) -> bool {
match has_test_signature(cx, i) {
Yes => true,
No => {
if cx.features.termination_trait {
if cx.features.termination_trait_test {
diag.span_err(i.span, "functions used as tests can not have any arguments");
} else {
diag.span_err(i.span, "functions used as tests must have signature fn() -> ()");
Expand Down Expand Up @@ -388,7 +388,7 @@ fn is_bench_fn(cx: &TestCtxt, i: &ast::Item) -> bool {

// If the termination trait is active, the compiler will check that the output
// type implements the `Termination` trait as `libtest` enforces that.
let output_matches = if cx.features.termination_trait {
let output_matches = if cx.features.termination_trait_test {
true
} else {
let no_output = match decl.output {
Expand Down Expand Up @@ -416,7 +416,7 @@ fn is_bench_fn(cx: &TestCtxt, i: &ast::Item) -> bool {
if has_bench_attr && !has_bench_signature {
let diag = cx.span_diagnostic;

if cx.features.termination_trait {
if cx.features.termination_trait_test {
diag.span_err(i.span, "functions used as benches must have signature \
`fn(&mut Bencher) -> impl Termination`");
} else {
Expand Down
22 changes: 22 additions & 0 deletions src/test/compile-fail/feature-gate-termination_trait_test.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// compile-flags: --test

fn main() {}

#[cfg(test)]
mod tests {
#[test]
fn it_works() -> Result<(), ()> {
//~^ ERROR functions used as tests must have signature fn() -> ()
Ok(())
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]
// error-pattern:`main` can only return types that implement std::process::Termination, not `i32`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice if this suggested some types that do work, like () and !. (Especially since Termination is unstable, which makes these errors more confusing, based on reaction to Carrier errors.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's a good idea. In particular, Result<(), E> where E: Debug helps me understand the motivation better. Is there any guidance for how long error messages can be?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like fitting the Result suggestion into an error message is going to make this note too long no matter what, though I'm open to suggestions. So we are left with () and !.

Only question is, will a new Rust user from C who writes fn main() -> i32 have any idea what ! is, and if not, how would they Google it? () and ! are both somewhat cryptic type names, and it seems like a bad experience for someone to come across error message with both of these types potentially very early on in their Rust language usage. That said, I can see how this would be helpful for people from other perspectives, and I'm sure I'm rehashing some argument from the original RFC. Perhaps we can use never.

For now, I've only included () as an example, with the expectation that someone who really wants to know what's going on can google std::process::Termination and read about other types that implement it. (I tried it, the first result gives all implementations!) I'd like to know what other people think about this as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The longer explanation at E0580 would do well to give the full list of types implementing Termination in the examples, and link to the std docs for the arcane types.

fn main() -> i32 {
0
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.
#![feature(termination_trait)]

// error-pattern:`main` can only return types that implement std::process::Termination, not `char
fn main() -> char {
//~^ ERROR: the trait bound `char: std::process::Termination` is not satisfied
' '
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(termination_trait)]

struct ReturnType {}

fn main() -> ReturnType { //~ ERROR `ReturnType: std::process::Termination` is not satisfied
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(termination_trait)]

// error-pattern:oh, dear

fn main() -> ! {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@
// must-compile-successfully
// failure-status: 1

#![feature(termination_trait)]

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

fn main() -> Result<(), Box<Error>> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,4 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(termination_trait)]

fn main() {}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(termination_trait)]
#![feature(process_exitcode_placeholder)]

use std::process::ExitCode;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(termination_trait)]

use std::io::Error;

fn main() -> Result<(), Box<Error>> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(termination_trait)]

use std::io::Error;

fn main() -> Result<(), Error> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

// compile-flags: --test

#![feature(termination_trait)]
#![feature(termination_trait_test)]
#![feature(test)]

extern crate test;
Expand Down