-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement a lint for implicit autoref of raw pointer dereference - take 2 #123239
base: master
Are you sure you want to change the base?
Conversation
The Miri subtree was changed cc @rust-lang/miri |
Sorry, I can't take on more reviews currently. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bb6ab41
to
2b3fe45
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2b3fe45
to
57f6416
Compare
This comment has been minimized.
This comment has been minimized.
57f6416
to
824c1f5
Compare
/// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// the usual reference requirements without you even knowing that. | |
/// the usual reference requirements potentially without you realizing that. |
More neutral, less “condescending” imo.
help: try using a raw pointer method instead; or if this reference is intentional, make it explicit | ||
| | ||
LL | addr_of!((&(*ptr))[..16]) | ||
| ++ + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, nothing in #122759 is meant to imply that we shouldn't have lints whose suggestion is #[allow]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to know. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @fmease, just wanted to remind you to take a look at this PR if you are still interested. Thanks! 🙂
This comment was marked as outdated.
This comment was marked as outdated.
824c1f5
to
c2d6e62
Compare
This comment was marked as outdated.
This comment was marked as outdated.
@Urgau if you can rebase the latest conflicts we can push this forward and maybe get it reviewed by another reviewer |
c2d6e62
to
78288af
Compare
@Dylan-DPC rebased. @rustbot review |
78288af
to
d060615
Compare
This comment has been minimized.
This comment has been minimized.
d060615
to
afe6661
Compare
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.
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:
#[rustc_no_implicit_refs]
.addr_of!
oraddr_of_mut!
. See bottom of post for details.There are several points that are not 100% clear to me when implementing the modifications:
cc @JakobDegen @WaffleLapkin
r? @RalfJung