Skip to content

Commit

Permalink
Remove flatten to improve deserialization error messages (#7598)
Browse files Browse the repository at this point in the history
## Summary

`#[serde(flatten)]` has a disastrous effect on error messages: serde no
longer tells you which field errored, nor does it show it to you in the
diagnostic output.

Before:

```
warning: Failed to parse `pyproject.toml` during settings discovery:
  TOML parse error at line 9, column 1
    |
  9 | [tool.uv]
    | ^^^^^^^^^
  invalid type: string "foo", expected a sequence
```

After:

```
warning: Failed to parse `pyproject.toml` during settings discovery:
  TOML parse error at line 10, column 19
     |
  10 | extra-index-url = "foo"
     |                   ^^^^^
  invalid type: string "foo", expected a sequence
```

Closes #7113.
  • Loading branch information
charliermarsh committed Sep 20, 2024
1 parent 85af273 commit 86ff740
Show file tree
Hide file tree
Showing 4 changed files with 240 additions and 29 deletions.
195 changes: 171 additions & 24 deletions crates/uv-settings/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,15 @@ pub(crate) struct Tools {
/// A `[tool.uv]` section.
#[allow(dead_code)]
#[derive(Debug, Clone, Default, Deserialize, CombineOptions, OptionsMetadata)]
#[serde(rename_all = "kebab-case", deny_unknown_fields)]
#[serde(from = "OptionsWire", rename_all = "kebab-case")]
#[cfg_attr(feature = "schemars", derive(schemars::JsonSchema))]
pub struct Options {
#[serde(flatten)]
pub globals: GlobalOptions,

#[serde(flatten)]
pub top_level: ResolverInstallerOptions,

#[option_group]
pub pip: Option<PipOptions>,

Expand Down Expand Up @@ -79,7 +81,6 @@ pub struct Options {
cache-keys = [{ file = "pyproject.toml" }, { file = "requirements.txt" }, { git = true }]
"#
)]
#[serde(default, skip_serializing)]
cache_keys: Option<Vec<CacheKey>>,

// NOTE(charlie): These fields are shared with `ToolUv` in
Expand All @@ -92,28 +93,6 @@ pub struct Options {

#[cfg_attr(feature = "schemars", schemars(skip))]
pub environments: Option<SupportedEnvironments>,

// NOTE(charlie): These fields should be kept in-sync with `ToolUv` in
// `crates/uv-workspace/src/pyproject.rs`.
#[serde(default, skip_serializing)]
#[cfg_attr(feature = "schemars", schemars(skip))]
workspace: serde::de::IgnoredAny,

#[serde(default, skip_serializing)]
#[cfg_attr(feature = "schemars", schemars(skip))]
sources: serde::de::IgnoredAny,

#[serde(default, skip_serializing)]
#[cfg_attr(feature = "schemars", schemars(skip))]
dev_dependencies: serde::de::IgnoredAny,

#[serde(default, skip_serializing)]
#[cfg_attr(feature = "schemars", schemars(skip))]
managed: serde::de::IgnoredAny,

#[serde(default, skip_serializing)]
#[cfg_attr(feature = "schemars", schemars(skip))]
r#package: serde::de::IgnoredAny,
}

impl Options {
Expand Down Expand Up @@ -1472,3 +1451,171 @@ impl From<ToolOptions> for ResolverInstallerOptions {
}
}
}

/// Like [`Options]`, but with any `#[serde(flatten)]` fields inlined. This leads to far, far
/// better error messages when deserializing.
#[derive(Debug, Clone, Default, Deserialize)]
#[serde(rename_all = "kebab-case", deny_unknown_fields)]
pub struct OptionsWire {
// #[serde(flatten)]
// globals: GlobalOptions,
native_tls: Option<bool>,
offline: Option<bool>,
no_cache: Option<bool>,
cache_dir: Option<PathBuf>,
preview: Option<bool>,
python_preference: Option<PythonPreference>,
python_downloads: Option<PythonDownloads>,
concurrent_downloads: Option<NonZeroUsize>,
concurrent_builds: Option<NonZeroUsize>,
concurrent_installs: Option<NonZeroUsize>,

// #[serde(flatten)]
// top_level: ResolverInstallerOptions,
index_url: Option<IndexUrl>,
extra_index_url: Option<Vec<IndexUrl>>,
no_index: Option<bool>,
find_links: Option<Vec<FlatIndexLocation>>,
index_strategy: Option<IndexStrategy>,
keyring_provider: Option<KeyringProviderType>,
allow_insecure_host: Option<Vec<TrustedHost>>,
resolution: Option<ResolutionMode>,
prerelease: Option<PrereleaseMode>,
dependency_metadata: Option<Vec<StaticMetadata>>,
config_settings: Option<ConfigSettings>,
no_build_isolation: Option<bool>,
no_build_isolation_package: Option<Vec<PackageName>>,
exclude_newer: Option<ExcludeNewer>,
link_mode: Option<LinkMode>,
compile_bytecode: Option<bool>,
no_sources: Option<bool>,
upgrade: Option<bool>,
upgrade_package: Option<Vec<Requirement<VerbatimParsedUrl>>>,
reinstall: Option<bool>,
reinstall_package: Option<Vec<PackageName>>,
no_build: Option<bool>,
no_build_package: Option<Vec<PackageName>>,
no_binary: Option<bool>,
no_binary_package: Option<Vec<PackageName>>,

pip: Option<PipOptions>,
cache_keys: Option<Vec<CacheKey>>,

// NOTE(charlie): These fields are shared with `ToolUv` in
// `crates/uv-workspace/src/pyproject.rs`, and the documentation lives on that struct.
override_dependencies: Option<Vec<Requirement<VerbatimParsedUrl>>>,
constraint_dependencies: Option<Vec<Requirement<VerbatimParsedUrl>>>,
environments: Option<SupportedEnvironments>,

// NOTE(charlie): These fields should be kept in-sync with `ToolUv` in
// `crates/uv-workspace/src/pyproject.rs`.
#[allow(dead_code)]
workspace: Option<serde::de::IgnoredAny>,
#[allow(dead_code)]
sources: Option<serde::de::IgnoredAny>,
#[allow(dead_code)]
dev_dependencies: Option<serde::de::IgnoredAny>,
#[allow(dead_code)]
managed: Option<serde::de::IgnoredAny>,
#[allow(dead_code)]
r#package: Option<serde::de::IgnoredAny>,
}

impl From<OptionsWire> for Options {
fn from(value: OptionsWire) -> Self {
let OptionsWire {
native_tls,
offline,
no_cache,
cache_dir,
preview,
python_preference,
python_downloads,
concurrent_downloads,
concurrent_builds,
concurrent_installs,
index_url,
extra_index_url,
no_index,
find_links,
index_strategy,
keyring_provider,
allow_insecure_host,
resolution,
prerelease,
dependency_metadata,
config_settings,
no_build_isolation,
no_build_isolation_package,
exclude_newer,
link_mode,
compile_bytecode,
no_sources,
upgrade,
upgrade_package,
reinstall,
reinstall_package,
no_build,
no_build_package,
no_binary,
no_binary_package,
pip,
cache_keys,
override_dependencies,
constraint_dependencies,
environments,
workspace: _,
sources: _,
dev_dependencies: _,
managed: _,
package: _,
} = value;

Self {
globals: GlobalOptions {
native_tls,
offline,
no_cache,
cache_dir,
preview,
python_preference,
python_downloads,
concurrent_downloads,
concurrent_builds,
concurrent_installs,
},
top_level: ResolverInstallerOptions {
index_url,
extra_index_url,
no_index,
find_links,
index_strategy,
keyring_provider,
allow_insecure_host,
resolution,
prerelease,
dependency_metadata,
config_settings,
no_build_isolation,
no_build_isolation_package,
exclude_newer,
link_mode,
compile_bytecode,
no_sources,
upgrade,
upgrade_package,
reinstall,
reinstall_package,
no_build,
no_build_package,
no_binary,
no_binary_package,
},
pip,
cache_keys,
override_dependencies,
constraint_dependencies,
environments,
}
}
}
67 changes: 66 additions & 1 deletion crates/uv/tests/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ fn invalid_pyproject_toml_syntax() -> Result<()> {
}

#[test]
fn invalid_pyproject_toml_schema() -> Result<()> {
fn invalid_pyproject_toml_project_schema() -> Result<()> {
let context = TestContext::new("3.12");
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str("[project]")?;
Expand All @@ -139,6 +139,71 @@ fn invalid_pyproject_toml_schema() -> Result<()> {
Ok(())
}

#[test]
fn invalid_pyproject_toml_option_schema() -> Result<()> {
let context = TestContext::new("3.12");
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! {r"
[tool.uv]
index-url = true
"})?;

uv_snapshot!(context.pip_install()
.arg("iniconfig"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
warning: Failed to parse `pyproject.toml` during settings discovery:
TOML parse error at line 2, column 13
|
2 | index-url = true
| ^^^^
invalid type: boolean `true`, expected a string
Resolved 1 package in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ iniconfig==2.0.0
"###
);

Ok(())
}

#[test]
fn invalid_pyproject_toml_option_unknown_field() -> Result<()> {
let context = TestContext::new("3.12");
let pyproject_toml = context.temp_dir.child("pyproject.toml");
pyproject_toml.write_str(indoc! {r#"
[tool.uv]
unknown = "field"
"#})?;

uv_snapshot!(context.pip_install()
.arg("-r")
.arg("pyproject.toml"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
warning: Failed to parse `pyproject.toml` during settings discovery:
TOML parse error at line 2, column 1
|
2 | unknown = "field"
| ^^^^^^^
unknown field `unknown`, expected one of `native-tls`, `offline`, `no-cache`, `cache-dir`, `preview`, `python-preference`, `python-downloads`, `concurrent-downloads`, `concurrent-builds`, `concurrent-installs`, `index-url`, `extra-index-url`, `no-index`, `find-links`, `index-strategy`, `keyring-provider`, `allow-insecure-host`, `resolution`, `prerelease`, `dependency-metadata`, `config-settings`, `no-build-isolation`, `no-build-isolation-package`, `exclude-newer`, `link-mode`, `compile-bytecode`, `no-sources`, `upgrade`, `upgrade-package`, `reinstall`, `reinstall-package`, `no-build`, `no-build-package`, `no-binary`, `no-binary-package`, `pip`, `cache-keys`, `override-dependencies`, `constraint-dependencies`, `environments`, `workspace`, `sources`, `dev-dependencies`, `managed`, `package`
Resolved in [TIME]
Audited in [TIME]
"###
);

Ok(())
}

/// For indirect, non-user controlled pyproject.toml, we don't enforce correctness.
///
/// If we fail to extract the PEP 621 metadata, we fall back to treating it as a source
Expand Down
6 changes: 3 additions & 3 deletions crates/uv/tests/show_settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3146,11 +3146,11 @@ fn resolve_config_file() -> anyhow::Result<()> {
----- stderr -----
error: Failed to parse: `[CACHE_DIR]/uv.toml`
Caused by: TOML parse error at line 1, column 1
Caused by: TOML parse error at line 1, column 2
|
1 | [project]
| ^
unknown field `project`
| ^^^^^^^
unknown field `project`, expected one of `native-tls`, `offline`, `no-cache`, `cache-dir`, `preview`, `python-preference`, `python-downloads`, `concurrent-downloads`, `concurrent-builds`, `concurrent-installs`, `index-url`, `extra-index-url`, `no-index`, `find-links`, `index-strategy`, `keyring-provider`, `allow-insecure-host`, `resolution`, `prerelease`, `dependency-metadata`, `config-settings`, `no-build-isolation`, `no-build-isolation-package`, `exclude-newer`, `link-mode`, `compile-bytecode`, `no-sources`, `upgrade`, `upgrade-package`, `reinstall`, `reinstall-package`, `no-build`, `no-build-package`, `no-binary`, `no-binary-package`, `pip`, `cache-keys`, `override-dependencies`, `constraint-dependencies`, `environments`, `workspace`, `sources`, `dev-dependencies`, `managed`, `package`
"###
);
Expand Down
1 change: 0 additions & 1 deletion uv.schema.json

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

0 comments on commit 86ff740

Please sign in to comment.