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

Define variants in a sub-namespace of their enum #390

Merged
merged 2 commits into from
Oct 31, 2014

Conversation

sfackler
Copy link
Member

@sfackler
Copy link
Member Author

I would still be up for implementing this before 1.0 to avoid any library cruft, but decided to write this up in the form that would be least likely to give @pcwalton a heart attack :)

@netvl
Copy link

netvl commented Oct 13, 2014

+1. I have several parser libraries which expose AST/parsing events as enums, and these enums tend to have a lot of variants. use clauses sometimes are huge.

BTW, not requiring to specify full path to enum variant in pattern matching:

mod other {
    enum Foo {
        Bar, Baz
    }
}

use other::Foo;

let x = Foo::Bar;
match x {
    Bar => {}
    Baz => {}
}

does have at least one precedent of general use, namely in Java. Its enums usually require writing class name as a prefix (if they are not statically imported), but switch statement over a value of enum type can contain enum variants without prefix:

package com.example;

public enum Foo {
    BAR, BAZ
}

// --------

package com.example.main;

import com.example.Foo;

public class Main {
    public static void main(String[] args) {
        Foo f = Foo.BAR;
        switch (f) {
            case BAR: break;
            case BAZ: break;
        }
    }
}

So maybe it can be considered not as a "fallback" for easier code migration but as a proper approach for writing matches.

@liigo
Copy link
Contributor

liigo commented Oct 13, 2014

+1, like this!
2014年10月13日 下午2:33于 "Steven Fackler" notifications@github.com写道:


You can merge this Pull Request by running

git pull https://github.com/sfackler/rfcs enum-namespace

Or view, comment on, or merge it at:

#390
Commit Summary

  • Namespace variants under the enum
  • Clarify a point in the fallback logic
  • Expand the summary
  • Work on motivation
  • More motivation
  • Finish up motivation section
  • Motivation summary
  • Remove hyperbolic statement
  • Tweaks
  • Add a compatibility clause for UFCS method calls

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#390.

@Kimundi
Copy link
Member

Kimundi commented Oct 13, 2014

+1 because it makes enums consistent with other items, solves the namespace collision problems and still allows them to be used as before as a convenience.

Question: Does the fallback also allow leaving off the explicit variant import completely in cases where just the enum is imported and no other colliding name is visible?

Also I don't see the problem in your unresolved question: Variants would no longer live in the modules namespace but in the enums namespace, so after the change it would be legal to define both the structs and the variants, and you'd have to refer to the variants qualified with the enum type name.

@arcto
Copy link

arcto commented Oct 13, 2014

Could inference be a viable alternative (post 1.0)? It seems neat. Of course, it lacks the explicitness and definitiveness of the qualified form, but so does type inference in Rust and that seems to work just fine.

enum Fruit {
    Apple,
    Orange,
}

enum Color {
    Red,
    Orange,
}

fn broken(f: Fruit) {
    match f {
        Apple => {} // fallback or inferred: Fruit::Apple
        Orange => {} // inferred: Fruit::Orange
    }
}

@thestinger
Copy link

@arcto: That seems to defeat the purpose of namespaces. It would stop compiling if another enum with a conflicting variant name was introduced. I think it would be a step back from what we already have... it's too magical.

@CloudiDust
Copy link
Contributor

Ideally I believe we should go the "completely namespaced enums" route. It is more consistent and we may choose to opt-in flat enums with a simple wildcard use. (Enums in prelude can have their variants auto-used so we need not to use Option::* everywhere.)

But yes, this will break a lot of code. Still I think wildcard uses are not too much change, no? :)

EDIT: And as the proposal points out, we can provide a transition period where both flat and namespaced usages are allowed by default (but flat usages come with warnings).

@bill-myers
Copy link

Probably not a good idea given the intention of extending enums to provide class hierarchies.

If you see enums as class hierarchies then the current behavior, as well as making variants types themselves, is correct.

@jfager
Copy link

jfager commented Oct 13, 2014

@thestinger I don't understand why that would stop compiling if another enum with a conflicting variant name was introduced - the compiler knows f is a Fruit, Orange is a variant of Fruit, how would Color even come into play? As @netvl pointed out, that's how Java works, and it's fine. (Trying to infer Fruit from Orange would be a problem, of course)

@CloudiDust
Copy link
Contributor

@bill-myers, I think having namespaced enums doesn't stop us from having variants as types. The only difference is where the "subtypes" are located. (And we can always "flatten" by useing.)

Visually enums introduce scopes, so it is natural for them to introduce new namespaces.

FWIW, when I emulate tagged unions in Java, I would define an abstract class to act as the "enum" (in Rust sense), and define nested subclasses to represent the variants. (As I prefer not "flattening" them into the enclosing package.)

@sfackler
Copy link
Member Author

@netvl Java's an interesting case in that you can only refer to variants in the "bare" form (FOO, not MyEnum.FOO) in switch statements. The situation in Rust is a bit more complex as match allows more powerful pattern matching than traditional switch statements. The closest analogue would probably be to implicitly treat all the relevant stuff as imported in a pattern. That seems like something sufficiently orthogonal to this proposal that it'd probably deserve its own RFC.

use messages::BackendMessage::*;
use messages::FrontendMessage::*;
use messages::{FrontendMessage, BackendMessage, WriteMessage, ReadMessage};
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Have glob imports been accepted for 1.0 yet? Because if not, then there's no benefit to a namespace here, as you'd have to import them all manually anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pcwalton do you know what the story is with the future of #[feature(globs)]?

The last I heard is that pub-use globs are the real troublemakers, but that the problems seem fixable.

Copy link
Contributor

Choose a reason for hiding this comment

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

@nikomatsakis's name resolution prototype includes support for globs. (From this I would assume that they will be supported, but don't take it from me as official.)

@bstrie
Copy link
Contributor

bstrie commented Oct 13, 2014

Presumably the variants for Option and Result will remain in the prelude, correct?

@sfackler
Copy link
Member Author

Yep, this wouldn't effect anything in the prelude.

@bstrie
Copy link
Contributor

bstrie commented Oct 13, 2014

Having thought about it, I do think we need to block this on the discussion of globs, as otherwise the line "It is trivial to emulate flat enums with namespaced enums" is dangerously incorrect.

@blaenk
Copy link
Contributor

blaenk commented Oct 13, 2014

YES please! I think that this would be consistent with the spirit of @aturon's PR #356 and would make for all-around cleaner code by better delineating this logical separation.

@bstrie
Copy link
Contributor

bstrie commented Oct 13, 2014

Despite my previous comments, I would be in favor of this if we can decide the glob issue. The introduction of enum class in C++ seems like a strong (and well-received) argument in favor of variant namespacing.

@mdinger
Copy link
Contributor

mdinger commented Oct 13, 2014

@arcto @netvl: That's rust-lang/rust#16681 which lacks a RFC.

@reem
Copy link

reem commented Oct 13, 2014

Huge +1 from me, especially if globs are added. Even without globs, this is still a win as it allows a pattern that was previously made extremely tricky (namespaced variants) and doesn't strongly affect the ergonomics of using enums un-namespaced.

@Valloric
Copy link

OH GOD YES PLEASE! Rust repeating the ~40-year-old mistake from C of enum items being imported to the scope of the enum itself is so incredibly embarrassing, especially given that C++11 introduced enum class to solve this exact issue and everyone welcomed it with open arms. I can't believe this has been in Rust for so long, in spite of repeated pleading to change it. Why repeat such an obvious mistake?

I for one welcome this RFC.

@pcwalton
Copy link
Contributor

@Valloric Let's not overstate things. I prefer the current behavior, because enums are so often used as class hierarchies. I think @bill-myers is correct.

However I'm not extremely opposed to having the option of namespacing enums.

@pcwalton
Copy link
Contributor

Glob imports are definitely not a 1.0 blocker.

@dobkeratops
Copy link

its not so clear cut, because rust's modules don't work like C++.. rust already namespaces things more than C++ does

@pcwalton
Copy link
Contributor

@dobkeratops That's correct. Idiomatic Rust uses nested namespaces far more than C++ does.

Moreover, I think making None and Some, for example, into Option::None and Option::Some, for example, has no benefit and would be somewhat of a usability regression. (Yes, they're in the prelude, but the same argument applies to any monadic type, including user-defined ones.) C++ isn't a great comparison here because C++ doesn't use enum for monads.

@mahkoh
Copy link
Contributor

mahkoh commented Oct 13, 2014

Everything that removes variants from the type namespace is welcome.

@Valloric
Copy link

@pcwalton You are of course entitled to your own opinion, as I am to mine. I have introduced Rust to several C++ developers and every time when they reach enums they ask me why is Rust making the same mistake as C and start doubting that Rust really is a cleaned-up C++ ("If they can't get freaking enums right, what else have they screwed up?"). Again, this has happened several times.

For issues like common enum names being scoped by default, as far as I'm reading this RFC, those specific enum names can be brought into the top-level scope. So it's still possible to "level-up" enum values, it's just not done by default (as it damn shouldn't be).

@pcwalton
Copy link
Contributor

@Valloric The reason why Rust "doesn't get enums right" is that Rust enums are different and used for different things. You might as well ask "if the Rust developers screwed up enums by allowing you to put data in them, what else did they screw up?"

@pcwalton
Copy link
Contributor

Given that the ADT tradition has it going the other way anyhow, we'd be equally likely to have ML/Haskell/Scala users telling us "how could you mess up ADTs?" if we made all enum variants namespaced.

It seems to me that what's good for C-style enums is bad for monadic-style/datatype-style enums (which C doesn't support), and vice-versa. This leads me to believe we should support both.

@npryce
Copy link

npryce commented Oct 14, 2014

Yuck! Please don't do this. We already have modules for namespacing and type aliases and 'use as' for name clash resolution

@jfager
Copy link

jfager commented Oct 14, 2014

@npryce Having to use a module to put an enum in a namespace is annoying and ugly:

mod foo {
    enum Foo {
        Bar,
        Baz
    }
}

Rightward drift and a stutter, awesome.

@CloudiDust
Copy link
Contributor

@npryce, there are two problems that cannot be solved with other namespacing/name clash resolution constructs:

  1. consistency. Enums introduce new blocks/scopes, and they are not macros, so why shouldn't they introduce new namespaces? AFAIK they are the only exception to the rules now.
  2. expectations. Many people expect "namespacing" to be the correct behaviour of enums. (C++ users can use namespace to emulate namespaced enums before C++11, but this trick doesn't stop people from wanting enum classes.)

@Kimundi
Copy link
Member

Kimundi commented Oct 14, 2014

@pcwalton: I'm a bit confused by your arguments, as they sound as if the proposal would forbid flat enums? (Eg, like older proposals)

The way I understood it, if you define a enum with this change, you can still refer to its variants in the flat scope, unless another item in the module defines a conflicting name, in which case the only remaining option becomes using the type-namespaced name. So its basically both a flat and a nested one at the same time, for convenience.

@Kimundi
Copy link
Member

Kimundi commented Oct 14, 2014

Also, I want to reiterate that I and @nikomatsakis think that globs can be made to work sanely.

@CloudiDust
Copy link
Contributor

@Kimundi, I think if we can have sane glob imports, we should then go with "only namespaced enums by default", which is more consistent and less magical.

Also if later we do miss "default flat enums", that would be a backwards compatible change, but the reverse is not true.

@CloudiDust
Copy link
Contributor

@Kimundi, actually I have a feeling that many in this comment thread were speaking as if "only namespaced enums by default" were proposed.

enum's variants into the enum's sub-namespace in a similar way to methods
defined in inherent `impl`s. Note that there is one key difference between
those methods and the variants: the methods cannot be `use`d, while the
variants can be.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do hope that we can allow inherent methods to be subject to use as well -- but certainly if we went this route, it would have to work for variants to be tolerable. In general I'm happier if we can make variants "just another" kind of associated item.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would certainly make the implementation of this RFC in resolve easier if we don't have to worry about a mix of importable and non-importable things in the same module. Has useable inherent methods already been covered by an existing RFC? I can write one up if not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it was explicitly described by the UFCS RFC. However, I think it's tied into some non-trivial questions regarding name resolution. For example, are imports restricted to inherent items? What about items defined in extension traits? Type aliases? This all gets increasingly complicated because you need to start bringing in the type checker to resolve a lot of it, and it's hard to use the type checker when you haven't even resolved names yet.

@ben0x539
Copy link

I hope we can do this and get it in early enough to not need the backwards compatibility clause.

How big an effort is the implementation gonna be?

@rkjnsn
Copy link
Contributor

rkjnsn commented Oct 15, 2014

+1. The fact that enum variants weren't namespaced has always felt like a huge wart to me. If possible, I'd like to see the flat fallback omitted entirely for 1.0.

@sfackler
Copy link
Member Author

I have a POC implementation of the initial stage of the conversion in a branch: https://github.com/sfackler/rust/commits/enum-namespace. It defines variants in both locations. If we decide to implement this before 1.0, this would be the initial commit to allow everything to work until the new logic makes it into a snapshot, at which point we could start moving code over. If we decide to punt until after 1.0, an adjustment to add the fallback logic will be pretty easy - just check if a conflicting definition already exists when declaring variants in the "flat" location and tack on an AMBIGUOUS modifier to the definition.

@dtantsur
Copy link

+1 to making this at least possible. See e.g. bencode enum https://github.com/arjantop/rust-bencode/blob/master/src/bencode.rs#L216 for example where it could be useful.

@yuriks
Copy link

yuriks commented Oct 29, 2014

+1 Having to prefix enums as a kludgy namespacing mechanism is silly when it could just be done properly. Monadic enums can just be pub used.

However, I think that having variants automatically be in scope inside a match is essential before the backwards compatibility resolution scheme is removed.

@nikomatsakis
Copy link
Contributor

On Fri, Oct 24, 2014 at 09:19:43PM -0700, Steven Fackler wrote:

I have a POC implementation of the initial stage of the conversion
in a branch:
https://github.com/sfackler/rust/commits/enum-namespace.

One thing I would really like to see are tests that use pub use foo::* to emulate the current behavior. That is, I want to see the
migration strategy for existing code once the old behavior no longer
exists.

@aturon aturon merged commit c733e04 into rust-lang:master Oct 31, 2014
@aturon
Copy link
Member

aturon commented Oct 31, 2014

Disucssion. Tracking.

@Centril Centril added A-enum Enum related proposals & ideas A-sum-types Sum types related proposals. A-data-types RFCs about data-types A-resolve Proposals relating to name resolution. A-paths Path related proposals & ideas labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-data-types RFCs about data-types A-enum Enum related proposals & ideas A-paths Path related proposals & ideas A-resolve Proposals relating to name resolution. A-sum-types Sum types related proposals.
Projects
None yet
Development

Successfully merging this pull request may close these issues.