Detect duplicate implementations of assoc. types and constants.

[nham]

Adds two error codes, one for duplicate associated constants and one for types. I'm not certain these should each have their own code, but E0201 is already solely for duplicate associated functions so at least it kinda matches. This will lead to somewhat redundant error explanations, but that's nothing new!

Fixes#23969.

[rust-highfive]

r? @nikomatsakis

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

[quantheory]

Hmm, one thing that seems to be missing here is that it should be an error for any pair of associated items to have the same name (e.g. a constant and a method). I believe I have a fix for this on a branch of mine, but that branch is temporarily stalled (and a long way from master now), so I will have to see if I can actually offer it as an alternative.

[arielb1]

@quantheory

That gives an error in the trait (trait declarations already check that you don't have multiple trait-items with the same name, and attempts to declare impl-items of the wrong kind are of course rejected).

[quantheory]

@arielb1

I don't have a recent rustc on hand right now, but what about inherent impls?

#![feature(associated_consts)]structFoo;implFoo{const bar:bool = true;fnbar(){}}

[arielb1]

That does pose a problem :-). Seems like a job for resolve through.

[quantheory]

@arielb1 I'm not sure about that. If you refer to Foo::bar, it's not clear in resolve whether you are referring to a trait impl or an inherent one, so I think we delay resolution until collection has already run. I think it will end up as an ambiguity error during method resolution right now (since that handles associated consts), and if we had inherent impls for associated types it would have to be dealt with in collect, too.

(All that said, code that follows the usual naming conventions will have uppercase names for constants and lowercase for methods, so this PR probably does cover the most important cases in practice.)

[arielb1]

@nham

Could you make the error for every 2 associated items, and add a test for the inherent impl case? I would change E201 for associated items in general (it was missed when associated types were added).

[nham]

@arielb1 I will take a look and see if I can figure out what to do.

[nikomatsakis]

If the problem is just the error code, it's easy enough to factor out the error reporting into a helper method (and indeed, you could just use one big set to detect duplicates between kinds of items).

[nham]

I've changed it to correctly handle @quantheory 's example (and also added it as a test). Everything is under E0201 now as well.

[nikomatsakis]

@nham sorry for being slow here -- I'm way being on reviewing but aiming to be keeping up from now. However, I think there is a slight issue here. Normally in Rust there are two namespaces: types and values. If I am reading the code right, this is reporting an error for something like:

traitFoo{typeBar;fnBar();}

I don't think this is correct. We should report an error if there are two constants/fns with the same name or two types, but between those two groups aliasing is legal.

[nham]

@nikomatsakis Ahh, interesting. Your example actually fails to compile (seems to be erroring out in resolve, see below), but you're right that if it did compile, my approach wouldn't work.

$ cat 25993.rs trait Foo {
type Bar;
fn Bar();
}

$ rustc --version
rustc 1.1.0 (35ceea399 2015-06-19)

$ rustc 25993.rs 25993.rs:3:5: 3:14 error: duplicate definition of type or module `Bar`
25993.rs:3 fn Bar();
^~~~~~~~~
25993.rs:2:5: 2:14 note: first definition of type or module `Bar` here
25993.rs:2 type Bar;
^~~~~~~~~
error: aborting due to previous error

To address this I'm changing it to use separate HashSets for types and values. Do you see any issues with this approach?

[nikomatsakis]

Seems good. 

Niko

-------- Original message --------

From: Nick Hamann notifications@github.com

Date:07/08/2015 17:32 (GMT-05:00)

To: rust-lang/rust rust@noreply.github.com

Cc: Niko Matsakis niko@alum.mit.edu

Subject: Re: [rust] Detect duplicate implementations of assoc. types and constants. (#25993)

@nikomatsakis Ahh, interesting. Your example actually fails to compile (seems to be erroring out in resolve, see below), but you're right that if it did compile, my approach wouldn't work.

$ cat 25993.rs
trait Foo {
type Bar;
fn Bar();
}
$ rustc --version
rustc 1.1.0 (35ceea3 2015-06-19)
$ rustc 25993.rs
25993.rs:3:5: 3:14 error: duplicate definition of type or module Bar
25993.rs:3 fn Bar();
^~~~~~~~~
25993.rs:2:5: 2:14 note: first definition of type or module Bar here
25993.rs:2 type Bar;
^~~~~~~~~
error: aborting due to previous error
To address this I'm changing it to use separate HashSets for types and values. Do you see any issues with this approach?

—
Reply to this email directly or view it on GitHub.

[nikomatsakis]

@nham sorry, one nit, can you rename the tests to something like associated-item-duplicate-names.rs? Including the issue number, either in the filename or in a comment in the file, is also great, but having JUST the issue number makes it hard to determine what kinds of tests are present in the codebase.

[nham]

@nikomatsakis Done! Let me know if you'd like me to squash these commits.

[nikomatsakis]

@nham looks good, r+. Squashing would be good though, yeah.

[nham]

Squashed.

[nikomatsakis]

@bors r+ thanks!

[bors]

📌 Commit 560bb0a has been approved by nikomatsakis

[bors]

⌛ Testing commit 560bb0a with merge 8690536...

[bors]

⌛ Testing commit 560bb0a with merge 898e18d...

[Manishearth]

(closing just because bors is being stubborn)

[Manishearth]

@bors r+

[bors]

📌 Commit 560bb0a has been approved by Manishearth

[Manishearth]

@bors r-

[Manishearth]

@bors r=nikomatsakis

[bors]

📌 Commit 560bb0a has been approved by nikomatsakis

[bors]

⌛ Testing commit 560bb0a with merge bab630e...

[Manishearth]

@bors force

[Manishearth]

@bors r=nikomatsakis

[bors]

📌 Commit 560bb0a has been approved by nikomatsakis

[bors]

⌛ Testing commit 560bb0a with merge 5930c94...

[bors]

⛄ The build was interrupted to prioritize another pull request.

[bors]

⌛ Testing commit 560bb0a with merge d977c82...

[bors]

⛄ The build was interrupted to prioritize another pull request.

[bors]

⌛ Testing commit 560bb0a with merge 57dcda1...

[bors]

⛄ The build was interrupted to prioritize another pull request.