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

negative explicit variant discriminator not allowed in enums #592

Closed
trevyn opened this issue Oct 8, 2022 · 3 comments · Fixed by #593
Closed

negative explicit variant discriminator not allowed in enums #592

trevyn opened this issue Oct 8, 2022 · 3 comments · Fixed by #593

Comments

@trevyn
Copy link
Contributor

trevyn commented Oct 8, 2022

From #537 (comment):

error: Invalid rust syntax, expected literal, got Some(Punct { ch: '-', spacing: Alone, span: #0 bytes(20762..20763) })

#[derive(Encode, Decode)]
pub enum TypeOfFile {
    Unknown = -1,
}

It appears that enum discriminators are encoded as u32, and changing this to i32 will change the byte encoding of positive discriminators as well.

I've done some work on a patch for this if this is a desirable change.

@VictorKoenders
Copy link
Contributor

It looks like serde encodes the variants as their index, not as the actual value:

#[derive(serde::Serialize)]
enum Foo {
    A = -1,
}

fn main() {
    println!("{:?}", bincode::serialize(&Foo::A))
}

Prints out Ok([0, 0, 0, 0])

Unfortunately we have to remain backwards compatible with bincode 1.3.3 so the change is that we should encode it as the variant index, not the value of the variant.

If you're willing to make this PR that would be appreciated!

@trevyn
Copy link
Contributor Author

trevyn commented Oct 8, 2022

I was looking at Encode/Decode; so changing its encoding from the current is OK?

@VictorKoenders
Copy link
Contributor

Yes, I believe the current implementation encodes enum variants as their values. They need to be encoded as their index

enum Foo {
    Bar = 5, // needs to be encoded as 0u32
}

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