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

Have a lint against usize-to-u64 casts (or, against *all* integer casts) #9231

Open
RalfJung opened this issue Jul 23, 2022 · 15 comments · Fixed by #10038 · May be fixed by #11955
Open

Have a lint against usize-to-u64 casts (or, against *all* integer casts) #9231

RalfJung opened this issue Jul 23, 2022 · 15 comments · Fixed by #10038 · May be fixed by #11955
Assignees
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 23, 2022

What it does

I would like clippy to lint against all integer casts. So I have set:

#![warn(
    clippy::cast_possible_wrap, // unsigned -> signed
    clippy::cast_sign_loss, // signed -> unsigned
    clippy::cast_lossless,
    clippy::cast_possible_truncation,
)]

However, I just by accident noticed that this does not lint against usize-to-u64 casts. I guess cast_possible_truncation says "this cannot truncate because we don't have more than 64bit pointer size", and "cast_lossless" says "ah this might be lossy on platforms with pointers larger than 64bit", and then neither of them does anything.

I would be happy to either have one of these lints also trigger on usize-to-u64 casts, or to have a new lint against all integer casts.

Lint Name

cast_integer

Category

pedantic

Advantage

Integer casts are subtle and should be done via From/TryFrom, never via as, so I want to rule out all of them in my codebase.

Drawbacks

No response

Example

pub fn foo(x: usize) -> u64 { x as u64 }

Could be written as:

pub fn foo(x: usize) -> u64 { u64::try_from(x).unwrap() }
@RalfJung RalfJung added the A-lint Area: New lints label Jul 23, 2022
@Alexendoo Alexendoo added the good-first-issue These issues are a good way to get started with Clippy label Jul 23, 2022
@jakubkosno
Copy link

@rustbot claim

navh pushed a commit to navh/rust-clippy that referenced this issue Oct 17, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 20, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 20, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
navh pushed a commit to navh/rust-clippy that referenced this issue Oct 21, 2022
@bors bors closed this as completed in 483b7ac Jan 14, 2023
@RalfJung
Copy link
Member Author

RalfJung commented Jan 25, 2023

Can someone please reopen? The bug is not fixed (tested on playground).

@xFrednet xFrednet reopened this Jan 25, 2023
@xFrednet
Copy link
Member

Yup, sorry there has been a misunderstanding, which is why it was closed.

@ctaymor
Copy link

ctaymor commented Feb 8, 2023

@xFrednet I was thinking about claiming this as a first RustClippy issue. it looks like the misunderstanding was whether PR #10038 fixed this issue, and that it actually doesn't. Am I correctly understanding? If so, if this one open to claim?

@ctaymor
Copy link

ctaymor commented Feb 8, 2023

@rustbot claim

@xFrednet
Copy link
Member

xFrednet commented Feb 8, 2023

Hey @ctaymor , welcome to Clippy! You're correct, #10038 updated the output a bit to be cleaner, but didn't address the issue. You're welcome to take it :)

@gernot-ohner
Copy link

Hey @xFrednet, I believe there has been no active development on this issue for the last couple of months, right?
Could I claim this?

Do I understand correctly that the approach would be to create a new lint that warns on integer casts using as?

@xFrednet
Copy link
Member

Hey @gernot-ohner, you should be able to claim this, since there hasn't been any activity to my knowledge.

For the correct approach, it depends a bit on how @RalfJung would like to use the new lint. My understanding, is that they want to restrict all integer casts. Looking at our lint list I noticed cast_lossless which sounds like a lint, that should trigger on this. But it doesn't (See Playground) and it's also interesting, that it doesn't even trigger on the example from the lint description.

So, I think this sounds like a false positive. It probably makes sense to first look at the code of that lint, and see why it doesn't trigger on the example from the lint description and the example from this issue description.

@gernot-ohner
Copy link

@rustbot claim

@rustbot rustbot assigned gernot-ohner and unassigned ctaymor Nov 13, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Nov 13, 2023

cast_lossless shouldn't trigger on usizeu64 casts as there is no corresponding Into implementation. This means there isn't an alternative other than try_into, which is not a lossless conversion.

cast_possible_truncation ignores the case because there aren't any platforms where usize is larger than u64, so triggering on those cases just ends up being noise.

@xFrednet
Copy link
Member

@Jarcho Thank you for the input. So, would you recommend adding a new lint?

@Jarcho
Copy link
Contributor

Jarcho commented Nov 13, 2023

I think a restriction lint for all integer casts would be the best way. It feels strange to make a lint for only the one cast and the catch-all lint could be made to not trigger when any of the other related lints do in order to avoid the duplicate messages.

We could also add the case to cast_possible_truncation since it technically fits there even though it's unlikely to ever truncate.

@RalfJung
Copy link
Member Author

RalfJung commented Nov 13, 2023

So cast_lossless doesn't warn since the cast is not lossless, and cast_possible_truncation doesn't warn since the cast cannot truncate (i.e., it is lossless)? That does not sound very consistent. ;)

@Jarcho
Copy link
Contributor

Jarcho commented Nov 13, 2023

If the lang team wants to limit the maximum size of usize to 64 bits then the cast would really be lossless for reals. It would be nice to be able to at least opt-in to that constraint since anything using an as cast there is already making that assumption.

Bonus points in the far off future when someone who hopefully isn't me gets to curse the idiots who couldn't conceive of the need for more 64 bits worth of byte-addressable storage.

@asquared31415
Copy link
Contributor

If we assume that we keep adding more larger usize, usize as N could possibly truncate for any integer type N. It's not clear how useful cast_possible_truncation firing on every case would be, but also I expect there to be few places using usize as N.

Having an integer type with no static upper bound means that reasoning about it for conversions is difficult.

@gernot-ohner gernot-ohner linked a pull request Dec 12, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints good-first-issue These issues are a good way to get started with Clippy
Projects
None yet
8 participants