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

Taking a shared reference of an array suppresses the unconditional_panic lint #98444

Closed
JanBeh opened this issue Jun 24, 2022 · 14 comments · Fixed by #108812 or #129517
Closed

Taking a shared reference of an array suppresses the unconditional_panic lint #98444

JanBeh opened this issue Jun 24, 2022 · 14 comments · Fixed by #108812 or #129517
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. L-unconditional_panic Lint: unconditional_panic P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@JanBeh
Copy link
Contributor

JanBeh commented Jun 24, 2022

I tried this code:

fn main() {
    let xs: [i32; 5] = [1, 2, 3, 4, 5];
    let _ = &xs; // this line suppresses the `unconditional_panic` lint
    let _ = xs[7];
}

I expected to see this happen:

error: this operation will panic at runtime
 --> src/main.rs:4:13
  |
4 |     let _ = xs[7];
  |             ^^^^^ index out of bounds: the length is 5 but the index is 7
  |
  = note: `#[deny(unconditional_panic)]` on by default

Instead, this happened:

thread 'main' panicked at 'index out of bounds: the len is 5 but the index is 7', src/main.rs:4:13

Apparently, taking a shared reference of xs will suppress the unconditional_panic lint. This seems like a false negative. While false negatives can't be avoided entirely, this case seems to be avoidable as taking a shared reference shouldn't change the type of xs (which is [i32; 5]) or modify any data in this example.

Note that

fn main() {
    let xs: [i32; 5] = [1, 2, 3, 4, 5];
    //let _ = &xs;
    let _ = xs[7];
}

will trigger the lint properly.

Meta

rustc --version --verbose:

rustc 1.63.0-nightly (43347397f 2022-06-23)
binary: rustc
commit-hash: 43347397f7c5ca9a670a3bb3890c7187e24a52ab
commit-date: 2022-06-23
host: x86_64-unknown-freebsd
release: 1.63.0-nightly
LLVM version: 14.0.5
@JanBeh JanBeh added the C-bug Category: This is a bug. label Jun 24, 2022
@JanBeh
Copy link
Contributor Author

JanBeh commented Jun 24, 2022

See also this thread on URLO.

@TheWastl
Copy link
Contributor

@rustbot claim

@poliorcetics
Copy link
Contributor

Note this is a problem because the book has an example which is supposed to fail to compile but this bug makes it fail at runtime instead

@kmdreko
Copy link

kmdreko commented Jul 15, 2022

In case no one has dug into it yet, the error is suppressed in 1.61 but not in 1.60 and before:

$ cargo +1.61 build
   Compiling mycrate v0.1.0 (/rust-tests)
    Finished dev [unoptimized + debuginfo] target(s) in 0.74s
$ cargo +1.60 build
   Compiling mycrate v0.1.0 (/rust-tests)
error: this operation will panic at runtime
   --> src/main.rs:129:13
    |
129 |     let _ = xs[7];
    |             ^^^^^ index out of bounds: the length is 5 but the index is 7
    |
    = note: `#[deny(unconditional_panic)]` on by default

error: could not compile `mycrate` due to previous error
warning: build failed, waiting for other jobs to finish...
error: build failed

@eggyal
Copy link
Contributor

eggyal commented Jul 20, 2022

@rustbot label +regression-from-stable-to-stable

@rustbot rustbot added regression-from-stable-to-stable Performance or correctness regression from one stable version to another. I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 20, 2022
@apiraino
Copy link
Contributor

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

@rustbot rustbot added P-medium Medium priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Jul 21, 2022
@TheWastl
Copy link
Contributor

This is caused by #94934. It only ever worked because the lint ran after optimizations.

I made a fix here. It specializes the length of arrays as constant, even if the array isn't. I'm going to open a PR when I have time, probably tomorrow.

In the long term, I'd like to implement some more extensive tracking of "what parts of a value are constant". For example, the compiler currently assumes that anything that had its reference taken can change, but actually, this can only happen inside UnsafeCell (as I understand it).

@steffahn
Copy link
Member

steffahn commented Sep 14, 2022

Labels from the closed duplicate #101470, @rustbot label A-diagnostics, A-lint, A-mir-opt, T-compiler.

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-mir-opt Area: MIR optimizations T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 14, 2022
@cjgillot
Copy link
Contributor

cjgillot commented Mar 6, 2023

Fixed on stable
@rustbot label E-needs-test

@rustbot rustbot added the E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. label Mar 6, 2023
@albertlarsan68
Copy link
Member

I claim the add test part

@albertlarsan68 albertlarsan68 self-assigned this Mar 6, 2023
@albertlarsan68
Copy link
Member

This lint only appears on full builds (not check), but this will be fixed once #108730 lands.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Mar 6, 2023
…rieb

Add regression test for rust-lang#98444

cc rust-lang#108730 this will need to be changed to a `check-fail` test once it lands.

Fixes rust-lang#98444
bors added a commit to rust-lang-ci/rust that referenced this issue Mar 6, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#107801 (const_eval: `implies_by` in `rustc_const_unstable`)
 - rust-lang#108750 (Fix `ObligationCtxt::sub`)
 - rust-lang#108780 (Add regression tests for issue 70919)
 - rust-lang#108786 (Check for free regions in MIR validation)
 - rust-lang#108790 (Do not ICE when interpreting a cast between non-monomorphic types)
 - rust-lang#108803 (Do not ICE when failing to normalize in ConstProp.)
 - rust-lang#108807 (Emit the suspicious_auto_trait_impls for negative impls as well)
 - rust-lang#108812 (Add regression test for rust-lang#98444)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@apiraino
Copy link
Contributor

Reopening because I feel this is not yet solved, see Zulip notes on #114840.

As per the prev. comment, #108730 should nail down this issue?

@apiraino apiraino reopened this Aug 15, 2023
@jieyouxu jieyouxu added the L-unconditional_panic Lint: unconditional_panic label May 13, 2024
@veera-sivarajan
Copy link
Contributor

veera-sivarajan commented Aug 24, 2024

MRE:

fn main() {
    let slice = &[0, 1];
    let _ = slice[2];
}

Also, duplicate of #38035

@veera-sivarajan
Copy link
Contributor

Another variant of this same issue: #109260

@bors bors closed this as completed in f09e5a7 Oct 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 5, 2024
Rollup merge of rust-lang#129517 - cjgillot:known-panic-array, r=pnkfelix

Compute array length from type for unconditional panic lint.

Fixes rust-lang#98444

The cases that involve slicing are harder, so rust-lang#38035 remains open.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Oct 10, 2024
Compute array length from type for unconditional panic lint.

Fixes rust-lang/rust#98444

The cases that involve slicing are harder, so rust-lang/rust#38035 remains open.
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-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. L-unconditional_panic Lint: unconditional_panic P-medium Medium priority regression-from-stable-to-stable Performance or correctness regression from one stable version to another. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet