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

Disambiguate enum variant names. #94

Closed
wants to merge 1 commit into from
Closed

Disambiguate enum variant names. #94

wants to merge 1 commit into from

Conversation

Kile-Asmussen
Copy link

An RFC to resolve enums with overlapping variant names in an elegant manner.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

Idea originated in this reddit thread: http://www.reddit.com/r/rust/comments/269zu4/add_a_new_language_design_faq/chp5k50

+1, unless someone points out a glaring flaw in name resolution or the like. It's very much surprising that something like Option::Some(1) is impossible today. This change would be effectively backwards-compatible (assuming that people consistently capitalize enum names, but do not capitalize module names), even if it is not strictly backwards-compatible. This means that it should be trivial to update existing code after this change, but the decision needs to be made before 1.0.

This RFC would also appease everyone who was previously requesting enum mod, as discussed and rejected back in December (and without requiring new syntax, which is the reason that proposal was rejected). See rust-lang/rust#10090 for the original rationales.

cc @cmr

@sfackler
Copy link
Member

Another (more radical) alternative would be to have variants only accessible under the type's namespace.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

We have discussed and rejected that idea in the past. This RFC acknowledges that both behaviors are desirable, and enables them without requiring any special syntax. Basically the only thing that this RFC lacks is the ability to enforce that a variant is always qualified with the enum's name, but I don't see this as an issue.

@lilyball
Copy link
Contributor

So if I understand this correctly, it allows the path self::Y to be ambiguous, referring to two values, and requiring the use of self::A::Y or self::B::Y to disambiguate? Do we have any precedent for having ambiguous values like this? I can't think of any. The closest I can think of is having multiple traits in-scope, but those aren't values, that's method resolution.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

@kballard, while not an ambiguity in the syntactical sense, please note that this ambiguity exists today. Consider:

enum IntList {
    Node(int, Box<IntList>),
    None
}

fn main() {
    let x = Some(box None);
}

Quick, off the top of your head: does this code compile? If so, what is the type of x?

Answer: it does compile, and the type of x is Option<Box<IntList>>. Heaven forbid if you want to do match x { None => ... }, because AFAICT there's no way to alias Option::None.

Note that this isn't a contrived example, as I have personally observed people asking for help after triggering this exact scenario.

Under this RFC, the above code would become a compiler error, and force you to disambiguate your variant usage.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

I should mention here that I'm not trying to derail us with a discussion of the merits of name shadowing, which doesn't exist only for enum variants. Take the following code, which compiles today:

use foo::Qux;
use bar::Qux;

mod foo {
    pub struct Qux(pub int);
}

mod bar {
    pub struct Qux(pub uint);
}

fn main() {
    let y = Qux(2);
}

The type of y is bar::Qux, because its use statement appeared last in the import list. That this behavior should be extended to enum variants, as it is today, is entirely consistent. The actual problem here lies with enum variant constructor injection. However, given that we do not want to require enum variants to be universally qualified (a decision that I agree with), nor to require all users to import any variants that they want to use unqualified (ditto), this RFC suffices to allow us to regain some control.

@lilyball
Copy link
Contributor

@bstrie That can be disambiguated today by using ::std::option::None. I didn't get the sense that the RFC would actually change this particular example, as it's a matter of name resolution. The RFC seemed to basically just say that enum variants are also accessible via the enum name, and that two enum variants with the same name can coexist at the same path without colliding.

The latter bit actually does concern me, though. Can enum variants collide with other things? Surely not, because the other value at that path would have no way of disambiguating itself. So this makes enum variants rather special in that they can collide with each other, but nothing else can collide. It deviates from the current strict rule that in the value namespace, a given path can only refer to a single value.

In any case, with the None example you gave, std::option::None and self::None don't actually collide. The only issue is that name lookup will find self::None before it finds std::option::None. In fact this example seems completely unaffected by this RFC.


The issue with variant collision seems a very strong reason to go for the enum mod approach as suggested in rust-lang/rust#10090. That provides an explicit mechanism for avoiding variant collisions, without changing the behavior of existing enums.

@lilyball
Copy link
Contributor

To clarify; with this RFC, if I want a List enum with a variant None, I cannot avoid shadowing Option::None. And referencing None in the same module would unambiguously resolve to List::None rather than being an ambiguity error.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

I'm not sure I understand your concerns regarding collision. To me, this RFC can be boiled down to the following two points:

  1. Allow enum variants to qualified with their type, as in Option::None.
  2. When using an enum variant, if its name collides (shadows) with that of any other enum variant in scope, require the variant to use its qualified form.

Indeed, this will be a special case to our current name lookup rules. But it's a special case that makes the rules stricter, which is why I don't see the problem. Even if we just adopted the first point without the second, I think it would be a step in the right direction.

@lilyball
Copy link
Contributor

Shadowing and colliding are different things. This RFC seems to assume that they're the same thing, but they're not. A fully-qualified path ::foo::bar::Baz is always unambiguous and can only refer to a single value. But the scenario described in this RFC:

enum A {
    X,
    Y
}

enum B {
    X,
    Y
}

has two X values that collide rather than shadowing, and ::path::to::X now refers to two values.

The RFC seems to just assume that this is ok and that ::path::to::X will be considered ambiguous, rather than recognizing that it's a full collision in the value namespace rather than merely shadowing. Making shadowing ambiguous seems like a reasonable thing to propose, but allowing full collision and merely considering it ambiguous is a more drastic change, and needs to be explicitly recognized by this RFC as such.

And without allowing collision, I don't think this RFC is actually doing anything, because as I said the fully-qualified path is always unambiguous. The only thing that I can think of that it would really affect is for loop desugaring, as it can be translated into Option::None/Option::Some and only require that Option is in scope, rather than relying that None/Some are in scope (it could use the fully-qualified path ::std::option::None but that will break #![no_std] clients that only pull in libcore). But this RFC seems like an overly-broad solution to the narrow case of for loop desugaring, which would be better-fixed by turning it into a real language construct (instead of syntactic sugar) and using lang items to identify None/Some.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

I was under the impression that there would be no implicit constructor injection in your given example. As in, there would be no ::path::to::X, there would be only ::path::to::A::X and ::path::to::B::X. Type-unqualified variant names would be considered merely sugar for the type-qualified variant names, but would only be allowed in cases where they do not shadow or collide with another variant name.

Regardless, this doesn't have anything to do with the real thrust of this RFC, which is contained in the two points of my previous comment. I'm not sure what this has to do with for loops at all.

@lilyball
Copy link
Contributor

@bstrie Really? Your impression is apparently what @sfackler said was an alternative approach, but is not in fact what this RFC is proposing. It's treating EnumName::Variant as sugar to disambiguate ambiguous enum variant names within the same module. But the only nod to allowing multiple enum variants with the same name in the same module comes from the summary line, and is not expanded upon anywhere in the detailed design.

@bstrie
Copy link
Contributor

bstrie commented May 28, 2014

I admit that the "Detailed design" section is woefully underdetailed and I confess to taking the RFC's sentiment to its furthest conclusion. That said, the example given is "a simple case of enumeration disambiguation", and clearly not the only intended case. I would be happy to open a more detailed RFC if the author of this one is unresponsive.

@lilyball
Copy link
Contributor

Here's a review of how I'm reading this RFC, and what the problems are with it. This is basically a summary of what I've said in response to @bstrie but hopefully putting it out on its own and not as a response will make it clearer:


This RFC appears to be asking for enum variants to become a special case to the value namespace rules, in that two enum variants are explicitly allowed to collide. This isn't expanded upon anywhere in the detailed description, but it's the only way to interpret the summary, and it's the only behavior that makes the rest of the RFC make sense.

This alone gives me cause for concern. Right now we have a strict rule where two values cannot collide (just as two types cannot collide). Values can shadow each other, as long as they live at different paths, and the shadowing is due to how name resolution works. But shadowing and collision are different, and this RFC is strictly concerned with collision.

Assuming the collision is allowed, this then proposes that the variants can be disambiguated by using the enum name as if it were a module, e.g. Option::None. This rule makes sense as a disambiguation, and it would be treated purely as sugar for picking the correct variant from the parent module of the enum. Although this also is a bit concerning, as we have no precedent today for "associated values", which Option::None appears to essentially be. Although in reality it's more like a pub use. It seems a bit more sensible to flip it around and make Option::None the "real" value and None just a pub use.

Of course, even treating this like a pub use is a bit worrisome, because we currently have a hard rule that use statements need to precede items, whereas this would introduce the idea of the value alias (i.e. pub use) that is introduced at the point of the enum definition. Presumably it would act as though there was a pub use EnumName::* at the top of the module, though.

In any case, we may end up with associated values at some point, but we don't have them today. A non-nullary enum variant is actually a function, so we could conceivably treat those as self-less functions in an impl of the enum, but that wouldn't work for nullary variants. It also breaks the pub use behavior, because you can't use a selfless function on a type. Similarly it breaks current behavior around generics, because Some::<int>(x) would have to be expressed as Option::<int>::Some(x) instead.

Overall, this RFC's goal of allowing multiple enums in the same module to have variants with the same name is a reasonable thing to want, but I don't think the proposed behavior works.

@lilyball
Copy link
Contributor

At this point I'm inclined to reintroduce the enum mod proposal as an RFC. It was closed with the suggestion to "revisit later" partly because nobody on the core team was a fan of the idea, and partly because, as @brson said:

Too late for this change. Quota is "one catastrophic change per quarter."

But I think now is a good time to revisit the idea using the more structured RFC process. I know kibwen commented that this isn't a good time because of the number of breaking changes floating around, but we have open RFCs that are over 2 months old, so obviously there's no pressure to accept/reject an RFC immediately.

@matthieu-m
Copy link

The disambiguation is indeed a concern, however I do wish that explicit qualification of enum variants would work at least:

fn foo() -> Option<int> {
    Option::None
}

It is annoying that this fails to compile, and for now the only work-around I have found (since aliases don't work either) is just to import all variants in the scope which is really unsatisfactory.

@pnkfelix
Copy link
Member

We believe if the compiler adhered the rules that we expect of it today, then this could be added backwards compatibly post 1.0. In particular, since today, types and module share the same namespace, it should not be legal (*) for one to actually write code that has an enum item and a mod item in the same scope, and therefore we should be able to add the new semantics proposed here backwards compatibly in the future.

So, following the above reasoning, we note that this is a potentially desirable feature, but not one that we want to invest time in before 1.0. So we are closing this PR as postponed, so that we can revisit it after 1.0 is released.


Footnote

(*) However: The team determined during review that the above reasoning is slightly flawed, in that there is a bug in the compiler that allows one to have an enum item and a mod item in the same scope, as illustrated here: playpen

This is a bug. The people present at the meeting agreed it is a bug, and we have now filed rust-lang/rust#15205 to address it.)

@brson brson closed this Jun 26, 2014
@steveklabnik steveklabnik removed the postponed RFCs that have been postponed and may be revisited at a later time. label Jan 22, 2016
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.

8 participants