cast_lossless: lint when converting usize, isize, char and float as well

[lapla-cogito]

close #14469

changelog: [case_lossless]: lint when converting usize, isize, char and float as well

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

Can you check that the following doesn't lint?

macro_rules! mac {
    (to_i32 $x:expr) => { $x as u32 };
    (zero_as $t:ty) => { 0u8 as $t };
    ($x:expr => $t:ty) => { $x as $t };
    ($x:expr, $k:ident, $t:ty) => { $x $k $t };
}
let _ = mac!(to_i32 0u16);
let _ = mac!(zero_as u32);
let _ = mac!(0u8 => u32);
let _ = mac!(0u8, as, u32);

You might want to check from the beginning that expr, cast_from_expr and cast_to_hir come from the same context. Also, the check || !cast_to_hir.span.eq_ctxt(expr.span) is misplaced and should be removed (even though it was not added in this patch), and it will become moot anyway.

This PR will cause many new (justified) warnings, especially because of usize handling, so we need to be extra meticulous there.

Edit: in addition it may be even safer not to lint in macros at all, i.e. if the expression context is not the root context

[samueltardieu]

Also, there is a typo in the second commit message (case_lossless instead of cast_lossless).

[tamird]

what happened here?

[samueltardieu]

Probably a side effect of not wanting to lint ($x:expr, $k:ident, $t:ty) => { $x $k $t }; when called with mac!(1u8, as, u32). Maybe we could still lint when all three parts come from the same context.

[lapla-cogito]

Ah, I was late in replying. For this patch, I want to essentially avoid linting macros. This is because linting macros triggers a lot of cases in lintcheck, so for safety, I’d like to hold off on linting macros for now and see how things go. WDYT?

[samueltardieu]

Could you add an extra commit which removes the !expr.span.ctxt().is_root() and only ensures that the expression, its argument being cast and its type are all from the same context, and push this? This will give us a lintcheck run that we can talk about, and then either squash or remove this commit.

[tamird]

It would probably be good to add a code comment to explain the new context check, and perhaps mention it in the commit message as well since it is an additional behavior change.

[lapla-cogito]

Regardless of which approach is chosen, it will likely be squashed anyway, so I was planning to handle that at that time.

Since the lintcheck results visible on GitHub are limited to 100 entries, I uploaded the full results of both lintchecks to a Gist, with each one as a separate link due to their length. I'll take a closer look at these.

before: https://gist.github.com/lapla-cogito/1e569c70a9456acee36795fb0329d1d0
after: https://gist.github.com/lapla-cogito/cd56f0ab8c2d64d9a4d5fb42977aa4e1

[samueltardieu]

Most of them look fine, but this change in num-traits is surprising and probably should not be linted:

-warning: casts from `i8` to `i16` can be expressed infallibly using `From`
+warning: casts from `i8` to `isize` can be expressed infallibly using `From`
    --> target/lintcheck/sources/num-traits-0.2.19/src/cast.rs:133:23
     |
 133 |             let min = $DstT::MIN as $SrcT;
     |                       ^^^^^^^^^^^^^^^^^^^
 ...
-194 | impl_to_primitive_int!(i16);
-    | --------------------------- in this macro invocation
+192 | impl_to_primitive_int!(isize);
+    | ----------------------------- in this macro invocation
     |
     = help: an `as` cast can become silently lossy if the types change in the future
+    = note: `--force-warn clippy::cast-lossless` implied by `--force-warn clippy::pedantic`
     = note: this warning originates in the macro `impl_to_primitive_int_to_int` which comes from the expansion of the macro `impl_to_primitive_int` (in Nightly builds, run with -Z macro-backtrace for more info)
 help: use `$SrcT::from` instead
     |
 133 -             let min = $DstT::MIN as $SrcT;
 133 +             let min = $SrcT::from($DstT::MIN);
     |