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

Add #[borsh(use_discriminant = <bool>)] that change enum discriminant de- and serialization behavior #148 #183

Merged
merged 35 commits into from
Jul 31, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
b1a029f
WIP
iho Jul 27, 2023
46d5d99
Update insta snapshots
iho Jul 27, 2023
cd178dc
clippy
iho Jul 27, 2023
a9634e2
clippy
iho Jul 27, 2023
889d3de
Add documentation
iho Jul 27, 2023
ae73c13
remove debug prints; add documentation
iho Jul 27, 2023
09f2872
fix doc
iho Jul 27, 2023
293e3c8
remove debug print
iho Jul 27, 2023
4e820fe
Apply suggestions from code review
iho Jul 27, 2023
a1ea2e4
Merge remote-tracking branch 'iho/iho_enum_discriminant_attr_macros_2…
iho Jul 27, 2023
b952d94
Add test; bump Rust version in CI; fix typos
iho Jul 27, 2023
20a5063
refactoring
iho Jul 27, 2023
e58f45b
Apply suggestion for check_item_attributes
iho Jul 27, 2023
80bf4e8
Add docs for
iho Jul 27, 2023
6516978
remove comment; add documentation
iho Jul 27, 2023
bfddc0c
clippy
iho Jul 27, 2023
6769a32
replace panic in macros to explicit error
iho Jul 27, 2023
d02b68e
Apply suggestions from code review
iho Jul 27, 2023
4268247
Merge remote-tracking branch 'iho/iho_enum_discriminant_attr_macros_2…
iho Jul 27, 2023
3a9a314
fix test
iho Jul 27, 2023
c5fed0d
Fix comments; Add suggested tests
iho Jul 27, 2023
ed91c8f
Add documentation; Bump Rust compiler version
iho Jul 28, 2023
5a37fc5
Apply suggestions from code review
iho Jul 28, 2023
2b0266b
Fix tests
iho Jul 28, 2023
52e5550
Fix test; clippy
iho Jul 28, 2023
dbb1338
Apply suggestions from code review
iho Jul 30, 2023
a99c500
Move enum related tests to separate file
iho Jul 30, 2023
0a342a5
fix doc CI
iho Jul 30, 2023
b5cb18c
docs: Update "Enum with explicit discriminant" README.md
frol Jul 30, 2023
dbd3382
fix(ci): conditional downgrade of time for older toolchain
iho Jul 31, 2023
64af896
Merge remote-tracking branch 'iho/iho_enum_discriminant_attr_macros_2…
iho Jul 31, 2023
01db1f4
ci: fixed the condition on when to downgrade `time` crate in CI workflow
frol Jul 31, 2023
144ffaf
Fix English in doc
iho Jul 31, 2023
f242f2e
chore: revert unneeded imports reordering
Jul 31, 2023
d6fdb9e
chore: duplicate `if let Err(...)` instead of `match`
Jul 31, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,37 @@ struct A {
}
```

### Enum with explicit discriminant

`#[borsh(use_discriminant=false|true])` is required if you have an enum with explicit discriminant. This settings affects `BorshSerialize` and `BorshDeserialize` behaviour at the same time.

If you don't specify `use_discriminant` option for enum with explicit discriminant, you will get an error:

````bash
error: You have to specify `#[borsh(use_discriminant=true)]` or `#[borsh(use_discriminant=false)]` for all structs that have enum with explicit discriminant
dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
```
```rust
#[derive(BorshDeserialize, BorshSerialize)]
#[borsh(use_discriminant=false)]
enum A {
X,
Y = 10,
}
````

Will keep old behaviour of borsh deserialization and will not use discriminant. This option is left to have backward compatability with previous versions of borsh and to have ability to deserialise data from previous versions of borsh.

```rust
#[derive(BorshDeserialize, BorshSerialize)]
#[borsh(use_discriminant=true)]
enum A {
X,
Y = 10,
}
```

This one will use proper version of serialization of enum with explicit discriminant.

## Releasing

The versions of all public crates in this repository are collectively managed by a single version in the [workspace manifest](https://github.com/near/borsh-rs/blob/master/Cargo.toml).
Expand Down
144 changes: 143 additions & 1 deletion borsh-derive-internal/src/attribute_helpers.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// TODO: remove this unused attribute, when the unsplit is done
#![allow(unused)]
use syn::{Attribute, Field, Path, WherePredicate};
use proc_macro2::TokenStream;
use quote::ToTokens;
use syn::{spanned::Spanned, Attribute, DeriveInput, Expr, Field, Path, WherePredicate};
pub mod parsing_helpers;
use parsing_helpers::get_where_predicates;

Expand All @@ -13,6 +15,8 @@ pub struct Symbol(pub &'static str);
pub const BORSH: Symbol = Symbol("borsh");
/// bound - sub-borsh nested meta, field-level only, `BorshSerialize` and `BorshDeserialize` contexts
pub const BOUND: Symbol = Symbol("bound");
// item level attribute for enums
dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
pub const USE_DISCRIMINANT: &str = "use_discriminant";
/// serialize - sub-bound nested meta attribute
pub const SERIALIZE: Symbol = Symbol("serialize");
/// deserialize - sub-bound nested meta attribute
Expand Down Expand Up @@ -42,6 +46,52 @@ pub(crate) fn contains_skip(attrs: &[Attribute]) -> bool {
attrs.iter().any(|attr| attr.path() == SKIP)
}

pub fn check_item_attributes(derive_input: &DeriveInput) -> Result<(), proc_macro2::TokenStream> {
for attr in derive_input.attrs.clone() {
if attr.path().is_ident(BORSH.0) {
if let syn::Data::Struct(ref _data) = derive_input.data {
return Err(syn::Error::new(
derive_input.ident.span(),
"borsh (use_discriminant=<bool>) does not support structs",
)
.to_compile_error());
}
}
}
Ok(())
}

pub(crate) fn contains_use_discriminant(attrs: &[Attribute]) -> Result<Option<bool>, TokenStream> {
iho marked this conversation as resolved.
Show resolved Hide resolved
let mut result = None;
for attr in attrs {
if attr.path().is_ident("borsh") {
dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
attr.parse_nested_meta(|meta| {
if meta.path.is_ident(USE_DISCRIMINANT) {
let value_expr: Expr = meta.value()?.parse()?;
let value = value_expr.to_token_stream().to_string();
// this goes to contains_use_discriminant
dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
match value.as_str() {
"true" => {
result = Some(true);
}
"false" => result = Some(false),
_ => {
return Err(syn::Error::new(
value_expr.span(),
"`use_discriminant` accept only `true` or `false`",
iho marked this conversation as resolved.
Show resolved Hide resolved
));
}
};
}

Ok(())
})
.map_err(|err| err.to_compile_error())?;
}
}
Ok(result)
}

pub(crate) fn contains_initialize_with(attrs: &[Attribute]) -> Option<Path> {
for attr in attrs.iter() {
if attr.path() == INIT {
Expand Down Expand Up @@ -387,4 +437,96 @@ mod tests {
let schema_params = parse_schema_attrs(&first_field.attrs).unwrap();
assert!(schema_params.is_none());
}

use super::*;
#[test]
fn test_check_use_discriminant() {
let item_enum: DeriveInput = syn::parse2(quote! {
#[derive(BorshDeserialize, Debug)]
#[borsh(use_discriminant = false)]
enum AWithUseDiscriminantFalse {
X,
Y,
}
})
.unwrap();
let actual = contains_use_discriminant(&item_enum.attrs);
assert!(actual.is_ok());
iho marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
fn test_check_use_discriminant_true() {
let item_enum: DeriveInput = syn::parse2(quote! {
#[derive(BorshDeserialize, Debug)]
#[borsh(use_discriminant = true)]
enum AWithUseDiscriminantTrue {
X,
Y,
}
})
.unwrap();
let actual = contains_use_discriminant(&item_enum.attrs);
assert!(actual.is_ok());
iho marked this conversation as resolved.
Show resolved Hide resolved
}

#[test]
fn test_check_use_discriminant_wrong_value() {
let item_enum: DeriveInput = syn::parse2(quote! {
#[derive(BorshDeserialize, Debug)]
#[borsh(use_discriminant = 111)]
enum AWithUseDiscriminantFalse {
X,
Y,
}
})
.unwrap();
let actual = contains_use_discriminant(&item_enum.attrs);
assert!(actual.is_err());
iho marked this conversation as resolved.
Show resolved Hide resolved
}
#[test]
#[should_panic]
iho marked this conversation as resolved.
Show resolved Hide resolved
fn test_check_use_discriminant_on_struct() {
let item_enum: DeriveInput = syn::parse2(quote! {
#[derive(BorshDeserialize, Debug)]
#[borsh(use_discriminant = false)]
struct AWithUseDiscriminantFalse {
x: X,
y: Y,
}
})
.unwrap();
let actual = contains_use_discriminant(&item_enum.attrs);
iho marked this conversation as resolved.
Show resolved Hide resolved
insta::assert_snapshot!(actual.unwrap_err().to_token_stream().to_string());
}
#[test]
#[should_panic]
iho marked this conversation as resolved.
Show resolved Hide resolved
fn test_check_use_borsh_skip_on_whole_struct() {
let item_enum: DeriveInput = syn::parse2(quote! {
#[derive(BorshDeserialize, Debug)]
#[borsh(use_discriminant = false)]
dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
#[borsh_skip=x]
iho marked this conversation as resolved.
Show resolved Hide resolved
struct AWithUseDiscriminantFalse {
x: X,
y: Y,
}
iho marked this conversation as resolved.
Show resolved Hide resolved
})
.unwrap();
let actual = contains_use_discriminant(&item_enum.attrs);
iho marked this conversation as resolved.
Show resolved Hide resolved
insta::assert_snapshot!(actual.unwrap_err().to_token_stream().to_string());
dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
}
#[test]
#[should_panic]
iho marked this conversation as resolved.
Show resolved Hide resolved
fn test_check_use_borsh_invalid_on_whole_struct() {
let item_enum: DeriveInput = syn::parse2(quote! {
#[derive(BorshDeserialize, Debug)]
#[borsh(invalid)]
struct AWithUseDiscriminantFalse {
x: X,
y: Y,
}
iho marked this conversation as resolved.
Show resolved Hide resolved
})
.unwrap();
let actual = contains_use_discriminant(&item_enum.attrs);
iho marked this conversation as resolved.
Show resolved Hide resolved
insta::assert_snapshot!(actual.unwrap_err().to_token_stream().to_string());
}
}
45 changes: 39 additions & 6 deletions borsh-derive-internal/src/enum_de.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,15 @@
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use syn::{Fields, Ident, ItemEnum, Path, WhereClause};

use crate::{
attribute_helpers::{
collect_override_bounds, contains_initialize_with, contains_skip, BoundType,
collect_override_bounds, contains_initialize_with, contains_skip,
contains_use_discriminant, BoundType,
},
enum_discriminant_map::discriminant_map,
generics::{compute_predicates, without_defaults, FindTyParams},
};
use proc_macro2::TokenStream as TokenStream2;
use quote::quote;
use std::convert::TryFrom;
use syn::{Fields, Ident, ItemEnum, Path, WhereClause};

pub fn enum_de(input: &ItemEnum, cratename: Ident) -> syn::Result<TokenStream2> {
let name = &input.ident;
Expand All @@ -27,9 +28,36 @@ pub fn enum_de(input: &ItemEnum, cratename: Ident) -> syn::Result<TokenStream2>
let mut default_params_visitor = FindTyParams::new(&generics);

let init_method = contains_initialize_with(&input.attrs);

let use_discriminant = contains_use_discriminant(&input.attrs).map_err(|err| {
syn::Error::new(
input.ident.span(),
format!("error parsing `#[borsh(use_discriminant = ...)]`: {}", err),
)
})?;

let has_explicit_discriminants = input
.variants
.iter()
.any(|variant| variant.discriminant.is_some());
if has_explicit_discriminants && use_discriminant.is_none() {
return Err(syn::Error::new(
input.ident.span(),
"You have to specify `#[borsh(use_discriminant=true)]` or `#[borsh(use_discriminant=false)]` for all structs that have enum with explicit discriminant",
));
}
let use_discriminant = use_discriminant.unwrap_or(false);

dj8yfo marked this conversation as resolved.
Show resolved Hide resolved
let mut variant_arms = TokenStream2::new();
let discriminants = discriminant_map(&input.variants);
for variant in input.variants.iter() {

for (variant_idx, variant) in input.variants.iter().enumerate() {
let variant_idx = u8::try_from(variant_idx).map_err(|err| {
syn::Error::new(
variant.ident.span(),
format!("up to 256 enum variants are supported. error{}", err),
)
})?;
let variant_ident = &variant.ident;
let discriminant = discriminants.get(variant_ident).unwrap();
let mut variant_header = TokenStream2::new();
Expand Down Expand Up @@ -85,6 +113,11 @@ pub fn enum_de(input: &ItemEnum, cratename: Ident) -> syn::Result<TokenStream2>
}
Fields::Unit => {}
}
let discriminant = if use_discriminant {
quote! { #discriminant }
} else {
quote! { #variant_idx }
};
variant_arms.extend(quote! {
if variant_tag == #discriminant { #name::#variant_ident #variant_header } else
});
Expand Down
2 changes: 1 addition & 1 deletion borsh-derive-internal/src/enum_discriminant_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ use syn::{punctuated::Punctuated, token::Comma, Variant};
/// See: https://doc.rust-lang.org/reference/items/enumerations.html#assigning-discriminant-values
pub fn discriminant_map(variants: &Punctuated<Variant, Comma>) -> HashMap<Ident, TokenStream> {
let mut map = HashMap::new();

let mut next_discriminant_if_not_specified = quote! {0};

for variant in variants {
let this_discriminant = variant.discriminant.clone().map_or_else(
|| quote! { #next_discriminant_if_not_specified },
|(_, e)| quote! { #e },
);

next_discriminant_if_not_specified = quote! { #this_discriminant + 1 };
map.insert(variant.ident.clone(), this_discriminant);
}
Expand Down
Loading
Loading