Check casting function item to usize

[tesuji]

What it does

Check a mistaken casting function item to usize or all other integer primitive types.

Categories (optional)

• Kind: correctness or pedantic

What is the advantage of the recommended code over the original code

• It is a mistake to cast a function item to usize instead of getting function result.
  This could be unnotice for a long time.
• If you want to get pointer address of a function, explicitly cast it to function pointer and usize.
• Explicit is better than implicit.

The bug appeared in real code: rust-lang/rustup@5d9d980
I was author of the bug :) .

Drawbacks

It is verbose and somewhat redundant.

Example

use std::mem::size_of;

extern "C" {
    fn foo(len: usize);
}

unsafe {
    foo(size_of::<usize> as usize);
}

Could be written as:

unsafe {
    foo(size_of::<usize>());
    // or
    foo(size_of::<usize> as fn() -> usize as usize);
    // or
    foo(size_of::<usize> as *const () as usize);
}

[Coder-256]

Could I give this a shot?

[flip1995]

Yeah sure, go ahead! Make sure to read the Basics documentation to get up and running. If you have any questions, feel free to ask here or open a WIP PR.

[Coder-256]

Great, thank you! I asked this question on Discord but I'm not sure that was the right place to ask:

I am using the master channel (following the Basics documentation) but it seems that the fmt test fails and complains about a compiler mismatch because it tries to use nightly? What's the best way to fix this?

[flip1995]

cargo dev fmt should use the rustfmt from nightly automatically. But you need an install of the nightly toolchain with the rustfmt component. rustup toolchain install nightly --profile=default will do that.

If you want to install rustfmt for the master toolchain, you can add -c rustfmt to the RTIM command:

rustup-toolchain-install-master -f -n master -c rustc-dev -c llvm-tools -c rustfmt [-c rust-src]

[Coder-256]

That's basically what I did. I had installed the master toolchain with:

rustup-toolchain-install-master -f -n master -c rustc-dev -c llvm-tools -c rustfmt -c rust-src

And I already have rustfmt installed for the nightly toolchain. Here is the error I get from cargo test:

running 1 test
test fmt ... FAILED

failures:

---- fmt stdout ----
status: exit code: 101
stdout: 
stderr:    Compiling memchr v2.3.3
   Compiling libc v0.2.74
   Compiling bitflags v1.2.1
   Compiling lazy_static v1.4.0
   Compiling unicode-width v0.1.8
   Compiling vec_map v0.8.2
   Compiling same-file v1.0.6
   Compiling regex-syntax v0.6.18
   Compiling strsim v0.8.0
   Compiling either v1.6.0
   Compiling ansi_term v0.11.0
   Compiling shell-escape v0.1.5
   Compiling bytecount v0.6.0
   Compiling textwrap v0.11.0
   Compiling thread_local v1.0.1
   Compiling walkdir v2.3.1
   Compiling itertools v0.9.0
error[E0514]: found crate `unicode_width` compiled by an incompatible version of rustc
  --> /Users/jacobgreenfield/.cargo/registry/src/gb.xjqchip.workers.dev-1ecc6299db9ec823/textwrap-0.11.0/src/lib.rs:52:1
   |
52 | extern crate unicode_width;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: please recompile that crate using this compiler (rustc 1.47.0-nightly (9b88e0a86 2020-08-15))
   = note: the following crate versions were found:
           crate `unicode_width` compiled by rustc 1.47.0-nightly (009551f75 2020-08-16): /Users/jacobgreenfield/git/rust-clippy/target/debug/deps/libunicode_width-98c510447f4b58b8.rmeta

error: aborting due to previous error

error: could not compile `textwrap`.

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error[E0514]: found crate `lazy_static` compiled by an incompatible version of rustc
  --> /Users/jacobgreenfield/.cargo/registry/src/gb.xjqchip.workers.dev-1ecc6299db9ec823/thread_local-1.0.1/src/lib.rs:74:1
   |
74 | extern crate lazy_static;
   | ^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: please recompile that crate using this compiler (rustc 1.47.0-nightly (9b88e0a86 2020-08-15))
   = note: the following crate versions were found:
           crate `lazy_static` compiled by rustc 1.47.0-nightly (009551f75 2020-08-16): /Users/jacobgreenfield/git/rust-clippy/target/debug/deps/liblazy_static-3ac5fd0e5fd029c7.rmeta

error: aborting due to previous error

error[E0514]: found crate `same_file` compiled by an incompatible version of rustc
   --> /Users/jacobgreenfield/.cargo/registry/src/gb.xjqchip.workers.dev-1ecc6299db9ec823/walkdir-2.3.1/src/lib.rs:120:5
    |
120 | use same_file::Handle;
    |     ^^^^^^^^^
    |
    = help: please recompile that crate using this compiler (rustc 1.47.0-nightly (9b88e0a86 2020-08-15))
    = note: the following crate versions were found:
            crate `same_file` compiled by rustc 1.47.0-nightly (009551f75 2020-08-16): /Users/jacobgreenfield/git/rust-clippy/target/debug/deps/libsame_file-309a10f48ac896b1.rmeta

error: aborting due to previous error

error[E0514]: found crate `either` compiled by an incompatible version of rustc
  --> /Users/jacobgreenfield/.cargo/registry/src/gb.xjqchip.workers.dev-1ecc6299db9ec823/itertools-0.9.0/src/lib.rs:53:9
   |
53 | pub use either::Either;
   |         ^^^^^^
   |
   = help: please recompile that crate using this compiler (rustc 1.47.0-nightly (9b88e0a86 2020-08-15))
   = note: the following crate versions were found:
           crate `either` compiled by rustc 1.47.0-nightly (009551f75 2020-08-16): /Users/jacobgreenfield/git/rust-clippy/target/debug/deps/libeither-13db02ffb53e76c7.rmeta

error: aborting due to previous error

error: build failed

thread 'fmt' panicked at 'Formatting check failed. Run `cargo dev fmt` to update formatting.', tests/fmt.rs:35:5
stack backtrace:
   0: std::panicking::begin_panic
             at /Users/jacobgreenfield/.rustup/toolchains/master/lib/rustlib/src/rust/library/std/src/panicking.rs:497
   1: fmt::fmt
             at ./tests/fmt.rs:35
   2: fmt::fmt::{{closure}}
             at ./tests/fmt.rs:5
   3: core::ops::function::FnOnce::call_once
             at /Users/jacobgreenfield/.rustup/toolchains/master/lib/rustlib/src/rust/library/core/src/ops/function.rs:233
   4: core::ops::function::FnOnce::call_once
             at /rustc/009551f758d1d007ad0f7b652bfa8ddba0738117/library/core/src/ops/function.rs:233
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.


failures:
    fmt

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out

error: test failed, to rerun pass '--test fmt'

$ rustc +nightly -V
rustc 1.47.0-nightly (9b88e0a86 2020-08-15)
$ rustc +master -V
rustc 1.47.0-nightly (009551f75 2020-08-16)

How should I resolve this?

[flip1995]

Does cargo clean help and recompiling it? I never saw this happening.

[Coder-256]

No, and neither does rm -rf target.

[flip1995]

Do you have a clippy_dev/target directory? If so try to delete it and run the test again.

These are just wild guesses by me, since I can't reproduce this locally.

[Coder-256]

Strangely enough, yes (no idea why), but removing it did not fix the issue.

[flip1995]

Does cargo dev fmt work?

[Coder-256]

Yes, it does. And strangely enough I got the test to pass like this:

diff --git a/tests/fmt.rs b/tests/fmt.rs
index 3aff8741f..9cf4df87d 100644
--- a/tests/fmt.rs
+++ b/tests/fmt.rs
@@ -8,15 +8,16 @@ fn fmt() {
     }
 
     // Skip this test if rustup nightly is unavailable
-    let rustup_output = Command::new("rustup")
-        .args(&["component", "list", "--toolchain", "nightly"])
-        .output()
-        .unwrap();
-    assert!(rustup_output.status.success());
-    let component_output = String::from_utf8_lossy(&rustup_output.stdout);
-    if !component_output.contains("rustfmt") {
-        return;
-    }
+    // let rustup_output = Command::new("rustup")
+    //     .args(&["component", "list", "--toolchain", "nightly"])
+    //     .output()
+    //     .unwrap();
+    // println!("*** OUT: {:?}", rustup_output);
+    // assert!(rustup_output.status.success());
+    // let component_output = String::from_utf8_lossy(&rustup_output.stdout);
+    // if !component_output.contains("rustfmt") {
+    //     return;
+    // }
 
     let root_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
     let dev_dir = root_dir.join("clippy_dev");
@@ -24,7 +25,7 @@ fn fmt() {
     let target_dir = target_dir.to_str().unwrap();
     let output = Command::new("cargo")
         .current_dir(dev_dir)
-        .args(&["+nightly", "run", "--target-dir", target_dir, "--", "fmt", "--check"])
+        .args(&["+master", "run", "--target-dir", target_dir, "--", "fmt", "--check"])
         .output()
         .unwrap();

[Coder-256]

I think I'm just going to stick to cargo uitest for now and let CI do the full tests. Thank you for helping.

[flip1995]

Yeah, you can ignore the fmt test locally, as long as you can run cargo dev fmt (After this command the fmt test is guaranteed to pass).

[Coder-256]

Alright. Anyway I just noticed the existing fn_to_numeric_cast lint, which "Checks for casts of function pointers to something other than usize".

Maybe it should apply to usize too?

[Coder-256]

I'm starting to wonder if this type of cast is intentionally supported. It is even mentioned in the Rust reference. Maybe that is because it was implemented before std::ffi::c_void?

[tesuji]

Maybe missing a row about casting function item to function pointer and/or usize address.
Opened rust-lang/reference#878 for discussing.

[Coder-256]

@lzutao I think maybe it actually is covered? Here are the relevant lines:

Type of e	U	Cast performed by e as U
Function pointer	*V where V: Sized	Function pointer to pointer cast
Function pointer	Integer	Function pointer to address cast

Which lines do you think are missing? Just function definition --> function pointer?

[tesuji]

It is not that. Those are 2 types: function item != function pointer.

[Coder-256]

Ok, I agree that should be added.

[tesuji]

Back to your question

this type of cast is intentionally supported

Maybe, but it causes real bug in practice. This kind of bug (typo) is hard to catch by eyes.

[Coder-256]

@lzutao I completely agree, and I'm working on a PR that implements this lint. I think that this syntax was meant for function pointers, not function items, so I'm planning to lint only against function items.

[Coder-256]

I opened #5914 last night (at first I linked to the wrong issue by mistake).

[camsteffen]

@Coder-256 @flip1995 Is this resolved by #7705?

[flip1995]

Yes, I think this should cover it. I think #5914 can now also be closed, after it was abandoned by me for so long (sorry...)