Implement a lint for implicit autoref of raw pointer dereference - take 2 #123239

Urgau · 2024-03-30T16:28:03Z

This PR aims at implementing a lint for implicit autoref of raw pointer dereference, it is based on #103735 with suggestion and improvements from #103735 (comment).

The goal is to catch cases like this, where the user probably doesn't realise it just created a reference.

pub struct Test {
    data: [u8],
}

pub fn test_len(t: *const Test) -> usize {
    unsafe { (*t).data.len() }  // this calls <[T]>::len(&self)
}

Since #103735 already went 2 times through T-lang, where they T-lang ended-up asking for a more restricted version (which is what this PR does), I would prefer this PR to be reviewed first before re-nominating it for T-lang.

Compared to the PR it is as based on, this PR adds 3 restrictions on the outer most expression, which must either be:

A deref followed by any non-deref place projection (that intermediate deref will typically be auto-inserted)
A method call annotated with #[rustc_no_implicit_refs].
A deref followed by a addr_of! or addr_of_mut!. See bottom of post for details.

There are several points that are not 100% clear to me when implementing the modifications:

"4. Any number of automatically inserted deref/derefmut calls." I as never able to trigger this. Am I missing something?
Are "index" and "field" enough?

cc @JakobDegen @WaffleLapkin
r? @RalfJung

rustbot · 2024-03-30T16:28:12Z

The Miri subtree was changed

cc @rust-lang/miri

RalfJung · 2024-03-30T16:32:58Z

Sorry, I can't take on more reviews currently.
r? compiler
(or feel free to pick someone specific who's suited)

fmease · 2024-05-15T12:00:19Z

compiler/rustc_lint/src/autorefs.rs

+    /// When working with raw pointers it's usually undesirable to create references,
+    /// since they inflict a lot of safety requirement. Unfortunately, it's possible
+    /// to take a reference to a dereference of a raw pointer implicitly, which inflicts
+    /// the usual reference requirements without you even knowing that.


Suggested change

/// the usual reference requirements without you even knowing that.

/// the usual reference requirements potentially without you realizing that.

More neutral, less “condescending” imo.

fmease · 2024-05-15T12:13:08Z

tests/ui/lint/implicit_autorefs.stderr

+help: try using a raw pointer method instead; or if this reference is intentional, make it explicit
+   |
+LL |     addr_of!((&(*ptr))[..16])
+   |              ++      +


While catching up with the comments posted in the now closed PR, I thought the plan was to avoid suggesting explicit borrows and to instead suggest #[allow]'ing the lint: Sth. like if this reference is intentional make it explicit by adding #[allow(implicit_unsafe_autorefs)].

CC 1st paragraph of #103735 (comment); #103735 (comment); #103735 (comment)

There has been concern from T-lang member about having only #[allow] as an option for lint, cc #118833 (comment) (and maybe a bit #122759 (comment)).

I think it would make sense to ask T-lang about this in the nomination, to see what they want, worst case it's just removing some code.

FWIW, nothing in #122759 is meant to imply that we shouldn't have lints whose suggestion is #[allow].

Good to know. Thanks.

I think it would make sense to ask T-lang about this in the nomination, to see what they want, worst case it's just removing some code.

@Urgau how do you want to proceed here, should we ask for T-lang feedback?

We want to nominate this for T-lang in any case -- once I've reviewed this PR ^^' I'll really try to get to it tonight.

Hi @fmease, just wanted to remind you to take a look at this PR if you are still interested. Thanks! 🙂

Dylan-DPC · 2024-08-12T11:58:19Z

@Urgau if you can rebase the latest conflicts we can push this forward and maybe get it reviewed by another reviewer

Urgau · 2024-08-12T17:49:41Z

@Dylan-DPC rebased.

@rustbot review

rustbot · 2025-03-03T18:56:28Z

Some changes occurred in compiler/rustc_passes/src/check_attr.rs

cc @jdonszelmann

RalfJung · 2025-03-05T20:31:24Z

@Urgau I recommend asking on Zulip to find another reviewer. I would generally recommend doing that after about 2 weeks of silence; it's been half a year since we last heard from @fmease so I think you've been unreasonably patient already. :)

Urgau · 2025-03-06T21:10:12Z

r? @jdonszelmann per https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/reviewer.20for.20.23123239

jdonszelmann · 2025-03-06T21:12:49Z

Heya, this looks somewhat interesting and I think I understand most parts involved so I'm willing to review it :)

jdonszelmann · 2025-03-08T19:17:36Z

It seems like there's still an open comment asking for T-lang feedback here? It's from august, did that ever happen? Not here at least it seems.

compiler/rustc_lint/src/autorefs.rs

Urgau · 2025-03-08T20:32:46Z

It seems like there's still an open comment asking for T-lang feedback here? It's from august, did that ever happen? Not here at least it seems.

Not yet. That will be a question for the T-lang nomination, which will be after the logic in this PR is checked to be correct-ish.

jdonszelmann · 2025-03-08T21:08:54Z

Hmm, okay. I'll give it one more look but I'm reasonably confident this now at least implements the logic from the linked comment.

compiler/rustc_lint/src/autorefs.rs

jdonszelmann · 2025-03-09T12:19:50Z

compiler/rustc_lint/src/autorefs.rs

+
+/// Peel derefs adjustments
+fn peel_derefs_adjustments<'a>(mut adjs: &'a [Adjustment<'a>]) -> &'a [Adjustment<'a>] {
+    while let [Adjustment { kind: Adjust::Deref(None), .. }, end @ ..] = adjs {


are you sure asserting None here is correct? I wrote some tests and can't find an example going wrong but I'm not 100% sure to be honest when this would be Some.

We check the Some() case in has_implicit_borrow so we can't peel it otherwise we will miss it in has_implicit_borrow.

ah, yea that makes sense!

could it ever happen that there are multiple Some cases in a row before a deref of a raw pointer?

(since has_implicit_borrow only strips one layer)

bors · 2025-03-09T15:41:35Z

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

rustbot assigned RalfJung Mar 30, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 30, 2024

rustbot assigned fmease and unassigned RalfJung Mar 30, 2024

This comment has been minimized.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from bb6ab41 to 2b3fe45 Compare March 30, 2024 17:17

WaffleLapkin mentioned this pull request Mar 31, 2024

Implement a lint for implicit autoref of raw pointer dereference #103735

Closed

WaffleLapkin mentioned this pull request Apr 13, 2024

request: lint when raw pointer slicing implicitly creates a temporary reference #99437

Open

This comment was marked as outdated.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from 2b3fe45 to 57f6416 Compare May 14, 2024 17:22

This comment has been minimized.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from 57f6416 to 824c1f5 Compare May 14, 2024 18:04

fmease reviewed May 15, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from 824c1f5 to c2d6e62 Compare May 23, 2024 18:49

This comment was marked as outdated.

Sign in to view

Dylan-DPC added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 12, 2024

Urgau force-pushed the dangerous_implicit_autorefs branch from c2d6e62 to 78288af Compare August 12, 2024 15:49

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2024

Urgau force-pushed the dangerous_implicit_autorefs branch from 78288af to d060615 Compare October 9, 2024 13:58

This comment has been minimized.

Sign in to view

This comment was marked as resolved.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from afe6661 to 0db3f30 Compare March 3, 2025 18:56

rustbot added the A-attributes Area: Attributes (`#[…]`, `#![…]`) label Mar 3, 2025

This comment was marked as resolved.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from 0db3f30 to 6ce9761 Compare March 5, 2025 19:29

Urgau unassigned fmease Mar 6, 2025

This comment was marked as resolved.

Sign in to view

Urgau assigned jdonszelmann Mar 6, 2025

jdonszelmann suggested changes Mar 8, 2025

View reviewed changes

compiler/rustc_lint/src/autorefs.rs Outdated Show resolved Hide resolved

compiler/rustc_lint/src/autorefs.rs Show resolved Hide resolved

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 8, 2025

Urgau force-pushed the dangerous_implicit_autorefs branch from 6ce9761 to 1e9582a Compare March 8, 2025 20:27

Urgau added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 8, 2025

This comment has been minimized.

Sign in to view

Urgau force-pushed the dangerous_implicit_autorefs branch from 1e9582a to ab6f4af Compare March 8, 2025 20:40

Add #[rustc_no_implicit_autorefs] and apply it to std methods

3fb1f3f

jdonszelmann reviewed Mar 8, 2025

View reviewed changes

compiler/rustc_lint/src/autorefs.rs Show resolved Hide resolved

Urgau added 2 commits March 9, 2025 12:49

Implement lint against dangerous implicit autorefs

a137868

Allow dangerous_implicit_autorefs lint in some tests

148db48

Urgau force-pushed the dangerous_implicit_autorefs branch from ab6f4af to 148db48 Compare March 9, 2025 11:49

jdonszelmann reviewed Mar 9, 2025

View reviewed changes

	/// the usual reference requirements without you even knowing that.
	/// the usual reference requirements potentially without you realizing that.

Implement a lint for implicit autoref of raw pointer dereference - take 2 #123239

Are you sure you want to change the base?

Implement a lint for implicit autoref of raw pointer dereference - take 2 #123239

Conversation

Urgau commented Mar 30, 2024 • edited by fmease Loading

rustbot commented Mar 30, 2024

RalfJung commented Mar 30, 2024

This comment has been minimized.

This comment has been minimized.

This comment was marked as outdated.

This comment has been minimized.

Choose a reason for hiding this comment

fmease May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Urgau May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmease Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

Dylan-DPC commented Aug 12, 2024

Urgau commented Aug 12, 2024

This comment has been minimized.

This comment was marked as resolved.

rustbot commented Mar 3, 2025

This comment was marked as resolved.

RalfJung commented Mar 5, 2025

Urgau commented Mar 6, 2025

This comment was marked as resolved.

jdonszelmann commented Mar 6, 2025 • edited Loading

jdonszelmann commented Mar 8, 2025

Urgau commented Mar 8, 2025 • edited Loading

This comment has been minimized.

jdonszelmann commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 9, 2025

Urgau commented Mar 30, 2024 •

edited by fmease

Loading

fmease May 15, 2024 •

edited

Loading

Urgau May 15, 2024 •

edited

Loading

fmease Aug 13, 2024 •

edited

Loading

jdonszelmann commented Mar 6, 2025 •

edited

Loading

Urgau commented Mar 8, 2025 •

edited

Loading