-
-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Add interior-mutability suggestion to static_mut_refs
#151362
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,12 @@ | ||
| use rustc_hir as hir; | ||
| use rustc_hir::def_id::DefId; | ||
| use rustc_hir::{Expr, Stmt}; | ||
| use rustc_middle::ty::{Mutability, TyKind}; | ||
| use rustc_session::lint::fcw; | ||
| use rustc_session::{declare_lint, declare_lint_pass}; | ||
| use rustc_span::{BytePos, Span}; | ||
|
|
||
| use crate::lints::{MutRefSugg, RefOfMutStatic}; | ||
| use crate::lints::{MutRefSugg, RefOfMutStatic, StaticMutRefsInteriorMutabilitySugg}; | ||
| use crate::{LateContext, LateLintPass, LintContext}; | ||
|
|
||
| declare_lint! { | ||
|
|
@@ -67,7 +68,7 @@ impl<'tcx> LateLintPass<'tcx> for StaticMutRefs { | |
| match expr.kind { | ||
| hir::ExprKind::AddrOf(borrow_kind, m, ex) | ||
| if matches!(borrow_kind, hir::BorrowKind::Ref) | ||
| && let Some(err_span) = path_is_static_mut(ex, err_span) => | ||
| && let Some(static_mut) = path_is_static_mut(ex, err_span) => | ||
| { | ||
| let source_map = cx.sess().source_map(); | ||
| let snippet = source_map.span_to_snippet(err_span); | ||
|
|
@@ -86,18 +87,32 @@ impl<'tcx> LateLintPass<'tcx> for StaticMutRefs { | |
| err_span.with_hi(ex.span.lo()) | ||
| }; | ||
|
|
||
| emit_static_mut_refs(cx, err_span, sugg_span, m, !expr.span.from_expansion()); | ||
| emit_static_mut_refs( | ||
| cx, | ||
| static_mut.err_span, | ||
| sugg_span, | ||
| m, | ||
| !expr.span.from_expansion(), | ||
| static_mut.def_id, | ||
| ); | ||
| } | ||
| hir::ExprKind::MethodCall(_, e, _, _) | ||
| if let Some(err_span) = path_is_static_mut(e, expr.span) | ||
| if let Some(static_mut) = path_is_static_mut(e, expr.span) | ||
| && let typeck = cx.typeck_results() | ||
| && let Some(method_def_id) = typeck.type_dependent_def_id(expr.hir_id) | ||
| && let inputs = | ||
| cx.tcx.fn_sig(method_def_id).skip_binder().inputs().skip_binder() | ||
| && let Some(receiver) = inputs.get(0) | ||
| && let TyKind::Ref(_, _, m) = receiver.kind() => | ||
| { | ||
| emit_static_mut_refs(cx, err_span, err_span.shrink_to_lo(), *m, false); | ||
| emit_static_mut_refs( | ||
| cx, | ||
| static_mut.err_span, | ||
| static_mut.err_span.shrink_to_lo(), | ||
| *m, | ||
| false, | ||
| static_mut.def_id, | ||
| ); | ||
| } | ||
| _ => {} | ||
| } | ||
|
|
@@ -108,14 +123,26 @@ impl<'tcx> LateLintPass<'tcx> for StaticMutRefs { | |
| && let hir::PatKind::Binding(ba, _, _, _) = loc.pat.kind | ||
| && let hir::ByRef::Yes(_, m) = ba.0 | ||
| && let Some(init) = loc.init | ||
| && let Some(err_span) = path_is_static_mut(init, init.span) | ||
| && let Some(static_mut) = path_is_static_mut(init, init.span) | ||
| { | ||
| emit_static_mut_refs(cx, err_span, err_span.shrink_to_lo(), m, false); | ||
| emit_static_mut_refs( | ||
| cx, | ||
| static_mut.err_span, | ||
| static_mut.err_span.shrink_to_lo(), | ||
| m, | ||
| false, | ||
| static_mut.def_id, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| fn path_is_static_mut(mut expr: &hir::Expr<'_>, mut err_span: Span) -> Option<Span> { | ||
| struct StaticMutInfo { | ||
| err_span: Span, | ||
| def_id: DefId, | ||
| } | ||
|
|
||
| fn path_is_static_mut(mut expr: &hir::Expr<'_>, mut err_span: Span) -> Option<StaticMutInfo> { | ||
| if err_span.from_expansion() { | ||
| err_span = expr.span; | ||
| } | ||
|
|
@@ -126,11 +153,11 @@ fn path_is_static_mut(mut expr: &hir::Expr<'_>, mut err_span: Span) -> Option<Sp | |
|
|
||
| if let hir::ExprKind::Path(qpath) = expr.kind | ||
| && let hir::QPath::Resolved(_, path) = qpath | ||
| && let hir::def::Res::Def(def_kind, _) = path.res | ||
| && let hir::def::Res::Def(def_kind, def_id) = path.res | ||
| && let hir::def::DefKind::Static { safety: _, mutability: Mutability::Mut, nested: false } = | ||
| def_kind | ||
| { | ||
| return Some(err_span); | ||
| return Some(StaticMutInfo { err_span, def_id }); | ||
| } | ||
| None | ||
| } | ||
|
|
@@ -141,6 +168,7 @@ fn emit_static_mut_refs( | |
| sugg_span: Span, | ||
| mutable: Mutability, | ||
| suggest_addr_of: bool, | ||
| def_id: DefId, | ||
| ) { | ||
| let (shared_label, shared_note, mut_note, sugg) = match mutable { | ||
| Mutability::Mut => { | ||
|
|
@@ -155,9 +183,106 @@ fn emit_static_mut_refs( | |
| } | ||
| }; | ||
|
|
||
| let (interior_mutability_help, interior_mutability_sugg) = | ||
| interior_mutability_suggestion(cx, def_id); | ||
|
|
||
| cx.emit_span_lint( | ||
| STATIC_MUT_REFS, | ||
| span, | ||
| RefOfMutStatic { span, sugg, shared_label, shared_note, mut_note }, | ||
| RefOfMutStatic { | ||
| span, | ||
| sugg, | ||
| shared_label, | ||
| shared_note, | ||
| mut_note, | ||
| interior_mutability_help, | ||
| interior_mutability_sugg, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| // FIXME: This builds suggestion spans by handcrafting from source text. | ||
| // Replace this with HIR-based handling once we can identify the `mut` token | ||
| // in the static declaration that way. | ||
| // Context: https://github.com/rust-lang/rust/pull/151362/changes#r3210767018 | ||
| fn interior_mutability_suggestion( | ||
| cx: &LateContext<'_>, | ||
| def_id: DefId, | ||
| ) -> (bool, Option<StaticMutRefsInteriorMutabilitySugg>) { | ||
| let static_ty = cx.tcx.type_of(def_id).skip_binder(); | ||
| let has_interior_mutability = !static_ty.is_freeze(cx.tcx, cx.typing_env()); | ||
|
|
||
| if !has_interior_mutability { | ||
| return (false, None); | ||
| } | ||
|
|
||
| let sugg = | ||
| static_mutability_span(cx, def_id).map(|span| StaticMutRefsInteriorMutabilitySugg { span }); | ||
| (sugg.is_none(), sugg) | ||
| } | ||
|
|
||
| fn static_mutability_span(cx: &LateContext<'_>, def_id: DefId) -> Option<Span> { | ||
| let hir_id = cx.tcx.hir_get_if_local(def_id)?; | ||
| let hir::Node::Item(item) = hir_id else { return None }; | ||
| let (mutability, ident) = match item.kind { | ||
| hir::ItemKind::Static(mutability, ident, _, _) => (mutability, ident), | ||
| _ => return None, | ||
| }; | ||
| if mutability != hir::Mutability::Mut { | ||
| return None; | ||
| } | ||
|
|
||
| let vis_span = item.vis_span.find_ancestor_inside(item.span)?; | ||
| if !item.span.can_be_used_for_suggestions() || !vis_span.can_be_used_for_suggestions() { | ||
| return None; | ||
| } | ||
|
|
||
| let header_span = vis_span.between(ident.span); | ||
| if !header_span.can_be_used_for_suggestions() { | ||
| return None; | ||
| } | ||
|
|
||
| let source_map = cx.sess().source_map(); | ||
| let snippet = source_map.span_to_snippet(header_span).ok()?; | ||
|
|
||
| let (_static_start, static_end) = find_word(&snippet, "static", 0)?; | ||
| let (mut_start, mut_end) = find_word(&snippet, "mut", static_end)?; | ||
| let mut_end = extend_trailing_space(&snippet, mut_end); | ||
|
Comment on lines
+245
to
+250
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there no way of attaining this from the HIR instead?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. AFAIK we cannot get precise
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm still unhappy with having to build spans from text, but let's land the improvement. Could you add a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added FIXME in ee8949e |
||
|
|
||
| Some( | ||
| header_span | ||
| .with_lo(header_span.lo() + BytePos(mut_start as u32)) | ||
| .with_hi(header_span.lo() + BytePos(mut_end as u32)), | ||
| ) | ||
| } | ||
|
|
||
| fn find_word(snippet: &str, word: &str, start: usize) -> Option<(usize, usize)> { | ||
| let bytes = snippet.as_bytes(); | ||
| let word_bytes = word.as_bytes(); | ||
| let mut search = start; | ||
| while search <= snippet.len() { | ||
| let found = snippet[search..].find(word)?; | ||
| let idx = search + found; | ||
| let end = idx + word_bytes.len(); | ||
| let before_ok = idx == 0 || !is_ident_char(bytes[idx - 1]); | ||
| let after_ok = end >= bytes.len() || !is_ident_char(bytes[end]); | ||
| if before_ok && after_ok { | ||
| return Some((idx, end)); | ||
| } | ||
| search = end; | ||
| } | ||
| None | ||
| } | ||
|
|
||
| fn is_ident_char(byte: u8) -> bool { | ||
| byte.is_ascii_alphanumeric() || byte == b'_' | ||
| } | ||
|
|
||
| fn extend_trailing_space(snippet: &str, mut end: usize) -> usize { | ||
| if let Some(ch) = snippet[end..].chars().next() | ||
| && (ch == ' ' || ch == '\t') | ||
| { | ||
| end += ch.len_utf8(); | ||
| } | ||
| end | ||
| } | ||
|
Comment on lines
+259
to
+288
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer for us not to have to do all this :-/ But that'll only be possible if we can take the info from the HIR. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| //@ edition:2024 | ||
| // Ensure that we don't make structured suggestions for interior mutability note | ||
| // when span is not available. | ||
|
|
||
| use std::sync::Mutex; | ||
|
|
||
| macro_rules! declare_mutex { | ||
| () => { | ||
| static mut MACRO_MUTEX: Mutex<bool> = Mutex::new(false); | ||
| }; | ||
| } | ||
|
|
||
| declare_mutex!(); | ||
|
|
||
| fn main() { | ||
| let _lock = unsafe { MACRO_MUTEX.lock().unwrap() }; | ||
| //~^ ERROR creating a shared reference to mutable static [static_mut_refs] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| error: creating a shared reference to mutable static | ||
| --> $DIR/static-mut-refs-interior-mutability-no-sugg.rs:16:26 | ||
| | | ||
| LL | let _lock = unsafe { MACRO_MUTEX.lock().unwrap() }; | ||
| | ^^^^^^^^^^^^^^^^^^ shared reference to mutable static | ||
| | | ||
| = note: shared references to mutable statics are dangerous; it's undefined behavior if the static is mutated or if a mutable reference is created for it while the shared reference lives | ||
| = help: use a type that relies on "interior mutability" instead; to read more on this, visit <https://doc.rust-lang.org/reference/interior-mutability.html> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This suggestion does not make sense to me. The type
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I filed an issue for this since it seems like a clearly incorrect diagnostic to me: #158735 |
||
| = note: for more information, see <https://doc.rust-lang.org/edition-guide/rust-2024/static-mut-references.html> | ||
| = note: `#[deny(static_mut_refs)]` (part of `#[deny(rust_2024_compatibility)]`) on by default | ||
|
|
||
| error: aborting due to 1 previous error | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| //@ edition:2024 | ||
| //@ run-rustfix | ||
|
|
||
| #![allow(unused_unsafe)] | ||
|
|
||
| use std::sync::Mutex; | ||
|
|
||
| static STDINOUT_MUTEX: Mutex<bool> = Mutex::new(false); | ||
|
|
||
| fn main() { | ||
| let _lock = unsafe { STDINOUT_MUTEX.lock().unwrap() }; | ||
| //~^ ERROR creating a shared reference to mutable static [static_mut_refs] | ||
| } |
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add another test for the case where the structured suggestion wouldn't trigger but the new note would?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added in 33d92f7. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| //@ edition:2024 | ||
| //@ run-rustfix | ||
|
|
||
| #![allow(unused_unsafe)] | ||
|
|
||
| use std::sync::Mutex; | ||
|
|
||
| static mut STDINOUT_MUTEX: Mutex<bool> = Mutex::new(false); | ||
|
|
||
| fn main() { | ||
| let _lock = unsafe { STDINOUT_MUTEX.lock().unwrap() }; | ||
| //~^ ERROR creating a shared reference to mutable static [static_mut_refs] | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| error: creating a shared reference to mutable static | ||
| --> $DIR/static-mut-refs-interior-mutability.rs:11:26 | ||
| | | ||
| LL | let _lock = unsafe { STDINOUT_MUTEX.lock().unwrap() }; | ||
| | ^^^^^^^^^^^^^^^^^^^^^ shared reference to mutable static | ||
| | | ||
| = note: shared references to mutable statics are dangerous; it's undefined behavior if the static is mutated or if a mutable reference is created for it while the shared reference lives | ||
| = note: for more information, see <https://doc.rust-lang.org/edition-guide/rust-2024/static-mut-references.html> | ||
| = note: `#[deny(static_mut_refs)]` (part of `#[deny(rust_2024_compatibility)]`) on by default | ||
| help: this type already provides "interior mutability", so its binding doesn't need to be declared as mutable | ||
| | | ||
| LL - static mut STDINOUT_MUTEX: Mutex<bool> = Mutex::new(false); | ||
| LL + static STDINOUT_MUTEX: Mutex<bool> = Mutex::new(false); | ||
| | | ||
|
|
||
| error: aborting due to 1 previous error | ||
|
|
Uh oh!
There was an error while loading. Please reload this page.
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 this is usually correct but not always correct. For instance, if the type is
(i32, Cell<i32>), then if one wants to mutate the first field one still needs astatic mut. I don't know why anyone would do that, but still, I am concerned this suggestion might reinforce an incorrect mental model where someone thinks having anUnsafeCellanywhere in thestaticallows them to mutate the entirestatic.View changes since the review