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

unused_imports shouldn't fire for alloc types included in the std prelude #121362

Closed
c410-f3r opened this issue Feb 20, 2024 · 15 comments
Closed
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut.

Comments

@c410-f3r
Copy link
Contributor

c410-f3r commented Feb 20, 2024

When working with libraries that can or can not be #[no_std], it is common to simply extern crate alloc; and then manually import types like alloc::vec::Vec. In this way everything works regardless of the configuration.

However, the latest versions of the compiler are warning such approach.

warning: the item Vec is imported redundantly
--> foo.rs:96:7
|
96 | use alloc::vec::Vec;
| ^^^^^^^^^^^^^^^
|
::: $HOME/.rustup/toolchains/nightly-2024-02-19-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/prelude/mod.rs:125:13
|
125 | pub use super::v1::*;
| --------- the item Vec is already defined here

In the above example, if use alloc::vec::Vec; is removed #[no_std] breaks. If not removed, the warning persists 😑

Maybe related: #121312 #121331 #121330 #120422

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 20, 2024
@kadiwa4
Copy link
Contributor

kadiwa4 commented Feb 20, 2024

Oftentimes, conditional no_std is done like this:

#![cfg_attr(not(feature = "std"), no_std)]

But if I understood your issue correctly, you can work around the warnings by instead doing:

#![no_std]

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

Which consistently results in the std prelude not being used.

@c410-f3r
Copy link
Contributor Author

#![no_std]

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

Lol, what sorcery is this?

Is extern crate std canceling #[no_std] if std is activated?

@ChrisDenton
Copy link
Member

#[no_std] prevents automatically linking in std. It doesn't stop you from doing so manually, which is what extern crate std does.

@MaxGraey
Copy link

#[no_std] prevents automatically linking in std

As I understand, no_std doing much more according to rust-embedded book:

@mumbleskates
Copy link
Contributor

mumbleskates commented Feb 23, 2024

@MaxGraey Heap and collections are available if you have access to alloc, even in no_std. Availability of std in a no_std library is similarly possible; you just need to add it with extern crate std;.

"Stack overflow protection" and "runs init code before main" are properties of the whole binary and its main entrypoint, not of your specific library.

I believe it is actually better, and not less capable or meaningfully different, to write no_std-compatible libraries to be unconditionally #[no_std], then include extern crate std; by feature. This way the prelude will always be the same and these warnings will not fire because imports aren't redundant even when std support is enabled.

I can't judge whether the warnings are a regression, though.

@MaxGraey
Copy link

I still can't figure out how to implement graceful switching between "std" and "core" when "std" is optional for a working crate.

Previous solution was:

#![cfg_attr(not(feature = "std"), no_std)]

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

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

If replace #![cfg_attr(not(feature = "std"), no_std)] to ![no_std] then you will need to use std::prelude::rust_2021::* explicitly, especially for macros like dbg!(...). All of this becomes difficult especially since std now overlaps with the core alias

@mumbleskates
Copy link
Contributor

You shouldn't be using the std prelude in no_std, nor should you import std with the name alloc. Only import the features that only exist in std from that crate; import everything else directly from core or alloc.

You will have to write extra imports for things that are missing from the core prelude, like dbg!. In that particular case, because dbg! doesn't exist outside std, you could write an alternative that does nothing:

#[cfg(feature = "std")]
use std::dbg;
#[cfg(not(feature = "std"))]
use fallback_dbg as dbg;

#[allow(unused_macros)]
macro_rules! fallback_dbg {
    ($x:expr) => { $x };
};

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Feb 26, 2024

Maybe #116033 introduced this change?

The lint is acting as intended and @kadiwa4 provided an elegant workaround for library authors, as such, I will close this issue. If desired, feel free to open another issue to continue discussing the matter.

@fmease fmease removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 29, 2024
@Systemcluster
Copy link

Ungated imports from alloc are incredibly common in existing code: https://github.com/search?q=%22use+alloc%3A%3A%22+language%3ARust&type=code

The change to this lint will result in a huge amount of new warnings across countless projects.

@kadiwa4
Copy link
Contributor

kadiwa4 commented Mar 1, 2024

@Systemcluster The problem was the conditional usage of the std prelude; that doesn't intrinsically have much to do with the alloc crate.

For example the linux repository – which was the first result in your search for me – will not run into this issue.

But you're still right in that this issue will probably occur in a lot of crates: github search

@petrochenkov
Copy link
Contributor

cc #121708

@elagergren-spideroak
Copy link

elagergren-spideroak commented Mar 13, 2024

The lint is acting as intended and @kadiwa4 provided an elegant workaround for library authors, as such, I will close this issue. If desired, feel free to open another issue to continue discussing the matter.

This issue needs to be reopened. Up until @kadiwa4 updated the docs last week, the canonical guidance was:

// In lib.rs

#![cfg_attr(not(feature = "std"), no_std)]

The new guidance isn't a drop-in replacement. For example, it causes this code (which with the old cfg_attr approach compiled just fine!) to not compile:

#![no_std]

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

#[cfg(feature = "std")]
mod foo {
    fn foo() -> Box<()> {
        Box::new(())
    }
}

Almost all of my crates are no_std with opt-in std. This is an incredibly disruptive change.

@mumbleskates
Copy link
Contributor

in no_std, Box is not defined or imported in the prelude.

#![no_std]

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

#[cfg(feature = "std")]
mod foo {
    use std::boxed::Box;

    fn foo() -> Box<()> {
        Box::new(())
    }
}

alternatively, if you rely on alloc even in the absence of std, you can unconditionally use alloc::boxed::Box;

@elagergren-spideroak
Copy link

in no_std, Box is not defined or imported in the prelude.

I understand this. It's why the updated 'conditional no_std' advice is problematic wrt this lint change: it's not a drop-in replacement and requires a lot of code changes.

alternatively, if you rely on alloc even in the absence of std, you can unconditionally use alloc::boxed::Box;

That doesn't change existing conditional std code which relies on the prelude.

@mumbleskates
Copy link
Contributor

it's not a drop-in replacement and requires a lot of code changes

i don't think it was meant to be a drop-in replacement. definitely agree that the rapid change of guidance from #![cfg(not(feature = "std"), no_std)] to #![no_std] is maybe too precipitous, but i do also believe that this is probably strictly better overall. updating my own libraries to be purely no_std and explicitly import everything they were using does feel cleaner, to me.

also, the code i showed that fixes your example only adds 1 line. i feel that is maybe not so bad.

i guess you can always suppress the lint until and unless you want to change your conditional 🤷‍♀️

rbradford added a commit to rbradford/acpi_tables that referenced this issue Apr 8, 2024
See - rust-lang/rust#121362 the recommendation
for how for no_std libraries has changed.

To avoid warnings around unncessary imports (due to the prelude when
compiling tests) make the tests also no_std and explicitly import the
require test dependencies.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
rbradford added a commit to rbradford/acpi_tables that referenced this issue Apr 8, 2024
See - rust-lang/rust#121362 the recommendation
for how for no_std libraries has changed.

To avoid warnings around unncessary imports (due to the prelude when
compiling tests) make the tests also no_std and explicitly import the
require test dependencies.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
rbradford added a commit to rust-vmm/acpi_tables that referenced this issue Apr 8, 2024
See - rust-lang/rust#121362 the recommendation
for how for no_std libraries has changed.

To avoid warnings around unncessary imports (due to the prelude when
compiling tests) make the tests also no_std and explicitly import the
require test dependencies.

Signed-off-by: Rob Bradford <rbradford@rivosinc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: Lints (warnings about flaws in source code) such as unused_mut.
Projects
None yet
Development

No branches or pull requests