Skip to content

Commit 192475a

Browse files
committed
feat(lint): new implicit_minimum_version_req lint
Add a new `cargo::implicit_minimum_version_req` lint: Only check if dependency has a single caret requirement. All other requirements (multiple, tilde, wildcard) are not linted by this rule, as they usually have significance on what version fields get specified. This currently lints only dependencies with no workspace inheritance.
1 parent 5e1f986 commit 192475a

File tree

5 files changed

+496
-198
lines changed

5 files changed

+496
-198
lines changed

src/cargo/core/workspace.rs

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,10 @@ use crate::util::context::FeatureUnification;
2626
use crate::util::edit_distance;
2727
use crate::util::errors::{CargoResult, ManifestError};
2828
use crate::util::interning::InternedString;
29-
use crate::util::lints::{
30-
analyze_cargo_lints_table, blanket_hint_mostly_unused, check_im_a_teapot,
31-
};
29+
use crate::util::lints::analyze_cargo_lints_table;
30+
use crate::util::lints::blanket_hint_mostly_unused;
31+
use crate::util::lints::check_im_a_teapot;
32+
use crate::util::lints::implicit_minimum_version_req;
3233
use crate::util::toml::{InheritableFields, read_manifest};
3334
use crate::util::{
3435
Filesystem, GlobalContext, IntoUrl, context::CargoResolverConfig, context::ConfigRelativePath,
@@ -1296,6 +1297,7 @@ impl<'gctx> Workspace<'gctx> {
12961297
self.gctx,
12971298
)?;
12981299
check_im_a_teapot(pkg, &path, &cargo_lints, &mut error_count, self.gctx)?;
1300+
implicit_minimum_version_req(pkg, &path, &cargo_lints, &mut error_count, self.gctx)?;
12991301
}
13001302

13011303
if error_count > 0 {
Lines changed: 259 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,259 @@
1+
use std::collections::HashMap;
2+
use std::path::Path;
3+
4+
use annotate_snippets::AnnotationKind;
5+
use annotate_snippets::Group;
6+
use annotate_snippets::Level;
7+
use annotate_snippets::Patch;
8+
use annotate_snippets::Snippet;
9+
use cargo_platform::Platform;
10+
use cargo_util_schemas::manifest::TomlToolLints;
11+
use toml::de::DeValue;
12+
13+
use crate::CargoResult;
14+
use crate::GlobalContext;
15+
use crate::core::Manifest;
16+
use crate::core::Package;
17+
use crate::util::OptVersionReq;
18+
use crate::util::lints::Lint;
19+
use crate::util::lints::LintLevel;
20+
use crate::util::lints::LintLevelReason;
21+
use crate::util::lints::get_key_value;
22+
use crate::util::lints::rel_cwd_manifest_path;
23+
24+
pub const LINT: Lint = Lint {
25+
name: "implicit_minimum_version_req",
26+
desc: "dependency version requirement without an explicit minimum version",
27+
groups: &[],
28+
default_level: LintLevel::Allow,
29+
edition_lint_opts: None,
30+
feature_gate: None,
31+
docs: Some(
32+
r#"
33+
### What it does
34+
35+
Checks for dependency version requirements
36+
that do not explicitly specify a full `major.minor.patch` version requirement,
37+
such as `serde = "1"` or `serde = "1.0"`.
38+
39+
This lint currently only applies to caret requirements
40+
(the [default requirements](specifying-dependencies.md#default-requirements)).
41+
42+
### Why it is bad
43+
44+
Version requirements without an explicit full version
45+
can be misleading about the actual minimum supported version.
46+
For example,
47+
`serde = "1"` has an implicit minimum bound of `1.0.0`.
48+
If your code actually requires features from `1.0.219`,
49+
the implicit minimum bound of `1.0.0` gives a false impression about compatibility.
50+
51+
Specifying the full version helps with:
52+
53+
- Accurate minimum version documentation
54+
- Better compatibility with `-Z minimal-versions`
55+
- Clearer dependency constraints for consumers
56+
57+
### Drawbacks
58+
59+
Even with a fully specified version,
60+
the minimum bound might still be incorrect if untested.
61+
This lint helps make the minimum version requirement explicit
62+
but doesn't guarantee correctness.
63+
64+
### Example
65+
66+
```toml
67+
[dependencies]
68+
serde = "1"
69+
```
70+
71+
Should be written as a full specific version:
72+
73+
```toml
74+
[dependencies]
75+
serde = "1.0.219"
76+
```
77+
"#,
78+
),
79+
};
80+
81+
pub fn implicit_minimum_version_req(
82+
pkg: &Package,
83+
manifest_path: &Path,
84+
cargo_lints: &TomlToolLints,
85+
error_count: &mut usize,
86+
gctx: &GlobalContext,
87+
) -> CargoResult<()> {
88+
let manifest = pkg.manifest();
89+
let (lint_level, reason) = LINT.level(
90+
cargo_lints,
91+
manifest.edition(),
92+
manifest.unstable_features(),
93+
);
94+
95+
if lint_level == LintLevel::Allow {
96+
return Ok(());
97+
}
98+
99+
let document = manifest.document();
100+
let contents = manifest.contents();
101+
let manifest_path = rel_cwd_manifest_path(manifest_path, gctx);
102+
let target_key_for_platform = target_key_for_platform(&manifest);
103+
104+
for dep in manifest.dependencies().iter() {
105+
let version_req = dep.version_req();
106+
let Some(suggested_req) = get_suggested_version_req(&version_req) else {
107+
continue;
108+
};
109+
110+
let name_in_toml = dep.name_in_toml().as_str();
111+
let key_path =
112+
if let Some(cfg) = dep.platform().and_then(|p| target_key_for_platform.get(p)) {
113+
&["target", &cfg, dep.kind().kind_table(), name_in_toml][..]
114+
} else {
115+
&[dep.kind().kind_table(), name_in_toml][..]
116+
};
117+
118+
let Some(span) = span_of_version_req(document, key_path) else {
119+
continue;
120+
};
121+
122+
let report = report(
123+
lint_level,
124+
reason,
125+
span,
126+
contents,
127+
&manifest_path,
128+
&suggested_req,
129+
);
130+
131+
if lint_level.is_error() {
132+
*error_count += 1;
133+
}
134+
gctx.shell().print_report(&report, lint_level.force())?;
135+
}
136+
137+
Ok(())
138+
}
139+
140+
pub fn span_of_version_req<'doc>(
141+
document: &'doc toml::Spanned<toml::de::DeTable<'static>>,
142+
path: &[&str],
143+
) -> Option<std::ops::Range<usize>> {
144+
let (_key, value) = get_key_value(document, path)?;
145+
146+
match value.as_ref() {
147+
DeValue::String(_) => Some(value.span()),
148+
DeValue::Table(map) if map.get("workspace").is_some() => {
149+
// We only lint non-workspace-inherited dependencies
150+
None
151+
}
152+
DeValue::Table(map) => {
153+
let Some(v) = map.get("version") else {
154+
panic!("version must be specified or workspace-inherited");
155+
};
156+
Some(v.span())
157+
}
158+
_ => unreachable!("dependency must be string or table"),
159+
}
160+
}
161+
162+
fn report<'a>(
163+
lint_level: LintLevel,
164+
reason: LintLevelReason,
165+
span: std::ops::Range<usize>,
166+
contents: &'a str,
167+
manifest_path: &str,
168+
suggested_req: &str,
169+
) -> [Group<'a>; 2] {
170+
let level = lint_level.to_diagnostic_level();
171+
let emitted_source = LINT.emitted_source(lint_level, reason);
172+
let replacement = format!(r#""{suggested_req}""#);
173+
let label = "missing full version components";
174+
let secondary_title = "consider specifying full `major.minor.patch` version components";
175+
[
176+
level.clone().primary_title(LINT.desc).element(
177+
Snippet::source(contents)
178+
.path(manifest_path.to_owned())
179+
.annotation(AnnotationKind::Primary.span(span.clone()).label(label)),
180+
),
181+
Level::HELP
182+
.secondary_title(secondary_title)
183+
.element(Snippet::source(contents).patch(Patch::new(span.clone(), replacement)))
184+
.element(Level::NOTE.message(emitted_source)),
185+
]
186+
}
187+
188+
fn get_suggested_version_req(req: &OptVersionReq) -> Option<String> {
189+
use semver::Op;
190+
let OptVersionReq::Req(req) = req else {
191+
return None;
192+
};
193+
let mut has_suggestions = false;
194+
let mut comparators = Vec::new();
195+
196+
for mut cmp in req.comparators.iter().cloned() {
197+
match cmp.op {
198+
Op::Caret | Op::GreaterEq => {
199+
// Only focus on comparator that has only `major` or `major.minor`
200+
if cmp.minor.is_some() && cmp.patch.is_some() {
201+
comparators.push(cmp);
202+
continue;
203+
} else {
204+
has_suggestions = true;
205+
cmp.minor.get_or_insert(0);
206+
cmp.patch.get_or_insert(0);
207+
comparators.push(cmp);
208+
}
209+
}
210+
Op::Exact | Op::Tilde | Op::Wildcard | Op::Greater | Op::Less | Op::LessEq => {
211+
comparators.push(cmp);
212+
continue;
213+
}
214+
_ => panic!("unknown comparator in `{cmp}`"),
215+
}
216+
}
217+
218+
if !has_suggestions {
219+
return None;
220+
}
221+
222+
// This is a lossy suggestion that
223+
//
224+
// * extra spaces are removed
225+
// * caret operator `^` is stripped
226+
let mut suggestion = String::new();
227+
228+
for cmp in &comparators {
229+
if !suggestion.is_empty() {
230+
suggestion.push_str(", ");
231+
}
232+
let s = cmp.to_string();
233+
234+
if cmp.op == Op::Caret {
235+
suggestion.push_str(s.strip_prefix('^').unwrap_or(&s));
236+
} else {
237+
suggestion.push_str(&s);
238+
}
239+
}
240+
241+
Some(suggestion)
242+
}
243+
244+
/// A map from parsed `Platform` to their original TOML key strings.
245+
/// This is needed for constructing TOML key paths in diagnostics.
246+
///
247+
/// This is only relevant for package dependencies.
248+
fn target_key_for_platform(manifest: &Manifest) -> HashMap<Platform, String> {
249+
manifest
250+
.normalized_toml()
251+
.target
252+
.as_ref()
253+
.map(|map| {
254+
map.keys()
255+
.map(|k| (k.parse().expect("already parsed"), k.clone()))
256+
.collect()
257+
})
258+
.unwrap_or_default()
259+
}

src/cargo/util/lints/mod.rs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,16 @@ use std::fmt::Display;
88
use std::ops::Range;
99
use std::path::Path;
1010

11+
mod implicit_minimum_version_req;
12+
pub use implicit_minimum_version_req::implicit_minimum_version_req;
13+
1114
const LINT_GROUPS: &[LintGroup] = &[TEST_DUMMY_UNSTABLE];
12-
pub const LINTS: &[Lint] = &[BLANKET_HINT_MOSTLY_UNUSED, IM_A_TEAPOT, UNKNOWN_LINTS];
15+
pub const LINTS: &[Lint] = &[
16+
BLANKET_HINT_MOSTLY_UNUSED,
17+
implicit_minimum_version_req::LINT,
18+
IM_A_TEAPOT,
19+
UNKNOWN_LINTS,
20+
];
1321

1422
pub fn analyze_cargo_lints_table(
1523
pkg: &Package,

src/doc/src/reference/lints.md

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@
22

33
Note: [Cargo's linting system is unstable](unstable.md#lintscargo) and can only be used on nightly toolchains
44

5+
## Allowed-by-default
6+
7+
These lints are all set to the 'allow' level by default.
8+
- [`implicit_minimum_version_req`](#implicit_minimum_version_req)
9+
510
## Warn-by-default
611

712
These lints are all set to the 'warn' level by default.
@@ -36,6 +41,55 @@ hint-mostly-unused = true
3641
```
3742

3843

44+
## `implicit_minimum_version_req`
45+
Set to `allow` by default
46+
47+
### What it does
48+
49+
Checks for dependency version requirements
50+
that do not explicitly specify a full `major.minor.patch` version requirement,
51+
such as `serde = "1"` or `serde = "1.0"`.
52+
53+
This lint currently only applies to caret requirements
54+
(the [default requirements](specifying-dependencies.md#default-requirements)).
55+
56+
### Why it is bad
57+
58+
Version requirements without an explicit full version
59+
can be misleading about the actual minimum supported version.
60+
For example,
61+
`serde = "1"` has an implicit minimum bound of `1.0.0`.
62+
If your code actually requires features from `1.0.219`,
63+
the implicit minimum bound of `1.0.0` gives a false impression about compatibility.
64+
65+
Specifying the full version helps with:
66+
67+
- Accurate minimum version documentation
68+
- Better compatibility with `-Z minimal-versions`
69+
- Clearer dependency constraints for consumers
70+
71+
### Drawbacks
72+
73+
Even with a fully specified version,
74+
the minimum bound might still be incorrect if untested.
75+
This lint helps make the minimum version requirement explicit
76+
but doesn't guarantee correctness.
77+
78+
### Example
79+
80+
```toml
81+
[dependencies]
82+
serde = "1"
83+
```
84+
85+
Should be written as a full specific version:
86+
87+
```toml
88+
[dependencies]
89+
serde = "1.0.219"
90+
```
91+
92+
3993
## `unknown_lints`
4094
Set to `warn` by default
4195

0 commit comments

Comments
 (0)