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 main with non-() return types #48453

Closed
2 tasks
nikomatsakis opened this issue Feb 23, 2018 · 28 comments
Closed
2 tasks

Stabilize main with non-() return types #48453

nikomatsakis opened this issue Feb 23, 2018 · 28 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Milestone

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Feb 23, 2018

This is a sub-issue of the larger tracking issue devoted to the ? in main RFC. It is specifically proposing that we stabilize the behavior that permits main to have a return type other than (). This return type must implement the Termination trait. The details of the trait itself (including its name and so forth) are not stabilized.

This commits us to the following (each link is to a test):

The changes from the RFC are as follows:

  • we modified the Result impl to use Debug

The following unresolved questions will be resolved:

But the following is not stabilized and free to change:

  • the name/location of the Termination trait and its methods
    • likely changing to std::process::Termination
  • the usage of -> Result<...> in unit tests (not yet landed)
  • the ErrCode newtype wrapper for returning a custom error code

--

UPDATE 1: Restricted the set of termination impls as proposed by @scottmcm in #48497.

@nikomatsakis nikomatsakis added the T-lang Relevant to the language team, which will review and decide on the PR/issue. label Feb 23, 2018
@nikomatsakis
Copy link
Contributor Author

@rfcbot fcp merge

I propose that we stabilize as described above.

@rfcbot
Copy link

rfcbot commented Feb 23, 2018

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged teams:

Concerns:

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Feb 23, 2018
@scottmcm
Copy link
Member

👍 to stabilizing the basics here.

👎 to some of the allowed return types.

I would propose only stabilizing the following new return types:

  • !
  • Result<(), E: Debug>
  • Result<!, E: Debug>

Thoughts on the others:

  • Arbitrary nesting is weird. Why would one have a Result<Result<!, io::Error>, fmt::Error>?
  • Nesting also makes return values weird, like with Result<i32, _> you run into collisions with Ok(1) and Err(_).
  • I still prefer some domain-specific type (strawman ExitCode) to i32. I could accept just fn main() -> i32, but it feels like primitive obsession (especially if nesting happens, where it's meaning is far less clear). There were some interesting discussions in the RFC about platform differences here too, which feel like they could be a good fit with the "vision for portability" stuff. And this case in particular isn't needed for the ?-in-main goals, so can easily wait. As a bonus, I think having a type here that's not a primitive would let us leave the impl but have have the type unstable as the details are figured out.

I'll happily make a PR restricting things as above if that'd be helpful.

@cramertj
Copy link
Member

cramertj commented Feb 23, 2018

@rfcbot concern delay stabilization of nesting and i32 exit code

I feel basically the same way that @scottmcm does-- nesting (e.g. Result<i32, Error>) seems confusing and hard to reason about, and i32 seems like it should be ExitCode, perhaps with a from_raw method. Can we delay stabilizing these particular impls pending further discussion? I'd love to see ! and Result<!/(), E: Debug> stabilize.

Edit: also bool?

@bkchr
Copy link
Contributor

bkchr commented Feb 23, 2018

Yeah it is the opposite, wasm is 0 and 1 and all other platforms use libc::*. I have done that, because libc::EXIT_SUCCESS and libc::EXIT_FAILURE were not available at that time for wasm in libc.

@scottmcm
Copy link
Member

I noticed that there's impl Termination for bool right now too (it's not mentioned in the OP), which I don't think should be part of the minimal set that we stabilize in the first pass.

@ExpHP
Copy link
Contributor

ExpHP commented Feb 23, 2018

The RFC says E: Display instead of E: Debug, and tbh, Display would have been my first choice as well. But I'm sure this has been discussed.

Should the RFC be amended to reflect this change, and to explain why Debug is the right choice?

@U007D
Copy link

U007D commented Feb 23, 2018

@ExpHP yes, you are correct. Something approaching a summary of thinking around Display vs Debug starts around here: #43301 (comment).

@withoutboats
Copy link
Contributor

I'm surprised by the concern about nesting - how would someone come to nest results in their main return anyway? It seems like the natural way to implement termination, which has the consequence of enabling this kind of thing, but that it happening in practice would be a non-issue.

Should RFC be amended to reflect this change, and to explain why Debug is the right choice?

we do not usually amend RFCs to cover changes before stabilization. they are design documents for implementation, they dont play any sort of 'constitutional' role.

@U007D
Copy link

U007D commented Feb 23, 2018

@withoutboats, that is interesting.

If I'm understanding you correctly, by allowing Result<T, E>, T itself may be a Result<U, F>, but that we should not invent extra machinery to disallow this? That does make a lot of sense...

Personally, I was only considering what to allow from the perspective of being conservative; but when being conservative mandates extra logic to specifically disallow certain types, I can see the problem. (Such a limitation could also feel quite arbitrary in blocking some scenario we might not be imagining at the moment...)

Thanks, as usual, for the enlightening perspective. :)

@nikomatsakis
Copy link
Contributor Author

Fixed the wasm reference.

I also thought we might want to limit the impls. Note that we have no mechanism for "feature-gating" impls, so the only way to limit things once we stabilize the basic concept is to remove the stuff we don't want.

I think that supporting only the basics (!, (), and Result<(), E>) seems good to start -- I anticipate that anyone who wants to use this feature for more complex things will define their own new-type and implement Terminate themselves. That is certainly what I plan to do in my own code, since I think it's a nice way to separate out the error handling from the rest.

If we wanted to go crazy (I don't), I could imagine adding a new trait like SuccessfulTerminate and then implementing Terminate for T and Result<T, E> where T: SuccessfulTerminate. This avoids nesting but keeps flexibility. (We would implement SuccessfulTerminate for () and ExitCode or something.) But it feels like overkill, and it can be readily modeled with your own newtype anyhow.

@nikomatsakis
Copy link
Contributor Author

@scottmcm

I noticed that there's impl Termination for bool right now too (it's not mentioned in the OP), which I don't think should be part of the minimal set that we stabilize in the first pass.

good catch, I missed that

@nikomatsakis
Copy link
Contributor Author

@scottmcm if you prep the PR, I will update the description

@withoutboats
Copy link
Contributor

I think the alternative that @cramertj and @scottmcm would prefer is that we replace the Result impl with two impls:

impl<E: Debug> Termination for Result<(), E>
impl<E: Debug> Termination for Result<!, E>

Then its not all types that implement Termination, just () and !.

I wouldn't really mind doing this, but I think philosophically I'd tend to prefer an open system that allows for some confusing cases than a closed system. Having the open impl could allow for use cases involving third party types we haven't thought of. As an example, I could see some framework with a special type that implements Termination, and its idiomatic within that framework is to return from main with Result<FrameworkTerminationType, Error>.

Its sort of the nature of a type class based polymorphism system that we enable extensions we haven't anticipated, including possibly bad ones.

I'd also draw a distinction between designs that make things that we don't like possible and designs that lead people into an outcome we don't like. Does anyone see a way that the current set of impls encourages users to do a nested Result? I don't.

I think all of the impls that exist - including i32 and bool - are fine and good and should exist, but I also don't see the ones that people want removed actually being used very often, so I'd be fine removing them to make progress on the impls that people very clearly want.

@nikomatsakis
Copy link
Contributor Author

@withoutboats ah, I somehow misread and thought that you had registered the concern (but I see now it was @cramertj).

I think I feel the same as you -- I found the nesting a bit odd but probably better on balance than The Right Thing of having more traits, and not necessarily bad.

Also, the design I mentioned is a bit busted, because any design that allows the nested type to specify the exit code means it can specify a non-zero error code, which is traditionally an error result (but not necessarily, there is no fixed interpretation; I mean consider a command intended to be used within the bash if -- is it an error if the condition returns 1).

Anyway, I also just kind of don't care that much. I'm happy to start with a simple set of impls. I'm also happy to start with a more expansive set.

@withoutboats
Copy link
Contributor

Anyway, I also just kind of don't care that much. I'm happy to start with a simple set of impls. I'm also happy to start with a more expansive set.

Agreed. If @cramertj and @scottmcm still feel strongly after our counterarguments, I wouldn't register a concern in the opposite direction. :-)

@cramertj
Copy link
Member

cramertj commented Feb 24, 2018

Ah, I understand the desire for nesting now if the goal was to reduce the number of traits involved. It seems unlikely that someone would write Result<Result<(), Error>, Error>, so I'm not really worried so much about that case. IMO -> Result<i32, Error> and -> Result<bool, Error> seem more likely to occur in practice, and their behavior is even less obvious, but I'd still be willing to file that under "things that are crazy but that people won't ever actually encounter unless they're intentionally messing with the system".

I still feel fairly strongly that -> i32 and -> bool are in need of more typesafe wrappers.

@scottmcm
Copy link
Member

PR up at #48497, with impls as boats guessed.

I added an unstable please-don't-rely-on-this-at-all std::process::ExitCode with an ugly feature name so I could keep the return-a-code-directly impl in a way that doesn't stabilize it; hopefully that's ok.

I'm definitely not saying we'll never re-broaden these impls, but I like stabilizing just the obviously valuable cases now and seeing what comes up, like rust-lang/rfcs#1937 (comment)

@nikomatsakis
Copy link
Contributor Author

@cramertj care to resolve your concern?

@cramertj
Copy link
Member

@rfcbot resolved delay stabilization of nesting and i32 exit code

@nikomatsakis Thanks! Just saw the r+ on @scottmcm's PR.

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 26, 2018
@rfcbot
Copy link

rfcbot commented Feb 26, 2018

🔔 This is now entering its final comment period, as per the review above. 🔔

kennytm added a commit to kennytm/rust that referenced this issue Feb 27, 2018
…n, r=nikomatsakis

Restrict the Termination impls to simplify stabilization

Make a minimal commitment in preparation for stabilization.  More impls, or broader ones, are likely in future, but are not necessary at this time and are more controversial.

cc rust-lang#48453 (comment)
r? @nikomatsakis
alexcrichton added a commit to alexcrichton/rust that referenced this issue Mar 7, 2018
…crichton

Better docs and associated SUCCESS/FAILURE for process::ExitCode

Follow-up to rust-lang#48497 (comment), since that PR was the minimal thing to unblock rust-lang#48453 (comment).

r? @nikomatsakis
@rfcbot
Copy link

rfcbot commented Mar 8, 2018

The final comment period is now complete.

@Centril Centril added this to the 1.25 milestone Mar 8, 2018
@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Mar 8, 2018
@nikomatsakis
Copy link
Contributor Author

I added #48854 to propose stabilizing unit tests that return results.

@nikomatsakis
Copy link
Contributor Author

nikomatsakis commented Mar 8, 2018

All right, let's do this! Here are some mentoring instructions for preparing a stabilization PR.

The general instructions for stabilization can be found on forge. We are stabilizing the termination_trait (but not termination_trait_lib) feature.

Technically, that stabilizes more than is described here, because it will also stabilize using non-() return types in unit tests. But I've already opened #48854 to vote on that, and I expect it to pass, so we can prep the PR doing both and then just wait to merge.

UPDATE: OK, looks like we may want to split the feature gates after all. See here for further mentoring instructions.

@scottmcm
Copy link
Member

cc #48890 for an ICE in this feature

@nikomatsakis
Copy link
Contributor Author

OK, it seems like I was wrong about #48854, perhaps we will wait to stabilize that feature. That means that the stabilization PR for this feature is going to need to do two things:

  • Introduce a new feature gate, covering only the use of this feature in unit tests. (Good first PR)
    • Good first PR!
    • Let's call it #[termination_trait_test] or something.
  • Stabilize the remaining #[termination_trait] use cases.

To do the first part (separation) will involve the following steps:

  • Create a new feature gate, termination_trait_test
    • The process is described in the "Stability in Code" section of this forge page
    • But you might also just run rg '\btermination_trait\b and copy what's there already
  • Modify the code in libsyntax that is gated on termination_trait to be gated on termination_trait_test:

https://github.com/rust-lang/rust/blob/master/src/libsyntax/test.rs#L334

https://github.com/rust-lang/rust/blob/master/src/libsyntax/test.rs#L361

https://github.com/rust-lang/rust/blob/master/src/libsyntax/test.rs#L390

https://github.com/rust-lang/rust/blob/master/src/libsyntax/test.rs#L418

There may be a few other places, not sure.

tmandry added a commit to tmandry/rust that referenced this issue Mar 19, 2018
This stabilizes `main` with non-() return types; see rust-lang#48453.
@nikomatsakis
Copy link
Contributor Author

@tmandry opened up #49162 =)

frewsxcv added a commit to frewsxcv/rust that referenced this issue 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 issue 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.
kennytm added a commit to kennytm/rust that referenced this issue 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 issue 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.
@jkordish jkordish added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Apr 25, 2018
@Centril Centril added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 24, 2018
@scottmcm
Copy link
Member

This stabilization happened; closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. finished-final-comment-period The final comment period is finished for this PR / Issue. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

10 participants