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

Lint for bool to integer casts in cast_lossless #7948

Merged
merged 4 commits into from
Nov 12, 2021

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Nov 8, 2021

The lint description says

Checks for casts between numerical types that may be replaced by safe conversion functions.

Which is strictly speaking being violated here, but it seems within the spirit of the lint. I think it is still a useful lint to have, and having a different lint for just this feels excessive. Thoughts?

Fixes #7947

changelog: Lint for bool to integer casts in [cast_lossless]

@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @llogiq (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 8, 2021
@5225225
Copy link
Contributor Author

5225225 commented Nov 9, 2021

Just realised this lint doesn't seem to check for the MSRV. Looks like all the From impls for bools were added in 1.28. (And the ones for integers are 1.5/1.6, old enough to not care about I suppose)

Worth adding a MSRV check for the bools?

@llogiq
Copy link
Contributor

llogiq commented Nov 9, 2021

The linting part looks really thoughtfully done. Great job! However, I think the lint message doesn't exactly fit the cast. I doubt we ever get to the point where we get a u0 type that cannot be created from a bool.

So I think we should change the message for bool → int casts.

@5225225
Copy link
Contributor Author

5225225 commented Nov 9, 2021

True, but then again, I can't really think of a better message. It can become silently lossy if you change the type (of the bool). But yeah, you're right, there's no possible change of the right hand side which would make it invalid.

error: casting `bool` to `i64` may become silently lossy if you later change the bool's type
error: casting `bool` to `i64` may become silently lossy if you later change the bool
error: casting `bool` to `i64` may become silently lossy if you later change the type of the bool

perhaps?

None of those seem obviously better than just leaving the message as is to me.

@llogiq
Copy link
Contributor

llogiq commented Nov 9, 2021

I'm currently short on time, but I'll think of something.

@llogiq
Copy link
Contributor

llogiq commented Nov 12, 2021

I don't think it's very likely that someone changes a value from bool to some integer type while forgetting some as conversions. How about

error: Casting `bool` to `u64` is more cleanly stated with `i64::from(_)`.

@5225225
Copy link
Contributor Author

5225225 commented Nov 12, 2021

Okay, that's changed.

I used a literal _ in the message rather than actually showing the real expression since we show the real expression in the suggestion anyways.

@llogiq
Copy link
Contributor

llogiq commented Nov 12, 2021

That's totally OK. No need to duplicate the suggestion. Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2021

📌 Commit d4c8cb6 has been approved by llogiq

@bors
Copy link
Contributor

bors commented Nov 12, 2021

⌛ Testing commit d4c8cb6 with merge 4f82dd8...

@bors
Copy link
Contributor

bors commented Nov 12, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: llogiq
Pushing 4f82dd8 to master...

@bors bors merged commit 4f82dd8 into rust-lang:master Nov 12, 2021
@5225225 5225225 deleted the castlosslessbool branch November 12, 2021 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cast_lossless should lint about bool -> integer casts?
4 participants