Skip to content

/rust

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

resolve: Improve duplicate glob detection #32814

Merged
merged 4 commits into from Apr 13, 2016

Conversation

Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
beta-accepted
Projects
None yet
Milestone
No milestone
5 participants
@jseyfried @nikomatsakis @bors @bluss @brson
@jseyfried
Copy link
Contributor

jseyfried commented Apr 8, 2016

This fixes a bug introduced in #31726 in which we erroneously allow multiple imports of the same item under some circumstances.

More specifically, we erroneously allow a module that is in a cycle of glob re-exports to have other re-exports (besides the glob from the cycle).
For example,

pub fn f() {}
mod foo {
    pub use f; // (1) This defines `foo::f`.
    pub use bar::*; // (3) This also defines `foo::f`, which should be a duplicate error but is currently allowed.
}
mod bar {
    pub use foo::*; // (2) This defines `bar::f`.
}

A module in a glob re-export cycle can still have pub items after this PR. For example,

mod foo {
    pub fn f() {}; // (1) This defines `foo::f`.
    pub use bar::*; // (3) This is not a duplicate error since items shadow glob-imported re-exports (cf #31337).
}
mod bar {
    pub use foo::*; // (2) This defines `bar::f`.
}

r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis Apr 8, 2016

@jseyfried

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jseyfried commented Apr 8, 2016

The bug isn't in beta yet, but it landed a month ago so it still might be a good idea to do a crater run.
src/librustc_resolve/resolve_imports.rs
@@ -275,7 +275,6 @@ impl<'a> ::ModuleS<'a> {
// Define the name or return the existing binding if there is a collision.
pub fn try_define_child(&self, name: Name, ns: Namespace, binding: NameBinding<'a>)
-> Result<(), &'a NameBinding<'a>> {
if self.resolutions.borrow_state() != ::std::cell::BorrowState::Unused { return Ok(()); }

This comment has been minimized.

Sign in to view
Copy link
@jseyfried

jseyfried Apr 8, 2016

Author Contributor

This line was preventing the duplicate errors from being reported.
src/librustc_resolve/resolve_imports.rs
}
};

self.define_in_glob_importers(name, ns, new_binding);

This comment has been minimized.

Sign in to view
Copy link
@jseyfried

jseyfried Apr 8, 2016

Author Contributor

The above change is so that resolution isn't borrowed during define_in_glob_importers, where it might end up getting re-defined through a glob cycle.

This comment has been minimized.

Sign in to view
Copy link
@nikomatsakis

nikomatsakis Apr 11, 2016

Contributor

@jseyfried

The above change is so that resolution isn't borrowed during define_in_glob_importers, where it might end up getting re-defined through a glob cycle.

I'd suggest adding this as a comment into the code :) (also the note you added below)
src/librustc_resolve/resolve_imports.rs
}).collect::<Vec<_>>();
for ((name, ns), binding) in bindings {
if binding.defined_with(DefModifiers::IMPORTABLE | DefModifiers::PUBLIC) {
let _ = module_.try_define_child(name, ns, directive.import(binding, None));

This comment has been minimized.

Sign in to view
Copy link
@jseyfried

jseyfried Apr 8, 2016

Author Contributor

resolutions shouldn't be borrowed during try_define_child for the same reason.

@jseyfried jseyfried referenced this pull request Apr 11, 2016

Closed

Fix ICE in `resolve` #32813

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Apr 11, 2016

r=me w/ comments
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 11, 2016

📌 Commit c0b3bb9 has been approved by nikomatsakis

@jseyfried jseyfried force-pushed the jseyfried:improve_duplicate_glob_detection branch from c0b3bb9 to bc6daea Apr 11, 2016

@jseyfried

This comment has been minimized.

Sign in to view
Copy link
Contributor Author

jseyfried commented Apr 11, 2016

@bors r=nikomatsakis
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 11, 2016

📌 Commit bc6daea has been approved by nikomatsakis
@bluss

This comment has been minimized.

Sign in to view
Copy link
Contributor

bluss commented Apr 11, 2016

This needs backport if it wants to get in Rust 1.9 beta

@nikomatsakis nikomatsakis added the beta-nominated label Apr 12, 2016

@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Apr 13, 2016

⌛️ Testing commit bc6daea with merge 4b71f8d...

bors added a commit that referenced this pull request Apr 13, 2016

@bors
Auto merge of #32814 - jseyfried:improve_duplicate_glob_detection, r=… 
…nikomatsakis

resolve: Improve duplicate glob detection

This fixes a bug introduced in #31726 in which we erroneously allow multiple imports of the same item under some circumstances.

More specifically, we erroneously allow a module that is in a cycle of glob re-exports to have other re-exports (besides the glob from the cycle).
For example,
```rust
pub fn f() {}
mod foo {
    pub use f; // (1) This defines `foo::f`.
    pub use bar::*; // (3) This also defines `foo::f`, which should be a duplicate error but is currently allowed.
}
mod bar {
    pub use foo::*; // (2) This defines `bar::f`.
}
```

A module in a glob re-export cycle can still have `pub` items after this PR. For example,
```rust
mod foo {
    pub fn f() {}; // (1) This defines `foo::f`.
    pub use bar::*; // (3) This is not a duplicate error since items shadow glob-imported re-exports (cf #31337).
}
mod bar {
    pub use foo::*; // (2) This defines `bar::f`.
}
```
r? @nikomatsakis
4b71f8d
@bors

@bors bors merged commit bc6daea into rust-lang:master Apr 13, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

This was referenced Apr 13, 2016

Merged
Closed

@jseyfried jseyfried referenced this pull request Apr 14, 2016

Merged

Fix duplicate errors #1

@nikomatsakis nikomatsakis added the beta-accepted label Apr 21, 2016

@nikomatsakis

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikomatsakis commented Apr 21, 2016

Fixes a regression, small patch. Approved for beta.

@alexcrichton alexcrichton referenced this pull request May 4, 2016

Merged

Merge beta-accepted into beta #33407

@brson brson removed the beta-nominated label May 4, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.