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

indexing_slicing should not fire if a valid array index comes from a constant function that is evaluated at compile-time #8348

Closed
c410-f3r opened this issue Jan 25, 2022 · 7 comments · Fixed by #8588
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-negative Issue: The lint should have been triggered on code, but wasn't

Comments

@c410-f3r
Copy link
Contributor

c410-f3r commented Jan 25, 2022

  1. From a inline constant block (indexing_slicing should not fire if a valid array index comes from a constant function that is evaluated at compile-time #8348 (comment)).
#![feature(inline_const)]
#![forbid(clippy::indexing_slicing)]

const FOO: [i32; 1] = [1];

const fn idx() -> usize {
    0
}

pub fn main() {
    assert_eq!(FOO[const { idx() }], 1);
}
  1. From a const declaration (indexing_slicing should not fire if a valid array index comes from a constant function that is evaluated at compile-time #8348 (comment)).
#![forbid(clippy::indexing_slicing)]

const FOO: [i32; 1] = [1];
const REF: &i32 = &FOO[idx()];

const fn idx() -> usize {
    0
}

pub fn main() {
  assert_eq!(REF, &1);
}
@c410-f3r c410-f3r added C-bug Category: Clippy is not doing the correct thing I-false-negative Issue: The lint should have been triggered on code, but wasn't labels Jan 25, 2022
@xFrednet
Copy link
Member

I think here we should distinguish if the type is an array with a fixed size, as that will be caught by the compiler or a slice. The lint should still trigger in the later case, as the range check will be done at runtime. This should be a simple type check 🙃

@rustbot label +good-first-issue

@rustbot rustbot added the good-first-issue These issues are a good way to get started with Clippy label Jan 25, 2022
@camsteffen
Copy link
Contributor

I think this is functioning as designed. const functions are not evaluated at compile-time unless it is in a const context. If you change idx to return 1, it will compile without warning and panic at runtime. Note that this is a restriction lint so it's generally not recommended unless you have a specific reason to use it.

@PatchMixolydic
Copy link
Contributor

Here's a disassembly of a similar code snippet showing that the call to idx isn't necessarily evaluated at compile time. Incidentally, this lint still trips when the compiler is forced to evaluate idx() at compile time using an inline const block (playground):

#![feature(inline_const)]
#![forbid(clippy::indexing_slicing)]

const FOO: [i32; 1] = [1];

const fn idx() -> usize {
    0
}

pub fn main() {
    assert_eq!(FOO[const { idx() }], 1);
}
error: indexing may panic
  --> src/main.rs:11:16
   |
11 |     assert_eq!(FOO[const { idx() }], 1);
   |                ^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> src/main.rs:2:11
   |
2  | #![forbid(clippy::indexing_slicing)]
   |           ^^^^^^^^^^^^^^^^^^^^^^^^
   = help: consider using `.get(n)` or `.get_mut(n)` instead
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#indexing_slicing

This should presumably be allowed for the same reason as normal array indexing (any misuse would be caught by const_err).

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jan 25, 2022

const functions are not evaluated at compile-time unless it is in a const context

Here's a disassembly of a similar code snippet showing that the call to idx isn't necessarily evaluated at compile time.

Oh, I forgot about this feature. Thanks for pointing out such behavior.

In addition to what @PatchMixolydic said, the following snippet should also not fire indexing_slicing:

#![forbid(clippy::indexing_slicing)]

const FOO: [i32; 1] = [1];
const REF: &i32 = &FOO[idx()];

const fn idx() -> usize {
    0
}

pub fn main() {
  assert_eq!(REF, &1);
}

@c410-f3r c410-f3r changed the title indexing_slicing should not fire if the index comes from a constant function indexing_slicing should not fire if the index comes from a constant function that is evaluated at compile-time Jan 25, 2022
@c410-f3r
Copy link
Contributor Author

Updated the issue based on the provided feedback

@c410-f3r c410-f3r changed the title indexing_slicing should not fire if the index comes from a constant function that is evaluated at compile-time indexing_slicing should not lint valid arrays if an index comes from a constant function that is evaluated at compile-time Jan 25, 2022
@c410-f3r c410-f3r changed the title indexing_slicing should not lint valid arrays if an index comes from a constant function that is evaluated at compile-time indexing_slicing should not fire if a valid array index comes from a constant function that is evaluated at compile-time Jan 25, 2022
@pitaj
Copy link
Contributor

pitaj commented Mar 26, 2022

@rustbot claim

@c410-f3r
Copy link
Contributor Author

Thanks @pitaj

bors added a commit that referenced this issue Mar 30, 2022
Do not fire `panic` in a constant environment

Let rustc handle panics in constant environments.

Since #8348 I thought that such modification would require a lot of work but thanks to #8588 I now know that it is not the case.

changelog: [`panic`]: No longer lint in constant context. `rustc` already handles this.
@bors bors closed this as completed in 41f2ecc Apr 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing good-first-issue These issues are a good way to get started with Clippy I-false-negative Issue: The lint should have been triggered on code, but wasn't
Projects
None yet
6 participants