Add new lint: `ref_as_ptr` #12087

marcin-serwin · 2024-01-03T21:20:12Z

Added new lint ref_as_ptr that checks for conversions from references to pointers and suggests using std::ptr::from_{ref, mut} instead.

The name is different than suggested in the issue (as_ptr_cast) since there were some other lints with similar names (ptr_as_ptr, borrow_as_ptr) and I wanted to follow the convention.

Note that this lint conflicts with the borrow_as_ptr lint in the sense that it recommends changing &foo as *const _ to std::ptr::from_ref(&foo) instead of std::ptr::addr_of!(foo). Personally, I think the former is more readable and, in contrast to addr_of macro, can be also applied to temporaries (cf. #9884).

changelog: New lint: [ref_as_ptr]
#12087

rustbot · 2024-01-03T21:20:17Z

r? @xFrednet

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

asquared31415 · 2024-01-09T04:04:11Z

In my opinion borrow_as_ptr isn't particularly useful anyway, I think that this lint is more useful and has better readability. The docs on borrow_as_ptr are also misleading because the code that it lints on can't cause the issues that it claims to prevent.

xFrednet · 2024-01-11T19:44:27Z

I'm currently busy with exam season. Queen @blyxyas, would you do me the honer and review this PR? :D

r? @blyxyas

rustbot · 2024-01-11T19:44:29Z

Could not assign reviewer from: blyxyas.
User(s) blyxyas are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

blyxyas · 2024-01-11T19:45:58Z

?? I'll check triagebot.toml, but AFAIK I'm still in the reviewer rotation and not on vacation (maybe a bors bug?)

xFrednet · 2024-01-11T19:49:55Z

Yeah IDK, was weird, does this work?

r? @xFrednet

xFrednet · 2024-01-11T19:50:10Z

😂

r? @blyxyas

rustbot · 2024-01-11T19:50:12Z

Could not assign reviewer from: blyxyas.
User(s) blyxyas are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

xFrednet · 2024-01-11T19:50:44Z

Lol, maybe it hasn't reloaded the configuration yet?

blyxyas · 2024-01-11T19:50:51Z

Philipp reverted my change unassigning me as on-vacation. (on a subtree sync, idk why or how)
I'll make another PR (=^w^=)

blyxyas · 2024-01-13T12:19:36Z

Hmmm... How is this different from borrow_as_ptr? The example is the same for both lints

marcin-serwin · 2024-01-13T12:28:18Z

The borrow_as_ptr is recommending the macro addr_of instead of the function from_ref (both in std::ptr).

blyxyas · 2024-01-14T01:02:24Z

Hmmm 🤔 I'm not really seeing the benefits of from_ref or addr_of over each other.
I think I'll nominate this, and let's see what the team thinks of this conundrum =^w^=

asquared31415 · 2024-01-14T01:36:01Z

from_ref directly turns a reference into its pointer, it's sugar for x as *const T where x: &T.

addr_of! gets the address of a "place" which can be many different things, such as the address of a local (addr_of!(my_owned_local)), it can go through field projections (addr_of!(my_struct.field)), and it interacts with pointer dereferences to prevent making an intermediate reference.

They are useful in slightly different situations, specifically addr_of! can technically express more, but because it can do so many things, it's more cognitive load to do addr_of!(*x) to turn a &T into a *const T than it is to do ptr::from_ref(x).

blyxyas · 2024-01-25T00:28:37Z

Okis, so the conclusions from our improvised meeting is that both lints can coexist. I'm still not to fond of this (e.g. what if someone used #[warn(clippy::all)] and --fix, or simply manually enabled them both?) but I'll review this because that's what the council decided

Meow meow, sorry for the long delay 🙏, I'll review it (at least start to do it) today

blyxyas

Very good first iteration! Following Philipp's suggestion, and because I think that this new lint is more useful than borrow_as_ptr (but they lint in identical situations), could you move that lint into restriction?

I don't remember if there's a formal process to move a lint's category (⁎˃ᆺ˂), but I've asked about it. If there isn't a necessary process we didn't do, I'll merge this.

marcin-serwin · 2024-01-26T15:42:21Z

I've changed the category of borrow_as_ptr though I have mixed feelings about that. For one thing, it's not a restriction: from the book: 'This group contains lints that "restrict" the language in some way.' and I don't think it's doing that, since it recommends using addr_of! instead of restricting as casts generally. Secondly, the from_ref functions aren't even stable yet, so we are effectively disabling linting &x as *const _ for someone using Rust before 1.76.0 unless they turn on this lint specifically.

blyxyas · 2024-01-29T22:25:30Z

Hmmm... you're right. We'll try with the second solution.
Make the check function return a boolean, and only if this check returns false (because the lint didn't lint, possibly because of MSRV), we'll then check for borrow_as_ptr.

This way, both lints can be suspicious, and we don't have to prioritize one over the other.

marcin-serwin · 2024-02-01T19:09:48Z

I've moved the msrv check higher instead of returning bool from the check function. I think it's equivalent since the ref_as_ptr should kick every time the borrow_as_ptr does (or at least that's my intention) and it's easier to read.

blyxyas

Just a little bit of polish. After this little change, could you squash the commits?

blyxyas · 2024-02-03T21:45:05Z

clippy_lints/src/casts/ref_as_ptr.rs

+            cx,
+            REF_AS_PTR,
+            expr.span,
+            "ref as raw pointer",


Suggested change

"ref as raw pointer",

"reference as raw pointer",

blyxyas · 2024-02-04T16:28:06Z

@bors r+

bors · 2024-02-04T16:28:09Z

📌 Commit 73f3fce has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-02-04T16:29:16Z

⌛ Testing commit 73f3fce with merge b151aa5...

Add new lint: `ref_as_ptr` Fixes #10130 Added new lint `ref_as_ptr` that checks for conversions from references to pointers and suggests using `std::ptr::from_{ref, mut}` instead. The name is different than suggested in the issue (`as_ptr_cast`) since there were some other lints with similar names (`ptr_as_ptr`, `borrow_as_ptr`) and I wanted to follow the convention. Note that this lint conflicts with the `borrow_as_ptr` lint in the sense that it recommends changing `&foo as *const _` to `std::ptr::from_ref(&foo)` instead of `std::ptr::addr_of!(foo)`. Personally, I think the former is more readable and, in contrast to `addr_of` macro, can be also applied to temporaries (cf. #9884). --- changelog: New lint: [`ref_as_ptr`] [#12087](#12087)

bors · 2024-02-04T16:34:15Z

💔 Test failed - checks-action_test

Author: Marcin Serwin <[email protected]>

blyxyas · 2024-02-04T16:38:24Z

@bors r+

bors · 2024-02-04T16:38:27Z

📌 Commit a3baebc has been approved by blyxyas

It is now in the queue for this repository.

bors · 2024-02-04T16:39:33Z

⌛ Testing commit a3baebc with merge 69f2dc0...

Add new lint: `ref_as_ptr` Fixes #10130 Added new lint `ref_as_ptr` that checks for conversions from references to pointers and suggests using `std::ptr::from_{ref, mut}` instead. The name is different than suggested in the issue (`as_ptr_cast`) since there were some other lints with similar names (`ptr_as_ptr`, `borrow_as_ptr`) and I wanted to follow the convention. Note that this lint conflicts with the `borrow_as_ptr` lint in the sense that it recommends changing `&foo as *const _` to `std::ptr::from_ref(&foo)` instead of `std::ptr::addr_of!(foo)`. Personally, I think the former is more readable and, in contrast to `addr_of` macro, can be also applied to temporaries (cf. #9884). --- changelog: New lint: [`ref_as_ptr`] [#12087](#12087)

bors · 2024-02-04T16:44:34Z

💔 Test failed - checks-action_test

blyxyas · 2024-02-04T16:58:12Z

@bors retry

bors · 2024-02-04T16:59:19Z

⌛ Testing commit a3baebc with merge 7066984...

Add new lint: `ref_as_ptr` Fixes #10130 Added new lint `ref_as_ptr` that checks for conversions from references to pointers and suggests using `std::ptr::from_{ref, mut}` instead. The name is different than suggested in the issue (`as_ptr_cast`) since there were some other lints with similar names (`ptr_as_ptr`, `borrow_as_ptr`) and I wanted to follow the convention. Note that this lint conflicts with the `borrow_as_ptr` lint in the sense that it recommends changing `&foo as *const _` to `std::ptr::from_ref(&foo)` instead of `std::ptr::addr_of!(foo)`. Personally, I think the former is more readable and, in contrast to `addr_of` macro, can be also applied to temporaries (cf. #9884). --- changelog: New lint: [`ref_as_ptr`] [#12087](#12087)

bors · 2024-02-04T17:04:13Z

💔 Test failed - checks-action_test

blyxyas · 2024-02-04T17:06:13Z

@bors retry

bors · 2024-02-04T17:07:20Z

⌛ Testing commit a3baebc with merge 34e4c9f...

bors · 2024-02-04T17:15:02Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 34e4c9f to master...

rustbot assigned xFrednet Jan 3, 2024

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jan 3, 2024

marcin-serwin changed the title ~~Ref as ptr cast~~ Add new lint: ref_as_ptr Jan 3, 2024

xFrednet assigned blyxyas and unassigned xFrednet Jan 11, 2024

rustbot assigned xFrednet and unassigned blyxyas Jan 11, 2024

xFrednet assigned blyxyas and unassigned xFrednet Jan 11, 2024

blyxyas added the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jan 14, 2024

blyxyas removed the I-nominated Issue: Nominated to be discussed at the next Clippy meeting label Jan 25, 2024

blyxyas requested changes Jan 26, 2024

View reviewed changes

marcin-serwin requested a review from blyxyas January 26, 2024 15:42

blyxyas approved these changes Feb 3, 2024

View reviewed changes

blyxyas force-pushed the ref_as_ptr_cast branch from e418020 to 73f3fce Compare February 4, 2024 16:27

Add ref_as_ptr lint

a3baebc

Author: Marcin Serwin <[email protected]>

blyxyas force-pushed the ref_as_ptr_cast branch from 73f3fce to a3baebc Compare February 4, 2024 16:38

bors merged commit 34e4c9f into rust-lang:master Feb 4, 2024
8 checks passed

bors mentioned this pull request Feb 4, 2024

Issue 9231: add cast_integer lint #11955

Closed

Add new lint: ref_as_ptr #12087

Add new lint: ref_as_ptr #12087

Conversation

marcin-serwin commented Jan 3, 2024 • edited by xFrednet Loading

rustbot commented Jan 3, 2024

asquared31415 commented Jan 9, 2024

xFrednet commented Jan 11, 2024

rustbot commented Jan 11, 2024

blyxyas commented Jan 11, 2024

xFrednet commented Jan 11, 2024

xFrednet commented Jan 11, 2024

rustbot commented Jan 11, 2024

xFrednet commented Jan 11, 2024

blyxyas commented Jan 11, 2024 • edited Loading

blyxyas commented Jan 13, 2024

marcin-serwin commented Jan 13, 2024 • edited Loading

blyxyas commented Jan 14, 2024

asquared31415 commented Jan 14, 2024

blyxyas commented Jan 25, 2024

blyxyas left a comment

Choose a reason for hiding this comment

marcin-serwin commented Jan 26, 2024 • edited Loading

blyxyas commented Jan 29, 2024

marcin-serwin commented Feb 1, 2024 • edited Loading

blyxyas left a comment

Choose a reason for hiding this comment

blyxyas Feb 3, 2024

Choose a reason for hiding this comment

blyxyas commented Feb 4, 2024

bors commented Feb 4, 2024

bors commented Feb 4, 2024

bors commented Feb 4, 2024

blyxyas commented Feb 4, 2024

bors commented Feb 4, 2024

bors commented Feb 4, 2024

bors commented Feb 4, 2024

blyxyas commented Feb 4, 2024

bors commented Feb 4, 2024

bors commented Feb 4, 2024

blyxyas commented Feb 4, 2024

bors commented Feb 4, 2024

bors commented Feb 4, 2024

Add new lint: `ref_as_ptr` #12087

Add new lint: `ref_as_ptr` #12087

marcin-serwin commented Jan 3, 2024 •

edited by xFrednet

Loading

blyxyas commented Jan 11, 2024 •

edited

Loading

marcin-serwin commented Jan 13, 2024 •

edited

Loading

marcin-serwin commented Jan 26, 2024 •

edited

Loading

marcin-serwin commented Feb 1, 2024 •

edited

Loading