Merged

Conversation

Copy link Copy Markdown

Contributor

changelog: [multiple_unsafe_ops_per_block]: Fix false positive in taking an pointer to an mutable static, but not read/wrtiting it and not field projecting into it

Since I already looked into the other multiple_unsafe_ops_per_block issue, I thought it best to tackle this issue as well. It is a bit more complex because of how fields and paths interact, so a custom visiotor seemed appropriate to keep the code readable.

I also think I covered everything with tests as far as I can, so should be an easy review.

Copy link Copy Markdown

Collaborator

Thanks for the pull request, and welcome!

You should hear from one of our reviewers after this PR gets at least 2 reviews from the community.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

… ptr's

		unsafe {
		//~^ multiple_unsafe_ops_per_block
		not_very_safe();
		let _ = &raw mut GLOBAL_STRUCT.s.x;

Copy link Copy Markdown

Contributor

Choose a reason for hiding this comment

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

Why is GLOBAL_STRUCT.s.x treated differently from value.s.x here, or from the direct &raw mut GLOBAL_INT case? &raw mut GLOBAL_STRUCT.s.x seem to only take the raw address of a field under &raw, without reading/writing the static or creating a reference.

View changes since the review

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

Yes, the rules around this are super confusing and a bit self-inconsistent.
I know and should have called this out a bit better.

The following PR made DIRECT references save.
It did not make field projections save though, so if you field project this is still unsafe.

treat &raw (const|mut) UNSAFE_STATIC implied deref as safe rust#125834

So essentially, this is codifying a gap in the compiler, but one which is a bit niche.
I did not find a newer issue tracking this, so I might be wrong on what is and what is not.

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

Copy link Copy Markdown

Contributor Author

Choose a reason for hiding this comment

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

Confirmed on zulip...
Not ideal, but that is the state we are in.

Copy link Copy Markdown

Collaborator

r? @Jarcho

rustbot has assigned @Jarcho for the project review.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

Owners of files modified in this PR: 8 candidates
8 candidates expanded to 8 candidates
Random selection from Jarcho, Manishearth, dswij, llogiq

samueltardieu changed the title ~~fix(multiple_unsafe_ops_per_block): false positive in with taking an refrence to an static, but not read/wrtiting it~~ fix(multiple_unsafe_ops_per_block): false positive in with taking an reference to a static, but not reading/writing it