Fallout from expansion of redundant import checking

[tmandry]

I've noticed there is a lot of fallout from the changes in #117772 expanding redundant import checking. See discussion and backlinks on that issue as well as #118692 for some evidence.

I have a theory that redundant imports should be split out into their own lint group (this was discussed in the past), and that in the future as a matter of policy, disruptive expansions like this should come with a machine-applicable fix to make rollout smoother.

For now, I'm opening this issue to collect data and feedback that I want to use to inform future policy discussions around lint expansions.

[tmandry]

FYI @surechen @petrochenkov - Not trying to single you out here, but I would like your input. This is a problem I've noticed in past lint expansions, and it seems useful to focus discussion around a concrete incidence.

[tmandry]

I'm interested to see what kind of numbers crater can give us.

@craterbot run name=redundant-imports start=master#6f726205a1b7992537ddec96c83f2b054b03e04f+rustflags=-Dunused-imports end=master#8b21296b5db6d5724d6b8440dcf459fa82fd88b5+rustflags=-Dunused-imports crates=top-1000 mode=check-only p=2

[craterbot]

👌 Experiment redundant-imports created and queued.
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[tmandry]

Now I see that the only p=1 in the queue has a similar number of crates to build, so let's use the same priority.

@craterbot name=redundant-imports p=1

[craterbot]

📝 Configuration of the redundant-imports experiment changed.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[surechen]

FYI @surechen @petrochenkov - Not trying to single you out here, but I would like your input. This is a problem I've noticed in past lint expansions, and it seems useful to focus discussion around a concrete incidence.

Hi, thank you for starting the discussion.

As a newbie to compilers, this PR was originally intended to address a smaller scope issue #117448, detecting redundant imports which should only be imported by prelude, petrochenkov provided a lot of help in the comments. I very much recognized his comment: #117772 (comment) and asked for help, so he patiently guided me to develop this PR. So my views here may not be professional enough and can only represent my own.

I think Rust's powerful detection capabilities make me more confident in the code I develop. In my mind, ideally, the compiler can help me find all problems, so stricter checking of redundant imports will not cause too much trouble, but it will help me.

Of course, if some lints will invalidate a large amount of existing code, it will really increase my burden, so if the community has some future policies or some measures to make the changes smoother, then I will appreciate it very much, and I will also be very happy to participate in the discussion or do some development.

[Robbepop]

Hi, today I noticed that this is causing a lot of trouble for my crates in the Wasmi workspace, particularly the wasmi_core crate. I mistakenly thought this was a clippy issue first and reported it there and got rerouted to this issue fortunately. For the sake of providing my use case that causes issues with the expanded import checking I will copy what I wrote in the clippy bug report below.

Summary

For my crate wasmi_core in https://github.com/wasmi-labs/wasmi/tree/master/crates/core I see the following warnings when running clippy rustc on nightly channel:

warning: the item `Box` is imported redundantly
   --> crates/wasmi/src/store.rs:32:5
    |
32  | use alloc::boxed::Box;
    |     ^^^^^^^^^^^^^^^^^
    |
   ::: /Users/robin/.rustup/toolchains/nightly-aarch64-apple-darwin/lib/rustlib/src/rust/library/std/src/prelude/mod.rs:125:13
    |
125 |     pub use super::v1::*;
    |             --------- the item `Box` is already defined here

Many more warnings pop up, but they all are very similar to the one above.
This import is required for proper no_std builds.

In lib.rs we do:

#[cfg(not(feature = "std"))]
extern crate alloc;

Thus alloc is always available as crate root import.

Lint Name

unused_imports

Reproducer

• Clone the repository https://github.com/wasmi-labs/wasmi (rev = a2f299af2f97af3691e195a4473abaa93bca72f7)
• Install rustc from current nightly channel.
• Run cargo +nightly check -p wasmi_core

Version

rustc 1.78.0-nightly (ef324565d 2024-02-27)
binary: rustc
commit-hash: ef324565d071c6d7e2477a195648549e33d6a465
commit-date: 2024-02-27
host: aarch64-apple-darwin
release: 1.78.0-nightly
LLVM version: 18.1.0

[craterbot]

🚧 Experiment redundant-imports is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment redundant-imports is completed!
📊 295 regressed and 0 fixed (1000 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[kayabaNerve]

My repository, which aims to support no-std when possible, explicitly imports many items within the std prelude. I effectively have to disable this lint or add tens to >100 of cfg statements, which makes no-std dev much more annoying.

EDIT: I cannot disable this without disabling unused import checks entirely, which I now understand ('moving to its own lint group' wasn't immediately clear to me). This is a blocker for me to update and really unfortunate.

[petrochenkov]

I've closed some issues that duplicate this one.

Issues with specific technical issues that may be fixed:

• unused_imports redundant import is already defined here points to self #121915
• Redundant import leaks prelude import #121312
• unused_qualifications clashes with unused_import #121331
• "item TryFrom is imported redundantly" doesn't have automatic fix #121315
• False positive unused_import warning on std::string::String #71450 (fixed, only needs a test)
• Incorrect unused_import lint firing for a function-local macro_rules! #78894 (fixed, only needs a test)

[petrochenkov]

In general, the new lint logic does exactly the same thing as all other existing unused lints - it detects some code (imports in this case) that can be removed, while not taking cfgs into account.

The only difference is that most other unused lints, including unused_imports, always existed, but this subset is added just recently.
So other unused code was timely removed and crates using conditional compilation, including no_std, invented some idioms for deailing with it, but with the new subset there's no such adaptation.

[petrochenkov]

@surechen
If you are still interested in doing something in rustc you could fix some of #121708 (comment).

[petrochenkov]

Note, that redundant imports specifically was previously reported by rustc as a part of unused_imports, but only in a minority of contexts (imports in blocks, introduced in #58805 in 2019), redundant imports in modules weren't reported.

[petrochenkov]

The change has already reached beta, we probably want to make some decision about it until it reaches stable.

[petrochenkov]

@joshtriplett

The lint is very likely to impact libs-api policy on additions to the prelude.

FWIW, I assumed that prelude additions already happen at edition boundaries only.

[joshtriplett]

@joshtriplett

The lint is very likely to impact libs-api policy on additions to the prelude.

FWIW, I assumed that prelude additions already happen at edition boundaries only.

There's no fundamental reason they have to be. Additions to the prelude can't create conflicts, because they'll be shadowed by any declarations by the user. (There are exceptions to that, such as traits, which can create ambiguities, so we only add traits at edition boundaries.)

[joshtriplett]

@petrochenkov wrote:

The change has already reached beta, we probably want to make some decision about it until it reaches stable.

Should we consider a beta revert to give time to evaluate this?

[petrochenkov]

Commenting out the line that emits the lint is trivial.
(Literally reverting the change is not trivial and I'd recommend against it.)

[kayabaNerve]

I legitimately believe it should be moved to clippy, arguably even pedantic. I do say this as someone not involved with Rust development however, solely as a strongly opinionated user. Apologies if I'm being too noisy on this.

[joshtriplett]

@petrochenkov I think the right fix is either going to be to keep it but under a different name and allow-by-default, or revert it entirely. Either way, I don't think we should keep it under the existing warn-by-default lint. (I know @tmandry is working on the more general policy questions here.)

Could you please make the one-line change so that we have time to evaluate this? (I'd appreciate a second from @rust-lang/lang for confirmation.)

Thank you.

[surechen]

@petrochenkov I think the right fix is either going to be to keep it but under a different name and allow-by-default, or revert it entirely. Either way, I don't think we should keep it under the existing warn-by-default lint. (I know @tmandry is working on the more general policy questions here.)

Could you please make the one-line change so that we have time to evaluate this? (I'd appreciate a second from @rust-lang/lang for confirmation.)

Thank you.

Hello, I have talked with @petrochenkov and I will submit a PR to beta to block it through comments first.

[surechen]

Oh, sorry, I didn't notice #123744. I will close #123768.

[apiraino]

visited during T-compiler triage meeting (on zulip). Taking note that it has been reverted in beta

@rustbot label -I-compiler-nominated

[petrochenkov]

Status update: the lint was temporarily disabled in #123744, and #123813 moves redundant import checking to a separate lint.

[RalfJung]

FWIW I was very happy to finally have rustc detect a whole bunch of unnecessary imports for me. :) But I also don't work on no_std code, let alone "conditionally no_std". I just wanted to say this to make this issue not just focus on the negative consequences of the lint expansion.