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

Allow enum variants to be used through a type alias of the enum #31179

Closed
wants to merge 3 commits into from

Conversation

@jseyfried
Copy link
Contributor

@jseyfried jseyfried commented Jan 25, 2016

This allows enum variants to be used as constructors and patterns though a type alias of the enum (including the alias Self when in an impl for an enum). For example,

enum Foo {
    Bar(i32),
    Baz { i: i32 },
}

type Alias = Foo;

fn main() {
    let t = Alias::Bar(0);
    let t = Alias::Baz { i: 0 };

    match t {
        Alias::Bar(_i) => {}
        Alias::Baz { i: _i } => {}
    }
}

Fixes #28556 and fixes #31168.

@rust-highfive
Copy link
Collaborator

@rust-highfive rust-highfive commented Jan 25, 2016

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@jseyfried jseyfried force-pushed the jseyfried:allow_alias_variant branch from b81d753 to b10df54 Jan 25, 2016
@sfackler
Copy link
Member

@sfackler sfackler commented Jan 25, 2016

I think this may fix #31168 as well - if it does, could you add a test for the Self case so we can close that issue?

@petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jan 25, 2016

Could you add a test with variant's name "conflicting" with some associated item's name?

enum E { V } // <- this should be selected for `E::V` and `Alias::V`
type Alias = E;
impl E { const V: u8 = 0; } // <- this should be selected for `<E>::V` and `<Alias>::V`
@jseyfried jseyfried force-pushed the jseyfried:allow_alias_variant branch from b10df54 to 6729479 Jan 25, 2016
@jseyfried
Copy link
Contributor Author

@jseyfried jseyfried commented Jan 25, 2016

@sfackler yeah it does fix that issue, I just added a test for the Self case.

@jseyfried
Copy link
Contributor Author

@jseyfried jseyfried commented Jan 25, 2016

@petrochenkov done.
Thanks for pointing that out, I hadn't considered that case and would have let the variant shadow the associated constant.

@pnkfelix pnkfelix closed this Jan 25, 2016
@pnkfelix pnkfelix reopened this Jan 25, 2016
@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jan 25, 2016

Cc @rust-lang/lang does a change like this need an RFC first? Seems borderline at least. ..

@pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Jan 25, 2016

(Or it may be an obvious language bug fix. But I want other eyeballs on it either way to help decide. )

@petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jan 25, 2016

Some more questions:

  1. How does it work with type aliases with type parameters?
type Alias<T> = Option<T>;

Option::<u8>::None // Currently prohibited
Option::None::<u8> // Ok
Alias::<u8>::None // ?
Alias::None::<u8> // ?
Alias::<u8>::None::<u8> // ??
Alias::<u8>::None::<u16> // ???
  1. How does it work with associated type aliases?
Type::AssociatedType::Variant // ?
@jseyfried
Copy link
Contributor Author

@jseyfried jseyfried commented Jan 26, 2016

@petrochenkov Good questions. I had been incorrectly assuming that variants behaved more like methods and associated constants.

Right now the type parameters for the alias get ignored and the type parameters for the variant are the only ones that matter, which is a fatal flaw in this PR as it stands.

I think it would be best to allow Option::<u8>::None and Alias::<u8>::None but not Alias::None::<u8>, but this would probably require an RFC and would take more work to implement.

Another possibility would be to only allow access to variants through a type alias if the enum has no type parameters, which would be future-proof with respect to the previous idea and would fix the issues referenced in this PR, but this feels a little hacky.

@aturon
Copy link
Member

@aturon aturon commented Jan 30, 2016

The lang team discussed this PR, and felt that, given some of the thornier questions here, we want to see a full design presented as an RFC before accepting these changes. Thanks @jseyfried for the PR, and feel free to reach out to me or others on the lang team if you'd like help putting together an RFC!

@aturon aturon closed this Jan 30, 2016
@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jul 28, 2016

I ran into a related case of this bug: a type alias of a tuple struct doesn't work as a constructor either. Minimal test case:

struct Generic<T>(T);
type Specific = Generic<usize>;

fn main() {
    let x = Generic::<usize>(42);
    let y = Specific(42);
}

This produces:

error: unresolved name `Specific` [--explain E0425]
 --> <anon>:6:13
6 |>     let y = Specific(42);
  |>             ^^^^^^^^

I had hoped to use this when converting an existing tuple struct to an alias for a generic tuple struct, with the type parameter specified. That would provide source compatibility with code written for the old version of the crate.

This behavior seems inconsistent with other uses of type aliases. A type alias for a non-tuple struct works for type construction: SomeAlias { field: value } works.

(I ended up working around this by declaring a function with the same name as the tuple struct to act as a constructor.)

Has anyone written the associated RFC for this? If not, I'd be happy to write it or to collaborate on it.

@nrc
Copy link
Member

@nrc nrc commented Jul 28, 2016

@joshtriplett this seems like expected behaviour to me. A tuple struct is two things - a type and a ctor function. The type alias makes an alias of the type but not the function. A struct is just a type, there is no ctor function, the struct literal syntax expects a type in it, which is why the type alias is fine (though I wonder about the behaviour when the type alias fixes generic parameters).

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Jul 28, 2016

@nrc It's not expected if you expect a type alias to work anywhere the original type name would, which seems like a reasonable expectation.

@aturon
Copy link
Member

@aturon aturon commented Aug 27, 2016

@joshtriplett I agree. But still, the next step here is going to be to open an RFC laying out a coherent vision of how aliasing should work in these respects.

@joshtriplett
Copy link
Member

@joshtriplett joshtriplett commented Aug 28, 2016

@jseyfried Do you plan to submit an RFC for this? Would you like any help doing so?

@jseyfried
Copy link
Contributor Author

@jseyfried jseyfried commented Aug 28, 2016

@joshtriplett I don't have plans to write up an RFC, but I think it'd be a good idea -- I'd be happy to help.

I agree with @nrc's comment that your example isn't a bug.

In Specific(42), Specific is resolved in the value namespace, just like f in f(42).
Since type Specific = ...;" only defines Specific in the type namespace, Specific(42) cannot resolve to it.

For example,

fn Specific() {}
type Specific = Generic<usize>;
fn f() {
    Specific(42) // Should `Specific` resolve to the function or the constructor?
}

The original intent of this PR was to allow this, i.e. to allow an enum variant to be used via a type aliases of the enum as if it were an associated const (Alias::Bar) or even an associated type (Alias::Baz).

However, as @petrochenkov pointed out here, we can't use enum variants as if they were associated consts even without aliases. For example, consider enum Foo<T>(Bar(T)); -- Foo<usize>::Bar(10); does not compile and we must use Foo::Bar<usize> instead.

@jseyfried jseyfried deleted the jseyfried:allow_alias_variant branch Sep 2, 2016
@djc
Copy link
Contributor

@djc djc commented Jan 3, 2018

I wrote up a pre-RFC for this over the holidays, which seemingly converges after some comments.

https://internals.rust-lang.org/t/pre-rfc-enum-variants-through-type-aliases/6433

Please have a look!

@alexreg
Copy link
Contributor

@alexreg alexreg commented May 9, 2018

@jseyfried / anyone else – plans to re-open this now that the RFC has landed? There's been a little discussion on the tracking issue lately.

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

Successfully merging this pull request may close these issues.

10 participants