Add interior-mutability suggestion to static_mut_refs#151362
Conversation
This comment has been minimized.
This comment has been minimized.
41e2b83 to
b9914a8
Compare
This comment has been minimized.
This comment has been minimized.
b9914a8 to
7c76d7e
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| = 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(); | ||
| | |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
Is there no way of attaining this from the HIR instead?
There was a problem hiding this comment.
AFAIK we cannot get precise mut spans on HIR and we would still require a string trick like this. Mutability itself doesn't contain spans currently, e.g. https://doc.rust-lang.org/nightly/nightly-rustc/rustc_ast/ast/struct.StaticItem.html.
There was a problem hiding this comment.
I'm still unhappy with having to build spans from text, but let's land the improvement. Could you add a // FIXME to these functions mentioning something along the way of "we want to replace this with operating on the HIR in some way"?
| 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 | ||
| } |
There was a problem hiding this comment.
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.
7c76d7e to
647dead
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@JohnTitor Seem there are a few comments pending, am I reading correctly? I'll switch the review flag to you to incorporate changes. Feel free to request a review with @rustbot author |
There was a problem hiding this comment.
Can we add another test for the case where the structured suggestion wouldn't trigger but the new note would?
| 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); |
There was a problem hiding this comment.
I'm still unhappy with having to build spans from text, but let's land the improvement. Could you add a // FIXME to these functions mentioning something along the way of "we want to replace this with operating on the HIR in some way"?
647dead to
ee8949e
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Thanks for the review! |
…, r=estebank Add interior-mutability suggestion to `static_mut_refs` Closes rust-lang#151131 r? @estebank I've skipped to expand catching below code as a mutable _reference_ shouldn't be involved (maybe a new lint would be needed?): ```rs static mut COUNTER: u64 = 0; fn main() { unsafe { COUNTER = 1 }; } ```
…uwer Rollup of 14 pull requests Successful merges: - #151742 (Remove redundant information in `rustc_abi::Variants`) - #151362 (Add interior-mutability suggestion to `static_mut_refs`) - #156121 (compiler: suggest `.collect()` when `String` is expected and `Iterator` is found) - #156208 (Emit retags in codegen to support BorrowSanitizer (part 1)) - #156596 (Split `LintExpectationId`s) - #156607 (ci: Update FreeBSD version to FreeBSD 14) - #156376 (suggest hex escapes for C-style escapes) - #156577 (Test EII UI tests with prefer-dynamic) - #156585 (explicit tail calls: ignore some tests on unsupported LLVM targets) - #156598 (Avoid rustfix suggestions for macro-expanded unused imports) - #156616 (rustdoc: add test case for `-Drustdoc::` and `--cap-lints`) - #156633 (Add regression test for issue #41261) - #156635 (rename unexpected_try_recover function) - #156636 (minor `rustc_mir_transform` cleanup)
…uwer Rollup of 14 pull requests Successful merges: - #151742 (Remove redundant information in `rustc_abi::Variants`) - #151362 (Add interior-mutability suggestion to `static_mut_refs`) - #156121 (compiler: suggest `.collect()` when `String` is expected and `Iterator` is found) - #156208 (Emit retags in codegen to support BorrowSanitizer (part 1)) - #156596 (Split `LintExpectationId`s) - #156607 (ci: Update FreeBSD version to FreeBSD 14) - #156376 (suggest hex escapes for C-style escapes) - #156577 (Test EII UI tests with prefer-dynamic) - #156585 (explicit tail calls: ignore some tests on unsupported LLVM targets) - #156598 (Avoid rustfix suggestions for macro-expanded unused imports) - #156616 (rustdoc: add test case for `-Drustdoc::` and `--cap-lints`) - #156633 (Add regression test for issue #41261) - #156635 (rename unexpected_try_recover function) - #156636 (minor `rustc_mir_transform` cleanup)
Rollup merge of #151362 - JohnTitor:interior-mutability-sugg, r=estebank Add interior-mutability suggestion to `static_mut_refs` Closes #151131 r? @estebank I've skipped to expand catching below code as a mutable _reference_ shouldn't be involved (maybe a new lint would be needed?): ```rs static mut COUNTER: u64 = 0; fn main() { unsafe { COUNTER = 1 }; } ```
…uwer Rollup of 14 pull requests Successful merges: - rust-lang/rust#151742 (Remove redundant information in `rustc_abi::Variants`) - rust-lang/rust#151362 (Add interior-mutability suggestion to `static_mut_refs`) - rust-lang/rust#156121 (compiler: suggest `.collect()` when `String` is expected and `Iterator` is found) - rust-lang/rust#156208 (Emit retags in codegen to support BorrowSanitizer (part 1)) - rust-lang/rust#156596 (Split `LintExpectationId`s) - rust-lang/rust#156607 (ci: Update FreeBSD version to FreeBSD 14) - rust-lang/rust#156376 (suggest hex escapes for C-style escapes) - rust-lang/rust#156577 (Test EII UI tests with prefer-dynamic) - rust-lang/rust#156585 (explicit tail calls: ignore some tests on unsupported LLVM targets) - rust-lang/rust#156598 (Avoid rustfix suggestions for macro-expanded unused imports) - rust-lang/rust#156616 (rustdoc: add test case for `-Drustdoc::` and `--cap-lints`) - rust-lang/rust#156633 (Add regression test for issue rust-lang/rust#41261) - rust-lang/rust#156635 (rename unexpected_try_recover function) - rust-lang/rust#156636 (minor `rustc_mir_transform` cleanup)
View all comments
Closes #151131
r? @estebank
I've skipped to expand catching below code as a mutable reference shouldn't be involved (maybe a new lint would be needed?):