Skip to content

Conversation

Centri3
Copy link
Member

@Centri3 Centri3 commented May 24, 2023

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

@rustbot
Copy link
Collaborator

rustbot commented May 24, 2023

r? @dswij

(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 24, 2023
Centri3 added 2 commits May 24, 2023 11:02
I should always run cargo test before committing 😅
@asquared31415
Copy link
Contributor

asquared31415 commented May 25, 2023

I think it would be useful to have tests for getting a pointer from somewhere else, example:

let ptr: *const () = 1 as *const ();
let x = ptr as *const ();

Does this lint handle generics properly? The following code should probably lint:

fn owo<T>(ptr: *const T) {
    let x = ptr as *const T;
}

A test case casting to a different generic *const U in that same style should also be added.

A test case involving a cast like x: *const u32 x as *const u32 as *const u8 might also be useful to make sure that the suggestion works properly and only removes the first useless cast.

EDIT: Huh, I just checked again, and unnecessary_cast doesn't lint on let x: u32 = 0_u32 as _;, so the following paragraph can be disregarded.

Also why does this explicitly allow code that uses as _ when that cast is a no-op? I would expect both of these test cases to emit a lint, because the code functions the same if the as _ is removed.

    let _: *const u8 = [1u8, 2].as_ptr() as _;
    let _: *mut u8 = [1u8, 2].as_mut_ptr() as _;

@Centri3
Copy link
Member Author

Centri3 commented May 25, 2023

I think it would be useful to have tests for getting a pointer from somewhere else, example:

I'm pretty sure this would function the same as any other cast but I added one anyway

Does this lint handle generics properly? The following code should probably lint:

Yes, since it checks cast_from and cast_to's kind which afaik will ensure that type, constness and generics will all be the same. I'm not sure if this has any unintended side effects, though

@asquared31415
Copy link
Contributor

Thanks for adding those tests! This all looks good to me.

A tiny wording nitpick: I think that rustc uses "mutability" to refer to what you called "constness" (and "const" is normally things like const fn), so would a better wording be "casting a raw pointer to the same mutability and type is unnecessary"?

@Centri3
Copy link
Member Author

Centri3 commented May 25, 2023

The std library's docs of pointer::cast_mut uses constness, so I'm not sure about that. Plus there's now ptr_cast_constness, so, they're kind of interchangeable here imo

@asquared31415
Copy link
Contributor

Oh look at that, it does. I guess in the context of raw pointers specifically, that wording is used instead? Weird. Then I have absolutely no issue with that in the name of consistency.

Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! This LGTM, just a small comment on the suggestion message.

@dswij
Copy link
Member

dswij commented Jun 2, 2023

Thank you! @bors r+

@bors
Copy link
Contributor

bors commented Jun 2, 2023

📌 Commit d7a98f5 has been approved by dswij

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jun 2, 2023

⌛ Testing commit d7a98f5 with merge c85ceea...

@bors
Copy link
Contributor

bors commented Jun 2, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij
Pushing c85ceea to master...

@bors bors merged commit c85ceea into rust-lang:master Jun 2, 2023
@Centri3 Centri3 deleted the unnecessary_operation_cast_underscore branch June 4, 2023 03:24
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.

5 participants