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

ci: add clippy job #1050

Closed
wants to merge 3 commits into from
Closed

ci: add clippy job #1050

wants to merge 3 commits into from

Conversation

Mossaka
Copy link
Member

@Mossaka Mossaka commented May 23, 2023

This PR adds a Clippy job to CI. This can wait until #956 and #1027 merged in to main.

The rest of the changes are auto-generated by cargo clippy --all --fix and then followed by cargo fmt --all

Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
…re some left for manuel fix

Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
Signed-off-by: Jiaxiao Zhou (Mossaka) <duibao55328@gmail.com>
@pchickey
Copy link
Contributor

pchickey commented May 23, 2023

Our (Fastly's wasmtime) team has found clippy suggestions to not be appropriate for a CI check. They are unstable across versions of clippy, and so create churn around new toolchain releases, and only rarely find correctness issues with code that rustc warnings don't find.

@Mossaka
Copy link
Member Author

Mossaka commented May 24, 2023

@pchickey ack, do you think the changed code by running clippy is useful or not? I can delete the CI part but leave the rest of the changes in this PR. Obviously this is just one-time fix but I do think some of the changes are meaningful and can potentially make code more performant.

@alexcrichton
Copy link
Member

I don't personally agree with all of clippy's changes, but many of them I think are fine to land, yes. I'm not a huge fan, for example, of changes like format!("foo") to "foo".to_string() when it ends up creating code that looks like:

let s = match foo {
    A(..) => format!(...),
    B(..) => format!(...),
    C => "...".to_string(),
    D(..) => format!(...),
};

where here while I don't disagree it's probably more performant to do .to_string() many of these usages are not hyper-optimized for performance and I find it idiomatically jarring for there to be an odd-one-out where it's otherwise important I think to have consistency across the local codebase.

Things like removing explicit <'a> or making &foo just foo is fine of course, but clippy also does opinionated things which I don't think are universal truths.

@Mossaka
Copy link
Member Author

Mossaka commented May 24, 2023

I don't have a strong opinion here, meaning that I am okay with closing this PR without merging it. I do agree with some of the assements here

  1. clippy as a CI job can create churn for new releases
  2. some of the clippy suggestions are not idiomatic

Perhaps there is a middle ground here in which I think will benefit this project as a whole. I am thinking about

  1. Create a list of lints to allow and pass them to clippy as arguments using cargo clippy -- -A clippy::lint_name
  2. Make the CI job optional. In fact, clippy has different levels of lints "allow/warn/deny". Perhaps we can allow the CI job to fail only if clippy find deny linting issues, and otherwise gives a warning without breaking the build.

Futhermore, the work we've done to create this allow-list for this project can potentially apply to other rust projects in the BCA. I am totally aware the assement of a list might be time-consuming and this is probably not the best time to do this work.

@alexcrichton
Copy link
Member

Personally I'm not a fan of an allow-fail CI job since that means it'll fail quickly and won't be maintained. Worse still contributors will feel obligated to make it pass and get confused when it's their code that's not causing the problems.

Additionally if the purpose is to not use clippy I would at least personally prefer to not have configuration files all pertaining to clippy and its lints. That also gives off the confusing impression that clippy is regularly run, which it's not.

I unfortunately don't see a middle ground here other than "we'll accept PRs where folks feel clippy lints if they feel so inclined to do so" along with reviewing the PR on its own merits independent of whateve clippy has to say.

@Mossaka
Copy link
Member Author

Mossaka commented May 25, 2023

Sounds good I will close this PR.

@Mossaka Mossaka closed this May 25, 2023
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