Taking a raw pointer on a union field is a safe operation

[samueltardieu]

changelog: [multiple_unsafe_ops_per_block]: taking a raw pointer on a union field is a safe operation

Fixes #16076

Summary Notes

• Beta nomination by samueltardieu

Managed by @rustbot—see help for details

[rustbot]

r? @y21

rustbot has assigned @y21.
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

[Jarcho]

You should switch away from for_each_expr to an explicit visitor. Note you need to skip all successive field expressions after a raw borrow, but then continue the visitor as normal after seeing any other expression. This is easy with a regular visitor, but for_each_expr has no way to do this other than maintaining extra flags.

We also need to do decide on what to do with the MSRV here since this changed as of 1.92. I'm leaning towards just ignoring it since it never had unchecked preconditions in the first place and unused_unsafe will also lint on it.

[samueltardieu]

Doing it with a while seems easy enough, I've added also added a test with multiple successive field expressions. This will not happen frequently so the runtime time will be negligible in practice, first of all because it is unlikely to take place inside an unsafe block since this is not an unsafe operation.

[Jarcho]

first of all because it is unlikely to take place inside an unsafe block since this is not an unsafe operation.

This is only true as of 1.92 (current beta). Anything with an MSRV before that needs to put this in an unsafe block. &raw has been stable (via the addr_of macro) since 1.51.

[samueltardieu]

Yes, I'm testing the MSRV check right now.

[Jarcho]

&raw const (*x).y will also be a thing in unsafe blocks that can't be broken apart if x is a raw pointer.

[samueltardieu]

&raw const (*x).y will also be a thing in unsafe blocks that can't be broken apart if x is a raw pointer.

It will be counted as one unsafe operation starting from Rust 1.92 (because of *x), and two unsafe operations before that and trigger the lint. This is possibly an issue, but a different one than #16076 that this PR fixes.

I think we should beta-nominate this PR to avoid a new false positive in Rust 1.92, while fixing the fact that &raw const (*x).y can't be broken apart if x is a raw pointer is not a regression.

[Jarcho]

That was in reference to your comment about it being unlikely to appear in an unsafe block. &raw const|mut (*x).y is needed to adjust a raw pointer to a field. There's code that will do that a lot and that needs to be a single expression in an unsafe block.

[samueltardieu]

@Jarcho This convinced me. I switched the code to a visitor, and determine the MSRV only when we detect a union field access under a raw pointer (and possible other fields accesses).

[github-actions]

No changes for ae5274d

[samueltardieu]

@rustbot note Beta nomination

This issue fixes a regression in the multiple_unsafe_ops_per_block in 1.92 due to a change in Rust: taking a raw pointer on a union field becomes a safe operation and should not be counted as an unsafe one. This may create new safe positives.

However, this happens in a restriction lint, so the impact may be moderate — but the consequences of new regressions that would have been inadvertently introduced in this PR would be as well.

@rustbot label beta-nominated

[Jarcho]

Before merging, do we really want to take the MSRV into account for this? It's an operation that was unsafe as an oversight, not something that is unsafe. Given that the intent of the lint is to essentially mark each unsafe operation, requiring this to be marked separately means the safety comment it would get is basically // Safety: It just is.

Basically this complicates the code and it doesn't seem to benefit anybody.

[Jarcho]

Without the MSRV handling this could be:

ExprKind::AddrOf(BorrowKind::Raw, _, mut e) => {
  while let ExprKind::Field(base, _) = e.kind
    && self.typeck_results.expr_adjustments(base).is_empty() // No deref
  { e = base; }
  return self.visit_expr(e); 
}

This removes all the complexity of Self::under_raw_ptr.

[samueltardieu]

Sure, but with Rust < 1.92 we would not flag multiple unsafe operations in the block as the lint indicates, while not letting the user get the ref to union field outside of the unsafe block because Rust won't allow it. That would be a regression in itself, as it could break some #[expect] that would have been placed on the block/function.

The MSRV logic is about 5 lines long + declarations in order to implement some caching, so it is neither complex nor particularly inefficient.

But I don't have a strong opinion about it, I'll let @y21 weigh in.

[Jarcho]

We don't guarantee expect will continue to trigger when updating so that's not, on it's own, a regression. Not taking the MSRV doesn't inconvenience anyone who already split the blocks, and anyone with an expect thought it was better to keep it as a single block. Anybody who separates this into a new block will also get rustc's unused_unsafe warning (they probably shouldn't). Who is actually in the set of people who actually want this MSRV dependent behaviour.

[Jarcho]

I wasn't trying to say the logic is particularly complex in this case. It isn't, but it is additional logic that seems to benefit nobody.

[samueltardieu]

Since it was easy to do that (thanks to jj), I've separated the PR into two commits:

• The first one uses a visitor instead of for_each_expr and doesn't lint raw pointers to union fields, regardless of the MSRV.
• The second one adds proper MSRV handling to distinguish between pre-1.92 and post-1.92.

@y21 We can either keep both commits or just the first one if you prefer not to have proper MSRV handling.

[rustbot]

☔ The latest upstream changes (possibly c48592e) made this pull request unmergeable. Please resolve the merge conflicts.