Items shadow glob-imported reexports #31337
Comments
jseyfried
changed the title
Seems like an unfinished sentence? |
|
I think there are two reasonable ways to fix this: |
|
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. |
|
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. |
|
@nrc Ok, I agree that glob imported items should be shadowable. 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. |
|
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 |
|
This is unfortunate. |
|
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. |
|
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 (Sorry, these comments are kind of rambly, but for context, I am discussing the algorithm here https://github.com/nikomatsakis/rust-name-resolution-algorithm.) |
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.) |
|
The above PRs are implementations of the two fixes I proposed earlier. |
|
@nrc 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. |
|
@petrochenkov @nikomatsakis |
|
One of the larger things is actually |
|
@jseyfried sorry, I'm maybe a bit confused. I thought we DID forbid shadowing now? Are we just inconsistent about it? |
|
On Tue, Feb 02, 2016 at 08:22:44PM -0800, Peter Atashian wrote:
I think everyone agrees this is the semantics we eventually want. |
|
@nikomatsakis We have been allowing local items to shadow glob-imported re-exports for a long time. 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. |
|
I see. I got confused. That does affect my opinion -- it's clear that On Wed, Feb 3, 2016 at 8:29 PM, Jeffrey Seyfried notifications@github.com
|
|
Nominating for discussion in the lang team meeting. |
nikomatsakis
added
the
T-lang
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. |
|
|
|
We discussed this in the @rust-lang/lang meeting. The conclusion was that we should adopt @jseyfried's "option 2":
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. |
|
Ok, sounds good. |
|
@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. |
|
By the way, what will happen with half-shadowed items in cross-crate scenarios? |
|
@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. |
Well, at least it doesn't depend on the phase of the moon. |
…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
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
|
This was fixed by stabilizing RFC 1560. |
For example,
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,
fis listed twice, both times as the glob imported version, i.e. the glob import still shadows the item in the crate documentation.The text was updated successfully, but these errors were encountered: