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

Serialisation regression in #1183 #1270

Closed
aborgna-q opened this issue Jul 8, 2024 · 2 comments · Fixed by #1271 or #1275
Closed

Serialisation regression in #1183 #1270

aborgna-q opened this issue Jul 8, 2024 · 2 comments · Fixed by #1271 or #1275
Assignees
Labels
bug Something isn't working

Comments

@aborgna-q
Copy link
Collaborator

#1183 included a breaking change to the serialisation format, removing the input_extensions field from the operations.

This should have either been a new v2 serialisation with backwards compatibility, or implemented so that the field gets silently ignored (if possible).

This json was generated by guppylang 0.5.2 (which uses hugr-py v0.2.0a1). Since it includes the input extensions fields it produces errors when decoded with the latest hugr crate, but works correctly once the fields are removed.
https://gist.github.com/aborgna-q/f4d6caa1fd7f9b5288935f39df4d4918

@aborgna-q aborgna-q added the bug Something isn't working label Jul 8, 2024
@ss2165 ss2165 self-assigned this Jul 8, 2024
@ss2165
Copy link
Member

ss2165 commented Jul 8, 2024

Not sure this is the regression as described.
Serde should by default just ignore extra fields.
This is confirmed by noting that just removing the input_extensions key from the first node in the example is enough for deserialization to succeed (every other node still has the key).

The issue in reality appears to be hinted at by the error message

invalid type: map, expected unit struct Module

Since OpType is flattened into NodeSer it seems that for unit struct Module, the input_extensions key is being interpreted as a field of the flattened inner struct. This may be a purely serde bug. A minimal example:

    #[derive(Serialize, Deserialize)]
    struct Unit;

    #[derive(Serialize, Deserialize)]
    #[serde(tag = "u")]
    enum UnitOrNot {
        Unit(Unit),
        NotUnit { field: String },
    }
    #[derive(Serialize, Deserialize)]
    struct Nested {
        #[serde(flatten)]
        inner: UnitOrNot,
    }

    let data = r#"
        [
            {
                "u": "NotUnit",
                "field": "value",
                "ignored": "bar"
            },
            {
                "u": "Unit",
                "ignored": "foo"
            }
        
        ]"#;

    let _: Vec<Nested> = serde_json::from_str(data).unwrap();

The first element of the array deserializes correctly but the second throws:

Error: Error("invalid type: map, expected unit struct Unit", line: 11, column: 13)

Worth reporting as a serde bug? It may be a known edge case interaction of unit struct/flatten/internal tagging.

In the mean-time, believing Module is the only op that is a unit struct, going to try a heinous hack fix...

ss2165 added a commit that referenced this issue Jul 8, 2024
Fixes #1270

A simpler test json might be better but kept the original just to show it is fixed.

BREAKING CHANGE: `ops::Module` no longer unit struct, must be constructed with `new()` or `default()`
@ss2165
Copy link
Member

ss2165 commented Jul 8, 2024

Serde issue: serde-rs/serde#2775

ss2165 added a commit that referenced this issue Jul 8, 2024
(skip serializing, deserialize to None)

non-breaking patch for #1270
github-merge-queue bot pushed a commit that referenced this issue Jul 8, 2024
ss2165 added a commit that referenced this issue Jul 8, 2024
Fixes #1270

A simpler test json might be better but kept the original just to show it is fixed.

BREAKING CHANGE: `ops::Module` no longer unit struct, must be constructed with `new()` or `default()`
aborgna-q pushed a commit that referenced this issue Jul 8, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 8, 2024
Fixes #1270 without keeping the unused `input_extensions` field. 

BREAKING CHANGE: `ops::Module` no longer unit struct, must be
constructed with `new()` or `default()`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
2 participants