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.

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

Improve deserialization errors of untagged enums #12574

Merged
merged 2 commits into from
Aug 28, 2023

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Aug 27, 2023

What does this PR try to resolve?

# .cargo/config.toml

[http]
ssl-version.min = false

Before:

$ cargo check
error: data did not match any variant of untagged enum SslVersionConfig

After:

$ cargo check
error: error in /path/to/.cargo/config.toml: could not load config key `http.ssl-version`

Caused by:
  error in /path/to/.cargo/config.toml: `http.ssl-version.min` expected a string, but found a boolean

How should we test and review this PR?

The first commit adds tests showing the pre-existing error messages — mostly just "data did not match any variant of untagged enum T" with no location information. The second commit replaces all #[derive(Deserialize)] #[serde(untagged)] with Deserialize impls based on https://docs.rs/serde-untagged/0.1, showing the effect on the error messages.

Tested with cargo test, and by handwriting some bad .cargo/config.toml files and looking at the error produced by rust-lang/cargo/target/release/cargo check.

@rustbot
Copy link
Collaborator

rustbot commented Aug 27, 2023

r? @ehuss

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-configuration Area: cargo config files and env vars A-manifest Area: Cargo.toml issues S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2023
@epage
Copy link
Contributor

epage commented Aug 28, 2023

Thanks! I'm assuming this will also offer some (slight) performance improvements based on my own profiling with untagged.

@bors r+

Any thoughts on adding derive support for this?

@bors
Copy link
Contributor

bors commented Aug 28, 2023

📌 Commit 3871aec has been approved by epage

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 28, 2023
@bors
Copy link
Contributor

bors commented Aug 28, 2023

⌛ Testing commit 3871aec with merge 97e73c7...

@dtolnay
Copy link
Member Author

dtolnay commented Aug 28, 2023

Yeah we could potentially add derive support by having the call site tag the type of each of the variants.

pub enum SslVersionConfig {
    #[serde(string)]
    Single(String),
    #[serde(map)]
    Range(SslVersionConfigRange),
}

@bors
Copy link
Contributor

bors commented Aug 28, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 97e73c7 to master...

@bors bors merged commit 97e73c7 into rust-lang:master Aug 28, 2023
@dtolnay dtolnay deleted the untagged branch August 28, 2023 20:59
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2023
Update cargo

18 commits in 925280f028db3a322935e040719a0754703947cf..96fe1c9e1aecd8f57063e3753969bb6418fd2fd5
2023-08-25 21:16:44 +0000 to 2023-08-29 20:10:34 +0000
- fix(lints): Fail when overriding inherited lints (rust-lang/cargo#12584)
- cargo install: suggest --git when package name is url (rust-lang/cargo#12575)
- chore: remove unstable-options for logout (rust-lang/cargo#12588)
- Improve logout message for asymmetric tokens (rust-lang/cargo#12587)
- fix(update): Remove references to -p in help (rust-lang/cargo#12586)
- fix(update): Make `-p` more convenient by being positional (rust-lang/cargo#12545)
- Set tracing target for networking messages. (rust-lang/cargo#12582)
- Retry docs (rust-lang/cargo#12583)
- feat(resolver): **Very** preliminary MSRV resolver support (rust-lang/cargo#12560)
- Update git2 (rust-lang/cargo#12580)
- Explain how `version` works for `git` dependencies (rust-lang/cargo#12270)
- Improve deserialization errors of untagged enums (rust-lang/cargo#12574)
- Add support for `target.'cfg(..)'.linker` (rust-lang/cargo#12535)
- Improve resolver version mismatch warning (rust-lang/cargo#12573)
- Stabilize `--keep-going` (rust-lang/cargo#12568)
- Define {{command}} for use in src/doc/man/includes (rust-lang/cargo#12570)
- Update serde (rust-lang/cargo#12569)
- chore: add missing `windows-sys` features back (rust-lang/cargo#12563)

r? ghost
bors added a commit that referenced this pull request Aug 31, 2023
refactor: Use more serde_untagged

### What does this PR try to resolve?

This is a follow up to #12574 that replaces hand-implemented Visitors with `serde_untagged` because I felt it is cleaner code

Due to an error reporting limitation in `serde_untagged`, I'm not using
this for some `MaybeWorkspace` types because it makes the errors worse.  See dtolnay/serde-untagged#2

I also held off on some config visitors because they seemed more
complicated and I didn't want to risk that code.

### How should we test and review this PR?

The main caveat is we might not have tests for every single error case.

### Additional information
@ehuss ehuss added this to the 1.74.0 milestone Sep 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-configuration Area: cargo config files and env vars A-manifest Area: Cargo.toml issues S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants