Skip to content

Resolve, rework, explain clippy lints #1661

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

Merged
merged 1 commit into from
Apr 3, 2025

Conversation

jhpratt
Copy link
Contributor

@jhpratt jhpratt commented Apr 2, 2025

Description

All remaining clippy lints are converted to #[expect] with reasons attached. The only exceptions are a handful of #[allow] attributes in testing due to the manner in which tests are compiled.

Checklist

  • Tests added/amended
  • bazel test //... passes locally — not done; cargo should be sufficient here
  • PR is contained in a single commit, using git amend see some docs

This change is Reviewable

Copy link
Contributor

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Looks really good!

As these are quite a few changes let's split this into two parts. I.e. one commit for the naming changes and one for the expect/allow changes.

Reviewed 44 of 46 files at r1, 1 of 1 files at r2.
Reviewable status: 0 of 2 LGTMs obtained, and 45 of 46 files reviewed, and 6 discussions need to be resolved


nativelink-service/src/bep_server.rs line 205 at r2 (raw file):

    type PublishBuildToolEventStreamStream = PublishBuildToolEventStreamStream;

    #[allow(clippy::blocks_in_conditions)]

btw why is this no longer necessary?


nativelink-config/src/stores.rs line 79 at r2 (raw file):

    /// ```
    ///
    ExperimentalS3Store(S3Spec),

nit: Please double-check that this one is still valid as experimental_s3_config. There is a chance that it could resolve to experimental_s_3_store now which would be breaking.


nativelink-config/src/stores.rs line 677 at r2 (raw file):

    ///
    /// see: <https://lz4.github.io/lz4/>
    Lz4(Lz4Config),

nit: Same here. It'll probably be fine, but better to be sure.


nativelink-store/tests/filesystem_store_test.rs line 670 at r2 (raw file):

#[nativelink_test]
#[allow(clippy::await_holding_refcell_ref)]

btw if this lint didn't trigger, shouldn't clippy have raised a warning that this line is redundant?


nativelink-worker/src/running_actions_manager.rs line 96 at r2 (raw file):

#[derive(Debug, Deserialize)]
#[serde(rename_all = "snake_case")]
enum SideChannelFailureReason {

I believe we can remove the serde rename here as it's not exposed to the config. The comment about the doc update seems to refer to the docstring here:

/// "failure": "timeout",


nativelink-util/src/origin_event_publisher.rs line 91 at r2 (raw file):

            #[expect(
                clippy::drain_collect,
                reason = "retain capacity for the next batch, as `mem::take` would move the \

This seems to make the reasoning less informative.

Copy link
Contributor Author

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 LGTMs obtained, and 45 of 46 files reviewed, and 2 discussions need to be resolved


nativelink-config/src/stores.rs line 79 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Please double-check that this one is still valid as experimental_s3_config. There is a chance that it could resolve to experimental_s_3_store now which would be breaking.

verified it is experimental_s3_config by viewing the macro expansion


nativelink-config/src/stores.rs line 677 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: Same here. It'll probably be fine, but better to be sure.

also verified as lz4


nativelink-service/src/bep_server.rs line 205 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

btw why is this no longer necessary?

Resolved by tokio-rs/tracing#2880


nativelink-store/tests/filesystem_store_test.rs line 670 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

btw if this lint didn't trigger, shouldn't clippy have raised a warning that this line is redundant?

Nope — that's the difference between #[allow] and #[expect].


nativelink-util/src/origin_event_publisher.rs line 91 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

This seems to make the reasoning less informative.

I'll copy the previous text in full.


nativelink-worker/src/running_actions_manager.rs line 96 at r2 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

I believe we can remove the serde rename here as it's not exposed to the config. The comment about the doc update seems to refer to the docstring here:

/// "failure": "timeout",

The reason I included it despite it not being necessary is in case variants were added in the future (despite the enum not being #[non_exhaustive]. If it's not necessary at all, why was it lowercased previously?

Copy link
Contributor Author

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

done

Reviewable status: 0 of 2 LGTMs obtained, and 26 of 46 files reviewed, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-cc / ubuntu-24.04, Local / lre-rs / macos-15, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, Web Platform Deployment / macos-15, Web Platform Deployment / ubuntu-24.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, pre-commit-checks, ubuntu-24.04, ubuntu-24.04 / stable, vale, windows-2022 / stable, and 2 discussions need to be resolved

All remaining clippy lints are converted to `#[expect]` with reasons
attached. The only exceptions are a handful of `#[allow]` attributes
  in testing due to the manner in which tests are compiled.
Copy link
Contributor

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 46 files at r1, 1 of 30 files at r3, 37 of 37 files at r4, all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, ubuntu-24.04 / stable, windows-2022 / stable

Copy link
Contributor

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Bazel Dev / macos-15, Bazel Dev / ubuntu-24.04, Cargo Dev / macos-15, Cargo Dev / ubuntu-24.04, Coverage, Installation / macos-13, Installation / macos-14, Installation / macos-15, Installation / ubuntu-22.04, Installation / ubuntu-24.04, Local / lre-rs / ubuntu-24.04, NativeLink.com Cloud / Remote Cache / macos-15, NativeLink.com Cloud / Remote Cache / ubuntu-24.04, Publish image, Publish nativelink-worker-init, Remote / lre-cc / large-ubuntu-22.04, Remote / lre-rs / large-ubuntu-22.04, asan / ubuntu-24.04, integration-tests (24.04), macos-15, ubuntu-24.04 / stable, windows-2022 / stable

@aaronmondal aaronmondal enabled auto-merge (squash) April 3, 2025 16:20
Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 1 of 2 LGTMs obtained, and all files reviewed, and pending CI: Coverage, Installation / macos-13

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 2 LGTMs obtained, and all files reviewed, and pending CI: Coverage, Installation / macos-13

@aaronmondal aaronmondal merged commit 8d97af7 into TraceMachina:main Apr 3, 2025
34 checks passed
@jhpratt jhpratt deleted the clippy-lints branch April 3, 2025 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants