`indexing_slicing` is not triggered in `const fn`s

[KizzyCode]

Summary

The indexing_slicing-lint is not triggered within const fns. In the following example, is_negative performs indexing on a slice (self.value[0]) which should get linted but isn't. However if I remove the const-attribute for is_negative, the lint gets triggered as expected.

#![warn(clippy::indexing_slicing)]

/// An opaque integer representation
pub struct Integer<'a> {
    /// The underlying data
    value: &'a [u8],
}
impl<'a> Integer<'a> {
    // Check whether `self` holds a negative number or not
    pub const fn is_negative(&self) -> bool {
        self.value[0] & 0b1000_0000 != 0
    }
}

Lint Name

indexing_slicing

Reproducer

I tried this code:

#![warn(clippy::indexing_slicing)]

/// An opaque integer representation
pub struct Integer<'a> {
    /// The underlying data
    value: &'a [u8],
}
impl<'a> Integer<'a> {
    // Check whether `self` holds a negative number or not
    pub const fn is_negative(&self) -> bool {
        self.value[0] & 0b1000_0000 != 0
    }
}

I expected to see this happen: The lint warns me on self.value[0].

Instead, this happened: The lint does not warn me on self.value[0] 😅

Version

rustc 1.65.0 (897e37553 2022-11-02)
binary: rustc
commit-hash: 897e37553bba8b42751c67658967889d11ecd120
commit-date: 2022-11-02
host: aarch64-apple-darwin
release: 1.65.0
LLVM version: 15.0.0

[xFrednet]

This is intentional, since all alternatives (get(), expect()) are currently sadly not possible in constant blocks. We could add a config value, to have such lints still report in const context. Would that solution work for you? 🙃

[KizzyCode]

Yes, this would work 😊
I use clippy mostly as a "reminder" with #[allow(...)] and "This is safe because ..."-comments (similar to unsafe); so for me, even if there is no other way in const-contexts, it forces me to double check if my assumptions really make sense.

Additionally it's probably a good idea to explicitly document this behavior? ☺️

[xFrednet]

Additionally it's probably a good idea to explicitly document this behavior?

Agreed, it might be good to also check other lints for this.

---

Adding the config value and adding the documentation should be fairly simple.

@rustbot label +good-first-issue

[naosense]

This seems good start to me, can I work on this? by the way, can you elaborate on the config?

[xFrednet]

@naosense yeah sure, welcome to Clippy. You can claim this issue my including @rustbot claim in a comment on the issue.

Now for the implementation. Clippy supports some configuration values to be set in a clippy.toml file. The task would basically be to add a new config value. Then find the location where the check if the expression is in a constant context happens, and make it dependent on the new configuration. Additionally, we should probably adjust the applicability and add a note to the lint message, but that can be adjusted after the initial steps :)

You can take a look at our docs for this: https://doc.rust-lang.org/clippy/development/adding_lints.html#adding-configuration-to-a-lint

You're welcome to reach out to me or any other Clippy member if you need help :)

[naosense]

@xFrednet thank you for your detailed explanation, I'll investigate it.

@rustbot claim

[naosense]

Hi, @xFrednet after i did some research, here is my plan:

1. add one config item in conf.rs, e.g. (allow_indexing_slicing_in_const: bool = true)
2. add code in clippy_lints/src/indexing_slicing.rs

#[derive(Copy, Clone)]
pub struct IndexingSlicing {
    allow_indexing_slicing_in_const: bool,
}

impl IndexingSlicing {
    pub fn new(allow_indexing_slicing_in_const: bool) -> Self {
        Self {
            allow_indexing_slicing_in_const,
        }
    }
}

3. change
   

   rust-clippy/clippy_lints/src/indexing_slicing.rs

   Lines 149 to 151 in dac600e

   	if let ExprKind::ConstBlock(..) = index.kind {
   	return;
   	}

   
   to

if self.allow_indexing_slicing_in_const && let ExprKind::ConstBlock(..) = index.kind {
    return;
}

Am i right? Thanks!

[xFrednet]

Hey @naosense, the approach is correct and looks good to me. Here are a few suggestions regarding these steps:

1. AFAIK there are other restriction lints, which are suppressed in const context for the same reason. It's probably nicer to have a more general name for the config value so that it can be reused in other lints. Maybe something like lint_restriction_in_const? (I'm not good with naming these things ^^). Including the lint category is most likely good, as I can see users wanting to enforce restriction lints in const while other category like style are less important and potentially annoying.
2. Looks perfect!
3. The main check, if the lint is currently checking a constant context, actually occurs earlier in line L89-L91.

   rust-clippy/clippy_lints/src/indexing_slicing.rs

   Lines 89 to 91 in dac600e

   	if cx.tcx.hir().is_inside_const_context(expr.hir_id) {
   	return;
   	}

   The code you marked is checking if the index value is a const block, which is interesting. It's probably good to add the config check in both locations.
4. The fourth step is, as usual, to add tests.
5. Ideally, I would also like the lint message to include a comment, that the suggestion is not applicable, if we're in a constant context. We can work on this, once the first 4 steps are done.

Besides these, that's an excellent start. You can also create a draft PR if you want me to look at your specific changes. You can assign it directly to me, by including r? @xFrendet in the PR description.