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

Missing parenthesis in static_mut_refs diagnostic suggestion #131977

Closed
CharlyCst opened this issue Oct 20, 2024 · 3 comments · Fixed by #132095 or #132756
Closed

Missing parenthesis in static_mut_refs diagnostic suggestion #131977

CharlyCst opened this issue Oct 20, 2024 · 3 comments · Fixed by #132095 or #132756
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@CharlyCst
Copy link

CharlyCst commented Oct 20, 2024

Code

static mut TEST: usize = 0;

fn main() {
    let _ = unsafe { (&TEST) as *const usize };
}

Current output

   Compiling playground v0.0.1 (/playground)
warning: creating a shared reference to mutable static is discouraged
 --> src/main.rs:4:22
  |
4 |     let _ = unsafe { (&TEST) as *const usize };
  |                      ^^^^^^^ shared reference to mutable static
  |
  = note: for more information, see <https://doc.rust-lang.org/nightly/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
  = note: `#[warn(static_mut_refs)]` on by default
help: use `&raw const` instead to create a raw pointer
  |
4 |     let _ = unsafe { &raw const TEST) as *const usize };
  |                      ~~~~~~~~~~

warning: `playground` (bin "playground") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.73s
     Running `target/debug/playground`

Desired output

   Compiling playground v0.0.1 (/playground)
warning: creating a shared reference to mutable static is discouraged
 --> src/main.rs:4:22
  |
4 |     let _ = unsafe { (&TEST) as *const usize };
  |                      ^^^^^^^ shared reference to mutable static
  |
  = note: for more information, see <https://doc.rust-lang.org/nightly/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
  = note: `#[warn(static_mut_refs)]` on by default
help: use `&raw const` instead to create a raw pointer
  |
4 |     let _ = unsafe { (&raw const TEST) as *const usize };
  |                       ~~~~~~~~~~

warning: `playground` (bin "playground") generated 1 warning
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.73s
     Running `target/debug/playground`

Rationale and extra context

The suggestion removes the left parenthesis, but keeps the right parenthesis. Of course the proposed change does not compile.
Original code: (&TEST) suggestion: &raw const TEST)

Other cases

No response

Rust Version

rustc 1.84.0-nightly (da93539 2024-10-19)
binary: rustc
commit-hash: da93539
commit-date: 2024-10-19
host: aarch64-apple-darwin
release: 1.84.0-nightly
LLVM version: 19.1.1

Anything else?

Link to rust playground

@CharlyCst CharlyCst added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 20, 2024
@jieyouxu jieyouxu added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Oct 21, 2024
@indegrowl
Copy link

Pretty straightforward! I'll work on it. It'll be my first PR.

@gechelberger
Copy link

This is actually more complicated than I assumed.

The offending code is operating on the HIR where parens have already been elided into the token tree heirarchy.

The suggested replacement of &raw const or &raw mut is dropped into the span err_span.with_hi(ex.span.lo()) from here.

The cleanest way to solve it would be to narrow the referenced span when lowering from the AST to the HIR so that the 'hir::Expr' node's Span doesn't point to or include the parens - only the contents of them - since the HIR isn't supposed to know about parens anyways.

I doubt that this would change the correctness of other lints and errors operating on the HIR, but it almost certainly breaks a lot of the ui compiletests which would then have different spans highlighted in their stderr contents.

Are there other lints/errors that have run into similar issues to reference?

@gechelberger
Copy link

For reference, I commented out this line which replaces the HIR Span with the AST Paren Span to get a sense for how significant of an effect it would have:

It is definitely not the solution on its own.

  • It does solve this issue.
  • It fails 95 cases in tests/ui
    • Some of these are still semantically correct with the only differences being the locations of the highlighted error spans.
    • It causes semantic problems for calls
      • method calls: (a.unwrap)()
      • closures: (|| {})(|| { let b = 1; })
    • It causes a reciprocal syntax problem with expressions that directly operate outside the parens contents
      • indexing: (a as [Foo; 3]).0 => (a as [Foo; 3][0] instead of (a as [Foo; 3])[0]
    • possibly other issues

ui-tests.txt

@jieyouxu jieyouxu added E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. and removed E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. labels Oct 24, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 7, 2024
…iser

Fix rust-lang#131977 parens mangled in shared mut static lint suggestion

Resolves rust-lang#131977 for static mut references after discussion with
Esteban & Jieyou on [t-compiler/help](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/linting.20with.20parens.20in.20the.20HIR).

This doesn't do anything to change the underlying issue if there are other expressions that generate lint suggestions which need to be applied within parentheses.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Nov 7, 2024
…iser

Fix rust-lang#131977 parens mangled in shared mut static lint suggestion

Resolves rust-lang#131977 for static mut references after discussion with
Esteban & Jieyou on [t-compiler/help](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/linting.20with.20parens.20in.20the.20HIR).

This doesn't do anything to change the underlying issue if there are other expressions that generate lint suggestions which need to be applied within parentheses.
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 7, 2024
…llaumeGomez

Rollup of 8 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132738 (Initialize channel `Block`s directly on the heap)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 8, 2024
…iser

Fix rust-lang#131977 parens mangled in shared mut static lint suggestion

Resolves rust-lang#131977 for static mut references after discussion with
Esteban & Jieyou on [t-compiler/help](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/linting.20with.20parens.20in.20the.20HIR).

This doesn't do anything to change the underlying issue if there are other expressions that generate lint suggestions which need to be applied within parentheses.
workingjubilee added a commit to workingjubilee/rustc that referenced this issue Nov 8, 2024
…iser

Fix rust-lang#131977 parens mangled in shared mut static lint suggestion

Resolves rust-lang#131977 for static mut references after discussion with
Esteban & Jieyou on [t-compiler/help](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/linting.20with.20parens.20in.20the.20HIR).

This doesn't do anything to change the underlying issue if there are other expressions that generate lint suggestions which need to be applied within parentheses.
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132639 (core: move intrinsics.rs into intrinsics folder)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132737 (bootstrap: Print better message if lock pid isn't available)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)
 - rust-lang#132741 (Update mips64 data layout to match LLVM 20 change)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors closed this as completed in cc0ec04 Nov 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Nov 8, 2024
Rollup merge of rust-lang#132095 - gechelberger:fix-131977, r=wesleywiser

Fix rust-lang#131977 parens mangled in shared mut static lint suggestion

Resolves rust-lang#131977 for static mut references after discussion with
Esteban & Jieyou on [t-compiler/help](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/linting.20with.20parens.20in.20the.20HIR).

This doesn't do anything to change the underlying issue if there are other expressions that generate lint suggestions which need to be applied within parentheses.
mati865 pushed a commit to mati865/rust that referenced this issue Nov 12, 2024
…iser

Fix rust-lang#131977 parens mangled in shared mut static lint suggestion

Resolves rust-lang#131977 for static mut references after discussion with
Esteban & Jieyou on [t-compiler/help](https://rust-lang.zulipchat.com/#narrow/channel/182449-t-compiler.2Fhelp/topic/linting.20with.20parens.20in.20the.20HIR).

This doesn't do anything to change the underlying issue if there are other expressions that generate lint suggestions which need to be applied within parentheses.
mati865 pushed a commit to mati865/rust that referenced this issue Nov 12, 2024
…kingjubilee

Rollup of 10 pull requests

Successful merges:

 - rust-lang#130586 (Set "symbol name" in raw-dylib import libraries to the decorated name)
 - rust-lang#131913 (Add `{ignore,needs}-{rustc,std}-debug-assertions` directive support)
 - rust-lang#132095 (Fix rust-lang#131977 parens mangled in shared mut static lint suggestion)
 - rust-lang#132131 ([StableMIR] API to retrieve definitions from crates)
 - rust-lang#132639 (core: move intrinsics.rs into intrinsics folder)
 - rust-lang#132696 (Compile `test_num_f128` conditionally on `reliable_f128_math` config)
 - rust-lang#132737 (bootstrap: Print better message if lock pid isn't available)
 - rust-lang#132739 (Fix `librustdoc/scrape_examples.rs` formatting)
 - rust-lang#132740 (Update test for LLVM 20's new vector splat syntax)
 - rust-lang#132741 (Update mips64 data layout to match LLVM 20 change)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. E-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
4 participants