Do not allow a module and tuple struct of the same name to coexist. #26421
Conversation
|
r? @pcwalton (rust_highfive has picked a reviewer for you, use r? to override) |
|
I've started this on crater. |
| struct Foo; | ||
| //~^ ERROR duplicate definition of type or module `Foo` |
There was a problem hiding this comment.
given that [RFC 218]("empty struct with braces") was accepted, it would probably be a good idea to have both this test and a separate one of non-empty tuple structs, since I expect the code paths for how the two are handled to diverge somewhat in the future.
|
@alexcrichton asked:
I'm pretty sure this was an oversight. Every kind of struct definition puts an entry into the type namespace (while only some put an entry into the value namespace, but this is not relevant). The collision at the type namespace level is what is problematic. In other words, this errors today: mod Foo {}
struct Foo { x: i32 }
fn main() {}so there's no reason for these to be succeeding: mod Foo {}
struct Foo(i32);
fn main() {}and mod Foo {}
struct Foo;
fn main() {} |
|
Here is a more concrete example of a problematic program this bug is exposing: #[allow(non_snake_case)]
mod Foo { pub fn foo() -> i32 { 4 } }
struct Foo(i32);
impl Foo { fn foo() -> i32 { 3 } }
fn main() { println!("foo: {}", Foo::foo()); } // what does this print? |
|
Having said that, its not a soundness issue to my knowledge. Fixing it would not break code that is following "usual" style conventions about struct and module names, but otherwise it is indeed a breaking change. Does it qualify as a course correction, or just a bug fix? We could certainly employ the |
|
This was not intentional. I think it is definitely a bug fix, not a course correction or 'proper' breaking change. |
alexcrichton
added
I-needs-decision
|
Tagging with T-lang and I-nominated, just getting some broader visibility (and will be dealt with during triage) |
|
Similarly to #27026 (which was specifically mentioned during the last lang team meeting, where the response was "warning cycle first, then release the breaking change"), I am again wondering whether this needs to go through a warning cycle or not. I interpret @nrc above as saying "no, it does not need a warning cycle." But if that is the case, then why apply a different policy to #27026 ? Are these two cases all that different? cc @rust-lang/lang |
|
I think it could (and probably should) get a warning cycle. I don't think it should get a legacy flag or anything more permanent than a warning cycle. |
|
So under this policy, does the change in #25993 need to be reverted and have a warning cycle added? The following program used to compile pre-patch, but does not after: trait Foo {
type Ty;
}
impl Foo for () {
type Ty = ();
type Ty = usize;
}
fn main() {
let _: <() as Foo>::Ty = ();
} |
|
Triage: removing I-nominated as we've taken a look. Seems like a warning cycle makes sense, given how widely used tuple structs and modules are. With respect to #25993, I think we can let that go --- istm that overlap there is less likely, and of course we'll see if crater picks up anything. |
|
ping? @nham would you be ok implementing a warning for this change? |
|
☔ The latest upstream changes (presumably #28816) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Re assigning to myself, I will try to revise with warning cycle support if we see no response from @nham |
|
Sorry, I've been busy the past couple of weeks. I have some time to look at it this week, but I still need to figure out how the warning cycles work, so if you want to take it @pnkfelix that's okay with me. |
|
@nham oh I have plenty of other stuff on my plate, so you've got time to look at it this week if you like. :) |
Currently it is possible to do the following: - define a module named `Foo` and then a unit or tuple struct also named `Foo` - define any struct named `Foo` and then a module named `Foo` This commit introduces a warning for both of these cases.
|
I've made an attempt at adding warnings. The warnings address both the original issue and another issue mentioned by @james-darkfox in #29185, which is that programs like this currently compile: struct Foo { x: i32 }
mod Foo { }
fn main() {}This is slightly different from the original issue because it is allowed only when the module comes after the struct, and also because it works for all structs, not just unit/tuple structs. I wasn't sure what to put for the warning message. Looking for feedback here! |
| @@ -404,6 +404,29 @@ impl<'a, 'b:'a, 'tcx:'b> GraphBuilder<'a, 'b, 'tcx> { | |||
| } | |||
|
|
|||
| ItemMod(..) => { | |||
| let child = parent.children.borrow().get(&name).cloned(); | |||
There was a problem hiding this comment.
Seems a shame to clone the child; I would have thought there was a way to structure this to avoid that.
|
@nham this looks fine to me, apart from the clone, which I assume you tried to avoid and I must just be missing some reason why you found it necessary |
|
@bors r+ |
|
📌 Commit f0af1eb has been approved by |
Fixes #21546.