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

Add new lint as_ptr_cast_underscore #10782

Closed
wants to merge 4 commits into from

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented May 15, 2023

This will lint when the result of as *<constness> _ is both the same type and mutability, as it is an unnecessary operation.

Fixes #10587

changelog: new lint [as_ptr_cast_underscore]

@rustbot
Copy link
Collaborator

rustbot commented May 15, 2023

r? @giraffate

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 15, 2023
@Alexendoo
Copy link
Member

This feels like it should be rolled into unnecessary_cast rather than a separate lint

@Centri3
Copy link
Member Author

Centri3 commented May 15, 2023

This feels like it should be rolled into unnecessary_cast rather than a separate lint

Potentially, though unnecessary_cast currently checks only primitive types and not raw pointers, at all. Just stuff like 1u32 as u32, not x.as_ptr() as *const _. This pattern is so common anyway that it feels like having this warn by default doesn't feel right since many use this to signal "I don't care about the resulting type, just make it work), and it currently doesn't give a suggested fix, but that should be easy to implement

e: unnecessary_cast also checks for casts where the writer explicitly declared the type as well, this checks for both where the type was given and is inferred.

@asquared31415
Copy link
Contributor

My lint #10567 checks for all uses of as *const _ (or *mut) but it's currently a restrict lint for similar reasons that you found, the code pattern is so common, especially in consts and with integers cast to pointers. It would entirely cover this lint by being more general.

Because this case is specifically for not changing the types, it is likely more suspicious, liable for the types to change due to type inference, and especially with pointers, that's probably not desirable, so I think that it should be something that's warn by default, but I understand that it may be very common.

I think that it would be useful for unnecessary_cast to be expanded to contain this lint, since it's a no-op cast and start handling as _ and perhaps have a message when the target type is _ that "the type was inferred to be X, but _ is fragile and this may silently change"

There's some times when I think that we should consider moving the restrict lint for "use of as at all" to a warn by default, since as has so many of these suspicious behaviors. 😆

@Centri3
Copy link
Member Author

Centri3 commented May 19, 2023

I think that it would be useful for unnecessary_cast to be expanded to contain this lint

If this is the general consensus, then I'm open to doing this if an accurate suggestion could be added (which one definitely could), so running cargo fix would change it automatically. In that case warn-by-default would be fine.

start handling as _ and perhaps have a message when the target type is _ that "the type was inferred to be X, but _ is fragile and this may silently change"

There's actually already as_underscore which does exactly this
I feel like this should be warn-by-default. Even though it's "less wordy".

@asquared31415
Copy link
Contributor

Sorry for not being clear, I specifically mean that unnecessary_cast should have a case for as _ when _ is inferred to be the same type as the left hand side of the as cast. unnecessary_cast doesn't appear to fire on code like let x: i32 = 0_i32 as _ or other cases where inference picks the same type for _, despite the cast being entirely redundant.

as_underscore would still be useful for cases when the _ inferred type is not the same, and probably would need to be kept in restrict. Unfortunately last I remember, as_underscore is far too common of a code pattern and feedback will be very negative from people denying clippy warnings in CI (even though we tell people not to do that).

@bors
Copy link
Contributor

bors commented May 23, 2023

☔ The latest upstream changes (presumably #10810) made this pull request unmergeable. Please resolve the merge conflicts.

@Centri3
Copy link
Member Author

Centri3 commented May 24, 2023

I'll close this and instead add this to unnecessary_cast at some point.

@Centri3 Centri3 closed this May 24, 2023
bors added a commit that referenced this pull request Jun 2, 2023
… r=dswij

Emit `unnecessary_cast` on raw pointers as well

Supersedes(?) #10782, since this and #10567 will cover the original issue.
Does not lint on type aliases or inferred types.

changelog: [`unnecessary_cast`]: Also emit on casts between raw pointers with the same type and constness
@Centri3 Centri3 deleted the pointer_as_underscore branch June 11, 2023 13:42
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.

New Lint: pointer_as_const_underscore
6 participants