Configure workspace lints, enable running some Clippy lints on CI #7561
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds necessary configuration knobs to have lints configured at the workspace level in Wasmtime rather than the crate level. This uses a feature of Cargo first released with 1.74.0 (last week) of the `[workspace.lints]` table. This should help create a more consistent set of lints applied across all crates in our workspace in addition to possibly running select clippy lints on CI as well.
This commit configures a `deny` lint level for the `unused_extern_crates` lint to the workspace level rather than the previous configuration at the individual crate level.
CI will ensure that these don't get checked into the codebase and otherwise provide fewer speed bumps for in-process development.
This commit configures our CI to run `cargo clippy --workspace` for all merged PRs. Historically this hasn't been all the feasible due to the amount of configuration required to control the number of warnings on CI, but with Cargo's new `[lint]` table it's possible to have a one-liner to silence all lints from Clippy by default. This commit by default sets the `all` lint in Clippy to `allow` to by-default disable warnings from Clippy. The goal of this PR is to enable selective access to Clippy lints for Wasmtime on CI.
This would have fixed bytecodealliance#7558 so try to head off future issues with that by warning against this situation in a few crates. This lint is still quite noisy though for Cranelift for example so it's not worthwhile at this time to enable it for the whole workspace.
alexcrichton
requested review from
abrown and
pchickey
and removed request for
a team
pchickey
approved these changes
Nov 20, 2023
github-actions
bot
added
cranelift
Subscribe to Label Action
This issue or pull request has been labeled: "cranelift", "cranelift:meta", "cranelift:module", "isle"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
prtest:full
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is a series of commits building up to the ability to selectively enable clippy lints to prevent issues such as #7558 from occurring. The root cause of the issue is the behavior of casting a smaller integer to a larger integer and switching signs. For example casting an i8 to a u32. I certainly don't remember whether this is a zero-extend or a sign-extend, and I suspect that I'm not alone. Ideally I was looking for a lint to help weed these out on CI.
Unfortunately rustc does not have a lint for this. Clippy, however, has a somewhat similar lint called
cast_sign_loss. Empowered with Rust's 1.74 release and Cargo's addition of a[lints]configuration for workspaces, I realized this seemed like a reasonable time to kick the tires. Historically Wasmtime hasn't enabled Clippy because it's too noisy by default and it's too cumbersome to turn it off for all crates. With[lints]however it's a single line to turn off all clippy for the entire workspace, which felt like a much better balance to me at least.First configuring
[lints]as well as[workspace.lints]helped moved some lint configuration for various crates into one location. Afterwards all clippy lints were then disabled and thencargo clippywas added to CI. It turns out thatcast_sign_lossis still too noisy for our purposes to enable for the whole workspace, but it can be selectively enabled for a few crates in question which do at least much of the casting for values at runtime.This is intended to be somewhat of a trial run to see if we can make running Clippy in CI useful. I'm sure Clippy has other useful lints for helping to catch issues as well as helping with stylistic things for contributors. For now everything is left disabled but if desired selective lints can be enabled as necessary.