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

Don't convert deserializer erros into custom errors #2

Open
epage opened this issue Aug 31, 2023 · 1 comment
Open

Don't convert deserializer erros into custom errors #2

epage opened this issue Aug 31, 2023 · 1 comment

Comments

@epage
Copy link

epage commented Aug 31, 2023

rust-lang/cargo#12581 originally included updating some of the code paths to use this package but ran into error problems.

Originally, the following error was reported

  TOML parse error at line 7, column 41
    |
  7 |             description = { workspace = false }
    |                                         ^^^^^
  `workspace` cannot be false

and now

  TOML parse error at line 7, column 27
    |
  7 |             description = { workspace = false }
    |                           ^^^^^^^^^^^^^^^^^^^^^
  `workspace` cannot be false
  in `workspace`

This is because the deserializer reports

  `workspace` cannot be false
  in `workspace`

and then gets passed through Error::custom, capturing that as-is. The deserializer sees this error, captures the span as of Error::custom, and then bubbles that up the stack. At the top, the rest of the context is added so the following gets prefixed to it

  TOML parse error at line 7, column 27
    |
  7 |             description = { workspace = false }
    |                           ^^^^^^^^^^^^^^^^^^^^^

Another "in ..." isn't added because that is only rendered when the source context isn't there to render the line.

@epage
Copy link
Author

epage commented Aug 31, 2023

One potential solution is to not provide a custom error type that sits inbetween the deserializer and itself but to instead be generic over E. Im assuming that was originally considered and there are drawbacks.

bors added a commit to rust-lang/cargo that referenced this issue 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant