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

edition idioms lints: unused_extern_crate false positives #53964

Open
japaric opened this issue Sep 5, 2018 · 3 comments
Open

edition idioms lints: unused_extern_crate false positives #53964

japaric opened this issue Sep 5, 2018 · 3 comments
Labels
A-edition-2018 Area: The 2018 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@japaric
Copy link
Member

japaric commented Sep 5, 2018

STR

  1. Crates that provide lang items
$ cargo new --bin foo && cd $_

$ cargo add panic-abort # or any other crate only provides a `panic_handler` function

$ cat >src/main.rs <<'EOF'
#![no_main]
#![no_std]

extern crate panic_abort;
EOF

$ # ignore linking; it's not important for this example
$ # also set panic to abort to simplify things
$ mkdir .cargo
$ cat >.cargo/config <<'EOF'
[build]
rustflags = ["-C", "panic=abort", "-C", "linker=true"]
EOF

$ cargo build && echo OK
OK

$ cargo rustc -- -D rust_2018_idioms
   Compiling foo v0.1.0 (file:///home/japaric/tmp/foo)
error: unused extern crate
 --> src/main.rs:4:1
  |
4 | extern crate panic_abort;
  | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: remove it
  |
  = note: `-D unused-extern-crates` implied by `-D rust-2018-idioms`

error: aborting due to previous error

Removing the extern crate as per the suggestion breaks compilation:

$ cat src/main.rs
#![no_main]
#![no_std]

// extern crate panic_abort;
$ cargo build
   Compiling foo v0.1.0 (file:///home/japaric/tmp/foo)
error: `#[panic_handler]` function required, but not found

error: aborting due to previous error
  1. Crates that inject symbols dependencies
$ git clone https://github.com/rust-embedded/embedonomicon --branch edition-lint-false-positive-do-not-delete

$ cd embedonomicon/ci/main/app

$ cat src/main.rs
#![feature(panic_handler)]
#![no_std]
#![no_main]

extern crate rt;

use core::panic::PanicInfo;

#[no_mangle]
pub fn main() -> ! {
    let _x = 42;

    loop {}
}

#[panic_handler]
fn panic(_panic: &PanicInfo<'_>) -> ! {
    loop {}
}
$ rustup target add thumbv7m-none-eabi
$ cargo build && echo OK
OK

$ cargo rustc -- -D rust_2018_idioms
cargo rustc -- -D rust_2018_idioms
   Compiling app v0.1.0 (file:///home/japaric/tmp/embedonomicon/ci/main/app)
error: unused extern crate
 --> src/main.rs:4:1
  |
4 | extern crate rt;
  | ^^^^^^^^^^^^^^^^ help: remove it
  |
  = note: `-D unused-extern-crates` implied by `-D rust-2018-idioms`

error: aborting due to previous error

Removing the extern crate as suggested breaks linking.

$ sed -i '/extern crate/d' src/main.rs
$ cargo build
error: linking with `rust-lld` failed: exit code: 1
  |
  = note: "rust-lld" "-flavor" "gnu" (..)
  = note: rust-lld: error: malformed / incomplete vector table

The error above is a custom assertion that checks that the binary has the right memory layout. Removing extern crate rt produces an invalid (empty) binary.

Metadata

$ rustc -V
rustc 1.30.0-nightly (1c2e17f4e 2018-09-04)

(2) seems unfixable, or at least very hard to properly fix, to me since it requires knowing the dependencies between symbols and symbol information is only complete / exact after optimization, plus linker scripts can make some symbols required. A good enough fix could be to not warn about crates that expose public, reachable #[no_mangle] / #[export_name] symbols. In any case, I expect that in practice not many people will encounter this warning.

(1) should be fixable and should be fixed. The compiler should be able to determine if a crate contains lang items. This scenario I expect to be much more common since some of us, embedded devs, like to pack panic_handlers in crates as that makes it easy to switch between them.

@csmoe csmoe added the A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. label Sep 6, 2018
japaric added a commit to japaric/rust that referenced this issue Sep 6, 2018
…crates`

fixes the *first* false positive reported in rust-lang#53964
kennytm added a commit to kennytm/rust that referenced this issue Sep 8, 2018
crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint

fixes the *first* false positive reported in rust-lang#53964
kennytm added a commit to kennytm/rust that referenced this issue Sep 8, 2018
crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint

fixes the *first* false positive reported in rust-lang#53964
@alexcrichton
Copy link
Member

@japaric is this fixed now that #54007 is merged?

@japaric
Copy link
Member Author

japaric commented Sep 28, 2018

is this fixed now that #54007 is merged?

I forgot to comment but that PR only fixed this for the 2015 edition. If you use edition = "2018" then the warning appears again! I'm not sure why, though; perhaps there are two versions of the unused_extern_crates lint, one for each edition?

@alexcrichton
Copy link
Member

I think it's probably the same warning but in two modes, probably saying it's unidiomatic in the 2018 edition. That being said it's probably a simple-ish bug fix as well!

@Enselic Enselic added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Oct 15, 2023
@fmease fmease added A-edition-2018 Area: The 2018 edition and removed A-edition-2018-lints labels Dec 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2018 Area: The 2018 edition A-lints Area: Lints (warnings about flaws in source code) such as unused_mut. C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

5 participants