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.

Sign up for GitHub

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

fix(msrv): Error, rather than panic, on rust-version 'x' #13771

Merged
merged 5 commits into from
Apr 18, 2024
Merged

fix(msrv): Error, rather than panic, on rust-version 'x' #13771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
af9288f
test(msrv): Show current parse behavior with X
epage Apr 17, 2024
675d67d
fix(msrv): Error, rather than panic, on rust-version 'x'
epage Apr 17, 2024
3a2cc82
test(msrv): Migrate most parse tests to unit tests
epage Apr 17, 2024
6f22e9d
test(schemas): Add PartialVersion unit tests
epage Apr 17, 2024
6d8d3b6
fix(schemas): Allow parsing pre-release with X
epage Apr 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions crates/cargo-util-schemas/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,6 @@ url.workspace = true

[lints]
workspace = true

[dev-dependencies]
snapbox.workspace = true
80 changes: 70 additions & 10 deletions crates/cargo-util-schemas/src/core/partial_version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,6 @@ impl std::str::FromStr for PartialVersion {
type Err = PartialVersionError;

fn from_str(value: &str) -> Result<Self, Self::Err> {
if is_req(value) {
return Err(ErrorKind::VersionReq.into());
}
match semver::Version::parse(value) {
Ok(ver) => Ok(ver.into()),
Err(_) => {
Expand All @@ -96,9 +93,16 @@ impl std::str::FromStr for PartialVersion {
Err(_) if value.contains('+') => return Err(ErrorKind::BuildMetadata.into()),
Err(_) => return Err(ErrorKind::Unexpected.into()),
};
assert_eq!(version_req.comparators.len(), 1, "guaranteed by is_req");
if version_req.comparators.len() != 1 {
return Err(ErrorKind::VersionReq.into());
}
let comp = version_req.comparators.pop().unwrap();
assert_eq!(comp.op, semver::Op::Caret, "guaranteed by is_req");
if comp.op != semver::Op::Caret {
return Err(ErrorKind::VersionReq.into());
} else if value.starts_with('^') {
// Can't distinguish between `^` present or not
return Err(ErrorKind::VersionReq.into());
}
let pre = if comp.pre.is_empty() {
None
} else {
Expand Down Expand Up @@ -179,9 +183,65 @@ enum ErrorKind {
Unexpected,
}

fn is_req(value: &str) -> bool {
let Some(first) = value.chars().next() else {
return false;
};
"<>=^~".contains(first) || value.contains('*') || value.contains(',')
#[cfg(test)]
mod test {
use super::*;
use snapbox::str;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question: we use snapbox here just for better assertion message, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline snapshotting!


#[test]
fn parse_success() {
let cases = &[
// Valid pre-release
("1.43.0-beta.1", str!["1.43.0-beta.1"]),
// Valid pre-release with wildcard
("1.43.0-beta.1.x", str!["1.43.0-beta.1.x"]),
];
for (input, expected) in cases {
let actual: Result<PartialVersion, _> = input.parse();
let actual = match actual {
Ok(result) => result.to_string(),
Err(err) => format!("didn't pass: {err}"),
};
snapbox::assert_eq(expected.clone(), actual);
}
}

#[test]
fn parse_errors() {
let cases = &[
// Disallow caret
(
"^1.43",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
// Bad pre-release
(
"1.43-beta.1",
str![[r#"unexpected prerelease field, expected a version like "1.32""#]],
),
// Weird wildcard
(
"x",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
(
"1.x",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
(
"1.1.x",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
// Non-sense
("foodaddle", str![[r#"expected a version like "1.32""#]]),
];
for (input, expected) in cases {
let actual: Result<PartialVersion, _> = input.parse();
let actual = match actual {
Ok(result) => format!("didn't fail: {result:?}"),
Err(err) => err.to_string(),
};
snapbox::assert_eq(expected.clone(), actual);
}
}
}
45 changes: 45 additions & 0 deletions crates/cargo-util-schemas/src/manifest/rust_version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ enum RustVersionErrorKind {
#[cfg(test)]
mod test {
use super::*;
use snapbox::str;

#[test]
fn is_compatible_with_rustc() {
Expand Down Expand Up @@ -170,4 +171,48 @@ mod test {
}
assert!(passed);
}

#[test]
fn parse_errors() {
let cases = &[
// Disallow caret
(
"^1.43",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
// Valid pre-release
(
"1.43.0-beta.1",
str![[r#"unexpected prerelease field, expected a version like "1.32""#]],
),
// Bad pre-release
(
"1.43-beta.1",
str![[r#"unexpected prerelease field, expected a version like "1.32""#]],
),
// Weird wildcard
(
"x",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
(
"1.x",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
(
"1.1.x",
str![[r#"unexpected version requirement, expected a version like "1.32""#]],
),
// Non-sense
("foodaddle", str![[r#"expected a version like "1.32""#]]),
];
for (input, expected) in cases {
let actual: Result<RustVersion, _> = input.parse();
let actual = match actual {
Ok(result) => format!("didn't fail: {result:?}"),
Err(err) => err.to_string(),
};
snapbox::assert_eq(expected.clone(), actual);
}
}
}
101 changes: 1 addition & 100 deletions tests/testsuite/rust_version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ fn rust_version_satisfied() {
}

#[cargo_test]
fn rust_version_bad_caret() {
fn rust_version_error() {
project()
.file(
"Cargo.toml",
Expand Down Expand Up @@ -58,105 +58,6 @@ fn rust_version_bad_caret() {
.run();
}

#[cargo_test]
fn rust_version_good_pre_release() {
project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []
rust-version = "1.43.0-beta.1"
[[bin]]
name = "foo"
"#,
)
.file("src/main.rs", "fn main() {}")
.build()
.cargo("check")
.with_status(101)
.with_stderr(
"\
[ERROR] unexpected prerelease field, expected a version like \"1.32\"
--> Cargo.toml:7:28
|
7 | rust-version = \"1.43.0-beta.1\"
| ^^^^^^^^^^^^^^^
|
",
)
.run();
}

#[cargo_test]
fn rust_version_bad_pre_release() {
project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []
rust-version = "1.43-beta.1"
[[bin]]
name = "foo"
"#,
)
.file("src/main.rs", "fn main() {}")
.build()
.cargo("check")
.with_status(101)
.with_stderr(
"\
[ERROR] unexpected prerelease field, expected a version like \"1.32\"
--> Cargo.toml:7:28
|
7 | rust-version = \"1.43-beta.1\"
| ^^^^^^^^^^^^^
|
",
)
.run();
}

#[cargo_test]
fn rust_version_bad_nonsense() {
project()
.file(
"Cargo.toml",
r#"
[package]
name = "foo"
version = "0.0.1"
edition = "2015"
authors = []
rust-version = "foodaddle"
[[bin]]
name = "foo"
"#,
)
.file("src/main.rs", "fn main() {}")
.build()
.cargo("check")
.with_status(101)
.with_stderr(
"\
[ERROR] expected a version like \"1.32\"
--> Cargo.toml:7:28
|
7 | rust-version = \"foodaddle\"
| ^^^^^^^^^^^
|
",
)
.run();
}

#[cargo_test]
fn rust_version_older_than_edition() {
project()
Expand Down