Implements RFC 1937: `?` in `main` by bkchr · Pull Request #46479 · rust-lang/rust

bkchr · 2017-12-03T21:28:58Z

This is the first part of the RFC 1937 that supports new
Termination trait in the rust main function.

Thanks @nikomatsakis, @arielb1 and all other people in the gitter channel for all your help!

The support for doctest and #[test] is still missing, bu as @nikomatsakis said, smaller pull requests are better :)

rust-highfive · 2017-12-03T21:29:04Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (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.

scottmcm · 2017-12-04T03:24:48Z

Consider having only this impl in the first commit, since some of the others are more controversial in the details.
(Tests of the feature can add their own impls for their own types.)

scottmcm · 2017-12-04T03:27:02Z

Reminder to undo this at some point.

steveklabnik · 2017-12-04T13:56:07Z

is this line expected?

Yeah, that is expected. There are multiple empty lines (scroll a little bit up in the file).

shepmaster · 2017-12-09T15:43:58Z

Feels unlikely that @arielb1 will have much time to review this before the end of the year, so let's reassign to...

r? @estebank

arielb1 · 2017-12-09T20:25:39Z

use self.tcx().lang_items().start_fn == Some(def_id) to avoid spurious errors on small tests.

(To elaborate, I believe @arielb1's goal here is to avoid requiring the start lang item to be present if you are trying to write a tiny test targeting e.g. a #![no_std] library, where the start lang item would not otherwise be needed. I agree this makes sense.)

Yeah, already done that! :)

arielb1 · 2017-12-09T20:27:42Z

Because user-defined start is supposed to be non-generic, passing a substs to it could cause problems, so please don't.

You are right, but user-defined start is handled in the else branch.

arielb1 · 2017-12-09T20:32:06Z

You'll need to add a feature gate to avoid this being insta-stable for types other than ().

That should also "fix" the compile-fail tests. Or you could just fix them for real and add #![feature(termination_trait)] to them.

arielb1 · 2017-12-09T20:35:19Z

You might want a
#[rustc_on_unimplemented("`main` can only return types that implement {Termination}, not `{Self}`")]

arielb1 · 2017-12-09T20:37:02Z

Why did you make this change? Seems like purely churn.

That was done by @nikomatsakis to fix the tests.

In particular, the new definition of start based around Termination winds up pulling in a lot of code related to panics and unwinds, causing the output of -Zprint-type-sizes to have a lot of unrelated gunk in it.

arielb1 · 2017-12-09T20:40:20Z

@shepmaster

I would rather r? @nikomatsakis if I don't finish this.

However, I think this is basically good to go, except:

this needs a feature-gate to avoid being insta-stable
I would like a run-fail (or run-make) test to see that if you return an integer other than 0 from main, this actually causes a failure.

shepmaster · 2017-12-09T22:59:34Z

@arielb1 sounds good; just didn't want to put too much on your plate! ❤️

r? @arielb1

bors · 2017-12-19T04:23:28Z

☔ The latest upstream changes (presumably #45525) made this pull request unmergeable. Please resolve the merge conflicts.

arielb1 · 2017-12-21T17:22:32Z

nit: replace this with
ifletSome(term_id) = fcx.tcx.lang_items().termination().is_some(){
And then you won't need to require it within.

arielb1 · 2017-12-21T17:24:58Z

nit: use if let Some(term_id) = fcx.tcx.lang_items().termination(), then you won't need to require it later

arielb1 · 2017-12-21T17:28:33Z

@bkchr

This still needs a feature gate, also it would bee nice if you fix the nit.

alexcrichton · 2017-12-26T22:03:29Z

+
+ sys::init();
+
+ process::exit(unsafe {


From our discussion on gitter, this is the cause of the problem in wasm (usage of process::exit which is an abort on wasm).
Let's change this to propagate the error code outwards I think?

This'll avoid exporting a symbol from binaries unnecessarily and should help the linker clean things up if it can.

arielb1 · 2017-12-26T23:20:57Z

@bors r+

bors · 2017-12-26T23:20:59Z

📌 Commit 5a4298b has been approved by arielb1

bors · 2017-12-26T23:30:22Z

⌛ Testing commit 5a4298b with merge 60fb575...

@nikomatsakis

Implements RFC 1937: `?` in `main` This is the first part of the RFC 1937 that supports new `Termination` trait in the rust `main` function. Thanks @nikomatsakis, @arielb1 and all other people in the gitter channel for all your help! The support for doctest and `#[test]` is still missing, bu as @nikomatsakis said, smaller pull requests are better :)

This reverts commit 267800a.

bkchr · 2017-12-27T08:52:58Z

Can someone approve?

killercup · 2017-12-27T13:51:11Z

@bors r=arielb1

bors · 2017-12-27T13:51:12Z

📌 Commit 09f94be has been approved by arielb1

bors · 2017-12-27T15:41:57Z

⌛ Testing commit 09f94be with merge bfbb1f5...

@nikomatsakis

Implements RFC 1937: `?` in `main` This is the first part of the RFC 1937 that supports new `Termination` trait in the rust `main` function. Thanks @nikomatsakis, @arielb1 and all other people in the gitter channel for all your help! The support for doctest and `#[test]` is still missing, bu as @nikomatsakis said, smaller pull requests are better :)

bors · 2017-12-27T18:27:17Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing bfbb1f5 to master...

bkchr · 2017-12-27T18:28:25Z

Finally! :)

Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

rust-highfive assigned arielb1 Dec 3, 2017

bkchr force-pushed the termination_trait branch from 3942e38 to c3ce50aCompare December 3, 2017 21:52

scottmcm reviewed Dec 4, 2017
View reviewed changes

Comment threadsrc/librustc_trans/lib.rs Outdated
scottmcmDec 4, 2017
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to undo this at some point.

bkchr force-pushed the termination_trait branch from c3ce50a to b79ba55Compare December 4, 2017 08:13

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 4, 2017

steveklabnik reviewed Dec 4, 2017
View reviewed changes

bkchr force-pushed the termination_trait branch from 6181689 to 3f80837Compare December 5, 2017 11:11

rust-highfive assigned estebank and unassigned arielb1 Dec 9, 2017

arielb1 reviewed Dec 9, 2017
View reviewed changes

rust-highfive assigned arielb1 and unassigned estebank Dec 9, 2017

arielb1 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 13, 2017

bkchr mentioned this pull request Dec 17, 2017
Tracking issue for RFC 1937: ? in main#43301
Closed
12 tasks

nikomatsakis mentioned this pull request Dec 20, 2017
Epoch.Next Tracking Issue #46889
Closed

bkchr force-pushed the termination_trait branch from 3f80837 to d4f1f05Compare December 21, 2017 12:11

arielb1 reviewed Dec 21, 2017
View reviewed changes

bkchr force-pushed the termination_trait branch from cd5ffc2 to 81e375dCompare December 26, 2017 12:08

alexcrichton reviewed Dec 26, 2017
View reviewed changes

alexcrichtonand others added 2 commits December 26, 2017 14:26

rustc: Switch start_fn to hidden visibility
2cdd1c4
This'll avoid exporting a symbol from binaries unnecessarily and should help the linker clean things up if it can.

Don't use process::exit as it is an unreachable on wasm32
5a4298b

Revert "New generated main returns void"
09f94be
This reverts commit 267800a.

kennytm added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 27, 2017

bors merged commit 09f94be into rust-lang:masterDec 27, 2017

bors mentioned this pull request Dec 27, 2017
Stabilized #[repr(align(x))] attribute (RFC 1358) #47006
Merged

This was referenced Dec 29, 2017
ICE when trying to cross compile with xargo #47040
Closed
[RFC] enforce main: fn() -> !rust-embedded/cortex-m-rt#50
Closed

U007D mentioned this pull request Jan 16, 2018
Using failure with #![termination_trait] (fn main() -> Result<T>) rust-lang-deprecated/failure#130
Open

sgrif added a commit to sgrif/rust that referenced this pull request Jan 31, 2018
Re-add some removed uses of Kind
cd8bc39
Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

sgrif added a commit to sgrif/rust that referenced this pull request Feb 6, 2018
Re-add some removed uses of Kind
d4727ef
Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

canarysnort01 mentioned this pull request Feb 12, 2018
Nightly: internal compiler error compiling robigalia #48119
Closed

sgrif added a commit to sgrif/rust that referenced this pull request Mar 1, 2018
Re-add some removed uses of Kind
4fb201d
Additional uses of this item were added to these files in rust-lang#45701 and rust-lang#46479

Uh oh!

Uh oh!

Conversation

bkchr commented Dec 3, 2017

Uh oh!

rust-highfive commented Dec 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchrDec 4, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shepmaster commented Dec 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bkchrDec 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Dec 9, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shepmaster commented Dec 9, 2017

Uh oh!

bors commented Dec 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arielb1 commented Dec 26, 2017

Uh oh!

bors commented Dec 26, 2017

Uh oh!

bors commented Dec 26, 2017

Uh oh!

bkchr commented Dec 27, 2017

Uh oh!

killercup commented Dec 27, 2017

Uh oh!

bors commented Dec 27, 2017

Uh oh!

bors commented Dec 27, 2017

Uh oh!

bors commented Dec 27, 2017

Uh oh!

bkchr commented Dec 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

bkchrDec 4, 2017•
edited
Loading

bkchrDec 21, 2017•
edited
Loading

arielb1 commented Dec 9, 2017•
edited
Loading