make unaligned_references future-incompat lint warn-by-default #82525

RalfJung · 2021-02-25T18:48:14Z

and also remove the safe_packed_borrows lint that it replaces.

std::ptr::addr_of! has hit beta now and will hit stable in a month, so I propose we start fixing #27060 for real: creating a reference to a field of a packed struct needs to eventually become a hard error; this PR makes it a warn-by-default future-incompat lint. (The lint already existed, this just raises its default level.) At the same time I removed the corresponding code from unsafety checking; really there's no reason an unsafe block should make any difference here.

For references to packed fields outside unsafe blocks, this means unaligned_refereces replaces the previous safe_packed_borrows warning with a link to #82523 (and no more talk about unsafe blocks making any difference). So behavior barely changes, the warning is just worded differently. For references to packed fields inside unsafe blocks, this PR shows a new future-incompat warning.

Closes #46043 because that lint no longer exists.

rust-highfive · 2021-02-25T18:48:17Z

r? @petrochenkov

(rust-highfive has picked a reviewer for you, use r? to override)

compiler/rustc_lint_defs/src/builtin.rs

digama0 · 2021-02-25T23:40:10Z

Why is it okay to make it not unsafe anymore? Doesn't this cause a soundness issue, at least in versions that don't give the hard error (i.e. the current version)?

RalfJung · 2021-02-26T08:07:03Z

Why is it okay to make it not unsafe anymore? Doesn't this cause a soundness issue, at least in versions that don't give the hard error (i.e. the current version)?

Instead of making it unsafe, we will forbid it entirely. This is always UB, which is not allowed even in unsafe blocks. Also note that it wasn't a hard unsafety error before either, it was a future-incompat warning.

Previously, outside unsafe blocks, it just caused a warning -- that was silenced by adding an unsafe block. That makes little sense, given that it is still UB. So the behavior does not change for what happens when you do this outside an unsafe block: both before and after this PR, you get a warning, saying "this will be a hard error eventually". This PR only changes behavior for doing this inside an unsafe block.

RalfJung · 2021-02-26T08:16:07Z

compiler/rustc_lint/src/lib.rs

@@ -336,6 +336,7 @@ fn register_builtins(store: &mut LintStore, no_interleave_lints: bool) {
    store.register_renamed("redundant_semicolon", "redundant_semicolons");
    store.register_renamed("intra_doc_link_resolution_failure", "broken_intra_doc_links");
    store.register_renamed("overlapping_patterns", "overlapping_range_endpoints");
+    store.register_renamed("safe_packed_borrows", "unaligned_references");


I called this a "rename" now because the lints are fairly similar... but maybe "remove" would be more appropriate?

No opinion, seems fine as is.

RalfJung · 2021-02-26T08:17:55Z

FWIW, I don't know what our policy is wrt how long we wait before shipping such things -- std::ptr::addr_of! is available on beta and nightly, but not yet on stable. So maybe we want to wait until it hits stable to ensure that people can properly fix this warning while remaining stable-compatible. But in case there need to be FCPs or so I wanted to already get the ball rolling here.

DemiMarie · 2021-02-26T19:38:36Z

In the future, I would like to see a type that wraps a (possibly unaligned) pointer that is guaranteed to be safe to dereference.

compiler/rustc_lint_defs/src/builtin.rs

petrochenkov · 2021-02-28T16:04:21Z

I think it's useful to land this even if std::ptr::addr_of is not yet stable, since the most common correct fix is to copy the field rather than to take a raw reference to it.
The old lint is not good, I've personally seen people wrongly silencing the warning by adding an unsafe block instead of copying the value.

petrochenkov · 2021-02-28T16:05:00Z

r=me with the doc tweaked (#82525 (comment)).

RalfJung · 2021-03-01T11:15:06Z

I think it's useful to land this even if std::ptr::addr_of is not yet stable, since the most common correct fix is to copy the field rather than to take a raw reference to it.

My concern here is that people will silence the lint if they cannot fix the error because addr_of is not yet stable.

Let's see what @rust-lang/lang says.

nikomatsakis · 2021-03-23T17:31:04Z

We discussed this in the @rust-lang/lang meeting and we would approve merging the PR. I'm happy to do any review necessary.

petrochenkov · 2021-03-27T15:07:13Z

r=me with commits squashed.

…move the safe_packed_borrows lint that it replaces

RalfJung · 2021-03-27T16:00:20Z

@bors r=petrochenkov

bors · 2021-03-27T16:00:21Z

📌 Commit fb4f48e has been approved by petrochenkov

Rollup of 8 pull requests Successful merges: - rust-lang#81351 (combine: stop eagerly evaluating consts) - rust-lang#82525 (make unaligned_references future-incompat lint warn-by-default) - rust-lang#82626 (update array missing `IntoIterator` msg) - rust-lang#82917 (Add function core::iter::zip) - rust-lang#82993 (rustdoc: Use diagnostics for error when including sources) - rust-lang#83522 (Improve fs error open_from unix) - rust-lang#83548 (Always preserve `None`-delimited groups in a captured `TokenStream`) - rust-lang#83555 (Add #[inline] to io::Error methods) Failed merges: - rust-lang#83130 (escape_ascii take 2) r? `@ghost` `@rustbot` modify labels: rollup

…safeck, r=RalfJung Clean up remnants of BorrowOfPackedField cc rust-lang#82525 which removed `BorrowOfPackedField` from unsafety-checking r? `@RalfJung`

…i-obk make unaligned_references lint deny-by-default This lint has been warn-by-default for a year now (since rust-lang#82525), so I think it is time to crank it up a bit. Code that triggers the lint causes UB (without `unsafe`) when executed, so we really don't want people to write code like this.

…ce, r=cjgillot make unaligned_reference a hard error The `unaligned_references` lint has been warn-by-default since Rust 1.53 (rust-lang#82525) and deny-by-default with mention in cargo future-incompat reports since Rust 1.62 (rust-lang#95372). Current nightly will become Rust 1.66, so (unless major surprises show up with crater) I think it is time we make this a hard error, and close this old soundness gap in the language. Fixes rust-lang#82523.

…, r=cjgillot,scottmcm make unaligned_reference a hard error The `unaligned_references` lint has been warn-by-default since Rust 1.53 (rust-lang#82525) and deny-by-default with mention in cargo future-incompat reports since Rust 1.62 (rust-lang#95372). Current nightly will become Rust 1.66, so (unless major surprises show up with crater) I think it is time we make this a hard error, and close this old soundness gap in the language. EDIT: Turns out this will only land for Rust 1.67, so there is another 6 weeks of time here for crates to adjust. Fixes rust-lang#82523.

…lot,scottmcm make unaligned_reference a hard error The `unaligned_references` lint has been warn-by-default since Rust 1.53 (rust-lang/rust#82525) and deny-by-default with mention in cargo future-incompat reports since Rust 1.62 (rust-lang/rust#95372). Current nightly will become Rust 1.66, so (unless major surprises show up with crater) I think it is time we make this a hard error, and close this old soundness gap in the language. EDIT: Turns out this will only land for Rust 1.67, so there is another 6 weeks of time here for crates to adjust. Fixes rust-lang/rust#82523.

rust-highfive assigned petrochenkov Feb 25, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 25, 2021

RalfJung mentioned this pull request Feb 25, 2021

repr(packed) allows invalid unaligned loads #27060

Closed

This comment has been minimized.

Sign in to view

nagisa reviewed Feb 25, 2021

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

ehuss reviewed Feb 25, 2021

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved

RalfJung force-pushed the unaligned-ref-warn branch from 1645e72 to aede37f Compare February 26, 2021 08:14

RalfJung commented Feb 26, 2021

View reviewed changes

RalfJung added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Feb 26, 2021

RalfJung changed the title ~~make unaligned_refereces future-incompat lint warn-by-default~~ make unaligned_references future-incompat lint warn-by-default Feb 26, 2021

RalfJung mentioned this pull request Feb 26, 2021

Promote Certain Lints to Errors in Rust 2021 Edition #80165

Closed

2 tasks

This was referenced Feb 27, 2021

Prepare for removal of safe_packed_borrows lint taiki-e/pin-project-lite#55

Merged

Enable report_in_external_macro in unaligned_references #82587

Merged

petrochenkov reviewed Feb 28, 2021

View reviewed changes

compiler/rustc_lint_defs/src/builtin.rs Outdated Show resolved Hide resolved

petrochenkov 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 Feb 28, 2021

RalfJung force-pushed the unaligned-ref-warn branch from a0dceed to 38262da Compare March 1, 2021 11:12

petrochenkov added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 1, 2021

petrochenkov 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Mar 23, 2021

RalfJung force-pushed the unaligned-ref-warn branch from 38262da to 052df96 Compare March 27, 2021 14:09

This comment has been minimized.

Sign in to view

make unaligned_refereces future-incompat lint warn-by-default, and re…

fb4f48e

…move the safe_packed_borrows lint that it replaces

RalfJung force-pushed the unaligned-ref-warn branch from 4bc5a33 to fb4f48e Compare March 27, 2021 15:59

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 27, 2021

Dylan-DPC-zz mentioned this pull request Mar 27, 2021

Rollup of 8 pull requests #83580

Merged

bors merged commit a900677 into rust-lang:master Mar 27, 2021

rustbot added this to the 1.53.0 milestone Mar 27, 2021

RalfJung deleted the unaligned-ref-warn branch March 28, 2021 09:29

This was referenced May 17, 2021

Clean up remnants of BorrowOfPackedField #85415

Merged

Implement the THIR unsafety checker rust-lang/project-thir-unsafeck#7

Closed

RalfJung mentioned this pull request Mar 27, 2022

make unaligned_references lint deny-by-default and make it show up in future-breakage report #95372

Merged

RalfJung mentioned this pull request Jul 20, 2022

Tracking Issue for future-incompatibility warning unaligned_references #82523

Closed

RalfJung mentioned this pull request Sep 30, 2022

make unaligned_reference a hard error #102513

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make unaligned_references future-incompat lint warn-by-default #82525

make unaligned_references future-incompat lint warn-by-default #82525

RalfJung commented Feb 25, 2021 •

edited

Loading

rust-highfive commented Feb 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

digama0 commented Feb 25, 2021

RalfJung commented Feb 26, 2021 •

edited

Loading

RalfJung Feb 26, 2021

petrochenkov Feb 28, 2021

RalfJung commented Feb 26, 2021

DemiMarie commented Feb 26, 2021

petrochenkov commented Feb 28, 2021

petrochenkov commented Feb 28, 2021

RalfJung commented Mar 1, 2021

nikomatsakis commented Mar 23, 2021

This comment has been minimized.

petrochenkov commented Mar 27, 2021

RalfJung commented Mar 27, 2021

bors commented Mar 27, 2021

make unaligned_references future-incompat lint warn-by-default #82525

make unaligned_references future-incompat lint warn-by-default #82525

Conversation

RalfJung commented Feb 25, 2021 • edited Loading

rust-highfive commented Feb 25, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

digama0 commented Feb 25, 2021

RalfJung commented Feb 26, 2021 • edited Loading

RalfJung Feb 26, 2021

Choose a reason for hiding this comment

petrochenkov Feb 28, 2021

Choose a reason for hiding this comment

RalfJung commented Feb 26, 2021

DemiMarie commented Feb 26, 2021

petrochenkov commented Feb 28, 2021

petrochenkov commented Feb 28, 2021

RalfJung commented Mar 1, 2021

nikomatsakis commented Mar 23, 2021

This comment has been minimized.

petrochenkov commented Mar 27, 2021

RalfJung commented Mar 27, 2021

bors commented Mar 27, 2021

RalfJung commented Feb 25, 2021 •

edited

Loading

RalfJung commented Feb 26, 2021 •

edited

Loading