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

Lint against shadowing the prelude #8439

Open
Tracked by #79
kpreid opened this issue Feb 15, 2022 · 5 comments
Open
Tracked by #79

Lint against shadowing the prelude #8439

kpreid opened this issue Feb 15, 2022 · 5 comments
Labels
A-lint Area: New lints

Comments

@kpreid
Copy link
Contributor

kpreid commented Feb 15, 2022

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")
}
@kpreid kpreid added the A-lint Area: New lints label Feb 15, 2022
@asquared31415
Copy link
Contributor

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
Copy link
Contributor Author

kpreid commented Feb 16, 2022

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
Copy link
Contributor

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
Copy link
Contributor Author

kpreid commented Feb 17, 2022

@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
Copy link
Contributor

jplatte commented Jun 8, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

4 participants