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

Unexpected behaviour of implicit_some #367

Closed
maurges opened this issue Feb 23, 2022 · 3 comments · Fixed by #368
Closed

Unexpected behaviour of implicit_some #367

maurges opened this issue Feb 23, 2022 · 3 comments · Fixed by #368

Comments

@maurges
Copy link

maurges commented Feb 23, 2022

The description of this extension is kind of casual, so I went to test it:

#[derive(Debug, serde::Deserialize, serde::Serialize)]
struct MaybeFields {
    f1: i64,
    f2: Option<i64>,
    f3: Option<Option<i64>>,
}

fn main() {
    let x1: std::result::Result<MaybeFields, _> = ron::from_str(
        "#![enable(implicit_some)]\n(f1: 1)"
    );
    let x2: std::result::Result<MaybeFields, _> = ron::from_str(
        "#![enable(implicit_some)]\n(f1: 1, f2: None, f3: None)"
    );
    let x3: std::result::Result<MaybeFields, _> = ron::from_str(
        "#![enable(implicit_some)]\n(f1: 1, f2: 2, f3: 3)"
    );
    let x4: std::result::Result<MaybeFields, _> = ron::from_str(
        "#![enable(implicit_some)]\n(f1: 1, f2: 2, f3: Some(3))"
    );
    let x5: std::result::Result<MaybeFields, _> = ron::from_str(
        "#![enable(implicit_some)]\n(f1: 1, f2: 2, f3: Some(Some(3)))"
    );
    let x6: std::result::Result<MaybeFields, _> = ron::from_str(
        "#![enable(implicit_some)]\n(f1: 1, f2: 2, f3: Some(None))"
    );

    println!("{:?}", &x1);
    println!("{:?}", &x2);
    println!("{:?}", &x3);
    println!("{:?}", &x4);
    println!("{:?}", &x5);
    println!("{:?}", &x6);
}

Gives

Ok(MaybeFields { f1: 1, f2: None, f3: None })
Ok(MaybeFields { f1: 1, f2: None, f3: None })
Ok(MaybeFields { f1: 1, f2: Some(2), f3: Some(Some(3)) })
Err(Error { code: ExpectedInteger, position: Position { line: 2, col: 20 } })
Err(Error { code: ExpectedInteger, position: Position { line: 2, col: 20 } })
Err(Error { code: ExpectedInteger, position: Position { line: 2, col: 20 } })

I expected None and Some to work the same way, but it seems that having this implicit_some turned on disallows you to write Some altogether. This means that you can't express {f2: Some(None)} with this extension.

Now actually I'm writing an implementation of RON in another language, and it seems to me now that with this extension some things just become unexpressible. This is fine, but I just need to be certain that this current behaviour is what's expected and not a bug in the implementation. If it is, can you please document it?

@maurges
Copy link
Author

maurges commented Feb 23, 2022

Also since we're talking documentation, my impression from the extensions page was that implicit_some only works on structs, but it actually works with anything:

    let y1: std::result::Result<Option<i64>, _> = ron::from_str(
        "#![enable(implicit_some)]\n1"
    );
    println!("{:?}", &y1);
    // gives Ok(Some(1))

And this impression comes from the phrase "if the deserialized struct requires it"

juntyr added a commit to juntyr/ron that referenced this issue Feb 24, 2022
@juntyr
Copy link
Member

juntyr commented Feb 24, 2022

@d86leader Thank you very much for reporting this issue! Since this behaviour was not documented and fixing it did not break any tests, I feel like allowing users to still explicitly write Some(..) is the better solution here (vs. the unwrap_X extensions which by design and necessity alter which configs can be parsed as they always have to unwrap).

@juntyr
Copy link
Member

juntyr commented Feb 24, 2022

I also clarified the docs a bit and added some unit tests to ensure this will now be the expected behaviour.

juntyr added a commit to juntyr/ron that referenced this issue Feb 24, 2022
kvark pushed a commit that referenced this issue Feb 25, 2022
torkleyy pushed a commit to torkleyy/ron that referenced this issue Jun 6, 2022
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

Successfully merging a pull request may close this issue.

2 participants