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

Resolve: allow non-imported items to shadow glob imports #31377

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
5 participants
@jseyfried
Copy link
Contributor

jseyfried commented Feb 3, 2016

This PR lets non-imported items shadow glob imported items (fixes #31337).
More precisely, a glob imported item will be shadowed (in all of the namespaces it defines) if any namespace it defines is already defined by a non-imported item.

Previously, it was possible to shadow only one namespace of a glob-imported re-export that defined both namespaces, so this is a [breaking-chage]. For example,

mod foo {
    mod bar { pub struct Bar; }
    pub use self::bar::Bar;
}

pub use foo::*; // This imports the item `foo::bar::Bar`.

// The following used to only shadow `Bar` in the value namespace,
// leaving `foo::bar::Bar` only partially accessible.
// Now, it shadows `Bar` in both namespaces.
pub fn Bar() {}

fn f() -> Bar { foo::Bar } // This breaks

The breakage can be fixed by importing the shadowed item again as a fresh name or by renaming the item that shadows it.
r? @nrc

@jseyfried jseyfried force-pushed the jseyfried:allow_shadowing branch from a21319e to 42b9a4e Feb 3, 2016

@nrc

This comment has been minimized.

Copy link
Member

nrc commented Feb 3, 2016

I think we want an RFC before accepting this, right? (As discussed in #31337)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 3, 2016

☔️ The latest upstream changes (presumably #31338) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -949,6 +949,7 @@ bitflags! {
// Variants are considered `PUBLIC`, but some of them live in private enums.
// We need to track them to prohibit reexports like `pub use PrivEnum::Variant`.
const PRIVATE_VARIANT = 1 << 2,
const LINKED_NAMESPACES = 1 << 3,

This comment has been minimized.

@petrochenkov

petrochenkov Feb 3, 2016

Contributor

Could you add a comment describing the purpose of this flag?

This comment has been minimized.

@jseyfried

jseyfried Feb 4, 2016

Author Contributor

Done

// aux-build:variant-namespacing.rs

extern crate variant_namespacing;
pub use variant_namespacing::XE::*;

This comment has been minimized.

@petrochenkov

petrochenkov Feb 3, 2016

Contributor

This test ensures that variants are defined in both namespaces, it shouldn't be deleted, it should be tweaked to avoid glob imports instead.
pub use variant_namespacing::XE::* -> pub use variant_namespacing::XE::{XStruct, XTuple, XUnit} etc.

This comment has been minimized.

@jseyfried

jseyfried Feb 4, 2016

Author Contributor

Good point, I added it back with your tweak.

dest_import_resolution.is_public = is_public;
dest_import_resolution.target = Some(target.clone());
self.add_export(module_, name, &dest_import_resolution);
}

This comment has been minimized.

@petrochenkov

petrochenkov Feb 3, 2016

Contributor

So, reexport sets are free from name conflicts now and #31308 can be landed?

This comment has been minimized.

@jseyfried

jseyfried Feb 4, 2016

Author Contributor

Indeed :)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Feb 3, 2016

Are "glob vs glob" conflicts still result in errors? (In this implementation, I mean, not in the potential RFC.)

@jseyfried jseyfried force-pushed the jseyfried:allow_shadowing branch from 42b9a4e to 7ab5316 Feb 4, 2016

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

jseyfried commented Feb 4, 2016

@petrochenkov yes -- glob vs glob conflicts are still errors, as are glob import vs single import conflicts.

@jseyfried jseyfried force-pushed the jseyfried:allow_shadowing branch 4 times, most recently from 2d62a8c to 819f8e2 Feb 4, 2016

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 11, 2016

☔️ The latest upstream changes (presumably #31461) made this pull request unmergeable. Please resolve the merge conflicts.

@nrc nrc added the T-compiler label Feb 11, 2016

@jseyfried jseyfried force-pushed the jseyfried:allow_shadowing branch from 819f8e2 to 4d56389 Feb 12, 2016

@jseyfried jseyfried force-pushed the jseyfried:allow_shadowing branch from 4d56389 to 9490e1e Feb 12, 2016

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

jseyfried commented Feb 12, 2016

rebased

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Feb 12, 2016

We discussed this in the recent @rust-lang/lang meeting. Our conclusion was that the correct semantics is not entirely clear here. @nrc is planning on introducing an RFC describing the name resolution system soon -- one that will also incorporate changes to hygiene and macro expansion -- and plans to raise this issue in that RFC. So we figured it'd be best to wait on making an official decision until we have time to discuss the alternatives in that forum.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2016

☔️ The latest upstream changes (presumably #31648) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

jseyfried commented Feb 18, 2016

Closing because of the decision in #31337 and because I no longer like the "linked namespaces" shadowing sematics I implemented here (see this comment).

@jseyfried jseyfried closed this Feb 18, 2016

@jseyfried jseyfried deleted the jseyfried:allow_shadowing branch Feb 18, 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.