Items shadow glob-imported reexports

[jseyfried]

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.

[Aatch]

Making matters worse, before PR #30843

Seems like an unfinished sentence?

[jseyfried]

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]

cc @nrc @nikomatsakis

[retep998]

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]

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]

@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]

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 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]

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]

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]

@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.