From 193341afb5a50bdbd48801a05e7d18a0aae51dba Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Wed, 19 Oct 2022 12:10:36 +0200 Subject: [PATCH 01/13] Support customization of fields on derive --- README.md | 25 ++++++++ derive/Cargo.toml | 2 +- derive/src/field_attributes.rs | 110 +++++++++++++++++++++++++++++++++ derive/src/lib.rs | 58 +++++++++++++---- tests/derive.rs | 39 ++++++++++++ 5 files changed, 221 insertions(+), 13 deletions(-) create mode 100644 derive/src/field_attributes.rs diff --git a/README.md b/README.md index 38bd949..e5522dc 100644 --- a/README.md +++ b/README.md @@ -61,6 +61,31 @@ pub struct Rgb { } ``` +### Customizing single fields + +This can be particular handy if your structure uses a type that does not implement `Arbitrary` or you want to have more customization for particular fields. + +```rust +#[derive(Arbitrary)] +pub struct Rgb { + // set `r` to Default::default() + #[arbitrary(default)] + pub r: u8, + + // set `g` to 255 + #[arbitrary(value = "255")] + pub g: u8, + + // generate `b` with a custom function + #[arbitrary(with = "arbitrary_b")] + pub b: u8, +} + +fn arbitrary_b(u: &mut Unstructured) -> arbitrary::Result { + u.int_in_range(64..=128) +} +``` + ### Implementing `Arbitrary` By Hand Alternatively, you can write an `Arbitrary` implementation by hand: diff --git a/derive/Cargo.toml b/derive/Cargo.toml index d532269..4ff114c 100644 --- a/derive/Cargo.toml +++ b/derive/Cargo.toml @@ -9,7 +9,7 @@ authors = [ "Corey Farwell ", ] categories = ["development-tools::testing"] -edition = "2018" +edition = "2021" keywords = ["arbitrary", "testing", "derive", "macro"] readme = "README.md" description = "Derives arbitrary traits" diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs new file mode 100644 index 0000000..c0ace3f --- /dev/null +++ b/derive/src/field_attributes.rs @@ -0,0 +1,110 @@ +use proc_macro2::{Literal, TokenStream, TokenTree}; +use quote::quote; +use syn::*; + +/// Used to filter out necessary field attribute and in panics. +static ARBITRARY_ATTRIBUTE_NAME: &str = "arbitrary"; + +/// Determines how a value for a field should be constructed. +#[cfg_attr(test, derive(Debug))] +pub enum FieldConstructor { + /// Assume that Arbitrary is defined for the type of this field and use it (default) + Arbitrary, + + /// Places `Default::default()` as a field value. + Default, + + /// Use custom function to generate a value for a field. + WithFunction(TokenStream), + + /// Set a field always to the given value. + Value(TokenStream), +} + +pub fn determine_field_constructor(field: &Field) -> FieldConstructor { + let opt_attr = fetch_attr_from_field(field); + match opt_attr { + Some(attr) => parse_attribute(attr), + None => FieldConstructor::Arbitrary, + } +} + +fn fetch_attr_from_field(field: &Field) -> Option<&Attribute> { + field.attrs.iter().find(|a| { + let path = &a.path; + let name = quote!(#path).to_string(); + name == ARBITRARY_ATTRIBUTE_NAME + }) +} + +fn parse_attribute(attr: &Attribute) -> FieldConstructor { + let group = { + let mut tokens_iter = attr.clone().tokens.into_iter(); + let token = tokens_iter + .next() + .unwrap_or_else(|| panic!("{ARBITRARY_ATTRIBUTE_NAME} attribute cannot be empty.")); + match token { + TokenTree::Group(g) => g, + t => panic!("{ARBITRARY_ATTRIBUTE_NAME} must contain a group, got: {t})"), + } + }; + parse_attribute_internals(group.stream()) +} + +fn parse_attribute_internals(stream: TokenStream) -> FieldConstructor { + let mut tokens_iter = stream.into_iter(); + let token = tokens_iter + .next() + .unwrap_or_else(|| panic!("{ARBITRARY_ATTRIBUTE_NAME} attribute cannot be empty.")); + match token.to_string().as_ref() { + "default" => FieldConstructor::Default, + "with" => { + let func_path = parse_assigned_value(tokens_iter); + FieldConstructor::WithFunction(func_path) + } + "value" => { + let value = parse_assigned_value(tokens_iter); + FieldConstructor::Value(value) + } + _ => panic!("Unknown options for {ARBITRARY_ATTRIBUTE_NAME}: {token}"), + } +} + +// Input: +// = "2 + 2" +// Output: +// 2 + 2 +fn parse_assigned_value(mut tokens_iter: impl Iterator) -> TokenStream { + let eq_sign = tokens_iter + .next() + .unwrap_or_else(|| panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute")); + if eq_sign.to_string() != "=" { + panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute"); + } + let lit_token = tokens_iter + .next() + .unwrap_or_else(|| panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute")); + let value = unwrap_token_as_string_literal(lit_token); + value.parse().unwrap() +} + +fn unwrap_token_as_string_literal(token: TokenTree) -> String { + let lit = unwrap_token_as_literal(token); + literal_to_string(lit) +} + +fn literal_to_string(lit: Literal) -> String { + let value = lit.to_string(); + if value.starts_with('"') && value.ends_with('"') { + // Trim the first and the last chars (double quotes) + return value[1..(value.len() - 1)].to_string(); + } + panic!("{ARBITRARY_ATTRIBUTE_NAME}() expected an attribute to be a string, but got: {value}",); +} + +fn unwrap_token_as_literal(token: TokenTree) -> Literal { + match token { + TokenTree::Literal(lit) => lit, + something => panic!("{ARBITRARY_ATTRIBUTE_NAME}() expected a literal, got: {something}"), + } +} diff --git a/derive/src/lib.rs b/derive/src/lib.rs index 7b16a8d..758f088 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -4,9 +4,12 @@ use proc_macro2::{Span, TokenStream}; use quote::quote; use syn::*; +mod field_attributes; +use field_attributes::{determine_field_constructor, FieldConstructor}; + static ARBITRARY_LIFETIME_NAME: &str = "'arbitrary"; -#[proc_macro_derive(Arbitrary)] +#[proc_macro_derive(Arbitrary, attributes(arbitrary))] pub fn derive_arbitrary(tokens: proc_macro::TokenStream) -> proc_macro::TokenStream { let input = syn::parse_macro_input!(tokens as syn::DeriveInput); let (lifetime_without_bounds, lifetime_with_bounds) = @@ -116,7 +119,7 @@ fn gen_arbitrary_method( let ident = &input.ident; let arbitrary_structlike = |fields| { - let arbitrary = construct(fields, |_, _| quote!(arbitrary::Arbitrary::arbitrary(u)?)); + let arbitrary = construct(fields, |_idx, field| gen_constructor_for_field(field)); let body = with_recursive_count_guard(recursive_count, quote! { Ok(#ident #arbitrary) }); let arbitrary_take_rest = construct_take_rest(fields); @@ -140,9 +143,7 @@ fn gen_arbitrary_method( Data::Enum(data) => { let variants = data.variants.iter().enumerate().map(|(i, variant)| { let idx = i as u64; - let ctor = construct(&variant.fields, |_, _| { - quote!(arbitrary::Arbitrary::arbitrary(u)?) - }); + let ctor = construct(&variant.fields, |_, field| gen_constructor_for_field(field)); let variant_name = &variant.ident; quote! { #idx => #ident::#variant_name #ctor } }); @@ -215,21 +216,45 @@ fn construct(fields: &Fields, ctor: impl Fn(usize, &Field) -> TokenStream) -> To } fn construct_take_rest(fields: &Fields) -> TokenStream { - construct(fields, |idx, _| { - if idx + 1 == fields.len() { - quote! { arbitrary::Arbitrary::arbitrary_take_rest(u)? } - } else { - quote! { arbitrary::Arbitrary::arbitrary(&mut u)? } + construct(fields, |idx, field| { + match determine_field_constructor(field) { + FieldConstructor::Default => quote!(Default::default()), + FieldConstructor::Arbitrary => { + if idx + 1 == fields.len() { + quote! { arbitrary::Arbitrary::arbitrary_take_rest(u)? } + } else { + quote! { arbitrary::Arbitrary::arbitrary(&mut u)? } + } + } + FieldConstructor::WithFunction(func_path) => quote!(#func_path(&mut u)?), + FieldConstructor::Value(value) => quote!(#value), } }) } fn gen_size_hint_method(input: &DeriveInput) -> TokenStream { let size_hint_fields = |fields: &Fields| { - let tys = fields.iter().map(|f| &f.ty); + let hints = fields.iter().map(|f| { + let ty = &f.ty; + match determine_field_constructor(f) { + FieldConstructor::Default | FieldConstructor::Value(_) => { + quote!((0, Some(0))) + } + FieldConstructor::Arbitrary => { + quote! { <#ty as arbitrary::Arbitrary>::size_hint(depth) } + } + + // Note that in this case it's hard to determine what size_hint must be, so size_of::() is + // just an educated guess, although it's gonna be inaccurate for dynamically + // allocated types (Vec, HashMap, etc.). + FieldConstructor::WithFunction(_) => { + quote! { (::core::mem::size_of::<#ty>(), None) } + } + } + }); quote! { arbitrary::size_hint::and_all(&[ - #( <#tys as arbitrary::Arbitrary>::size_hint(depth) ),* + #( #hints ),* ]) } }; @@ -261,3 +286,12 @@ fn gen_size_hint_method(input: &DeriveInput) -> TokenStream { } } } + +fn gen_constructor_for_field(field: &Field) -> TokenStream { + match determine_field_constructor(field) { + FieldConstructor::Default => quote!(Default::default()), + FieldConstructor::Arbitrary => quote!(arbitrary::Arbitrary::arbitrary(u)?), + FieldConstructor::WithFunction(func_path) => quote!(#func_path(u)?), + FieldConstructor::Value(value) => quote!(#value), + } +} diff --git a/tests/derive.rs b/tests/derive.rs index 2d666f6..c87efdf 100644 --- a/tests/derive.rs +++ b/tests/derive.rs @@ -231,3 +231,42 @@ fn recursive_and_empty_input() { let _ = Nat5::arbitrary(&mut Unstructured::new(&[])); } + +#[test] +fn test_field_attributes() { + // A type that DOES NOT implement Arbitrary + #[derive(Debug)] + struct Weight(u8); + + #[derive(Debug, Arbitrary)] + struct Parcel { + #[arbitrary(with = "arbitrary_weight")] + weight: Weight, + + #[arbitrary(default)] + width: u8, + + #[arbitrary(value = "2 + 2")] + length: u8, + + height: u8, + } + + fn arbitrary_weight(u: &mut Unstructured) -> arbitrary::Result { + u.int_in_range(45..=56).map(Weight) + } + + let parcel: Parcel = arbitrary_from(&[6, 199]); + + // 45 + 6 = 51 + assert_eq!(parcel.weight.0, 51); + + // u8::default() + assert_eq!(parcel.width, 0); + + // 2 + 2 = 4 + assert_eq!(parcel.length, 4); + + // 199 is the second byte, used by arbitrary + assert_eq!(parcel.height, 199); +} From f73d5f2333f4004ccdd5250c198044e5102d61f9 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 11:08:03 +0200 Subject: [PATCH 02/13] Do not use double quotes for arbitrary macro attribute --- derive/src/field_attributes.rs | 29 ++--------------------------- tests/derive.rs | 4 ++-- 2 files changed, 4 insertions(+), 29 deletions(-) diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index c0ace3f..c585e9d 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -1,4 +1,4 @@ -use proc_macro2::{Literal, TokenStream, TokenTree}; +use proc_macro2::{TokenStream, TokenTree}; use quote::quote; use syn::*; @@ -81,30 +81,5 @@ fn parse_assigned_value(mut tokens_iter: impl Iterator) -> Tok if eq_sign.to_string() != "=" { panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute"); } - let lit_token = tokens_iter - .next() - .unwrap_or_else(|| panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute")); - let value = unwrap_token_as_string_literal(lit_token); - value.parse().unwrap() -} - -fn unwrap_token_as_string_literal(token: TokenTree) -> String { - let lit = unwrap_token_as_literal(token); - literal_to_string(lit) -} - -fn literal_to_string(lit: Literal) -> String { - let value = lit.to_string(); - if value.starts_with('"') && value.ends_with('"') { - // Trim the first and the last chars (double quotes) - return value[1..(value.len() - 1)].to_string(); - } - panic!("{ARBITRARY_ATTRIBUTE_NAME}() expected an attribute to be a string, but got: {value}",); -} - -fn unwrap_token_as_literal(token: TokenTree) -> Literal { - match token { - TokenTree::Literal(lit) => lit, - something => panic!("{ARBITRARY_ATTRIBUTE_NAME}() expected a literal, got: {something}"), - } + tokens_iter.collect() } diff --git a/tests/derive.rs b/tests/derive.rs index c87efdf..45b12e3 100644 --- a/tests/derive.rs +++ b/tests/derive.rs @@ -240,13 +240,13 @@ fn test_field_attributes() { #[derive(Debug, Arbitrary)] struct Parcel { - #[arbitrary(with = "arbitrary_weight")] + #[arbitrary(with = arbitrary_weight)] weight: Weight, #[arbitrary(default)] width: u8, - #[arbitrary(value = "2 + 2")] + #[arbitrary(value = 2 + 2)] length: u8, height: u8, From 8913ec0d40af472deec4e4ab01eb63ee69d73c80 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 12:26:52 +0200 Subject: [PATCH 03/13] Introduce trybuild to test failing compilation --- Cargo.toml | 1 + derive/src/field_attributes.rs | 48 +++++++++++++------- tests/compiletest.rs | 5 ++ tests/ui/multiple_arbitrary.rs | 11 +++++ tests/ui/multiple_arbitrary.stderr | 7 +++ tests/ui/unknown_attribute.rs | 10 ++++ tests/ui/unknown_attribute.stderr | 7 +++ tests/ui/unknown_attribute_with_value.rs | 10 ++++ tests/ui/unknown_attribute_with_value.stderr | 7 +++ tests/ui/value_no_rhs.rs | 10 ++++ tests/ui/value_no_rhs.stderr | 7 +++ tests/ui/with_no_rhs.rs | 10 ++++ tests/ui/with_no_rhs.stderr | 7 +++ 13 files changed, 123 insertions(+), 17 deletions(-) create mode 100644 tests/compiletest.rs create mode 100644 tests/ui/multiple_arbitrary.rs create mode 100644 tests/ui/multiple_arbitrary.stderr create mode 100644 tests/ui/unknown_attribute.rs create mode 100644 tests/ui/unknown_attribute.stderr create mode 100644 tests/ui/unknown_attribute_with_value.rs create mode 100644 tests/ui/unknown_attribute_with_value.stderr create mode 100644 tests/ui/value_no_rhs.rs create mode 100644 tests/ui/value_no_rhs.stderr create mode 100644 tests/ui/with_no_rhs.rs create mode 100644 tests/ui/with_no_rhs.stderr diff --git a/Cargo.toml b/Cargo.toml index 4cf3970..3ad0421 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -23,6 +23,7 @@ rust-version = "1.63.0" derive_arbitrary = { version = "1.1.6", path = "./derive", optional = true } [dev-dependencies] +trybuild = { version = "1.0.71", features = ["diff"] } [features] # Turn this feature on to enable support for `#[derive(Arbitrary)]`. diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index c585e9d..0b0cf9a 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -30,11 +30,22 @@ pub fn determine_field_constructor(field: &Field) -> FieldConstructor { } fn fetch_attr_from_field(field: &Field) -> Option<&Attribute> { - field.attrs.iter().find(|a| { - let path = &a.path; - let name = quote!(#path).to_string(); - name == ARBITRARY_ATTRIBUTE_NAME - }) + let found_attributes: Vec<_> = field + .attrs + .iter() + .filter(|a| { + let path = &a.path; + let name = quote!(#path).to_string(); + name == ARBITRARY_ATTRIBUTE_NAME + }) + .collect(); + if found_attributes.len() > 1 { + let name = field.ident.as_ref().unwrap(); + panic!( + "Multiple conflicting #[{ARBITRARY_ATTRIBUTE_NAME}] attributes found on field `{name}`" + ); + } + found_attributes.into_iter().next() } fn parse_attribute(attr: &Attribute) -> FieldConstructor { @@ -42,10 +53,10 @@ fn parse_attribute(attr: &Attribute) -> FieldConstructor { let mut tokens_iter = attr.clone().tokens.into_iter(); let token = tokens_iter .next() - .unwrap_or_else(|| panic!("{ARBITRARY_ATTRIBUTE_NAME} attribute cannot be empty.")); + .unwrap_or_else(|| panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty.")); match token { TokenTree::Group(g) => g, - t => panic!("{ARBITRARY_ATTRIBUTE_NAME} must contain a group, got: {t})"), + t => panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] must contain a group, got: {t})"), } }; parse_attribute_internals(group.stream()) @@ -55,31 +66,34 @@ fn parse_attribute_internals(stream: TokenStream) -> FieldConstructor { let mut tokens_iter = stream.into_iter(); let token = tokens_iter .next() - .unwrap_or_else(|| panic!("{ARBITRARY_ATTRIBUTE_NAME} attribute cannot be empty.")); + .unwrap_or_else(|| panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty.")); match token.to_string().as_ref() { "default" => FieldConstructor::Default, "with" => { - let func_path = parse_assigned_value(tokens_iter); + let func_path = parse_assigned_value("with", tokens_iter); FieldConstructor::WithFunction(func_path) } "value" => { - let value = parse_assigned_value(tokens_iter); + let value = parse_assigned_value("value", tokens_iter); FieldConstructor::Value(value) } - _ => panic!("Unknown options for {ARBITRARY_ATTRIBUTE_NAME}: {token}"), + _ => panic!("Unknown option for #[{ARBITRARY_ATTRIBUTE_NAME}]: `{token}`"), } } // Input: -// = "2 + 2" +// = 2 + 2 // Output: // 2 + 2 -fn parse_assigned_value(mut tokens_iter: impl Iterator) -> TokenStream { - let eq_sign = tokens_iter - .next() - .unwrap_or_else(|| panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute")); +fn parse_assigned_value( + opt_name: &str, + mut tokens_iter: impl Iterator, +) -> TokenStream { + let eq_sign = tokens_iter.next().unwrap_or_else(|| { + panic!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], `{opt_name}` is missing RHS.") + }); if eq_sign.to_string() != "=" { - panic!("Invalid syntax for {ARBITRARY_ATTRIBUTE_NAME}() attribute"); + panic!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], expected `=` after `{opt_name}`, got: `{eq_sign}`"); } tokens_iter.collect() } diff --git a/tests/compiletest.rs b/tests/compiletest.rs new file mode 100644 index 0000000..bd72ecd --- /dev/null +++ b/tests/compiletest.rs @@ -0,0 +1,5 @@ +#[test] +fn ui() { + let t = trybuild::TestCases::new(); + t.compile_fail("tests/ui/**/*.rs"); +} diff --git a/tests/ui/multiple_arbitrary.rs b/tests/ui/multiple_arbitrary.rs new file mode 100644 index 0000000..1253eab --- /dev/null +++ b/tests/ui/multiple_arbitrary.rs @@ -0,0 +1,11 @@ +use arbitrary::Arbitrary; + +#[derive(Arbitrary)] +struct Point { + #[arbitrary(value = 2)] + #[arbitrary(value = 3)] + x: i32, + y: i32 +} + +fn main() { } diff --git a/tests/ui/multiple_arbitrary.stderr b/tests/ui/multiple_arbitrary.stderr new file mode 100644 index 0000000..1b23df4 --- /dev/null +++ b/tests/ui/multiple_arbitrary.stderr @@ -0,0 +1,7 @@ +error: proc-macro derive panicked + --> tests/ui/multiple_arbitrary.rs:3:10 + | +3 | #[derive(Arbitrary)] + | ^^^^^^^^^ + | + = help: message: Multiple conflicting #[arbitrary] attributes found on field `x` diff --git a/tests/ui/unknown_attribute.rs b/tests/ui/unknown_attribute.rs new file mode 100644 index 0000000..3b65151 --- /dev/null +++ b/tests/ui/unknown_attribute.rs @@ -0,0 +1,10 @@ +use arbitrary::Arbitrary; + +#[derive(Arbitrary)] +struct Point { + #[arbitrary(unknown_attr)] + x: i32, + y: i32 +} + +fn main() { } diff --git a/tests/ui/unknown_attribute.stderr b/tests/ui/unknown_attribute.stderr new file mode 100644 index 0000000..a10c8a5 --- /dev/null +++ b/tests/ui/unknown_attribute.stderr @@ -0,0 +1,7 @@ +error: proc-macro derive panicked + --> tests/ui/unknown_attribute.rs:3:10 + | +3 | #[derive(Arbitrary)] + | ^^^^^^^^^ + | + = help: message: Unknown option for #[arbitrary]: `unknown_attr` diff --git a/tests/ui/unknown_attribute_with_value.rs b/tests/ui/unknown_attribute_with_value.rs new file mode 100644 index 0000000..a95b246 --- /dev/null +++ b/tests/ui/unknown_attribute_with_value.rs @@ -0,0 +1,10 @@ +use arbitrary::Arbitrary; + +#[derive(Arbitrary)] +struct Point { + #[arbitrary(unknown_attr = 255)] + x: i32, + y: i32 +} + +fn main() { } diff --git a/tests/ui/unknown_attribute_with_value.stderr b/tests/ui/unknown_attribute_with_value.stderr new file mode 100644 index 0000000..3890695 --- /dev/null +++ b/tests/ui/unknown_attribute_with_value.stderr @@ -0,0 +1,7 @@ +error: proc-macro derive panicked + --> tests/ui/unknown_attribute_with_value.rs:3:10 + | +3 | #[derive(Arbitrary)] + | ^^^^^^^^^ + | + = help: message: Unknown option for #[arbitrary]: `unknown_attr` diff --git a/tests/ui/value_no_rhs.rs b/tests/ui/value_no_rhs.rs new file mode 100644 index 0000000..35ff767 --- /dev/null +++ b/tests/ui/value_no_rhs.rs @@ -0,0 +1,10 @@ +use arbitrary::Arbitrary; + +#[derive(Arbitrary)] +struct Point { + #[arbitrary(value)] + x: i32, + y: i32 +} + +fn main() { } diff --git a/tests/ui/value_no_rhs.stderr b/tests/ui/value_no_rhs.stderr new file mode 100644 index 0000000..c512529 --- /dev/null +++ b/tests/ui/value_no_rhs.stderr @@ -0,0 +1,7 @@ +error: proc-macro derive panicked + --> tests/ui/value_no_rhs.rs:3:10 + | +3 | #[derive(Arbitrary)] + | ^^^^^^^^^ + | + = help: message: Invalid syntax for #[arbitrary], `value` is missing RHS. diff --git a/tests/ui/with_no_rhs.rs b/tests/ui/with_no_rhs.rs new file mode 100644 index 0000000..0663d92 --- /dev/null +++ b/tests/ui/with_no_rhs.rs @@ -0,0 +1,10 @@ +use arbitrary::Arbitrary; + +#[derive(Arbitrary)] +struct Point { + #[arbitrary(with)] + x: i32, + y: i32 +} + +fn main() { } diff --git a/tests/ui/with_no_rhs.stderr b/tests/ui/with_no_rhs.stderr new file mode 100644 index 0000000..6f0b95f --- /dev/null +++ b/tests/ui/with_no_rhs.stderr @@ -0,0 +1,7 @@ +error: proc-macro derive panicked + --> tests/ui/with_no_rhs.rs:3:10 + | +3 | #[derive(Arbitrary)] + | ^^^^^^^^^ + | + = help: message: Invalid syntax for #[arbitrary], `with` is missing RHS. From 66b12e1f1b1963089cbf65104c67023aa9b42eb7 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 12:34:19 +0200 Subject: [PATCH 04/13] A few improvements in README about regarding #[arbitrary] --- README.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index e5522dc..6e8f525 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ pub struct Rgb { } ``` -### Customizing single fields +#### Customizing single fields This can be particular handy if your structure uses a type that does not implement `Arbitrary` or you want to have more customization for particular fields. @@ -73,11 +73,15 @@ pub struct Rgb { pub r: u8, // set `g` to 255 - #[arbitrary(value = "255")] + #[arbitrary(value = 255)] pub g: u8, - // generate `b` with a custom function - #[arbitrary(with = "arbitrary_b")] + // Generate `b` with a custom function of type + // + // fn(&mut Unstructured) -> arbitrary::Result + // + // where `T` is the field's type. + #[arbitrary(with = arbitrary_b)] pub b: u8, } From 9dd430d2d295337d3b5f17162c3f31ef9b9f0d64 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 12:48:45 +0200 Subject: [PATCH 05/13] Run compiletest only if derive feature is on --- tests/compiletest.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/compiletest.rs b/tests/compiletest.rs index bd72ecd..9ba9f56 100644 --- a/tests/compiletest.rs +++ b/tests/compiletest.rs @@ -1,3 +1,4 @@ +#[cfg(feature = "derive")] #[test] fn ui() { let t = trybuild::TestCases::new(); From 727ab88363108e041243375c5ed2e31389a78d32 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 13:36:22 +0200 Subject: [PATCH 06/13] Support #[arbitrary(with = ...)] with closures --- README.md | 6 +++++- derive/src/field_attributes.rs | 6 +++--- derive/src/lib.rs | 6 +++--- tests/derive.rs | 10 ++++++++-- 4 files changed, 19 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 6e8f525..3f5619b 100644 --- a/README.md +++ b/README.md @@ -67,7 +67,7 @@ This can be particular handy if your structure uses a type that does not impleme ```rust #[derive(Arbitrary)] -pub struct Rgb { +pub struct Rgba { // set `r` to Default::default() #[arbitrary(default)] pub r: u8, @@ -83,6 +83,10 @@ pub struct Rgb { // where `T` is the field's type. #[arbitrary(with = arbitrary_b)] pub b: u8, + + // Generate `a` with a custom closure (shortuct to avoid a custom funciton) + #[arbitrary(with = |u: &mut Unstructured| u.int_in_range(0..=64))] + pub a: u8, } fn arbitrary_b(u: &mut Unstructured) -> arbitrary::Result { diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index 0b0cf9a..a92f610 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -14,8 +14,8 @@ pub enum FieldConstructor { /// Places `Default::default()` as a field value. Default, - /// Use custom function to generate a value for a field. - WithFunction(TokenStream), + /// Use custom function or closure to generate a value for a field. + With(TokenStream), /// Set a field always to the given value. Value(TokenStream), @@ -71,7 +71,7 @@ fn parse_attribute_internals(stream: TokenStream) -> FieldConstructor { "default" => FieldConstructor::Default, "with" => { let func_path = parse_assigned_value("with", tokens_iter); - FieldConstructor::WithFunction(func_path) + FieldConstructor::With(func_path) } "value" => { let value = parse_assigned_value("value", tokens_iter); diff --git a/derive/src/lib.rs b/derive/src/lib.rs index 758f088..bf2183a 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -226,7 +226,7 @@ fn construct_take_rest(fields: &Fields) -> TokenStream { quote! { arbitrary::Arbitrary::arbitrary(&mut u)? } } } - FieldConstructor::WithFunction(func_path) => quote!(#func_path(&mut u)?), + FieldConstructor::With(function_or_closure) => quote!((#function_or_closure)(&mut u)?), FieldConstructor::Value(value) => quote!(#value), } }) @@ -247,7 +247,7 @@ fn gen_size_hint_method(input: &DeriveInput) -> TokenStream { // Note that in this case it's hard to determine what size_hint must be, so size_of::() is // just an educated guess, although it's gonna be inaccurate for dynamically // allocated types (Vec, HashMap, etc.). - FieldConstructor::WithFunction(_) => { + FieldConstructor::With(_) => { quote! { (::core::mem::size_of::<#ty>(), None) } } } @@ -291,7 +291,7 @@ fn gen_constructor_for_field(field: &Field) -> TokenStream { match determine_field_constructor(field) { FieldConstructor::Default => quote!(Default::default()), FieldConstructor::Arbitrary => quote!(arbitrary::Arbitrary::arbitrary(u)?), - FieldConstructor::WithFunction(func_path) => quote!(#func_path(u)?), + FieldConstructor::With(function_or_closure) => quote!((#function_or_closure)(u)?), FieldConstructor::Value(value) => quote!(#value), } } diff --git a/tests/derive.rs b/tests/derive.rs index 45b12e3..f29d227 100644 --- a/tests/derive.rs +++ b/tests/derive.rs @@ -250,13 +250,16 @@ fn test_field_attributes() { length: u8, height: u8, + + #[arbitrary(with = |u: &mut Unstructured| u.int_in_range(0..=100))] + price: u8, } fn arbitrary_weight(u: &mut Unstructured) -> arbitrary::Result { u.int_in_range(45..=56).map(Weight) } - let parcel: Parcel = arbitrary_from(&[6, 199]); + let parcel: Parcel = arbitrary_from(&[6, 199, 17]); // 45 + 6 = 51 assert_eq!(parcel.weight.0, 51); @@ -267,6 +270,9 @@ fn test_field_attributes() { // 2 + 2 = 4 assert_eq!(parcel.length, 4); - // 199 is the second byte, used by arbitrary + // 199 is the 2nd byte used by arbitrary assert_eq!(parcel.height, 199); + + // 17 is the 3rd byte used by arbitrary + assert_eq!(parcel.price, 17); } From f3225dddfead7034441829846394b6b30604f4c8 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 15:49:20 +0200 Subject: [PATCH 07/13] Refactor derive functions return syn::Result<_> --- derive/src/field_attributes.rs | 33 ++--- derive/src/lib.rs | 228 ++++++++++++++++++++------------- 2 files changed, 156 insertions(+), 105 deletions(-) diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index a92f610..c48927a 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -21,15 +21,16 @@ pub enum FieldConstructor { Value(TokenStream), } -pub fn determine_field_constructor(field: &Field) -> FieldConstructor { - let opt_attr = fetch_attr_from_field(field); - match opt_attr { - Some(attr) => parse_attribute(attr), +pub fn determine_field_constructor(field: &Field) -> Result { + let opt_attr = fetch_attr_from_field(field)?; + let ctor = match opt_attr { + Some(attr) => parse_attribute(attr)?, None => FieldConstructor::Arbitrary, - } + }; + Ok(ctor) } -fn fetch_attr_from_field(field: &Field) -> Option<&Attribute> { +fn fetch_attr_from_field(field: &Field) -> Result> { let found_attributes: Vec<_> = field .attrs .iter() @@ -45,10 +46,10 @@ fn fetch_attr_from_field(field: &Field) -> Option<&Attribute> { "Multiple conflicting #[{ARBITRARY_ATTRIBUTE_NAME}] attributes found on field `{name}`" ); } - found_attributes.into_iter().next() + Ok(found_attributes.into_iter().next()) } -fn parse_attribute(attr: &Attribute) -> FieldConstructor { +fn parse_attribute(attr: &Attribute) -> Result { let group = { let mut tokens_iter = attr.clone().tokens.into_iter(); let token = tokens_iter @@ -62,20 +63,20 @@ fn parse_attribute(attr: &Attribute) -> FieldConstructor { parse_attribute_internals(group.stream()) } -fn parse_attribute_internals(stream: TokenStream) -> FieldConstructor { +fn parse_attribute_internals(stream: TokenStream) -> Result { let mut tokens_iter = stream.into_iter(); let token = tokens_iter .next() .unwrap_or_else(|| panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty.")); match token.to_string().as_ref() { - "default" => FieldConstructor::Default, + "default" => Ok(FieldConstructor::Default), "with" => { - let func_path = parse_assigned_value("with", tokens_iter); - FieldConstructor::With(func_path) + let func_path = parse_assigned_value("with", tokens_iter)?; + Ok(FieldConstructor::With(func_path)) } "value" => { - let value = parse_assigned_value("value", tokens_iter); - FieldConstructor::Value(value) + let value = parse_assigned_value("value", tokens_iter)?; + Ok(FieldConstructor::Value(value)) } _ => panic!("Unknown option for #[{ARBITRARY_ATTRIBUTE_NAME}]: `{token}`"), } @@ -88,12 +89,12 @@ fn parse_attribute_internals(stream: TokenStream) -> FieldConstructor { fn parse_assigned_value( opt_name: &str, mut tokens_iter: impl Iterator, -) -> TokenStream { +) -> Result { let eq_sign = tokens_iter.next().unwrap_or_else(|| { panic!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], `{opt_name}` is missing RHS.") }); if eq_sign.to_string() != "=" { panic!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], expected `=` after `{opt_name}`, got: `{eq_sign}`"); } - tokens_iter.collect() + Ok(tokens_iter.collect()) } diff --git a/derive/src/lib.rs b/derive/src/lib.rs index bf2183a..4cd80b6 100644 --- a/derive/src/lib.rs +++ b/derive/src/lib.rs @@ -12,6 +12,12 @@ static ARBITRARY_LIFETIME_NAME: &str = "'arbitrary"; #[proc_macro_derive(Arbitrary, attributes(arbitrary))] pub fn derive_arbitrary(tokens: proc_macro::TokenStream) -> proc_macro::TokenStream { let input = syn::parse_macro_input!(tokens as syn::DeriveInput); + expand_derive_arbitrary(input) + .unwrap_or_else(syn::Error::into_compile_error) + .into() +} + +fn expand_derive_arbitrary(input: syn::DeriveInput) -> Result { let (lifetime_without_bounds, lifetime_with_bounds) = build_arbitrary_lifetime(input.generics.clone()); @@ -21,8 +27,8 @@ pub fn derive_arbitrary(tokens: proc_macro::TokenStream) -> proc_macro::TokenStr ); let arbitrary_method = - gen_arbitrary_method(&input, lifetime_without_bounds.clone(), &recursive_count); - let size_hint_method = gen_size_hint_method(&input); + gen_arbitrary_method(&input, lifetime_without_bounds.clone(), &recursive_count)?; + let size_hint_method = gen_size_hint_method(&input)?; let name = input.ident; // Add a bound `T: Arbitrary` to every type parameter T. let generics = add_trait_bounds(input.generics, lifetime_without_bounds.clone()); @@ -37,7 +43,7 @@ pub fn derive_arbitrary(tokens: proc_macro::TokenStream) -> proc_macro::TokenStr // Build TypeGenerics and WhereClause without a lifetime let (_, ty_generics, where_clause) = generics.split_for_impl(); - (quote! { + Ok(quote! { const _: () = { thread_local! { #[allow(non_upper_case_globals)] @@ -50,7 +56,6 @@ pub fn derive_arbitrary(tokens: proc_macro::TokenStream) -> proc_macro::TokenStr } }; }) - .into() } // Returns: (lifetime without bounds, lifetime with bounds) @@ -115,18 +120,21 @@ fn gen_arbitrary_method( input: &DeriveInput, lifetime: LifetimeDef, recursive_count: &syn::Ident, -) -> TokenStream { - let ident = &input.ident; - - let arbitrary_structlike = |fields| { - let arbitrary = construct(fields, |_idx, field| gen_constructor_for_field(field)); +) -> Result { + fn arbitrary_structlike( + fields: &Fields, + ident: &syn::Ident, + lifetime: LifetimeDef, + recursive_count: &syn::Ident, + ) -> Result { + let arbitrary = construct(fields, |_idx, field| gen_constructor_for_field(field))?; let body = with_recursive_count_guard(recursive_count, quote! { Ok(#ident #arbitrary) }); - let arbitrary_take_rest = construct_take_rest(fields); + let arbitrary_take_rest = construct_take_rest(fields)?; let take_rest_body = with_recursive_count_guard(recursive_count, quote! { Ok(#ident #arbitrary_take_rest) }); - quote! { + Ok(quote! { fn arbitrary(u: &mut arbitrary::Unstructured<#lifetime>) -> arbitrary::Result { #body } @@ -134,25 +142,43 @@ fn gen_arbitrary_method( fn arbitrary_take_rest(mut u: arbitrary::Unstructured<#lifetime>) -> arbitrary::Result { #take_rest_body } - } - }; + }) + } - match &input.data { - Data::Struct(data) => arbitrary_structlike(&data.fields), - Data::Union(data) => arbitrary_structlike(&Fields::Named(data.fields.clone())), + let ident = &input.ident; + let output = match &input.data { + Data::Struct(data) => arbitrary_structlike(&data.fields, ident, lifetime, recursive_count)?, + Data::Union(data) => arbitrary_structlike( + &Fields::Named(data.fields.clone()), + ident, + lifetime, + recursive_count, + )?, Data::Enum(data) => { - let variants = data.variants.iter().enumerate().map(|(i, variant)| { - let idx = i as u64; - let ctor = construct(&variant.fields, |_, field| gen_constructor_for_field(field)); - let variant_name = &variant.ident; - quote! { #idx => #ident::#variant_name #ctor } - }); - let variants_take_rest = data.variants.iter().enumerate().map(|(i, variant)| { - let idx = i as u64; - let ctor = construct_take_rest(&variant.fields); - let variant_name = &variant.ident; - quote! { #idx => #ident::#variant_name #ctor } - }); + let variants: Vec = data + .variants + .iter() + .enumerate() + .map(|(i, variant)| { + let idx = i as u64; + let variant_name = &variant.ident; + construct(&variant.fields, |_, field| gen_constructor_for_field(field)) + .map(|ctor| quote! { #idx => #ident::#variant_name #ctor }) + }) + .collect::>()?; + + let variants_take_rest: Vec = data + .variants + .iter() + .enumerate() + .map(|(i, variant)| { + let idx = i as u64; + let variant_name = &variant.ident; + construct_take_rest(&variant.fields) + .map(|ctor| quote! { #idx => #ident::#variant_name #ctor }) + }) + .collect::>()?; + let count = data.variants.len() as u64; let arbitrary = with_recursive_count_guard( @@ -191,33 +217,44 @@ fn gen_arbitrary_method( } } } - } + }; + Ok(output) } -fn construct(fields: &Fields, ctor: impl Fn(usize, &Field) -> TokenStream) -> TokenStream { - match fields { +fn construct( + fields: &Fields, + ctor: impl Fn(usize, &Field) -> Result, +) -> Result { + let output = match fields { Fields::Named(names) => { - let names = names.named.iter().enumerate().map(|(i, f)| { - let name = f.ident.as_ref().unwrap(); - let ctor = ctor(i, f); - quote! { #name: #ctor } - }); + let names: Vec = names + .named + .iter() + .enumerate() + .map(|(i, f)| { + let name = f.ident.as_ref().unwrap(); + ctor(i, f).map(|ctor| quote! { #name: #ctor }) + }) + .collect::>()?; quote! { { #(#names,)* } } } Fields::Unnamed(names) => { - let names = names.unnamed.iter().enumerate().map(|(i, f)| { - let ctor = ctor(i, f); - quote! { #ctor } - }); + let names: Vec = names + .unnamed + .iter() + .enumerate() + .map(|(i, f)| ctor(i, f).map(|ctor| quote! { #ctor })) + .collect::>()?; quote! { ( #(#names),* ) } } Fields::Unit => quote!(), - } + }; + Ok(output) } -fn construct_take_rest(fields: &Fields) -> TokenStream { +fn construct_take_rest(fields: &Fields) -> Result { construct(fields, |idx, field| { - match determine_field_constructor(field) { + determine_field_constructor(field).map(|field_constructor| match field_constructor { FieldConstructor::Default => quote!(Default::default()), FieldConstructor::Arbitrary => { if idx + 1 == fields.len() { @@ -228,70 +265,83 @@ fn construct_take_rest(fields: &Fields) -> TokenStream { } FieldConstructor::With(function_or_closure) => quote!((#function_or_closure)(&mut u)?), FieldConstructor::Value(value) => quote!(#value), - } + }) }) } -fn gen_size_hint_method(input: &DeriveInput) -> TokenStream { +fn gen_size_hint_method(input: &DeriveInput) -> Result { let size_hint_fields = |fields: &Fields| { - let hints = fields.iter().map(|f| { - let ty = &f.ty; - match determine_field_constructor(f) { - FieldConstructor::Default | FieldConstructor::Value(_) => { - quote!((0, Some(0))) - } - FieldConstructor::Arbitrary => { - quote! { <#ty as arbitrary::Arbitrary>::size_hint(depth) } - } - - // Note that in this case it's hard to determine what size_hint must be, so size_of::() is - // just an educated guess, although it's gonna be inaccurate for dynamically - // allocated types (Vec, HashMap, etc.). - FieldConstructor::With(_) => { - quote! { (::core::mem::size_of::<#ty>(), None) } + fields + .iter() + .map(|f| { + let ty = &f.ty; + determine_field_constructor(f).map(|field_constructor| { + match field_constructor { + FieldConstructor::Default | FieldConstructor::Value(_) => { + quote!((0, Some(0))) + } + FieldConstructor::Arbitrary => { + quote! { <#ty as arbitrary::Arbitrary>::size_hint(depth) } + } + + // Note that in this case it's hard to determine what size_hint must be, so size_of::() is + // just an educated guess, although it's gonna be inaccurate for dynamically + // allocated types (Vec, HashMap, etc.). + FieldConstructor::With(_) => { + quote! { (::core::mem::size_of::<#ty>(), None) } + } + } + }) + }) + .collect::>>() + .map(|hints| { + quote! { + arbitrary::size_hint::and_all(&[ + #( #hints ),* + ]) } - } - }); - quote! { - arbitrary::size_hint::and_all(&[ - #( #hints ),* - ]) - } + }) }; let size_hint_structlike = |fields: &Fields| { - let hint = size_hint_fields(fields); - quote! { - #[inline] - fn size_hint(depth: usize) -> (usize, Option) { - arbitrary::size_hint::recursion_guard(depth, |depth| #hint) + size_hint_fields(fields).map(|hint| { + quote! { + #[inline] + fn size_hint(depth: usize) -> (usize, Option) { + arbitrary::size_hint::recursion_guard(depth, |depth| #hint) + } } - } + }) }; match &input.data { Data::Struct(data) => size_hint_structlike(&data.fields), Data::Union(data) => size_hint_structlike(&Fields::Named(data.fields.clone())), - Data::Enum(data) => { - let variants = data.variants.iter().map(|v| size_hint_fields(&v.fields)); - quote! { - #[inline] - fn size_hint(depth: usize) -> (usize, Option) { - arbitrary::size_hint::and( - ::size_hint(depth), - arbitrary::size_hint::recursion_guard(depth, |depth| { - arbitrary::size_hint::or_all(&[ #( #variants ),* ]) - }), - ) + Data::Enum(data) => data + .variants + .iter() + .map(|v| size_hint_fields(&v.fields)) + .collect::>>() + .map(|variants| { + quote! { + #[inline] + fn size_hint(depth: usize) -> (usize, Option) { + arbitrary::size_hint::and( + ::size_hint(depth), + arbitrary::size_hint::recursion_guard(depth, |depth| { + arbitrary::size_hint::or_all(&[ #( #variants ),* ]) + }), + ) + } } - } - } + }), } } -fn gen_constructor_for_field(field: &Field) -> TokenStream { - match determine_field_constructor(field) { +fn gen_constructor_for_field(field: &Field) -> Result { + let ctor = match determine_field_constructor(field)? { FieldConstructor::Default => quote!(Default::default()), FieldConstructor::Arbitrary => quote!(arbitrary::Arbitrary::arbitrary(u)?), FieldConstructor::With(function_or_closure) => quote!((#function_or_closure)(u)?), FieldConstructor::Value(value) => quote!(#value), - } + }; + Ok(ctor) } From fb58d7a992bbcb42ebaedb0f23b2de435d41809e Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 15:55:54 +0200 Subject: [PATCH 08/13] Return syn::Error instead of panic!() in proc_macro --- derive/src/field_attributes.rs | 5 ++++- tests/ui/unknown_attribute.stderr | 10 ++++------ tests/ui/unknown_attribute_with_value.stderr | 10 ++++------ 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index c48927a..8aab213 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -78,7 +78,10 @@ fn parse_attribute_internals(stream: TokenStream) -> Result { let value = parse_assigned_value("value", tokens_iter)?; Ok(FieldConstructor::Value(value)) } - _ => panic!("Unknown option for #[{ARBITRARY_ATTRIBUTE_NAME}]: `{token}`"), + _ => { + let msg = format!("Unknown option for #[{ARBITRARY_ATTRIBUTE_NAME}]: `{token}`"); + Err(syn::Error::new(token.span(), msg)) + } } } diff --git a/tests/ui/unknown_attribute.stderr b/tests/ui/unknown_attribute.stderr index a10c8a5..10e17d9 100644 --- a/tests/ui/unknown_attribute.stderr +++ b/tests/ui/unknown_attribute.stderr @@ -1,7 +1,5 @@ -error: proc-macro derive panicked - --> tests/ui/unknown_attribute.rs:3:10 +error: Unknown option for #[arbitrary]: `unknown_attr` + --> tests/ui/unknown_attribute.rs:5:17 | -3 | #[derive(Arbitrary)] - | ^^^^^^^^^ - | - = help: message: Unknown option for #[arbitrary]: `unknown_attr` +5 | #[arbitrary(unknown_attr)] + | ^^^^^^^^^^^^ diff --git a/tests/ui/unknown_attribute_with_value.stderr b/tests/ui/unknown_attribute_with_value.stderr index 3890695..8161e1e 100644 --- a/tests/ui/unknown_attribute_with_value.stderr +++ b/tests/ui/unknown_attribute_with_value.stderr @@ -1,7 +1,5 @@ -error: proc-macro derive panicked - --> tests/ui/unknown_attribute_with_value.rs:3:10 +error: Unknown option for #[arbitrary]: `unknown_attr` + --> tests/ui/unknown_attribute_with_value.rs:5:17 | -3 | #[derive(Arbitrary)] - | ^^^^^^^^^ - | - = help: message: Unknown option for #[arbitrary]: `unknown_attr` +5 | #[arbitrary(unknown_attr = 255)] + | ^^^^^^^^^^^^ From 8cbad14fd9c672413f15d3d53d1b2072cf72fbc9 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 16:10:29 +0200 Subject: [PATCH 09/13] Return syn::Error instead of panic!() --- derive/src/field_attributes.rs | 30 +++++++++++++++++++----------- tests/ui/value_no_rhs.stderr | 10 ++++------ tests/ui/with_no_rhs.stderr | 10 ++++------ 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index 8aab213..96341b0 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -1,4 +1,4 @@ -use proc_macro2::{TokenStream, TokenTree}; +use proc_macro2::{Group, Span, TokenStream, TokenTree}; use quote::quote; use syn::*; @@ -60,10 +60,11 @@ fn parse_attribute(attr: &Attribute) -> Result { t => panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] must contain a group, got: {t})"), } }; - parse_attribute_internals(group.stream()) + parse_attribute_internals(group) } -fn parse_attribute_internals(stream: TokenStream) -> Result { +fn parse_attribute_internals(group: Group) -> Result { + let stream = group.stream(); let mut tokens_iter = stream.into_iter(); let token = tokens_iter .next() @@ -71,11 +72,11 @@ fn parse_attribute_internals(stream: TokenStream) -> Result { match token.to_string().as_ref() { "default" => Ok(FieldConstructor::Default), "with" => { - let func_path = parse_assigned_value("with", tokens_iter)?; + let func_path = parse_assigned_value("with", tokens_iter, group.span())?; Ok(FieldConstructor::With(func_path)) } "value" => { - let value = parse_assigned_value("value", tokens_iter)?; + let value = parse_assigned_value("value", tokens_iter, group.span())?; Ok(FieldConstructor::Value(value)) } _ => { @@ -92,12 +93,19 @@ fn parse_attribute_internals(stream: TokenStream) -> Result { fn parse_assigned_value( opt_name: &str, mut tokens_iter: impl Iterator, + default_span: Span, ) -> Result { - let eq_sign = tokens_iter.next().unwrap_or_else(|| { - panic!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], `{opt_name}` is missing RHS.") - }); - if eq_sign.to_string() != "=" { - panic!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], expected `=` after `{opt_name}`, got: `{eq_sign}`"); + let eq_sign = tokens_iter.next().ok_or_else(|| { + let msg = format!( + "Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], `{opt_name}` is missing assignment." + ); + syn::Error::new(default_span, msg) + })?; + + if eq_sign.to_string() == "=" { + Ok(tokens_iter.collect()) + } else { + let msg = format!("Invalid syntax for #[{ARBITRARY_ATTRIBUTE_NAME}], expected `=` after `{opt_name}`, got: `{eq_sign}`"); + Err(syn::Error::new(eq_sign.span(), msg)) } - Ok(tokens_iter.collect()) } diff --git a/tests/ui/value_no_rhs.stderr b/tests/ui/value_no_rhs.stderr index c512529..4ae4a4f 100644 --- a/tests/ui/value_no_rhs.stderr +++ b/tests/ui/value_no_rhs.stderr @@ -1,7 +1,5 @@ -error: proc-macro derive panicked - --> tests/ui/value_no_rhs.rs:3:10 +error: Invalid syntax for #[arbitrary], `value` is missing assignment. + --> tests/ui/value_no_rhs.rs:5:16 | -3 | #[derive(Arbitrary)] - | ^^^^^^^^^ - | - = help: message: Invalid syntax for #[arbitrary], `value` is missing RHS. +5 | #[arbitrary(value)] + | ^^^^^^^ diff --git a/tests/ui/with_no_rhs.stderr b/tests/ui/with_no_rhs.stderr index 6f0b95f..2bf5236 100644 --- a/tests/ui/with_no_rhs.stderr +++ b/tests/ui/with_no_rhs.stderr @@ -1,7 +1,5 @@ -error: proc-macro derive panicked - --> tests/ui/with_no_rhs.rs:3:10 +error: Invalid syntax for #[arbitrary], `with` is missing assignment. + --> tests/ui/with_no_rhs.rs:5:16 | -3 | #[derive(Arbitrary)] - | ^^^^^^^^^ - | - = help: message: Invalid syntax for #[arbitrary], `with` is missing RHS. +5 | #[arbitrary(with)] + | ^^^^^^ From 479da322a5b65917bb933dc43b03a83ecdec0c03 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 17:17:51 +0200 Subject: [PATCH 10/13] Get rid of the rest of panics within determine_field_constructor() --- derive/src/field_attributes.rs | 26 ++++++++++++++++---------- tests/ui/multiple_arbitrary.stderr | 12 ++++++------ 2 files changed, 22 insertions(+), 16 deletions(-) diff --git a/derive/src/field_attributes.rs b/derive/src/field_attributes.rs index 96341b0..ccaba74 100644 --- a/derive/src/field_attributes.rs +++ b/derive/src/field_attributes.rs @@ -1,8 +1,8 @@ use proc_macro2::{Group, Span, TokenStream, TokenTree}; use quote::quote; -use syn::*; +use syn::{spanned::Spanned, *}; -/// Used to filter out necessary field attribute and in panics. +/// Used to filter out necessary field attribute and within error messages. static ARBITRARY_ATTRIBUTE_NAME: &str = "arbitrary"; /// Determines how a value for a field should be constructed. @@ -42,9 +42,10 @@ fn fetch_attr_from_field(field: &Field) -> Result> { .collect(); if found_attributes.len() > 1 { let name = field.ident.as_ref().unwrap(); - panic!( + let msg = format!( "Multiple conflicting #[{ARBITRARY_ATTRIBUTE_NAME}] attributes found on field `{name}`" ); + return Err(syn::Error::new(field.span(), msg)); } Ok(found_attributes.into_iter().next()) } @@ -52,12 +53,16 @@ fn fetch_attr_from_field(field: &Field) -> Result> { fn parse_attribute(attr: &Attribute) -> Result { let group = { let mut tokens_iter = attr.clone().tokens.into_iter(); - let token = tokens_iter - .next() - .unwrap_or_else(|| panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty.")); + let token = tokens_iter.next().ok_or_else(|| { + let msg = format!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty."); + syn::Error::new(attr.span(), msg) + })?; match token { TokenTree::Group(g) => g, - t => panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] must contain a group, got: {t})"), + t => { + let msg = format!("#[{ARBITRARY_ATTRIBUTE_NAME}] must contain a group, got: {t})"); + return Err(syn::Error::new(attr.span(), msg)); + } } }; parse_attribute_internals(group) @@ -66,9 +71,10 @@ fn parse_attribute(attr: &Attribute) -> Result { fn parse_attribute_internals(group: Group) -> Result { let stream = group.stream(); let mut tokens_iter = stream.into_iter(); - let token = tokens_iter - .next() - .unwrap_or_else(|| panic!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty.")); + let token = tokens_iter.next().ok_or_else(|| { + let msg = format!("#[{ARBITRARY_ATTRIBUTE_NAME}] cannot be empty."); + syn::Error::new(group.span(), msg) + })?; match token.to_string().as_ref() { "default" => Ok(FieldConstructor::Default), "with" => { diff --git a/tests/ui/multiple_arbitrary.stderr b/tests/ui/multiple_arbitrary.stderr index 1b23df4..d0be819 100644 --- a/tests/ui/multiple_arbitrary.stderr +++ b/tests/ui/multiple_arbitrary.stderr @@ -1,7 +1,7 @@ -error: proc-macro derive panicked - --> tests/ui/multiple_arbitrary.rs:3:10 +error: Multiple conflicting #[arbitrary] attributes found on field `x` + --> tests/ui/multiple_arbitrary.rs:5:5 | -3 | #[derive(Arbitrary)] - | ^^^^^^^^^ - | - = help: message: Multiple conflicting #[arbitrary] attributes found on field `x` +5 | / #[arbitrary(value = 2)] +6 | | #[arbitrary(value = 3)] +7 | | x: i32, + | |__________^ From 2701ed82c642a6ffab75e0ed6b77a7d30173101a Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 17:31:22 +0200 Subject: [PATCH 11/13] Update CHANGELOG.md --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5660d53..5929b93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,10 @@ Released YYYY-MM-DD. Released 2022-09-08. +### Added + +* Support custom arbitrary implementation for fields on derive. [#129](https://github.com/rust-fuzz/arbitrary/pull/129) + ### Fixed * Fixed a potential panic due to an off-by-one error in the `Arbitrary` From 4fdf16f71c358d688e3ddd39da35cd67fe3d579a Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 17:41:04 +0200 Subject: [PATCH 12/13] Fix compiletest, make it pass on stable (not on nightly) --- tests/ui/multiple_arbitrary.stderr | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/ui/multiple_arbitrary.stderr b/tests/ui/multiple_arbitrary.stderr index d0be819..f683d45 100644 --- a/tests/ui/multiple_arbitrary.stderr +++ b/tests/ui/multiple_arbitrary.stderr @@ -1,7 +1,5 @@ error: Multiple conflicting #[arbitrary] attributes found on field `x` --> tests/ui/multiple_arbitrary.rs:5:5 | -5 | / #[arbitrary(value = 2)] -6 | | #[arbitrary(value = 3)] -7 | | x: i32, - | |__________^ +5 | #[arbitrary(value = 2)] + | ^ From 755b8e0f5856acb4d6eb9b9b8d4fabcb16564159 Mon Sep 17 00:00:00 2001 From: Sergey Potapov Date: Thu, 20 Oct 2022 20:00:29 +0200 Subject: [PATCH 13/13] Replace trybuild ui tests with doc tests --- Cargo.toml | 3 -- src/lib.rs | 48 ++++++++++++++++++++ tests/compiletest.rs | 6 --- tests/ui/multiple_arbitrary.rs | 11 ----- tests/ui/multiple_arbitrary.stderr | 5 -- tests/ui/unknown_attribute.rs | 10 ---- tests/ui/unknown_attribute.stderr | 5 -- tests/ui/unknown_attribute_with_value.rs | 10 ---- tests/ui/unknown_attribute_with_value.stderr | 5 -- tests/ui/value_no_rhs.rs | 10 ---- tests/ui/value_no_rhs.stderr | 5 -- tests/ui/with_no_rhs.rs | 10 ---- tests/ui/with_no_rhs.stderr | 5 -- 13 files changed, 48 insertions(+), 85 deletions(-) delete mode 100644 tests/compiletest.rs delete mode 100644 tests/ui/multiple_arbitrary.rs delete mode 100644 tests/ui/multiple_arbitrary.stderr delete mode 100644 tests/ui/unknown_attribute.rs delete mode 100644 tests/ui/unknown_attribute.stderr delete mode 100644 tests/ui/unknown_attribute_with_value.rs delete mode 100644 tests/ui/unknown_attribute_with_value.stderr delete mode 100644 tests/ui/value_no_rhs.rs delete mode 100644 tests/ui/value_no_rhs.stderr delete mode 100644 tests/ui/with_no_rhs.rs delete mode 100644 tests/ui/with_no_rhs.stderr diff --git a/Cargo.toml b/Cargo.toml index 3ad0421..633a153 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,9 +22,6 @@ rust-version = "1.63.0" [dependencies] derive_arbitrary = { version = "1.1.6", path = "./derive", optional = true } -[dev-dependencies] -trybuild = { version = "1.0.71", features = ["diff"] } - [features] # Turn this feature on to enable support for `#[derive(Arbitrary)]`. derive = ["derive_arbitrary"] diff --git a/src/lib.rs b/src/lib.rs index 6df225f..a3fa48b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1292,3 +1292,51 @@ mod test { assert_eq!((1, None), <(u8, Vec) as Arbitrary>::size_hint(0)); } } + +/// Multiple conflicting arbitrary attributes are used on the same field: +/// ```compile_fail +/// #[derive(::arbitrary::Arbitrary)] +/// struct Point { +/// #[arbitrary(value = 2)] +/// #[arbitrary(value = 2)] +/// x: i32, +/// } +/// ``` +/// +/// An unknown attribute: +/// ```compile_fail +/// #[derive(::arbitrary::Arbitrary)] +/// struct Point { +/// #[arbitrary(unknown_attr)] +/// x: i32, +/// } +/// ``` +/// +/// An unknown attribute with a value: +/// ```compile_fail +/// #[derive(::arbitrary::Arbitrary)] +/// struct Point { +/// #[arbitrary(unknown_attr = 13)] +/// x: i32, +/// } +/// ``` +/// +/// `value` without RHS: +/// ```compile_fail +/// #[derive(::arbitrary::Arbitrary)] +/// struct Point { +/// #[arbitrary(value)] +/// x: i32, +/// } +/// ``` +/// +/// `with` without RHS: +/// ```compile_fail +/// #[derive(::arbitrary::Arbitrary)] +/// struct Point { +/// #[arbitrary(with)] +/// x: i32, +/// } +/// ``` +#[cfg(all(doctest, feature = "derive"))] +pub struct CompileFailTests; diff --git a/tests/compiletest.rs b/tests/compiletest.rs deleted file mode 100644 index 9ba9f56..0000000 --- a/tests/compiletest.rs +++ /dev/null @@ -1,6 +0,0 @@ -#[cfg(feature = "derive")] -#[test] -fn ui() { - let t = trybuild::TestCases::new(); - t.compile_fail("tests/ui/**/*.rs"); -} diff --git a/tests/ui/multiple_arbitrary.rs b/tests/ui/multiple_arbitrary.rs deleted file mode 100644 index 1253eab..0000000 --- a/tests/ui/multiple_arbitrary.rs +++ /dev/null @@ -1,11 +0,0 @@ -use arbitrary::Arbitrary; - -#[derive(Arbitrary)] -struct Point { - #[arbitrary(value = 2)] - #[arbitrary(value = 3)] - x: i32, - y: i32 -} - -fn main() { } diff --git a/tests/ui/multiple_arbitrary.stderr b/tests/ui/multiple_arbitrary.stderr deleted file mode 100644 index f683d45..0000000 --- a/tests/ui/multiple_arbitrary.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Multiple conflicting #[arbitrary] attributes found on field `x` - --> tests/ui/multiple_arbitrary.rs:5:5 - | -5 | #[arbitrary(value = 2)] - | ^ diff --git a/tests/ui/unknown_attribute.rs b/tests/ui/unknown_attribute.rs deleted file mode 100644 index 3b65151..0000000 --- a/tests/ui/unknown_attribute.rs +++ /dev/null @@ -1,10 +0,0 @@ -use arbitrary::Arbitrary; - -#[derive(Arbitrary)] -struct Point { - #[arbitrary(unknown_attr)] - x: i32, - y: i32 -} - -fn main() { } diff --git a/tests/ui/unknown_attribute.stderr b/tests/ui/unknown_attribute.stderr deleted file mode 100644 index 10e17d9..0000000 --- a/tests/ui/unknown_attribute.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Unknown option for #[arbitrary]: `unknown_attr` - --> tests/ui/unknown_attribute.rs:5:17 - | -5 | #[arbitrary(unknown_attr)] - | ^^^^^^^^^^^^ diff --git a/tests/ui/unknown_attribute_with_value.rs b/tests/ui/unknown_attribute_with_value.rs deleted file mode 100644 index a95b246..0000000 --- a/tests/ui/unknown_attribute_with_value.rs +++ /dev/null @@ -1,10 +0,0 @@ -use arbitrary::Arbitrary; - -#[derive(Arbitrary)] -struct Point { - #[arbitrary(unknown_attr = 255)] - x: i32, - y: i32 -} - -fn main() { } diff --git a/tests/ui/unknown_attribute_with_value.stderr b/tests/ui/unknown_attribute_with_value.stderr deleted file mode 100644 index 8161e1e..0000000 --- a/tests/ui/unknown_attribute_with_value.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Unknown option for #[arbitrary]: `unknown_attr` - --> tests/ui/unknown_attribute_with_value.rs:5:17 - | -5 | #[arbitrary(unknown_attr = 255)] - | ^^^^^^^^^^^^ diff --git a/tests/ui/value_no_rhs.rs b/tests/ui/value_no_rhs.rs deleted file mode 100644 index 35ff767..0000000 --- a/tests/ui/value_no_rhs.rs +++ /dev/null @@ -1,10 +0,0 @@ -use arbitrary::Arbitrary; - -#[derive(Arbitrary)] -struct Point { - #[arbitrary(value)] - x: i32, - y: i32 -} - -fn main() { } diff --git a/tests/ui/value_no_rhs.stderr b/tests/ui/value_no_rhs.stderr deleted file mode 100644 index 4ae4a4f..0000000 --- a/tests/ui/value_no_rhs.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Invalid syntax for #[arbitrary], `value` is missing assignment. - --> tests/ui/value_no_rhs.rs:5:16 - | -5 | #[arbitrary(value)] - | ^^^^^^^ diff --git a/tests/ui/with_no_rhs.rs b/tests/ui/with_no_rhs.rs deleted file mode 100644 index 0663d92..0000000 --- a/tests/ui/with_no_rhs.rs +++ /dev/null @@ -1,10 +0,0 @@ -use arbitrary::Arbitrary; - -#[derive(Arbitrary)] -struct Point { - #[arbitrary(with)] - x: i32, - y: i32 -} - -fn main() { } diff --git a/tests/ui/with_no_rhs.stderr b/tests/ui/with_no_rhs.stderr deleted file mode 100644 index 2bf5236..0000000 --- a/tests/ui/with_no_rhs.stderr +++ /dev/null @@ -1,5 +0,0 @@ -error: Invalid syntax for #[arbitrary], `with` is missing assignment. - --> tests/ui/with_no_rhs.rs:5:16 - | -5 | #[arbitrary(with)] - | ^^^^^^