Skip to content

Feat: Add clippy stackslib, pass indexing_slicing #6188

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

kantai
Copy link
Contributor

@kantai kantai commented Jun 9, 2025

This gets stackslib to pass the first lint listed in #6156

This does a couple of things in the refactor beyond just replacing foo[..] with foo.get(..)? in order to make the usage a little more idiomatic in rust:

  • More usage of TryInto<[u8; T]> -- we previously did a lot of buffering in our codebase to get from &[u8] into a [u8; T]. This not only involves indexing or invocations of copy_from_slice(), but it also is less performant.
  • More reliance on iterator operations like .take(), .chunks(), .zip(), and .skip(), etc.

Some of the previous avoidance of iterators (in MARF in particular) had comments indicating that iterators were slower than the alternatives (for loops, and even manual for loops). I'm not sure if that used to be the case in older versions of rustc, but as of this PR, there was no measurable difference when measured using the MARF benchmarks (with optimizations turned on: I think in unoptimized builds, there is a performance difference).

@kantai kantai requested review from a team as code owners June 9, 2025 17:50
@obycode obycode self-requested a review June 11, 2025 15:15
@aldur aldur moved this to Status: In Review in Stacks Core Eng Jun 11, 2025
@aldur aldur added this to the 3.1.0.0.13 milestone Jun 11, 2025
@aldur aldur requested review from Jiloc and hstove June 12, 2025 14:52
hstove
hstove previously approved these changes Jun 25, 2025
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

}

for _i in 0..burn_sample.len() {
for _sample in burn_sample.iter() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
for _sample in burn_sample.iter() {
for sample in burn_sample.iter() {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should stay _sample because its only ever used in the test_debug!() macro, which is elided in non-test builds.

@@ -157,19 +157,12 @@ impl SortitionHash {
}

/// Convert a SortitionHash into a (little-endian) uint256
#[allow(clippy::indexing_slicing)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL about chunks_exact, which cleans this up and avoids skipping clippy. Thanks, LLM.

Also, TIL about stacks_common::util::uint. Was this just before there were better uint helpers in Rust std?

 pub fn to_uint256(&self) -> Uint256 {
      let tmp = self
          .0
          .chunks_exact(8)
          .map(|chunk| u64::from_le_bytes(chunk.try_into().unwrap()))
          .collect::<Vec<u64>>();
      Uint256(tmp.try_into().unwrap())
  }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- though maybe even as_chunks is the best thing here:

    pub fn to_uint256(&self) -> Uint256 {
        let (u64_chunks, []) = self.0.as_chunks::<8>() else {
            panic!("SortitionHash was not evenly divisible by 8")
        };

        let tmp: Vec<u64> = u64_chunks.iter().map(|chunk| u64::from_le_bytes(*chunk)).collect();
        Uint256(tmp.try_into().expect("SortitionHash should have 4 chunks of 8 bytes"))
    }

This is brand new in stable rust (as in, today's release). It creates arrays from constant-sized chunks. It's like the chunks iterator, but it returns arrays instead of slices and it doesn't need to be an iterator, because the arrays are all guaranteed to be inline.

Jiloc
Jiloc previously approved these changes Jun 26, 2025
Copy link
Contributor

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

Copy link
Contributor

@Jiloc Jiloc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm after the clippy warning is fixed!

@Jiloc Jiloc self-requested a review June 26, 2025 13:26
@kantai kantai dismissed stale reviews from Jiloc and hstove via c1a7ad4 June 26, 2025 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: In Review
Development

Successfully merging this pull request may close these issues.

4 participants