feat(lint): imprecise_version_requirements #16321
Conversation
Add a new `cargo::imprecise_version_requirements` lint: Only check if dependency has a single caret requirement. All other requirements (multiple, tilde, wildcard) are not linted by this rule.
rustbot
added
A-documenting-cargo-itself
| --> Cargo.toml:7:[..] | ||
| | | ||
| 7 | bar = { git = '[ROOTURL]/bar', version = "0.1" } | ||
| | [..]^^^^^ |
There was a problem hiding this comment.
Because the redaction of [ROOTURL], we have no control oveer column number, so redact them.
There was a problem hiding this comment.
At some point we should move each lint to its own library, as well as having a lint context instead of bloating the function argument list
| 7 | dep = "1.0" | ||
| | ^^^^^ | ||
| | | ||
| = [NOTE] `cargo::imprecise_version_requirements` is set to `warn` in `[lints]` |
There was a problem hiding this comment.
The lint diagnostic is too thin. We probably want to add something like
help: specify version with full `major.minor.patch` SemVer requirement
though I am losing my creativity writing a better one 😞.
| [WARNING] dependency version requirement lacks full precision | ||
| --> Cargo.toml:7:7 | ||
| | | ||
| 7 | dep = "1" | ||
| | ^^^ | ||
| | | ||
| = [NOTE] `cargo::imprecise_version_requirements` is set to `warn` in `[lints]` |
There was a problem hiding this comment.
(maybe for follow-up) Should Cargo suggest a minimal bound?
Can we look up the currently resolved version and suggest an edit to use it?
Suggestions in most cases feel like extra polish that wouldn't be blocking. The fact that this is for avoiding people under-specifying version requirements, they could resolve this warning by just under-specifying it explicitly which defeats the purpose. Not saying a suggestion is required but worth further consideration.
There was a problem hiding this comment.
Can we look up the currently resolved version and suggest an edit to use it?
By "resolved version" you mean the version in the Resolve graph right?
No. The lint is emitted before dependency resolution. We probably want introduce lint passes to get exposed to different information.
| [WARNING] dependency version requirement lacks full precision | ||
| --> Cargo.toml:7:7 | ||
| | | ||
| 7 | dep = "1" | ||
| | ^^^ | ||
| | | ||
| [NOTE] dependency `dep` was inherited | ||
| --> member/Cargo.toml:8:5 | ||
| | | ||
| 8 | dep.workspace = true | ||
| | ---------------- | ||
| = [NOTE] `cargo::imprecise_version_requirements` is set to `warn` in `[lints]` |
There was a problem hiding this comment.
So we report the lint on each inherited instance?
Alternatively, we could lint the workspace table on its own and skip the deps that inherit. The up/downside is that that part would be operating at the workspace level and would only read workspace.lints (or whatever we do for real packages in that situation)
There was a problem hiding this comment.
I feel like we had a similar discussion with @Muscraft a while ago, but can't remember any conclusion (or there was no conclusion).
So we report the lint on each inherited instance?
Yeah. Personally a bit towards not linting things not being used, though that means if we will emit duplicate suggestions (that is fine because rustfix dedupe them). I feel like that is the current approach rustc took?
There was a problem hiding this comment.
though that means if we will emit duplicate suggestions (that is fine because rustfix dedupe them)
but pretty noisy for users
I feel like that is the current approach rustc took?
Depends on your perspective. Do you see this like a macro being expanded where the lint gets applied on each instantiation or like a function being called where the lint only fires in the function?
There was a problem hiding this comment.
Linting workspace inherited values is always a bit weird. The suggestion fix is going to affect all packages inheriting it However, we have only package level [lints] table. What if one package allows this lint but the other denies? Maybe, for workspace inherited values, we do something like this:
- Check if any package enable the lint
- If any, lint against workspace manifest
- If lint issue is found, emit diagnostics against workspace manifest, and list all affected manifests
Chances are that most workspace members has lints.workspace = true so the above case should be uncommon and also not too meaningful.
| let [cmp] = req.comparators.as_slice() else { | ||
| continue; | ||
| }; | ||
| if cmp.op != semver::Op::Caret { |
There was a problem hiding this comment.
Should we lint beyond caret requirements?
At first, my thought was "yes" and we name this for being semver. However, using >=, < is common for multi-major semver versions and would be sad to miss out on those.
=, *, and ~ have significance on what requirement fields get specified.
There was a problem hiding this comment.
Find a good lint name
I think these conversations are related.
One thought: implicit_minimum_version_req. By focusing on the behavior for the minimum bound, it technically differentiates what operators it applies to
| name: "imprecise_version_requirements", | ||
| desc: "dependency version requirement lacks full precision", | ||
| groups: &[], | ||
| default_level: LintLevel::Allow, |
There was a problem hiding this comment.
Find a proper lint level (currently set to allow because this may get people a lot of warnings)
I think this is reasonable
4 participants
What does this PR try to resolve?
Add a new
cargo::imprecise_version_requirementslint:Only check if dependency has a single caret requirement.
All other requirements (multiple, tilde, wildcard)
are not linted by this rule.
Fixes #15577
How to test and review this PR?
allowbecause this may get people a lot of warnings)