Skip to content
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

Disallow reference to static mut and adding static_mut_ref lint #117556

Merged
merged 6 commits into from
Jan 9, 2024

Conversation

obeis
Copy link
Contributor

@obeis obeis commented Nov 3, 2023

Closes #114447

r? @scottmcm

@rustbot 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. labels Nov 3, 2023
@rust-log-analyzer

This comment has been minimized.

@est31
Copy link
Member

est31 commented Nov 4, 2023

I wonder if you could also implement the error part for edition 2024, similar to how it's done here. Asking because then it might not be done in a lint pass any more? E.g. maybe_lint_bare_trait is called during the hir::Ty -> rustc_middle::ty::Ty lowering.

@obeis obeis marked this pull request as draft November 7, 2023 18:33
@obeis obeis marked this pull request as ready for review November 10, 2023 17:43
@rustbot
Copy link
Collaborator

rustbot commented Nov 10, 2023

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

@obeis obeis changed the title Add static_mut_ref lint Disallow reference to static mut and adding static_mut_ref lint Nov 10, 2023
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

compiler/rustc_hir_analysis/src/check/region.rs Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/messages.ftl Outdated Show resolved Hide resolved
compiler/rustc_hir_analysis/messages.ftl Outdated Show resolved Hide resolved
@est31
Copy link
Member

est31 commented Nov 10, 2023

Some additional points:

  • could you add a test that ptr::addr_of_mut still works?
  • could you add a test that uses dot notation so there is no explicit syntax including &? Maybe like static mut foo: [u8; 3] = [1, 2, 3]; let _ = FOO.len();?
  • this seems like a good use for test revisions where in one revision you enable the lint, and in the other revision you enable edition 2024. then that can cut down on duplication.
  • the messaging around the lint/error is that all uses are illegal, but it's only the references and mutable references that are. In fact, the lint/error could suggest adopting addr_of_mut.

@est31
Copy link
Member

est31 commented Nov 10, 2023

the messaging around the lint/error is that all uses are illegal, but it's only the references and mutable references that are.

Note that I'm not arguing here that we should never make all of static mut deprecated (maybe a good idea, maybe not, in any case it's right now it's too early because SyncUnsafeCell is still unstable). But there is an inconsistency between what the lint does and its name on one hand, and what its messaging is on the other.

@scottmcm
Copy link
Member

Thanks for making a PR for this! Unfortunately, I'm not personally familiar with the lint system, so while I'm happy to look at the tests to see what it's doing (est31's suggestions in #117556 (comment) sound good), I'm not a good reviewer for it technically.

r? compiler

@rustbot
Copy link
Collaborator

rustbot commented Nov 14, 2023

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@obeis
Copy link
Contributor Author

obeis commented Nov 15, 2023

cc @RalfJung because it was your idea and unsafe is your area

@rust-log-analyzer
Copy link
Collaborator

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

@fmease
Copy link
Member

fmease commented Jan 8, 2024

Hmm, let's retry that?
@bors retry

@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117556 (Disallow reference to `static mut` and adding `static_mut_ref` lint)
 - rust-lang#118748 (std: getrandom simplification for freebsd.)
 - rust-lang#119282 (Rework and improve the unstable documentation of check-cfg)
 - rust-lang#119527 (don't reexport atomic::ordering via rustc_data_structures, use std import)
 - rust-lang#119668 (Simplify implementation of MIR promotion)
 - rust-lang#119699 (Merge dead bb pruning and unreachable bb deduplication.)
 - rust-lang#119723 (Remove `-Zdont-buffer-diagnostics`.)
 - rust-lang#119756 (rustdoc-search: reuse individual types in function signatures)
 - rust-lang#119758 (GNU/Hurd: unconditionally use inline stack probes)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
…llaumeGomez

Rollup of 9 pull requests

Successful merges:

 - rust-lang#117556 (Disallow reference to `static mut` and adding `static_mut_ref` lint)
 - rust-lang#118748 (std: getrandom simplification for freebsd.)
 - rust-lang#119282 (Rework and improve the unstable documentation of check-cfg)
 - rust-lang#119527 (don't reexport atomic::ordering via rustc_data_structures, use std import)
 - rust-lang#119668 (Simplify implementation of MIR promotion)
 - rust-lang#119699 (Merge dead bb pruning and unreachable bb deduplication.)
 - rust-lang#119723 (Remove `-Zdont-buffer-diagnostics`.)
 - rust-lang#119756 (rustdoc-search: reuse individual types in function signatures)
 - rust-lang#119758 (GNU/Hurd: unconditionally use inline stack probes)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 4a24b5b into rust-lang:master Jan 9, 2024
11 of 12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 9, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jan 9, 2024
Rollup merge of rust-lang#117556 - obeis:static-mut-ref-lint, r=davidtwco

Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? `@scottmcm`
@ehuss
Copy link
Contributor

ehuss commented Jan 9, 2024

Some questions about this:

  • Is there an intended migration path for this, or is the user expected to rewrite their code if it is encountered?
  • The current suggestion of adding addr_of_mut! does not compile. At a minimum, I would think it needs to be qualified with core::ptr::. Can that be fixed?
  • Is there an explanation of why that is a "maybe incorrect" suggestion?
  • Was there a crater run done to see how much of an impact this will have?
  • Was there an RFC for this, or any public discussion about the lang team's decision? It seems unusual to make language changes without an RFC.

@ehuss ehuss added A-edition-2024 Area: The 2024 edition relnotes Marks issues that should be documented in the release notes of the next release. labels Jan 9, 2024
@scottmcm
Copy link
Member

scottmcm commented Jan 9, 2024

As mentioned in #114447 (comment), you can always change &BLAH to &*ptr::addr_of!(BLAH), which will compile, but still be just as UB-prone.

The problem with taking references to static mut is that it's most often insta-UB, so there's no local change that can fix that, and yes, larger rework will typically be needed.


For more reading, there's been discussion and slow movement for at least 5 years towards limiting static mut. See #53639 for example.

@obeis obeis deleted the static-mut-ref-lint branch January 9, 2024 19:57
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Jan 26, 2024
…twco

Disallow reference to `static mut` and adding `static_mut_ref` lint

Closes rust-lang#114447

r? `@scottmcm`
wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request Mar 29, 2024
Pkgsrc changes:
 * Adapt checksums and patches.

Upstream chnages:

Version 1.77.0 (2024-03-21)
==========================

- [Reveal opaque types within the defining body for exhaustiveness checking.]
  (rust-lang/rust#116821)
- [Stabilize C-string literals.]
  (rust-lang/rust#117472)
- [Stabilize THIR unsafeck.]
  (rust-lang/rust#117673)
- [Add lint `static_mut_refs` to warn on references to mutable statics.]
  (rust-lang/rust#117556)
- [Support async recursive calls (as long as they have indirection).]
  (rust-lang/rust#117703)
- [Undeprecate lint `unstable_features` and make use of it in the compiler.]
  (rust-lang/rust#118639)
- [Make inductive cycles in coherence ambiguous always.]
  (rust-lang/rust#118649)
- [Get rid of type-driven traversal in const-eval interning]
  (rust-lang/rust#119044),
  only as a [future compatiblity lint]
  (rust-lang/rust#122204) for now.
- [Deny braced macro invocations in let-else.]
  (rust-lang/rust#119062)

Compiler
--------

- [Include lint `soft_unstable` in future breakage reports.]
  (rust-lang/rust#116274)
- [Make `i128` and `u128` 16-byte aligned on x86-based targets.]
  (rust-lang/rust#116672)
- [Use `--verbose` in diagnostic output.]
  (rust-lang/rust#119129)
- [Improve spacing between printed tokens.]
  (rust-lang/rust#120227)
- [Merge the `unused_tuple_struct_fields` lint into `dead_code`.]
  (rust-lang/rust#118297)
- [Error on incorrect implied bounds in well-formedness check]
  (rust-lang/rust#118553),
  with a temporary exception for Bevy.
- [Fix coverage instrumentation/reports for non-ASCII source code.]
  (rust-lang/rust#119033)
- [Fix `fn`/`const` items implied bounds and well-formedness check.]
  (rust-lang/rust#120019)
- [Promote `riscv32{im|imafc}-unknown-none-elf` targets to tier 2.]
  (rust-lang/rust#118704)
- Add several new tier 3 targets:
  - [`aarch64-unknown-illumos`]
    (rust-lang/rust#112936)
  - [`hexagon-unknown-none-elf`]
    (rust-lang/rust#117601)
  - [`riscv32imafc-esp-espidf`]
    (rust-lang/rust#119738)
  - [`riscv32im-risc0-zkvm-elf`]
    (rust-lang/rust#117958)

Refer to Rust's [platform support page][platform-support-doc]
for more information on Rust's tiered platform support.

Libraries
---------

- [Implement `From<&[T; N]>` for `Cow<[T]>`.]
  (rust-lang/rust#113489)
- [Remove special-case handling of `vec.split_off
  (0)`.](rust-lang/rust#119917)

Stabilized APIs
---------------

- [`array::each_ref`]
  (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_ref)
- [`array::each_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.array.html#method.each_mut)
- [`core::net`]
  (https://doc.rust-lang.org/stable/core/net/index.html)
- [`f32::round_ties_even`]
  (https://doc.rust-lang.org/stable/std/primitive.f32.html#method.round_ties_even)
- [`f64::round_ties_even`]
  (https://doc.rust-lang.org/stable/std/primitive.f64.html#method.round_ties_even)
- [`mem::offset_of!`]
  (https://doc.rust-lang.org/stable/std/mem/macro.offset_of.html)
- [`slice::first_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk)
- [`slice::first_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.first_chunk_mut)
- [`slice::split_first_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk)
- [`slice::split_first_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_first_chunk_mut)
- [`slice::last_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk)
- [`slice::last_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.last_chunk_mut)
- [`slice::split_last_chunk`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk)
- [`slice::split_last_chunk_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.split_last_chunk_mut)
- [`slice::chunk_by`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by)
- [`slice::chunk_by_mut`]
  (https://doc.rust-lang.org/stable/std/primitive.slice.html#method.chunk_by_mut)
- [`Bound::map`]
  (https://doc.rust-lang.org/stable/std/ops/enum.Bound.html#method.map)
- [`File::create_new`]
  (https://doc.rust-lang.org/stable/std/fs/struct.File.html#method.create_new)
- [`Mutex::clear_poison`]
  (https://doc.rust-lang.org/stable/std/sync/struct.Mutex.html#method.clear_poison)
- [`RwLock::clear_poison`]
  (https://doc.rust-lang.org/stable/std/sync/struct.RwLock.html#method.clear_poison)

Cargo
-----

- [Extend the build directive syntax with `cargo::`.]
  (rust-lang/cargo#12201)
- [Stabilize metadata `id` format as `PackageIDSpec`.]
  (rust-lang/cargo#12914)
- [Pull out as `cargo-util-schemas` as a crate.]
  (rust-lang/cargo#13178)
- [Strip all debuginfo when debuginfo is not requested.]
  (rust-lang/cargo#13257)
- [Inherit jobserver from env for all kinds of runners.]
  (rust-lang/cargo#12776)
- [Deprecate rustc plugin support in cargo.]
  (rust-lang/cargo#13248)

Rustdoc
-----

- [Allows links in markdown headings.]
  (rust-lang/rust#117662)
- [Search for tuples and unit by type with `()`.]
  (rust-lang/rust#118194)
- [Clean up the source sidebar's hide button.]
  (rust-lang/rust#119066)
- [Prevent JS injection from `localStorage`.]
  (rust-lang/rust#120250)

Misc
----

- [Recommend version-sorting for all sorting in style guide.]
  (rust-lang/rust#115046)

Internal Changes
----------------

These changes do not affect any public interfaces of Rust, but they represent
significant improvements to the performance or internals of rustc and related
tools.

- [Add more weirdness to `weird-exprs.rs`.]
  (rust-lang/rust#119028)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2024 Area: The 2024 edition A-testsuite Area: The testsuite used to check the correctness of rustc O-unix Operating system: Unix-like relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallow *references* to static mut [Edition Idea]