-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
base: main
Are you sure you want to change the base?
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,102 @@ 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, | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| 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 }); | ||
| (true, 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); | ||
|
|
||
| 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
+255
to
+284
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 |
|---|---|---|
|
|
@@ -11,7 +11,13 @@ LL | | }); | |
| | | ||
| = note: for more information, see <https://doc.rust-lang.org/edition-guide/rust-2024/static-mut-references.html> | ||
| = 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> | ||
| = note: `#[warn(static_mut_refs)]` (part of `#[warn(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 ONCE: Once = Once::new(); | ||
| LL + static ONCE: Once = Once::new(); | ||
| | | ||
|
Comment on lines
+14
to
+20
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. Should the note be presented given that we have the suggestion? We're telling people "use internal mutability type" and "you are already using internal mutability type". I think we can just make the bool false if the suggestion is Some. |
||
|
|
||
| warning: 1 warning emitted | ||
|
|
||
| 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] | ||
| } |
| 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,18 @@ | ||
| 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: for more information, see <https://doc.rust-lang.org/edition-guide/rust-2024/static-mut-references.html> | ||
| = 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> | ||
| = 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 | ||
|
|
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.
Is there no way of attaining this from the HIR instead?