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

Items shadow glob-imported reexports #31337

Closed
jseyfried opened this issue Feb 1, 2016 · 29 comments

Comments

Projects
None yet
8 participants
@jseyfried
Copy link
Contributor

commented Feb 1, 2016

For example,

mod foo {
    mod bar {
        pub fn f() { println!("foo"); }
    }
    pub use self::bar::f;
}

pub use foo::*;
pub fn f() { println!("baz"); }

fn main() {
    f(); // Prints "baz"
}

Also, considering the above example as a crate, the glob import used to shadow the item from the perspective of users of the crate before #30843. After #30843, the item shadows the glob import both inside and outside the crate. I can easily revert that breaking change if desired (crater found no breakage in practice).

Finally, in the generated documentation for the crate, f is listed twice, both times as the glob imported version, i.e. the glob import still shadows the item in the crate documentation.

@jseyfried jseyfried changed the title Items can be shadowed by glob-imported reexports Items shadow glob-imported reexports Feb 1, 2016

@Aatch

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

Making matters worse, before PR #30843

Seems like an unfinished sentence?

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

I think there are two reasonable ways to fix this:

  1. Disallow this shadowing, perhaps with a warning cycle first.
  2. Continue allowing items to shadow glob-imported reexports and fix the generated crate documentation, expecting that we will eventually allow all glob imports to be shadowed by items and single imports as proposed here and here.
@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

@retep998

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

I think it was intentional for non-glob items (whether declared there or via imports) to be able to override glob imports and I'd rather keep it that way.

@nrc

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

My opinion here is that we should allow local items to shadow glob imported items, otherwise adding an item to the upstream module can break the downstream one, and it is fairly clear that local declarations should have priority (I'm less sure about glob and non-glob imports, I like the idea that we can have multiple imports if they import the same item or the import is not used, but re-exports must count as 'used', so I'm not sure there).

Anyway, this is basically arguing for option 2 - fix the docs.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2016

@nrc Ok, I agree that glob imported items should be shadowable.
Do you think we should allow only one namespace of a name that is defined in both namespaces by a single item to be shadowed? For example,

mod foo {
    mod bar { pub struct Bar; }
    pub use self::bar::Bar;
}
pub use foo::*; // This defines Bar in both namespaces
pub fn Bar() {} // This shadows the old definition in the value namespace

fn main() {
    Bar; // This refers to the shadowing function
    fn g(_b: Bar) {} // This refers to the struct, since it was not shadowed in the type namespace
}

When generating the documentation for this example, would we list the struct Bar and the function Bar? That seems like it would cause confusion, since the constructor for the struct is not in fact exported.

We could instead not list the struct at all and disallow use of the type Bar from outside of the crate but continue allowing it within the crate.

Finally, we could disallow use of the type Bar inside and outside the crate, i.e. we could have the function shadow the struct in both namespaces, effectively "undefining" Bar in the type namespace.

@nrc

This comment has been minimized.

Copy link
Member

commented Feb 1, 2016

I think that we should at least lint against this, if not forbid it out right (i.e., make it an error to shadow one namespace but not the other, when the items in the same namespace are 'linked'). This means making a connection between the two Bar names in different namespaces, which is unfortunate, but I think the least confusing path for the user. If we lint instead of error, then I think we should adopt the semantics from your last option.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

This is unfortunate.
Glob shadowing should either be properly (not accidentally and only in some cases) implemented or prohibited.
The former is certainly the way to go in the long term, probably with an RFC describing the design in detail (for all cases including glob vs glob, glob vs prelude etc.).
The latter is a better immediate solution, I think (with a crater run, of course), unless @jseyfried has a working implementation for the former :)

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

I think we should revert the change, even though I prefer the shadowing semantics in the long term. Basically, I think we ought to do an RFC that describes this change, in addition to incorporating the more general work we've been planning on macro and name resolution.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2016

Also, the question of multiple namespaces is an important one. I think it came up in the revised name resolution algorithm I was planning -- though I didn't extend my latest prototype to cover multiple paths, so maybe I am remembering details from an older version. But I remember thinking that there are some somewhat subtle interactions arising there.

Ah, I sort of remember now. The problem was I think specific to my older, monotonic variants, where we had to know whether use foo::bar::f; should shadow a module that came in via glob, and we couldn't necessarily know that until we resolved foo::bar fully (which might itself depend on the glob). I think it's less of an issue with the newer, "eager" variants.

(Sorry, these comments are kind of rambly, but for context, I am discussing the algorithm here https://github.com/nikomatsakis/rust-name-resolution-algorithm.)

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 2, 2016

@nrc

I'm less sure about glob and non-glob imports, I like the idea that we can have multiple imports if they import the same item or the import is not used, but re-exports must count as 'used', so I'm not sure there

I'd prefer non-glob imports to behave exactly like other items, i.e. they cannot have name conflicts, are visible from other modules even when private, can shadow glob imports etc. This give us one simple scheme which is easy to teach and contains no surprises. (Yes, it could be useful sometimes to import the same thing twice in macros, but it's not a great loss IMO, we don't have it now and I haven't seem complaints so far.)
All the shadowing rules and complexities will be concentrated around globs and prelude then.
But I'm digressing, this would be better discussed in the RFC.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

The above PRs are implementations of the two fixes I proposed earlier.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

@nrc
I agree that we should use the semantics from my last option, but I don't agree we should have the lint. I think it makes more sense to think of non-use items as the objects that are imported, not abstract bindings of names in namespaces. From this perspective, it is clear that (for example) a function would prevent a tuple struct with the same name from being glob imported, since it would be meaningless to import "half a tuple struct item".

Furthermore, to fix the lint, one would have to define an unused dummy item to shadow the other namespace (which would be tempting, but ugly), or one would have to rename the shadowing item or refactor the glob import into single imports, which would defeat the purpose of glob shadowing.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 3, 2016

@petrochenkov @nikomatsakis
If disallowing shadowing causes little to no breakage, I would support it.
Otherwise I don't think it's worth it to break people's code (or warn them into changing it) when we can instead allow non-import items to shadow globs with the above semantics for now, which is future-compatible with the more detailed proposals, simple to implement, and almost backwards-compatible.

@retep998

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

One of the larger things is actually winapi which a lot of people glob import. Since winapi is always growing, being able to shadow stuff from that glob import is really useful for forwards compatibility. Note that crater does not work on Windows yet so it wouldn't be able to tell you the potential breakage.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2016

@jseyfried sorry, I'm maybe a bit confused. I thought we DID forbid shadowing now? Are we just inconsistent about it?

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 3, 2016

On Tue, Feb 02, 2016 at 08:22:44PM -0800, Peter Atashian wrote:

Since winapi is always growing, being able to shadow stuff from that glob import is really useful for forwards compatibility.

I think everyone agrees this is the semantics we eventually want.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2016

@nikomatsakis We have been allowing local items to shadow glob-imported re-exports for a long time.
I am using "glob-imported re-export" to mean an item that was glob-imported from a module that itself imported the item, as in the example.

Before #30843, the shadowed glob-imported re-export was visible to users of the crate and the local item was not, so that from the perspective of users of the crate, the glob-imported re-export shadowed the local item. After #30843, the local item shadows the glob-imported re-export both inside and outside the crate.

The issue here is that when documenting a crate, we include both the shadowed glob-imported re-export and the local item (provided they are both public, of course). This is easy to fix in the case of a function shadowing a function for example (just don't report the shadowed function).

It's trickier, however, if (say) a tuple struct is shadowed by a function. I described some options of what to do in this case in this comment and advocated for my preferred solution in this comment.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2016

I see. I got confused. That does affect my opinion -- it's clear that
prohibiting the shadowing would be fixing a bug, but I also agree that
causing breakage to enforce a rule we don't like seems excessive.

On Wed, Feb 3, 2016 at 8:29 PM, Jeffrey Seyfried notifications@github.com
wrote:

@nikomatsakis https://github.com/nikomatsakis We have been allowing
local items to shadow glob-imported re-exports for a long time.
I am using "glob-imported re-export" to mean an item that was
glob-imported from a module that itself imported the item, as in the
example.

Before #30843 #30843, the
shadowed glob-imported re-export was visible to users of the crate and the
local item was not, so that from the perspective of users of the crate, the
glob-imported re-export shadowed the local item. After #30843
#30843, the local item shadows
the glob-imported re-export both inside and outside the crate.

The issue here is that when documenting a crate, we include both the
shadowed glob-imported re-export and the local item (provided they are both
public, of course). This is easy to fix in the case of a function shadowing
a function for example (just don't report the shadowed function).

It's trickier, however, if (say) a tuple struct is shadowed by a function.
I described some options of what to do in this case in this comment
#31337 (comment)
and advocated for my preferred solution in this comment
#31337 (comment).


Reply to this email directly or view it on GitHub
#31337 (comment).

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

Nominating for discussion in the lang team meeting.

@nikomatsakis nikomatsakis added the T-lang label Feb 11, 2016

@aturon

This comment has been minimized.

Copy link
Member

commented Feb 11, 2016

it's clear that prohibiting the shadowing would be fixing a bug, but I also agree that causing breakage to enforce a rule we don't like seems excessive.

I strongly agree with this sentiment. Shadowing is what we want here in the long run, and it doesn't make sense to me to break code that's using it, only to more completely implement shadowing later on.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 11, 2016

The regression in tiled-0.1.4 is related to glob shadowing, see https://internals.rust-lang.org/t/regression-report-stable-2016-01-21-vs-nightly-2016-02-11/3172 for details
Actually, nevermind, this is #30882

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2016

We discussed this in the @rust-lang/lang meeting. The conclusion was that we should adopt @jseyfried's "option 2":

Continue allowing items to shadow glob-imported reexports and fix the generated crate documentation, expecting that we will eventually allow all glob imports to be shadowed by items and single imports as proposed here and here.

Specifically, we do eventually want shadowing semantics, and therefore it doesn't make sense to regress crates that are taking advantage of it, even if the current support is a bug.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2016

Ok, sounds good.
How should we fix the documentation for the example in this comment?

@nrc

This comment has been minimized.

Copy link
Member

commented Feb 14, 2016

@jseyfried Are talking about Rustdoc? I think listing a struct when it is only exported in a single namespace is OK (better than not listing it, since it is in fact exported in some sense). Perhaps adding notes for such situations would be nice.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2016

By the way, what will happen with half-shadowed items in cross-crate scenarios?
Metadata doesn't care about namespaces currently, it will keep the half-exported items as whole items.
Then some of the conflicting names will be unpredictably ignored again due to try_define when this metadata is read.

@jseyfried

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2016

@nrc Yeah, I was talking about Rustdoc. Thinking about this some more, I agree that it is OK to list a partially shadowed struct -- it looks like it would be very uncommon in the wild and we already have far more common cases of unnameable paths in rustdoc thanks to the distinction between visibility and nameability.

@petrochenkov Indeed, except they will be predictably ignored -- items are listed before re-exports in metadata so that when there is a conflict between an item and an import, the item will always win. This probably isn't the most elegant solution but it works for now.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2016

items are listed before re-exports in metadata

Well, at least it doesn't depend on the phase of the moon.

bors added a commit that referenced this issue Apr 13, 2016

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

bors added a commit that referenced this issue Oct 4, 2016

Auto merge of #36814 - petrochenkov:def, r=eddyb
Refactoring/bugfixing around definitions for struct/variant constructors

 d917c36 separates definitions for struct/variant constructors living in value namespace from struct/variant type definitions.

adfb378 fixes cross-crate resolution of reexports reexporting half-items, like struct constructors without struct type or types without constructor. Such reexports can appear due to glob shadowing.
Resolution now is not affected by the order in which items and reexports are decoded from metadata (cc #31337 (comment)). `try_define` is not used during building reduced graph anymore.
500 lines of this PR are tests for this exotic situation, the remaining line diff count is actually negative! :)

c695d0c (and partially aabf132) moves most of pattern resolution checks from typeck to resolve (except those checking for associated items), uses the same wording for pattern resolution error messages from both typeck and resolve and makes the messages more precise.

11e3524 fixes seemingly incorrectly set `NON_ZERO_SIZED` attributes for struct/variant ctors in const eval.

4586fea eliminates `ty::VariantKind` in favor of `def::CtorKind`. The logic is that variant kinds are irrelevant for types, they make sense only when we deal with constructor functions/constants. Despite that `VariantDefData` still keeps a copy of `CtorKind`, but it's used only for various kinds of pretty-printing (and for storing in metadata).

aabf132 is mostly a cleanup of various impossible or improperly used definitions, and other small definition cleanups.

cc @jseyfried
r? @eddyb
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2017

This was fixed by stabilizing RFC 1560.
@jseyfried please reopen if something is missing.

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.