From 20908496ae5c657dfcec0f5857bbaef63c9febf3 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 30 Sep 2024 19:55:30 -0400 Subject: [PATCH] Add detailed errors for tool.uv.sources deserialization failures --- .../uv-distribution/src/metadata/lowering.rs | 11 - .../src/metadata/requires_dist.rs | 22 +- crates/uv-workspace/src/pyproject.rs | 312 ++++++++++++++++-- uv.schema.json | 60 ---- 4 files changed, 301 insertions(+), 104 deletions(-) diff --git a/crates/uv-distribution/src/metadata/lowering.rs b/crates/uv-distribution/src/metadata/lowering.rs index 564cd6f82f9f..ee604a2f0202 100644 --- a/crates/uv-distribution/src/metadata/lowering.rs +++ b/crates/uv-distribution/src/metadata/lowering.rs @@ -209,10 +209,6 @@ impl LoweredRequirement { }; (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()); @@ -325,11 +321,6 @@ impl LoweredRequirement { Source::Workspace { .. } => { 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()); @@ -364,8 +355,6 @@ pub enum LoweringError { UndeclaredWorkspacePackage, #[error("Can only specify one of: `rev`, `tag`, or `branch`")] MoreThanOneGitRef, - #[error("Unable to combine options in `tool.uv.sources`")] - InvalidEntry, #[error("Workspace members are not allowed in non-workspace contexts")] WorkspaceMember, #[error(transparent)] diff --git a/crates/uv-distribution/src/metadata/requires_dist.rs b/crates/uv-distribution/src/metadata/requires_dist.rs index f47c058eb3b1..cb504e59ec23 100644 --- a/crates/uv-distribution/src/metadata/requires_dist.rs +++ b/crates/uv-distribution/src/metadata/requires_dist.rs @@ -245,7 +245,6 @@ mod test { 8 | tqdm = true | ^^^^ invalid type: boolean `true`, expected an array or map - "###); } @@ -263,8 +262,12 @@ mod test { "#}; assert_snapshot!(format_err(input).await, @r###" - error: Failed to parse entry for: `tqdm` - Caused by: Can only specify one of: `rev`, `tag`, or `branch` + error: TOML parse error at line 8, column 8 + | + 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" } "#}; - // TODO(konsti): This should tell you the set of valid fields assert_snapshot!(format_err(input).await, @r###" error: TOML parse error at line 8, column 8 | 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" } "#}; - // TODO(konsti): This should tell you the set of valid fields assert_snapshot!(format_err(input).await, @r###" error: TOML parse error at line 8, column 8 | 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 expected `"`, `'` - "###); } @@ -371,8 +369,10 @@ mod test { | 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` "###); } diff --git a/crates/uv-workspace/src/pyproject.rs b/crates/uv-workspace/src/pyproject.rs index 57582d93f43d..90419804a872 100644 --- a/crates/uv-workspace/src/pyproject.rs +++ b/crates/uv-workspace/src/pyproject.rs @@ -8,7 +8,7 @@ use glob::Pattern; use owo_colors::OwoColorize; -use serde::{de::IntoDeserializer, Deserialize, Serialize}; +use serde::{de::IntoDeserializer, Deserialize, Deserializer, Serialize}; use std::ops::Deref; use std::path::{Path, PathBuf}; use std::str::FromStr; @@ -455,7 +455,7 @@ enum SourcesWire { impl<'de> serde::de::Deserialize<'de> for SourcesWire { fn deserialize(deserializer: D) -> Result where - D: serde::de::Deserializer<'de>, + D: Deserializer<'de>, { serde_untagged::UntaggedEnumVisitor::new() .map(|map| map.deserialize().map(SourcesWire::One)) @@ -520,7 +520,7 @@ impl TryFrom for Sources { } /// 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))] #[serde(rename_all = "kebab-case", untagged, deny_unknown_fields)] pub enum Source { @@ -602,24 +602,293 @@ pub enum Source { )] marker: MarkerTree, }, - /// A catch-all variant used to emit precise error messages when deserializing. - CatchAll { - git: String, - subdirectory: Option, - rev: Option, - tag: Option, - branch: Option, - url: String, - path: PortablePathBuf, - index: String, - workspace: bool, - #[serde( - skip_serializing_if = "pep508_rs::marker::ser::is_empty", - serialize_with = "pep508_rs::marker::ser::serialize", - default - )] - marker: MarkerTree, - }, +} + +/// A custom deserialization implementation for [`Source`]. This is roughly equivalent to +/// `#[serde(untagged)]`, but provides more detailed error messages. +impl<'de> Deserialize<'de> for Source { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + #[derive(Deserialize, Debug, Clone)] + #[serde(rename_all = "kebab-case", deny_unknown_fields)] + struct CatchAll { + git: Option, + subdirectory: Option, + rev: Option, + tag: Option, + branch: Option, + url: Option, + path: Option, + editable: Option, + index: Option, + workspace: Option, + #[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)] @@ -763,7 +1032,6 @@ impl Source { Source::Path { marker, .. } => marker.clone(), Source::Registry { marker, .. } => marker.clone(), Source::Workspace { marker, .. } => marker.clone(), - Source::CatchAll { marker, .. } => marker.clone(), } } } diff --git a/uv.schema.json b/uv.schema.json index fa14d871eefe..ced2bb33b3f2 100644 --- a/uv.schema.json +++ b/uv.schema.json @@ -1356,66 +1356,6 @@ } }, "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 } ] },