-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat(lint): imprecise_version_requirements #16321
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
base: master
Are you sure you want to change the base?
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.
| --> Cargo.toml:7:[..] | ||
| | | ||
| 7 | bar = { git = '[ROOTURL]/bar', version = "0.1" } | ||
| | [..]^^^^^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the redaction of [ROOTURL], we have no control oveer column number, so redact them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I can see how we want to run some lints immediately but lints like this can run later and having more information can be a requirement for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we think this is a blocker for this PR, or we can mark this lint as "unstable" until we have a better linting infra to support this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our other options are
- Merge anyways and say "eh"
- Suggest the lowest bound possible because that is literally what the user is saying in this (so no behavior change to apply the suggestion), and then rely on Lint that a version requirement is lower than what is currently in
Cargo.lock#15583 for upping that.
| [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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When @Muscraft and I have talked about workspace-level lints, the thought is that they would respond to workspace.lints, so there are no unification issues.
Overall, I am in favor of linting at the level things happen, so if the linted information is in the workspace dependency, we lint that. We show one lint with one suggestion. If/when we have mutually exclusive lints, this avoids the problem of two packages conflicting on what the workspace field should do.
The main downside I see to doing this is that this lint is a hybrid workspae/package lint. That can feel a bit off. Would be helped if we had inheriting of lints w/ local overrides but I don't see that as blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I am in favor of linting at the level things happen, so if the linted information is in the workspace dependency, we lint that. We show one lint with one suggestion. If/when we have mutually exclusive lints, this avoids the problem of two packages conflicting on what the workspace field should do.
Hmm, edition related things might be an exception. The only situation I can think of is #12162 (comment) which I framed around how you are mixing the package dep with the workspace dep, rather than being about the workspace dep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When Muscraft and I have talked about workspace-level lints, the thought is that they would respond to workspace.lints, so there are no unification issues.
So that means adding a Cargo lint to workspace.lints implicit enable it? Sounds like Cargo lints are a bit special than other tools, but understandable.
Though people may use workspace with large numbers of disjoint crates, and don't want to touch workspace level stuff because it may be owned by other teams. This is probably not a recommended use case for workspace, but that still happens in the wild.
Maybe the real question is: Should workspace lints always be fired, even the selected package has nothing to do with workspace inheritance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though people may use workspace with large numbers of disjoint crates, and don't want to touch workspace level stuff because it may be owned by other teams. This is probably not a recommended use case for workspace, but that still happens in the wild.
In that case,
- you probably shouldn't be inheriting dependencies
- having a package lint lint the workspace would be unactionable and so can't use it for non-inherited dependencies since that will lint dependencies you can't touch.
Maybe the real question is: Should workspace lints always be fired, even the selected package has nothing to do with workspace inheritance?
That is an interesting way of framing this. Another is "should they fire for non-inherited inheritable fields"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this grow to a bigger topic to discuss than this PR itself.
My idea is that we could add more feature-gated/unstable lint to the system so we can discover more cases like this to help improve the infrastructure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to feature gate this (directly or by making this an Unresolved Question on the lint feature itself) and see how this all evolves as we add more lints, sure.
| let [cmp] = req.comparators.as_slice() else { | ||
| continue; | ||
| }; | ||
| if cmp.op != semver::Op::Caret { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Find a proper lint level (currently set to allow because this may get people a lot of warnings)
I think this is reasonable
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)