Lint against shadowing the prelude

[kpreid]

What it does

Lints cases where a use, item declaration, or local binding shadows an item in the Rust prelude.

In the case of a use, it suggests instead using the containing module, i.e. replacing use std::io::Result; with use std::io;

Lint Name

shadow_prelude

Category

style, pedantic, restriction

Advantage

• Code which redefines well-known names can be confusing to read, especially for beginners, or people who are trying to help a beginner but have only a code excerpt rather than a complete file with uses visible. (“This return type needs to be Result<(), MyError>.” “I did that, but it says that Result only takes 1 generic argument.” “Oh, you must have imported another Result; you need to remove that and change all your usages.”)

• If an author intends to avoid shadowing, they may benefit from the lint catching when e.g. their IDE helpfully inserts an unwanted use.

• It would also catch when a library author accidentally chooses a name for an item that conflicts with the standard prelude, allowing them to decide whether to reuse the name or choose a new one.

Drawbacks

• It increases the verbosity of code obeying the restriction.

• Shadowing Result and Error names is common enough practice that it might be objectionable to enable this lint by default, which limits its utility in the primary use case of helping beginners.

Example

use std::fs::read_to_string;
use std::io::Result;

fn load_config() -> Result<String> {
    read_to_string("config.toml")
}

Could be written as:

use std::fs::read_to_string;
use std::io;

fn load_config() -> io::Result<String> {
    read_to_string("config.toml")
}

[asquared31415]

I believe this is the same issue and this can produce incredibly surprising behavior. playground

#![deny(clippy::all, clippy::pedantic)]
fn format() {
    println!("my format function");
}
use format as renamed;

fn main() {
    renamed();
    let x = 42_i32;
    let y = renamed!("foo{}", x);
    println!("{y}");
}

outputs:

my format function
foo42

renamed can be both the local function and the macro in the prelude.

[kpreid]

I hadn't even thought of different namespaces, but that's a good point — even without any use ... as, a library author could accidentally reexport a prelude item sharing the name. (And such a reexport could even be introduced by an edition change, where the prelude item is newer than the source code.)

Speaking precisely, that's not shadowing (since both items stay accessible), but it seems closely enough related to the goals of this lint to include.

[hellow554]

What about use crate::error::Result. I really like that, because typically Result is type Result<T, E = MyErrorEnum> = ::std::result::Result<T, E>, which you then can use without repeating the error type every now and then.

[kpreid]

@hellow554 Yes, that is exactly what would be flagged by this lint. It is a tradeoff between different features the code could have, just like many other clippy lints — by disabling the lint, you can have concise conventional names, and by enabling the lint, the reader can know that all prelude names mean what they usually do.

[jplatte]

Maybe a "subsection" (it actually isn't, but it's very similar) that could be warn-by-default would be primitive types, i.e. bool, char, u{8,16,32,64,128,size}, i{8,16,32,64,128,size}, f{32,64} and str.

[joshtriplett]

I'd love to see this lint as well, specifically to catch cases like use somecrate::Result.

[kornelski]

It's possible to make accidental import of Result harmless while still supporting a custom error type:

type Result<T, E = MyError> = StdResult<T, E>

It would be nice if this fail-safe form was excluded from the lint.

[joshtriplett]

@kornelski I'd expect the lint to be allow-by-default, and if explicitly changed to warn I would expect to get a warning for shadowing Result, even if doing so to add a default like that.

[kpreid]

As the author of this proposal, one of the things I would like it to be good for helping people avoid confusion, or writing confusing code, by unintentionally shadowing a prelude item that is not like Result. To serve that goal, it must be on-by-default, because the people who will most benefit are those who wouldn't think of seeking out and enabling it. But, also, it would be nice to have the option of a style restriction “never shadow the prelude”.

Perhaps a way to resolve this tension would be to make it warn-by-default with a configurable exclusion list, set by default to ["Result", "Error"]. Then users wishing for a restriction can configure the exclusions to be [].

[joshtriplett]

Having a more conservative variant that's warn-by-default and a lint for all shadowing that's allow-by-default sounds reasonable.