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

Add feature-gated Never-based types #101

Closed
wants to merge 4 commits into from
Closed

Conversation

cramertj
Copy link
Member

This is a resumption of #75. Fix #32.

@alexcrichton Is this what you had in mind for how the feature flag would look?


/// A future which is never resolved.
///
/// This future can be created with the `empty` function.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/empty/never/

@cramertj
Copy link
Member Author

cramertj commented Aug 26, 2016

Also, I was going to give ! as a default type parameter for the function versions of never, finished, and failed, but I got this error: "defaults for type parameters are only allowed in struct, enum, type, or trait definitions" which led me to this PR. Do you know why default parameters for functions have been removed? It seems clear from the initial RFC that they were intended to be included.

@alexcrichton
Copy link
Member

Nah I unfortunately don't know much about default type paramters :(

@kamalmarhubi
Copy link
Contributor

@cramertj the discussion on this issue looks relevant: rust-lang/rust#27336

@cramertj
Copy link
Member Author

@kamalmarhubi Thanks! So, after all that, it works just fine like this as long as I include the default_type_parameter_fallback feature flag. However, there seems to be quite a lot of disagreement about whether or not this will stabilize. I suppose I could go ahead and fix this up to work with that feature flag, but it's kind of a bummer to depend on a feature that may not ever stabilize.

@alexcrichton
Copy link
Member

Unfortunately the I've heard musings that it's pretty unlikely for the default_type_parameter_fallback feature to ever become being stable, but despite that having something like:

struct Empty<T=!, E=!> { ... }

seems pretty neat! I actually really like how that easily works with invocations today and would just make some other invocations more ergonomic.

I wonder, could this PR basically do that? If you enable the never feature on the crate it just adds a bunch of default type parameters to the existing structs?

We could then perhaps pursue in parallel combinators on Future itself to deal with them.

@cramertj
Copy link
Member Author

@alexcrichton Yeah, I'd love to have this implemented as just default type parameters on existing structs. I've just been floundering trying to figure out how to do that since default type parameters don't work on functions.

Do you have an idea for how to do it without depending on the default_type_parameter_fallback flag (since that would mean we could never stabilize)?

@alexcrichton
Copy link
Member

We don't get the fallback benefits from default type parameter fallback, but I think we should be able to do it nonetheless? Ah yeah and it looks like functions won't work, just structs.

@cramertj
Copy link
Member Author

@alexcrichton Yes, but it seems like the primary use of Empty etc. is through the empty fn, which won't work.

@nikomatsakis
Copy link
Contributor

I'm afraid I can't quite follow the discussion in these comments -- but I would say that the future of default type parameters is definitely in doubt, and getting more data on how people would like to use them might help to resolve some of that uncertainty.

@alexcrichton
Copy link
Member

@cramertj so to clarify, if you add default type parameters to the Empty struct, it doesn't really seem to help ergonomics?

@cramertj
Copy link
Member Author

cramertj commented Sep 1, 2016

@alexcrichton Correct. It only helps with the case where you're declaring Empty via Empty { _data: PhantomData } directly.

@alexcrichton
Copy link
Member

Ah oh well :(

In that case, are there still vectors we should modify in this PR? Or should we close?

@cramertj
Copy link
Member Author

cramertj commented Sep 1, 2016

We could go ahead and make the default type parameter addition with the understanding that it might never get stabilized. Otherwise, I'm inclined to close. I'm kind of bummed out about it though, since there are several nice things about this approach. It's just not worth having to type never().coerce() or some other similarly awkward thing.

@alexcrichton
Copy link
Member

Ok, I'd be game for that modification!

@cramertj
Copy link
Member Author

cramertj commented Sep 1, 2016

Okay! I'll do that tonight and we can finally put this silly thing to rest. Thanks for your patience.

@cramertj
Copy link
Member Author

cramertj commented Sep 3, 2016

Alas, there are some missing trait impls for ! that are now blocking this. Namely, there should probably be a impl<T> From<!> for T, since ! is trivially convertible to any other type. We also need an impl<T> Add<T> for ! with an output of !.

@cramertj
Copy link
Member Author

cramertj commented Sep 4, 2016

It sounds like From<!> for T (and possibly Add<T> for !) is probably coming at some point, but on a longer timescale than this PR. I'll just get the tests to pass so we can merge this in.

@alexcrichton
Copy link
Member

Thanks! What did you think though about the never feature changing the definition of the existing Empty, Failed, and Finished structs? That I believe should be backwards compatible?

@cramertj
Copy link
Member Author

cramertj commented Sep 7, 2016

Yes, that should be backwards compatible. However, I'd still argue that Empty should become Never-- I think it's a much more descriptive name.

It'd also require switching out the definitions based on whether or not the feature flag is set (i.e. not including the old struct and fn definitions if never is set), but I think that's fine so long as the feature stays additive from the user's perspective.

@alexcrichton
Copy link
Member

Yeah I think that'd be my preferred course of action here. I think it's additive too as well, right? It's just adding defaults for a few type parameters?

@cramertj
Copy link
Member Author

cramertj commented Sep 8, 2016

It's additive in terms of behavior, but the actual implementation will require omitting some blocks of code based on the feature, i.e. #cfg[not(feature = "never")] Never<T, E> {...} and #cfg[feature = "never"] Never<T=!, E=!> {...} etc.

@alexcrichton
Copy link
Member

Ah yeah having duplicate struct definitions is fine

@cramertj
Copy link
Member Author

After all that, it's still not possible to make this a backwards compatible change. Defaulted type parameters must be come after type parameters with no defaults. This means that Failed<T, E> must have its type parameters re-ordered to Failed<E, T=!> in order to provide a default.

I'm going to close this PR, as it seems to me that the additional ergonomics offered by this PR are not worth the additional headache caused by dealing with two unstable features and their own set of ergonomic problems. It might be worth revisiting this after ! has been around for a while and there's been a chance to figure out how ! should interact with other types and traits.

@cramertj cramertj closed this Sep 11, 2016
@alexcrichton
Copy link
Member

Ok, thanks for the investigation regardless @cramertj!

@cramertj
Copy link
Member Author

Thanks for your support @alexcrichton! I appreciate it.

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

Successfully merging this pull request may close these issues.

5 participants