librustc: Disallow modules and types from having the same name.

[pcwalton]

This will break code that looks like:

struct Foo {
    ...
}

mod Foo {
    ...
}

Change this code to:

struct Foo {
    ...
}

impl Foo {
    ...
}

Or rename the module.

Closes #15205.

[breaking-change]

r? @nick29581

[lilyball]

I'm currently using something similar in rust-lua, which is the workaround for the lack of enum mod Foo. This looks like

pub type LoadError = LoadError::LoadError;
pub mod LoadError {
    pub enum LoadError {
        ErrSyntax,
        ErrMem
    }
}

This is done so the variants are accessed as LoadError::ErrSyntax, etc.

Does this PR break that code too? If so, then I have an extremely strong 👎.

[pcwalton]

Yes, it breaks that. It was a bug in rustc that your code compiled. It was never intended to.

That pattern is broken in some cases anyway, for example if you have an impl LoadError.

[lilyball]

It works today, and the existence of the workaround is the main reason why I personally did not push on the "enum mod" issue (#10090) after it was rejected in the meeting (where the rejection boiled down to "one catastrophic change per quarter").

I have no information on what other code uses it, but I use this pattern very deliberately inside rust-lua because it's the best fit for the lua API. Besides making the enum variant name most closely match the C API, the C API in this case actually uses the same error cases for two different error-returning APIs, where one scenario allows for more errors than the other. This was easy to do in C, but in Rust, I needed to model the two error-erturning APIs with separate enums. In order to preserve the fact that the Lua C API uses the same name in both scenarios, I wanted to use the same variant name for my enums, which means the enums needed to be scoped into their own modules.

Basically, removing this feature makes the rust-lua API objectively worse, with no recourse. I see no benefit in doing that.

[pcwalton]

It's obviously a bug and needs to be fixed. I'm not interested in debating whether an obvious bug is a bug.

[huonw]

@kballard making this change allows enum mod-like features to be added backwards compatibly in future.

[lilyball]

@huonw What part of not doing this prevents enum mod from being added today?

@pcwalton It's a bug by whose definition? To me, it's obviously a feature. In fact, given the pretty obvious interpretation of this as a feature, I think you absolutely need an RFC before breaking this. Even if it was accidental, it works today, and nothing about the defined semantics of the language says it shouldn't.

[huonw]

If enum variants can be explicitly qualified:

enum Foo {
   X
}
mod Foo {
    static X: uint = 1;
}

Foo::X // what is it?

[pcwalton]

Types and modules are in the same namespace. Rust doesn't allow multiple objects in the same namespace (with the one exception of the primitive types, which behave kinda strangely). This is nothing new.

Your feature was rejected for 1.0. I'm sorry that this patch breaks your code that abused a compiler bug to try to hack your feature into the language, but I am not going to file an RFC for this. It is a bug by any reasonable definition.

[lilyball]

For reference, this was very explicitly listed as something one could do today back in October of last year in the enum mod proposal (PR #10090), and at no point during the discussion on that proposal or the meeting notes did anyone suggest it was a bug that this worked.

[huonw]

Could this message change to "duplicate definition of type or module Foo"?

[pcwalton]

Sure.

[pcwalton]

I'm sorry that nobody pointed out that the "solution" exploited a compiler bug, but that's water under the bridge and I am not going to file an RFC.

[lilyball]

@pcwalton Once again you are prioritizing your own idealistic view of the way languages should work over the actual practicalities of using the language. A bug may have been responsible for this working originally, but the ability to do this has been known about for at least the past 8 months, and the ability to do this was cited as a workaround for not implementing a proposed feature, and it's not until this past week has anyone said it's actually just a bug.

I say again, this has been known functionality for eight months. You cannot reasonably claim "it's just a bug" and disallow it now.

[pcwalton]

My "idealistic view of the way languages should work" is that they should not have bugs. The fact that a bug existed for 8 months doesn't make it less of a bug.

[lilyball]

But it hasn't been considered a bug for 8 months. It's been considered a bug for one week. That's 8 months of being a feature. Realizing that it works because of a compiler bug a week ago does not negate the past 8 months of it being considered valid and proper Rust code.

[zwarich]

This text from the Rust manual is from late 2012:

"Modules and types share the same namespace. Declaring a named type that has the same name as a module in scope is forbidden: that is, a type definition, trait, struct, enumeration, or type parameter can't shadow the name of a module in scope, or vice versa."

[pcwalton]

Borrow check bugs are also very useful bugs (as circumventing type safety is very useful!) and have existed for a long time, but I'm not going to file RFCs for removing them.

[lilyball]

@zwarich The Rust manual has never been an accurate reflection of the semantics of the language, I do not consider citing it to be at all persuasive.

[lilyball]

@pcwalton Borrow check bugs were never considered features though. Any time a borrow check bug has surfaced in the past, the response has been "that's a bug, don't rely on that, it's unsafe and will stop working once it's fixed".

And if someone had said 8 months ago "your workaround is a bug, it's not supposed to work", then yeah, that would be that. But nobody said that. It was a very obvious part of a language proposal, that was discussed on github, and was talked about in a meeting, and not once did anyone say it was a bug. And code was subsequently written that depended on it working, with no notion of it being a bug.

[pcwalton]

Any time a borrow check bug has surfaced in the past, the response has been "that's a bug, don't rely on that, it's unsafe and will stop working once it's fixed".

And that is precisely what I am saying regarding this bug.

And if someone had said 8 months ago "your workaround is a bug, it's not supposed to work", then yeah, that would be that. But nobody said that. It was a very obvious part of a language proposal, that was discussed on github, and was talked about in a meeting, and not once did anyone say it was a bug.

I wasn't paying attention to the PR. If I had, I would have mentioned that it was a bug. The fact that I didn't chime in 8 months ago does not remove my right to fix a bug. The manual clearly stated that this was a bug.

When I wrote resolve, I never intended for multiple objects to exist in the same namespace. It's an incoherent notion.

And code was subsequently written that depended on it working, with no notion of it being a bug.

I'm sorry you abused a bug to try to hack around a missing rejected feature, but we're before 1.0 and we have the right to fix it.

[lilyball]

And that is precisely what I am saying regarding this bug.

You're saying it 8 months too late. And there's no demonstrable safety hazard requiring the bug to be fixed, or making my code unsafe for using it, and therefore no obvious reason why it must be fixed.

I wasn't paying attention to the PR. If I had, I would have mentioned that it was a bug.

You were present at the meeting that discussed (and rejected) the PR. You apparently didn't speak up on that particular issue, but you did talk on the other issues, so you must have been there.

I'm sorry you abused a bug to try to hack around a missing rejected feature, but we're before 1.0 and that's why we're fixing it.

Fix bugs all you want, but you're removing legitimately useful functionality, and providing no replacement.

[lilyball]

I want to see what @nikomatsakis has to say on this. He spoke up in the enum mod PR. And @SimonSapin very explicitly pointed out to him about the type alias and the module sharing the same name.

[pcwalton]

You're saying it 8 months too late.

There's no statute of limitations on bugs. That is ridiculous.

And there's no demonstrable safety hazard requiring the bug to be fixed, or making my code unsafe for using it, and therefore no obvious reason why it must be fixed.

Yes, there is. It means we can't add enum mod later without breaking code, as @huonw pointed out. And it's bizarre that you can define a module with the same name as a type if and only if the type doesn't have an impl. That prevents us from, for example, making static methods work on typedefs in the future.

You were present at the meeting that discussed (and rejected) the PR. You apparently didn't speak up on that particular issue, but you did talk on the other issues, so you were present.

I wasn't paying close enough attention at that meeting. That doesn't negate my right to fix a bug.

[huonw]

There's no "8 months too late": Rust has no guarantees of stability yet.

And there's no demonstrable safety hazard requiring the bug to be fixed, or making my code unsafe for using it, and therefore no obvious reason why it must be fixed.

Thinking about it now: it is needed for backwards compatibility with associated items, as well as the example I gave above.

[pcwalton]

I should mention Niko's comment in that bug:

So I was thinking "can't this just be a macro?" and the answer is "not quite" -- in particular, the type of the enum would have to be Speed::t or something like that, since Speed is taken as the name of the module and modules/types inhabit the same namespace.

That was the thinking from the beginning. Simon mentioned that the compiler wasn't enforcing this, but I guess that comment got lost as there was no response. Regardless, Niko's understanding matches mine.

I think the workaround is going to be something like LoadError::LoadError or LoadError::T.

[zwarich]

@kballard I'm curious about how you think types and modules in the same namespace should work. There is only a single use construct, not a use type or use mod, etc. so there is currently no way to distinguish between the two in a use item. For example, this program compiles:

use A::B;

mod A {
    pub mod B {
        pub struct C { pub n: int }
    }
    pub struct B { pub n: int }
}

fn main() {
    let _ = B { n: 1 };
    let _ = B::C { n: 1 };
}

If you use the name of a type or module with a function, e.g.

use A::B;

mod A {
    pub struct B { n: int }
    fn B() { }
}

fn main() {
    let _ = B();
}

then you get an error:

test.rs:1:5: 1:10 error: type `B` is inaccessible
test.rs:1 use A::B;
              ^~~~~

Strangely, the error goes away if I delete the actual use of B() in main, which seems broken. The error makes more sense if the struct is a tuple struct:

test.rs:5:5: 5:15 error: duplicate definition of value `B`
test.rs:5     fn B() { }
              ^~~~~~~~~~
test.rs:4:5: 4:23 note: first definition of value `B` here
test.rs:4     pub struct B(int);
              ^~~~~~~~~~~~~~~~~~

[huonw]

This guard could be child.get_module_if_available().map(|m| m.kind.get()) == Some(ImplModuleKind).

[SimonSapin]

I’ve read through all comments so far, and now I’m more confused than before as to what kinds of items "live in the same namespace" and therefore can not share a name. For example, serialize::json::List refers to both an enum variant and a type. Is that a bug too?

pub enum Json {
    Number(f64),
    String(String),
    Boolean(bool),
    List(List),
    Object(Object),
    Null,
}

pub type List = Vec<Json>;
pub type Object = TreeMap<String, Json>;

Modules vs. types are the only case mentioned in the manual. What about every other combination?

Other random test cases:

Errors:

fn foo() {}
struct foo;

Compiles:

fn foo() {}
enum foo {}

@pcwalton, what’s obviously a bug in your mental model is not at all obvious to me when I don’t know what the rules are supposed to be. Can i haz docs please?

[huonw]

Tuple structs and tuple enum variants (including those with no arguments) are in the value namespace: they are either "statics" (for things without arguments, like struct foo;) or functions (for List).

Non-tuple structs and enum names are in the type namespace, not the value one.

[SimonSapin]

Thanks @huonw, but this is only one additional data point where I’m missing the entire picture. Is there some documentation that explains everything that is in "the value namespace", which other namespaces exist, and everything that’s in those? If not, could someone please write it?

[pcwalton]

Is there some documentation that explains everything that is in "the value namespace", which other namespaces exist, and everything that’s in those? If not, could someone please write it?

There's no such documentation, but it would be good to have as part of the spec.

[lilyball]

After giving this some more thought, I'm likely the only person who really cares about the ability to use this functionality, so I withdraw my objection. I still believe this is being handled wrong, and that 8 months of having such functionality be accepted as something one can legitimately do in Rust (especially publicized as an alternative to a language proposal) warrants more than just declaring "it's a bug" and submitting a fix without notice. But if I really am the only person who genuinely cares, then I guess I can deal with it.

FWIW, the mental model that says "types and modules live in the same namespace" only makes sense if you consider every type to have an associated impl TypeName {} block implied. Because apparently it's the impl that produces a module for the given type, which in fact already means that we cannot write ambiguous code today with the bug in effect (because the only way to write ambiguous code is to have an impl block on the type).

In fact, in light of that observation, I'm not even convinced this really is a bug anymore. At least, not in the compiler. I'm wondering if this is instead merely a bug in your mental model that says "types and modules share a namespace', because apparently the behavior of the language says that types and modules don't share a namespace, but that types gain an associated module when you write an impl block on the type. With that model, there's no need to change the compiler at all. But again, if I am really the only person who cares, then I won't try to stop you.

[pcwalton]

FWIW, the mental model that says "types and modules live in the same namespace" only makes sense if you consider every type to have an associated impl TypeName {} block implied.

Yes, I consider that to be the case. A lot of people want to be able to write type Foo = Bar; Foo::new(), which goes along with that.

[lilyball]

A lot of people want to be able to write type Foo = Bar; Foo::new(), which goes along with that.

This is the first truly convincing argument I've seen in favor of making this change. In fact, I actually hit this precise error while working on my local_data rewrite and didn't even realize at the time what the implications were. After landing this PR, are you planning on fixing it so that works?

[pcwalton]

It's not a trivial change (involves moving some stuff that was in resolve to typeck), and backwards incompatible language changes take priority, but I would be happy to mentor and/or review a patch that implemented it.

[pcwalton]

r=nick29581

[bors]

saw approval from nick29581
at pcwalton@3c9443b