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

Use statements don't check for duplicates #7663

Closed
Blei opened this issue Jul 9, 2013 · 4 comments
Closed

Use statements don't check for duplicates #7663

Blei opened this issue Jul 9, 2013 · 4 comments
Labels
A-resolve Area: Path resolution

Comments

@Blei
Copy link
Contributor

Blei commented Jul 9, 2013

I stumbled upon this issue by accident, as I noticed that using a symbol that has been imported in multiple glob statements does not result in an error:

mod foo {
    pub fn p() { println("foo"); }
}
mod bar {
    pub fn p() { println("bar"); }
}

mod baz {
   use foo::*;
   use bar::*;
   #[main]
   fn my_main() {
       p();
   }
}

A simple oversight, I thought, and dove into resolve to fix the bug. Then I noticed that not even single imports are checked for conflicts:

mod foo {
    pub fn p() { println("foo"); }
}
mod bar {
    pub fn p() { println("bar"); }
}

mod baz {
   use foo::p;
   use bar::p;
   #[main]
   fn my_main() {
       p();
   }
}

I assumed that the reason for this was that currently both glob and single imports are flattened into a hashmap per module and tried to introduce a 2-level import scheme that allows duplicates only in glob imports but not in single imports.

And then I noticed that it's possible to export glob imports from a crate, making that scheme impossible. Even worse: this makes the global API of a crate dependent on the vigilance of the developer to not export multiple symbols with the same name, as a simple rearranging of use statements in the source code could break existing users of the crate.

So, there are four possible ways to go about this problem:

  1. Don't change anything. I.e. don't check for duplicate imported symbols, neither single imports nor glob imports. (Maybe add a lint, but that would probably have to be turned of in many cases, e.g. libstd is full of duplicate glob imports).

  2. Disallow duplicate imports, even when glob importing. This is IMO not workable.

  3. Disallow exporting glob imports from crates, making the aforementioned 2-level duplicate checking possible (i.e. disallow duplicate single imports, disallow used duplicate glob imports, allow unused duplicate glob imports).

  4. A variant on 3): allow exporting glob imports, implement the 2-level scheme for imports that are not visible from outside the crate, but disallow any duplicate imports otherwise.

  5. or 4) would be my preferred solution, but would incur a lot of work, both in implementing the scheme and restructuring existing code. 1) is the most realistic solution, but I personally don't really like it, as Rust is all about safety after all.

Fun example at the end:

mod foo {
    pub fn p() { println("foo"); }
}
mod bar {
    pub fn p() { println("bar"); }
}

mod baz {
   use bar::p;
   use foo::*;
   #[main]
   fn my_main() {
       p();
   }
}

Here, the use foo::*; is marked as being unused, even though when running the program, foo::foo() is actually the implementation of foo() that is used.

@huonw
Copy link
Member

huonw commented Jul 9, 2013

cc @pcwalton

(Personally, I prefer 1 with the lint (on warn by default), since it's easy to reason about.)

@pnkfelix
Copy link
Member

So, just so its clear: the current semantics (and thus what we'd stick with with option 1) is that the last use takes precedence over the earlier ones.

I agree with @huonw : the lint seems like the obvious choice here. Who cares if we have to turn it off in libstd, which user code won't be mucking with anyway? (I don't see this as a safety concern the same way that memory-management is; I expect in common cases for any bugs relating to this to be exposed with relative ease, expecially since one already gets the unused variable warning.)

(The fact that the unused variable warning is falling down on the glob should be filed as a separate bug; I'll do that now.)

@alexcrichton
Copy link
Member

I would consider #8869 as closing this because I'm also in favor of leaving the rules as is. Resolve is already really tricky and having fewer rules is always a good thing. If we had extra rules about globs specifically and whether they could shadow or not, I think that'd complicate things further. I feel that with the lint actually being correct it would actually help things be clearer.

That being said, perhaps some of these other rules could help simplify things in the long run :).

@huonw
Copy link
Member

huonw commented Mar 2, 2014

Triage: no big change about shadowing; I'm still in favour of the lint.

#12612 and #10884 are similar.

alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 8, 2014
I think that the test case from this issue has become out of date with resolve
changes in the past 9 months, and it's not entirely clear to me what the
original bug was.

Regardless, it seems like tricky resolve behavior, so tests were added to make
sure things resolved correctly and warnings were correctly reported.

Closes rust-lang#7663
bors added a commit that referenced this issue Apr 10, 2014
Closes #13441 (debuginfo: Fixes and improvements for #12840, #12886, and #13213)
Closes #13433 (Remove references to @trait from a compiler error message)
Closes #13430 (Fix outdated lint warning about inner attribute)
Closes #13425 (Remove a pile of (mainly) internal `~[]` uses)
Closes #13419 (Stop using transmute_mut in RefCell)
Closes #13417 (Remove an unnecessary file `src/libnative/io/p`.)
Closes #13409 (Closing assorted resolve bugs)
Closes #13406 (Generalized the pretty-print entry points to support `-o <file>`.)
Closes #13403 (test: Add a test for #7663)
Closes #13402 (rustdoc: Prune the paths that do not appear in the index.)
Closes #13396 (rustc: Remove absolute rpaths)
Closes #13371 (Rename ast::Purity and ast::Impure Function. Closes #7287)
Closes #13350 (collections: replace all ~[T] with Vec<T>.)
flip1995 pushed a commit to flip1995/rust that referenced this issue Sep 28, 2021
Fix result order for `manual_split_once` when `rsplitn` is used

fixes: rust-lang#7656

changelog: Fix result order for `manual_split_once` when `rsplitn` is used
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Path resolution
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants