Skip to content

resolve: fix bug in visibility of shadowed use declarations #30294

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

Merged
merged 2 commits into from
Dec 11, 2015

Conversation

jseyfried
Copy link
Contributor

This fixes a bug in which the visibility of a use declaration defining a name in one namespace (e.g. the value namespace) is overridden by a later use declaration defining the same name in the other namespace (e.g. the type namespace). For example,

fn f() {}
pub mod bar {}

mod foo {
    use f; // This import should not be visible outside `foo`,
    pub use bar as f; // but it visible outside of `foo` because of this import.
}

fn main() { foo::f(); }

As the example demonstrates, this is a [breaking-change], but it looks unlikely to cause breakage in practice, and any breakage can be fixed by correcting visibility modifiers.

…e in one namespace (e.g. the value namespace) is overridden by a later use declaration defining the same name in the other namespace (e.g. the type namespace).
@rust-highfive
Copy link
Contributor

r? @alexcrichton

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

@sanxiyn
Copy link
Member

sanxiyn commented Dec 10, 2015

cc #30159.

@alexcrichton
Copy link
Member

r? @nrc

@jseyfried
Copy link
Contributor Author

This also fixes #30159.

match import_resolution.target_for_namespace(ns) {
Some(target) => {
if !import_resolution[ns].is_public {
continue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a labelled continue? It looks like you should continue to the next import_resolution, not to the next namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, if the import resolution is not public in one namespace it still could be public in the other namespace. For example, consider

mod foo {}
pub fn f() {}

use foo as bar;
pub use f as bar;

Here, there is a single import_resolution corresponding to bar that is private in the type namespace (i.e. !import_resolution[TypeNS].is_public) but public in the value namespace (i.e. import_resolution[ValueNS].is_public). If we skipped to the next import_resolution after seeing that the type namespace is private, we would not export bar in the value namespace.

@nrc
Copy link
Member

nrc commented Dec 11, 2015

LGTM, modulo the two comments

@jseyfried
Copy link
Contributor Author

@nrc Thanks for the feedback. I addressed both comments.

@nrc
Copy link
Member

nrc commented Dec 11, 2015

Sounds good, thanks!

@bors: r+

@bors
Copy link
Collaborator

bors commented Dec 11, 2015

📌 Commit ada87fa has been approved by nrc

@bors
Copy link
Collaborator

bors commented Dec 11, 2015

⌛ Testing commit ada87fa with merge 672a3d9...

@bors bors merged commit ada87fa into rust-lang:master Dec 11, 2015
@jseyfried jseyfried deleted the fix_shadowed_use_visibility branch December 18, 2015 10:27
@jseyfried jseyfried changed the title rustc_resolve: Fix bug in visibility of shadowed use declarations resolve: fix bug in visibility of shadowed use declarations Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants