Add detailed errors for `tool.uv.sources` deserialization failures (#7823)

## Summary

Closes https://github.com/astral-sh/uv/issues/7817.
This commit is contained in:
Charlie Marsh 2024-10-01 11:49:06 -04:00 committed by GitHub
parent bd3c462674
commit f0f2f897de
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 301 additions and 104 deletions

View File

@ -209,10 +209,6 @@ impl LoweredRequirement {
}; };
(source, marker) (source, marker)
} }
Source::CatchAll { .. } => {
// Emit a dedicated error message, which is an improvement over Serde's default error.
return Err(LoweringError::InvalidEntry);
}
}; };
marker.and(requirement.marker.clone()); marker.and(requirement.marker.clone());
@ -325,11 +321,6 @@ impl LoweredRequirement {
Source::Workspace { .. } => { Source::Workspace { .. } => {
return Err(LoweringError::WorkspaceMember); return Err(LoweringError::WorkspaceMember);
} }
Source::CatchAll { .. } => {
// Emit a dedicated error message, which is an improvement over Serde's default
// error.
return Err(LoweringError::InvalidEntry);
}
}; };
marker.and(requirement.marker.clone()); marker.and(requirement.marker.clone());
@ -364,8 +355,6 @@ pub enum LoweringError {
UndeclaredWorkspacePackage, UndeclaredWorkspacePackage,
#[error("Can only specify one of: `rev`, `tag`, or `branch`")] #[error("Can only specify one of: `rev`, `tag`, or `branch`")]
MoreThanOneGitRef, MoreThanOneGitRef,
#[error("Unable to combine options in `tool.uv.sources`")]
InvalidEntry,
#[error("Workspace members are not allowed in non-workspace contexts")] #[error("Workspace members are not allowed in non-workspace contexts")]
WorkspaceMember, WorkspaceMember,
#[error(transparent)] #[error(transparent)]

View File

@ -245,7 +245,6 @@ mod test {
8 | tqdm = true 8 | tqdm = true
| ^^^^ | ^^^^
invalid type: boolean `true`, expected an array or map invalid type: boolean `true`, expected an array or map
"###); "###);
} }
@ -263,8 +262,12 @@ mod test {
"#}; "#};
assert_snapshot!(format_err(input).await, @r###" assert_snapshot!(format_err(input).await, @r###"
error: Failed to parse entry for: `tqdm` error: TOML parse error at line 8, column 8
Caused by: Can only specify one of: `rev`, `tag`, or `branch` |
8 | tqdm = { git = "https://github.com/tqdm/tqdm", rev = "baaaaaab", tag = "v1.0.0" }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
expected at most one of `rev`, `tag`, or `branch`
"###); "###);
} }
@ -281,14 +284,12 @@ mod test {
tqdm = { git = "https://github.com/tqdm/tqdm", ref = "baaaaaab" } tqdm = { git = "https://github.com/tqdm/tqdm", ref = "baaaaaab" }
"#}; "#};
// TODO(konsti): This should tell you the set of valid fields
assert_snapshot!(format_err(input).await, @r###" assert_snapshot!(format_err(input).await, @r###"
error: TOML parse error at line 8, column 8 error: TOML parse error at line 8, column 8
| |
8 | tqdm = { git = "https://github.com/tqdm/tqdm", ref = "baaaaaab" } 8 | tqdm = { git = "https://github.com/tqdm/tqdm", ref = "baaaaaab" }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum Source unknown field `ref`, expected one of `git`, `subdirectory`, `rev`, `tag`, `branch`, `url`, `path`, `editable`, `index`, `workspace`, `marker`
"###); "###);
} }
@ -305,14 +306,12 @@ mod test {
tqdm = { path = "tqdm", index = "torch" } tqdm = { path = "tqdm", index = "torch" }
"#}; "#};
// TODO(konsti): This should tell you the set of valid fields
assert_snapshot!(format_err(input).await, @r###" assert_snapshot!(format_err(input).await, @r###"
error: TOML parse error at line 8, column 8 error: TOML parse error at line 8, column 8
| |
8 | tqdm = { path = "tqdm", index = "torch" } 8 | tqdm = { path = "tqdm", index = "torch" }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum Source cannot specify both `path` and `index`
"###); "###);
} }
@ -349,7 +348,6 @@ mod test {
| ^ | ^
invalid string invalid string
expected `"`, `'` expected `"`, `'`
"###); "###);
} }
@ -371,8 +369,10 @@ mod test {
| |
8 | tqdm = { url = "§invalid#+#*Ä" } 8 | tqdm = { url = "§invalid#+#*Ä" }
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
data did not match any variant of untagged enum Source invalid value: string "§invalid#+#*Ä", expected relative URL without a base
in `url`
"###); "###);
} }

View File

@ -8,7 +8,7 @@
use glob::Pattern; use glob::Pattern;
use owo_colors::OwoColorize; use owo_colors::OwoColorize;
use serde::{de::IntoDeserializer, Deserialize, Serialize}; use serde::{de::IntoDeserializer, Deserialize, Deserializer, Serialize};
use std::ops::Deref; use std::ops::Deref;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use std::str::FromStr; use std::str::FromStr;
@ -455,7 +455,7 @@ enum SourcesWire {
impl<'de> serde::de::Deserialize<'de> for SourcesWire { impl<'de> serde::de::Deserialize<'de> for SourcesWire {
fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
where where
D: serde::de::Deserializer<'de>, D: Deserializer<'de>,
{ {
serde_untagged::UntaggedEnumVisitor::new() serde_untagged::UntaggedEnumVisitor::new()
.map(|map| map.deserialize().map(SourcesWire::One)) .map(|map| map.deserialize().map(SourcesWire::One))
@ -520,7 +520,7 @@ impl TryFrom<SourcesWire> for Sources {
} }
/// A `tool.uv.sources` value. /// A `tool.uv.sources` value.
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] #[derive(Serialize, Debug, Clone, PartialEq, Eq)]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))] #[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
#[serde(rename_all = "kebab-case", untagged, deny_unknown_fields)] #[serde(rename_all = "kebab-case", untagged, deny_unknown_fields)]
pub enum Source { pub enum Source {
@ -602,24 +602,293 @@ pub enum Source {
)] )]
marker: MarkerTree, marker: MarkerTree,
}, },
/// A catch-all variant used to emit precise error messages when deserializing. }
CatchAll {
git: String, /// A custom deserialization implementation for [`Source`]. This is roughly equivalent to
subdirectory: Option<PortablePathBuf>, /// `#[serde(untagged)]`, but provides more detailed error messages.
rev: Option<String>, impl<'de> Deserialize<'de> for Source {
tag: Option<String>, fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
branch: Option<String>, where
url: String, D: Deserializer<'de>,
path: PortablePathBuf, {
index: String, #[derive(Deserialize, Debug, Clone)]
workspace: bool, #[serde(rename_all = "kebab-case", deny_unknown_fields)]
#[serde( struct CatchAll {
skip_serializing_if = "pep508_rs::marker::ser::is_empty", git: Option<Url>,
serialize_with = "pep508_rs::marker::ser::serialize", subdirectory: Option<PortablePathBuf>,
default rev: Option<String>,
)] tag: Option<String>,
marker: MarkerTree, branch: Option<String>,
}, url: Option<Url>,
path: Option<PortablePathBuf>,
editable: Option<bool>,
index: Option<String>,
workspace: Option<bool>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
}
// Attempt to deserialize as `CatchAll`.
let CatchAll {
git,
subdirectory,
rev,
tag,
branch,
url,
path,
editable,
index,
workspace,
marker,
} = CatchAll::deserialize(deserializer)?;
// If the `git` field is set, we're dealing with a Git source.
if let Some(git) = git {
if index.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `git` and `index`",
));
}
if workspace.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `git` and `workspace`",
));
}
if path.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `git` and `path`",
));
}
if url.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `git` and `url`",
));
}
if editable.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `git` and `editable`",
));
}
match (rev.as_ref(), tag.as_ref(), branch.as_ref()) {
(None, None, None) => {}
(Some(_), None, None) => {}
(None, Some(_), None) => {}
(None, None, Some(_)) => {}
_ => {
return Err(serde::de::Error::custom(
"expected at most one of `rev`, `tag`, or `branch`",
))
}
};
return Ok(Self::Git {
git,
subdirectory,
rev,
tag,
branch,
marker,
});
}
// If the `url` field is set, we're dealing with a URL source.
if let Some(url) = url {
if index.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `index`",
));
}
if workspace.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `workspace`",
));
}
if path.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `path`",
));
}
if git.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `git`",
));
}
if rev.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `rev`",
));
}
if tag.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `tag`",
));
}
if branch.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `branch`",
));
}
if editable.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `url` and `editable`",
));
}
return Ok(Self::Url {
url,
subdirectory,
marker,
});
}
// If the `path` field is set, we're dealing with a path source.
if let Some(path) = path {
if index.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `index`",
));
}
if workspace.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `workspace`",
));
}
if git.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `git`",
));
}
if url.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `url`",
));
}
if rev.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `rev`",
));
}
if tag.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `tag`",
));
}
if branch.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `path` and `branch`",
));
}
return Ok(Self::Path {
path,
editable,
marker,
});
}
// If the `index` field is set, we're dealing with a registry source.
if let Some(index) = index {
if workspace.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `workspace`",
));
}
if git.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `git`",
));
}
if url.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `url`",
));
}
if path.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `path`",
));
}
if rev.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `rev`",
));
}
if tag.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `tag`",
));
}
if branch.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `branch`",
));
}
if editable.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `editable`",
));
}
return Ok(Self::Registry { index, marker });
}
// If the `workspace` field is set, we're dealing with a workspace source.
if let Some(workspace) = workspace {
if index.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `index`",
));
}
if git.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `git`",
));
}
if url.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `url`",
));
}
if path.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `path`",
));
}
if rev.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `rev`",
));
}
if tag.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `tag`",
));
}
if branch.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `branch`",
));
}
if editable.is_some() {
return Err(serde::de::Error::custom(
"cannot specify both `index` and `editable`",
));
}
return Ok(Self::Workspace { workspace, marker });
}
// If none of the fields are set, we're dealing with an error.
Err(serde::de::Error::custom(
"expected one of `git`, `url`, `path`, `index`, or `workspace`",
))
}
} }
#[derive(Error, Debug)] #[derive(Error, Debug)]
@ -763,7 +1032,6 @@ impl Source {
Source::Path { marker, .. } => marker.clone(), Source::Path { marker, .. } => marker.clone(),
Source::Registry { marker, .. } => marker.clone(), Source::Registry { marker, .. } => marker.clone(),
Source::Workspace { marker, .. } => marker.clone(), Source::Workspace { marker, .. } => marker.clone(),
Source::CatchAll { marker, .. } => marker.clone(),
} }
} }
} }

60
uv.schema.json generated
View File

@ -1356,66 +1356,6 @@
} }
}, },
"additionalProperties": false "additionalProperties": false
},
{
"description": "A catch-all variant used to emit precise error messages when deserializing.",
"type": "object",
"required": [
"git",
"index",
"path",
"url",
"workspace"
],
"properties": {
"branch": {
"type": [
"string",
"null"
]
},
"git": {
"type": "string"
},
"index": {
"type": "string"
},
"marker": {
"$ref": "#/definitions/MarkerTree"
},
"path": {
"$ref": "#/definitions/String"
},
"rev": {
"type": [
"string",
"null"
]
},
"subdirectory": {
"anyOf": [
{
"$ref": "#/definitions/String"
},
{
"type": "null"
}
]
},
"tag": {
"type": [
"string",
"null"
]
},
"url": {
"type": "string"
},
"workspace": {
"type": "boolean"
}
},
"additionalProperties": false
} }
] ]
}, },