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

Tracking issue for RFC 2338, "Type alias enum variants" #49683

Closed
Centril opened this issue Apr 5, 2018 · 43 comments

Comments

Projects
None yet
@Centril
Copy link
Member

commented Apr 5, 2018

This is a tracking issue for the RFC "Type alias enum variants" (rust-lang/rfcs#2338).

Steps:

Unresolved questions:

  • Decide how to resolve in case of ambiguities between associate types and variants. (Currently low-priority resolution for variants.) See #56225 (comment). Resolved in favour of high-priority resolution with #57501.
@djc

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

I'd like to try implementing this. Since there was a previous implementation attempt in #31179, I can probably start off of that, but mentoring instructions would be quite welcome.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2018

Short instructions:

  • Find a place where associated types are resolved to their definitions (probably fn associated_path_def_to_ty) and add high-priority resolution to variant definitions.
  • Find a place where associated values are resolved to their definitions (probably fn resolve_ufcs) and add high-priority resolution to variant definitions.
  • Try to apply the "argument transfer" scheme (Type::AssocItem<Args> -> Type<Args>::AssocItem) to generic arguments of associated items if they resolve to variants. This can meet complications because this is not something that can (or should) be done naturally, as I previously described.
@earthengine

This comment has been minimized.

Copy link

commented May 8, 2018

I have a specific concern on this. Right now, every concrete type name contains information of its component type names, making it very easy to have the name length being explode. Introducing something that making this issue worse may not be a good idea if we haven't get the type names shorted.

Here is an example of how bad this problem can be.

Internally, a type should contain all information to identify itself. But it is not necessary appear in the name, especially when the name is too long, because type names areintended to be read by human, and no one will read a name with a million chars. On the other hand, computer code will feel more comfortable to manipulating referencing structures. So by limiting the type name size we wouldn't lose anything.

To making things worse, I just figured out that the length of a single type is not the issue I experienced. The problem is that there are too many (and too long) type names being generated, and so exceeded the compiler limit. This is why it is getting worse when I extract methods as it forces the compiler to generate more types.

This RFC, once implemented, will add much more types to the system, so please measure a typical closure-heavy-and-impl-trait-heavy program, to see how bad this problem is. It would also be good to allow the compiler to generate statistics of the types it generates, so we can keep tracking this.

@alexreg

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

@djc Did you manage to make any headway on the implementation of this?

@djc

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

@alexreg no, not yet. Feel free to move forward if you want to take a crack at it!

@alexreg

This comment has been minimized.

Copy link
Contributor

commented May 8, 2018

Too busy right now, I'm afraid. I'd need more detailed mentoring instructions too, as too many of these concepts are foreign to me... best someone else tackles it.

@pravic

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

It was already implemented. Why not to reopen the previous PR?

@alexreg

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@pravic Where’s that PR?

@pravic

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

@alexreg

This comment has been minimized.

Copy link
Contributor

commented May 9, 2018

Thanks.

@tlively

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2018

Is there anyone currently working on this? I would love to see this RFC implemented. If there were mentoring instructions available I would take a stab at it.

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2018

@tlively Do you feel you need something more detailed than
#49683 (comment)? That's fine if so; maybe ping @petrochenkov or get on Discord. :-)

@djc

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@tlively I'm currently not working on this. Between the stuff that @petrochenkov already wrote here and the earlier implementation attempt I linked from the second comment, I think you should probably have a good starting point to dig in?

@tlively

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

Ok, I think I should be able to figure it out. I'll be trying this out over the next few days. Thanks, everyone!

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Sep 12, 2018

@tlively Glad to hear! Let us know how you get on.

@tlively

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

@alexreg So far it's slow-going, but I'm still chipping away at it!

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Sep 13, 2018

@tlively Okay. If you need any help, pop into Discord. Even I may be able to assist you a bit with it.

@leonardo-m

This comment has been minimized.

Copy link

commented Nov 11, 2018

I was about to open an enhancement request to support code like:

#![feature(self_in_typedefs, box_syntax)]
enum PeanoNumber {
    Zero,
    Succ(Box<Self>),
}
impl PeanoNumber {
    fn add_one(n: Self) -> Self {
        Self::Succ(box n)
    }
}
@alexreg

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2018

Any updates on this lately, @tlively? :-)

@tlively

This comment has been minimized.

Copy link
Contributor

commented Nov 11, 2018

No, I wasn’t able to make meaningful progress and I haven’t looked at it in a while. If someone else wants to take a look they should go for it.

@eddyb

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

Should this really mention type aliases at all?
I expect <Option<_>>::None to require the exact same implementation, unless it's been hackily implemented today in some other fashion.
Similarly, Self::None should really desugar to <Self>::None and then get resolved by the exact same machinery that type aliases would need.

@alexreg

This comment has been minimized.

Copy link
Contributor

commented Jan 31, 2019

@jesskfullwood What @eddyb writes is correct... and yes, it's unfortunate. I don't know the details of how the import (use) system works right now, and it may require some overhauling to support things like this, but an RFC on such enhancements would of course be welcome!

@nvzqz

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

So for some reason this nightly feature is required when implementing the following macro (found in nvzqz/static-assertions-rs#11):

macro_rules! assert_variant {
    ($e:ty, $v:ident) => {
        const _: fn($e) -> () = |e| match e {
            <$e>::$v => {},
            _ => {},
        };
    }
}

Does anyone have an idea as to why?

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 23, 2019

@nvzqz
The reason is that the feature is not actually "type alias enum variants", but "type relative enum variants" or "variants as associated items".
When writing <TYPE>::NAME you are asking to search for an associated item named NAME on type TYPE, for variants this kind of requests is feature gated.

The stable solution will look something like:

macro_rules! assert_variant {
    ($e:ident, $v:ident) => {
        const _: fn($e) -> () = |e| match e {
            $e::$v => {},
            _ => {},
        };
    }
}

or

macro_rules! assert_variant {
    ($e:path, $v:ident) => {
        const _: fn($e) -> () = {
            use $e as my_ident; // On 2018 edition, or if `$e` is guaranteed to be a crate-relative path
            |e| match e {
                my_ident::$v => {},
                _ => {},
            }
        };
    }
}

(See also #48067 that would provide a better solution for the second case.)

@nvzqz

This comment has been minimized.

Copy link
Contributor

commented Feb 24, 2019

So the issue with using path or ident in this case is that neither supports generic arguments. I would like it such that $e could be any enum, generic or otherwise.

@alexreg alexreg added the B-unstable label Feb 28, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2019

I thinks we can start the stabilization process once the current implementation (#57501) lands on stable.

#57501 is now in the released stable Rust 1.34

@estebank

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

Once this is stabilized, we can close #52118 if it hasn't been closed earlier by adding a suggestion to use the enum's path instead of Self.

@Centril

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

We discussed this on the language team meeting today and gave the green light for a stabilization report which I (@Centril) will write.

@alexreg

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

I should be able to stabilise this if I'm not too busy with other things at the time. (Or @Centril is welcome to after writing the report.)

@Centril

This comment has been minimized.

Copy link
Member Author

commented May 31, 2019

I will do both things at the same time... :)

@Centril Centril self-assigned this May 31, 2019

bors added a commit that referenced this issue Jun 14, 2019

Auto merge of #61825 - Centril:tauv-infer-fix, r=<try>
type_alias_enum_variants: fix #61801; allow a path pattern to infer

Fix #61801.

Given a type-relative path pattern referring to an enum variant through a type alias, allow inferring the generic argument applied in the expectation set by the scrutinee of a `match` expression.

Similar issues may exist for `let` statements but I don't know how to test for that since `PhantomData<T>` is necessary...)

The gist of the problem here was that `resolve_ty_and_res_ufcs` was called twice which is apparently no good... It is possible that this PR is papering over some deeper problem, but that is beyond my knowledge of the compiler.

r? @petrochenkov
cc @eddyb @alexreg
cc #61682
cc #49683

Centril added a commit to Centril/rust that referenced this issue Jun 14, 2019

Rollup merge of rust-lang#61825 - Centril:tauv-infer-fix, r=petrochenkov
type_alias_enum_variants: fix rust-lang#61801; allow a path pattern to infer

Fix rust-lang#61801.

Given a type-relative path pattern referring to an enum variant through a type alias, allow inferring the generic argument applied in the expectation set by the scrutinee of a `match` expression.

Similar issues may exist for `let` statements but I don't know how to test for that since `PhantomData<T>` is necessary...)

The gist of the problem here was that `resolve_ty_and_res_ufcs` was called twice which is apparently no good... It is possible that this PR is papering over some deeper problem, but that is beyond my knowledge of the compiler.

r? @petrochenkov
cc @eddyb @alexreg
cc rust-lang#61682
cc rust-lang#49683

Centril added a commit to Centril/rust that referenced this issue Jun 14, 2019

Rollup merge of rust-lang#61825 - Centril:tauv-infer-fix, r=petrochenkov
type_alias_enum_variants: fix rust-lang#61801; allow a path pattern to infer

Fix rust-lang#61801.

Given a type-relative path pattern referring to an enum variant through a type alias, allow inferring the generic argument applied in the expectation set by the scrutinee of a `match` expression.

Similar issues may exist for `let` statements but I don't know how to test for that since `PhantomData<T>` is necessary...)

The gist of the problem here was that `resolve_ty_and_res_ufcs` was called twice which is apparently no good... It is possible that this PR is papering over some deeper problem, but that is beyond my knowledge of the compiler.

r? @petrochenkov
cc @eddyb @alexreg
cc rust-lang#61682
cc rust-lang#49683

Centril added a commit to Centril/rust that referenced this issue Jun 14, 2019

Rollup merge of rust-lang#61825 - Centril:tauv-infer-fix, r=petrochenkov
type_alias_enum_variants: fix rust-lang#61801; allow a path pattern to infer

Fix rust-lang#61801.

Given a type-relative path pattern referring to an enum variant through a type alias, allow inferring the generic argument applied in the expectation set by the scrutinee of a `match` expression.

Similar issues may exist for `let` statements but I don't know how to test for that since `PhantomData<T>` is necessary...)

The gist of the problem here was that `resolve_ty_and_res_ufcs` was called twice which is apparently no good... It is possible that this PR is papering over some deeper problem, but that is beyond my knowledge of the compiler.

r? @petrochenkov
cc @eddyb @alexreg
cc rust-lang#61682
cc rust-lang#49683

Centril added a commit to Centril/rust that referenced this issue Jun 14, 2019

Rollup merge of rust-lang#61825 - Centril:tauv-infer-fix, r=petrochenkov
type_alias_enum_variants: fix rust-lang#61801; allow a path pattern to infer

Fix rust-lang#61801.

Given a type-relative path pattern referring to an enum variant through a type alias, allow inferring the generic argument applied in the expectation set by the scrutinee of a `match` expression.

Similar issues may exist for `let` statements but I don't know how to test for that since `PhantomData<T>` is necessary...)

The gist of the problem here was that `resolve_ty_and_res_ufcs` was called twice which is apparently no good... It is possible that this PR is papering over some deeper problem, but that is beyond my knowledge of the compiler.

r? @petrochenkov
cc @eddyb @alexreg
cc rust-lang#61682
cc rust-lang#49683

bors added a commit that referenced this issue Jun 15, 2019

Auto merge of #61825 - Centril:tauv-infer-fix, r=petrochenkov
type_alias_enum_variants: fix #61801; allow a path pattern to infer

Fix #61801.

Given a type-relative path pattern referring to an enum variant through a type alias, allow inferring the generic argument applied in the expectation set by the scrutinee of a `match` expression.

Similar issues may exist for `let` statements but I don't know how to test for that since `PhantomData<T>` is necessary...)

The gist of the problem here was that `resolve_ty_and_res_ufcs` was called twice which is apparently no good... It is possible that this PR is papering over some deeper problem, but that is beyond my knowledge of the compiler.

r? @petrochenkov
cc @eddyb @alexreg
cc #61682
cc #49683
@Centril

This comment has been minimized.

Copy link
Member Author

commented Jun 16, 2019

I have proposed stabilization in #61682.

@Centril

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2019

Stabilization has been done.

Documentation issue is rust-lang-nursery/reference#631.

Everything that needs to be done on rust-lang/rust is done so I'm closing this issue as a result.

@Centril Centril closed this Jul 5, 2019

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.