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

Error spans incorrect when using flatten #589

Open
wez opened this issue Jul 28, 2023 · 6 comments
Open

Error spans incorrect when using flatten #589

wez opened this issue Jul 28, 2023 · 6 comments
Labels
A-error Area: Error reporting A-serde Area: Serde integration C-bug Category: Things not working as expected

Comments

@wez
Copy link

wez commented Jul 28, 2023

Here's a link to my example/repro:

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5651e21bcb73e6fabf7e1d0ef67660d2

and inline for convenience:

use serde::Deserialize;
use std::collections::HashMap;

#[derive(Deserialize, Debug)]
struct Thing {
   #[serde(flatten)]
   entries: HashMap<String, Entry>,
}

#[derive(Deserialize, Debug)]
struct Entry {
   foo: u32,
}

fn main() {
    let data = "# comment line\n[\"default\"]\nfooo = 42";
    match toml::from_str::<Thing>(&data) {
        Ok(data) => println!("{data:#?}"),
        Err(err) => println!("{err:#}"),
    }
}

produces:

TOML parse error at line 1, column 1
  |
1 | # comment line
  | ^
missing field `foo`

I would expect the line number to be 3 rather than 1, and for the context in the error message to refer to the appropriate part of the file.

@epage
Copy link
Member

epage commented Jul 28, 2023

Without debugging this, my best guess is that something in the way our code or serde is structured is preventing us from associating the foo error with the default table, making us associate it with the whole document which puts the error at the beginning of the file.

@epage epage added C-bug Category: Things not working as expected A-error Area: Error reporting labels Jul 28, 2023
@soywod
Copy link

soywod commented Dec 29, 2023

I have the same issue with a #[serde(flatten)] on a HashMap as well, which is a real problem for my tool because any wrong option points to the whole document:

#[derive(Clone, Debug, Default, Eq, PartialEq, Deserialize, Serialize)]
#[serde(rename_all = "kebab-case")]
pub struct TomlConfig {
    #[serde(alias = "name")]
    pub display_name: Option<String>,
    pub signature: Option<String>,
    pub signature_delim: Option<String>,
    pub downloads_dir: Option<PathBuf>,

    #[serde(flatten)]
    pub accounts: HashMap<String, TomlAccountConfig>,
}
[example]
backend = "imapp"
Error: cannot parse config file at "~/code/himalaya/config.sample.toml"

Caused by:
    TOML parse error at line 1, column 1
      |
    1 | [example]
      | ^
    unknown variant `imapp`, expected one of `maildir`, `imap`, `smtp`, `sendmail`

@baszalmstra
Copy link

I ran into this as well. I think it has something to do with flatten. See https://www.rustexplorer.com/b/uspq0k for a minimal reproducible case.

@epage
Copy link
Member

epage commented Jan 6, 2024

If I'm reading the serde_derive expanded code, it looks like this is a problem with how serde_derive deserializes things.

serde_derive seems to flatten content into something like serde_value (which can't error) and then deserializes into the flattened type (can error). This means the error is coming out of the deserializer for the outer table, and not on the individual field, and that is all toml/toml_edit can rely on for associating it with the code.

@soywod
Copy link

soywod commented Jan 6, 2024

Indeed, looks like a know issue at serde_derive, see serde-rs/serde#2581 and serde-rs/serde#1183. Should we close this issue or wait for serde_derive to fix it before closing it?

@epage
Copy link
Member

epage commented Jan 6, 2024

I think keeping it open but blocked on serde will better help people discover it.

@epage epage changed the title Line numbers and context are incorrect when data contains comments Error spans incorrect when using flatten Mar 8, 2024
@epage epage added the A-serde Area: Serde integration label Mar 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-error Area: Error reporting A-serde Area: Serde integration C-bug Category: Things not working as expected
Projects
None yet
Development

No branches or pull requests

4 participants