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

Some errors are missing line number information. #203

Closed
halvnykterist opened this issue Mar 6, 2020 · 8 comments · Fixed by #356
Closed

Some errors are missing line number information. #203

halvnykterist opened this issue Mar 6, 2020 · 8 comments · Fixed by #356
Labels
feature A new feature for the syntax or library. help wanted

Comments

@halvnykterist
Copy link

Hi,
I'm not very familiar with serde so I don't know if this is a problem with that, but certain error messages are pretty unhelpful right now for fixing errors. In particular, a great deal of errors are missing information about what line they came from (or just have it baked into the message - this is better than nothing, but it would be more pleasant to actually get the location out so I can present it to the user.)

As an example step, the following code (edited from the example):

use ron::de::from_str;
use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct Config {
    boolean: bool,
    tuple: (u32, u32),
}

const CONFIG: &str = "
(
    boolean: true, 
    //tuple: (3 /*(2 + 1)*/, 7 /*(2 * 5 - 3)*/), //Notice the commented out field here
)";

fn main() {
    let config: Config = match from_str(CONFIG) {
        Ok(x) => x,
        Err(e) => {
            panic!("{:?}", e)
        }
    };

    println!("Config: {:?}", &config);
}

will just print out missing field \tuple``, which isn't super helpful in larger files.

I took a look around in the code to see if there was anything I could hack in myself to help with this, but I had a hard time actually tracing the path the code follows.

@kvark kvark added feature A new feature for the syntax or library. help wanted labels Mar 11, 2020
@kvark
Copy link
Collaborator

kvark commented Mar 11, 2020

Yes, line number information would be terrific to have!
I happen to be working on WGSL parsing in gfx-rs/naga#16 and realized there is no way to debug this without proper line/column numbers. Moreover, ideally we'd have a stack of scopes in an error, each with its own line/column on where it started.

@halvnykterist
Copy link
Author

I wouldn't mind trying to take a look at this, but do you have any idea of where I should start digging?

@kvark
Copy link
Collaborator

kvark commented Apr 8, 2020

@halvnykterist you could start by looking at how Naga does it for WGSL. It's unlikely the best example, but it's much more useful than nothing that RON has :)

@halvnykterist
Copy link
Author

halvnykterist commented Apr 11, 2020

As a sanity check I wanted to make sure that serde_json does handle this correctly, and it does:

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct Config {
    boolean: bool,
    tuple: (u32, u32),
}

const CONFIG: &str = "
(
    boolean: true, 
    //tuple: (3 /*(2 + 1)*/, 7 /*(2 * 5 - 3)*/), //Notice the commented out field here
)";

const CONFIG_JSON: &str = "
{
    \"boolean\": true
}";

fn main() {
    let config_json: Config = match serde_json::de::from_str(CONFIG_JSON) {
        Ok(x) => x,
        Err(e) => {
            panic!("ERROR: {:?}", e)
        }
    };

    println!("JSON: Config: {:?}", &config);
}

prints out
ERROR: Error("missing field `tuple`", line: 4, column: 1)

I'm looking through serde_json now to see how they actually store that.

Stacked scope of errors would be interesting, although I'm not sure how that'd work with serde's error handling.

@halvnykterist
Copy link
Author

I found a way to add them, using serde's json deserializer as a model and I'm doing a pull request of a sketch of how it could be done, it feels a bit messy however and I'm not sure if there would be a way to handle it more cleanly. Basically, I'm taking all the visit_X calls and mapping the error to include the line number if applicable.

@halvnykterist
Copy link
Author

#209

@github-actions
Copy link

Issue has had no activity in the last 180 days and is going to be closed in 7 days if no further activity occurs

@github-actions github-actions bot added the stale label Nov 18, 2021
@kvark kvark reopened this Dec 3, 2021
@kvark kvark reopened this Dec 17, 2021
@kvark
Copy link
Collaborator

kvark commented Dec 17, 2021

wtf bot? going to close this every 2 weeks now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature for the syntax or library. help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants