use statement fails silently if it would shadow an existing name

[HildarTheDorf]

If a use statement would shadow an existing name, it does nothing, not even emit a warning. While the rule on not importing if it would shadow seems to be "working as intended", it might be helpful to issue a warning (or error).

use module::module;
use mod1::mod2;
use light::ShadowMe;

// Test for shadowing it's own module name.
mod module {
    struct module;

    impl module {
        pub fn new() -> module {
            module
        }
    }
}

// Test for shadowing another module
mod mod1 {
    struct mod2;

    impl mod2 {
        pub fn new() -> mod2 {
            mod2
        }
    }
}

mod mod2 {
    struct somethingtomakethismoduleworthwhile;
}

// Test for shadowing a struct name
struct ShadowMe;

impl ShadowMe {
    fn DoNothing(&self) {}
}

mod light {
    pub struct ShadowMe;
}

fn main()
{
    // Fails.
    //let test1fail = module::new();

    // Succeeds.
    let test1pass = module::module::new();

    // Fails
    //let test2fail = mod2::new();

    // Succeeds.
    let test2pass = mod1::mod2::new();

    // Succeeds, test3 is of type ::ShadowMe, not light::ShadowMe
    let test3 = ShadowMe;
    test3.DoNothing();
}

[alexcrichton]

This is currently all working as intended as you mentioned. This is why we require that use statements appear before other items (like modules), to signify that the binding of a name is the first thing you find when you look upwards in the file.

That being said, the idea of having a warning about this is interesting. I personally would not want the lint to be warn-by-default, and allow-by-default lints have questionable usefulness when their purpose is to catch a possible bug like this, but the idea may be worth exploring.

[HildarTheDorf]

I think that it is worth warning-by-default if it was an explicit use (it's basically a no-op, and we just had a patch warning on unused code ^^) but not for a globbed use (that would be allow-by-default or not checked at all). Of course, globs aren't finished yet (and might not be sticking around at all), so that point could well moot.

There is also the warn-by-default unused_import for use statements that could be but aren't used. use statements that can't ever be used seem to be 'worse' to me.

But that's just my 2 pence.

[huonw]

FWIW, any resolution errors caused by this will make the compiler quit before even hitting the lint pass; and in cases where it is shadowed without causing an error, wouldn't it be flagged by unused import errors?

[alexcrichton]

I'm going to close this as working as intended. @huonw has a good point that a lint wouldn't be too useful in terms of errors, and I think that any errors should be accompanied with extra machinery from resolve instead. Resolve could keep a history of shadowed items and attempt to resolve through all shadowed items, providing a suggestion that perhaps you didn't mean to shadow when an error occurs.

For now though, everything is working as intended. I think that a different bug should be opened for improving error messages of resolve.