From 28589620f6609efedcf4b775dc67051576475858 Mon Sep 17 00:00:00 2001 From: David Tolnay Date: Tue, 14 Jun 2016 11:22:49 -0700 Subject: [PATCH] Error on duplicate attributes --- serde_codegen/src/attr.rs | 268 +++++++++++------- serde_codegen/src/de.rs | 16 +- serde_codegen/src/ser.rs | 6 +- .../compile-fail/duplicate_attributes.rs | 25 ++ .../compile-fail/duplicate_attributes.rs_ | 13 - 5 files changed, 199 insertions(+), 129 deletions(-) create mode 100644 serde_macros/tests/compile-fail/duplicate_attributes.rs delete mode 100644 serde_macros/tests/compile-fail/duplicate_attributes.rs_ diff --git a/serde_codegen/src/attr.rs b/serde_codegen/src/attr.rs index 5dd14c25..459fc867 100644 --- a/serde_codegen/src/attr.rs +++ b/serde_codegen/src/attr.rs @@ -1,7 +1,7 @@ use std::rc::Rc; use syntax::ast::{self, TokenTree}; use syntax::attr; -use syntax::codemap::Span; +use syntax::codemap::{Span, Spanned, respan}; use syntax::ext::base::ExtCtxt; use syntax::fold::Folder; use syntax::parse::parser::{Parser, PathStyle}; @@ -11,9 +11,68 @@ use syntax::print::pprust::{lit_to_string, meta_item_to_string}; use syntax::ptr::P; use aster::AstBuilder; +use aster::ident::ToIdent; use error::Error; +struct Attr<'a, 'b: 'a, T> { + cx: &'a ExtCtxt<'b>, + name: &'static str, + value: Option>, +} +impl<'a, 'b, T> Attr<'a, 'b, T> { + fn none(cx: &'a ExtCtxt<'b>, name: &'static str) -> Self { + Attr { + cx: cx, + name: name, + value: None, + } + } + + fn set(&mut self, span: Span, t: T) { + if let Some(Spanned { span: prev_span, .. }) = self.value { + let mut err = self.cx.struct_span_err( + span, + &format!("duplicate serde attribute `{}`", self.name)); + err.span_help(prev_span, "previously set here"); + err.emit(); + } else { + self.value = Some(respan(span, t)); + } + } + + fn set_opt(&mut self, v: Option>) { + if let Some(v) = v { + self.set(v.span, v.node); + } + } + + fn set_if_none(&mut self, span: Span, t: T) { + if self.value.is_none() { + self.value = Some(respan(span, t)); + } + } + + fn get(self) -> Option { + self.value.map(|spanned| spanned.node) + } +} + +struct BoolAttr<'a, 'b: 'a>(Attr<'a, 'b, ()>); +impl<'a, 'b> BoolAttr<'a, 'b> { + fn none(cx: &'a ExtCtxt<'b>, name: &'static str) -> Self { + BoolAttr(Attr::none(cx, name)) + } + + fn set_true(&mut self, span: Span) { + self.0.set(span, ()); + } + + fn get(&self) -> bool { + self.0.value.is_some() + } +} + #[derive(Debug)] pub struct Name { ident: ast::Ident, @@ -22,14 +81,6 @@ pub struct Name { } impl Name { - fn new(ident: ast::Ident) -> Self { - Name { - ident: ident, - serialize_name: None, - deserialize_name: None, - } - } - /// Return the container name for the container when serializing. pub fn serialize_name(&self) -> InternedString { match self.serialize_name { @@ -69,55 +120,47 @@ pub struct ContainerAttrs { impl ContainerAttrs { /// Extract out the `#[serde(...)]` attributes from an item. pub fn from_item(cx: &ExtCtxt, item: &ast::Item) -> Result { - let mut container_attrs = ContainerAttrs { - name: Name::new(item.ident), - deny_unknown_fields: false, - ser_bound: None, - de_bound: None, - }; + let mut ser_name = Attr::none(cx, "rename"); + let mut de_name = Attr::none(cx, "rename"); + let mut deny_unknown_fields = BoolAttr::none(cx, "deny_unknown_fields"); + let mut ser_bound = Attr::none(cx, "bound"); + let mut de_bound = Attr::none(cx, "bound"); for meta_items in item.attrs().iter().filter_map(get_serde_meta_items) { for meta_item in meta_items { + let span = meta_item.span; match meta_item.node { // Parse `#[serde(rename="foo")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"rename" => { let s = try!(get_str_from_lit(cx, name, lit)); - container_attrs.name.serialize_name = Some(s.clone()); - container_attrs.name.deserialize_name = Some(s); + ser_name.set(span, s.clone()); + de_name.set(span, s); } // Parse `#[serde(rename(serialize="foo", deserialize="bar"))]` ast::MetaItemKind::List(ref name, ref meta_items) if name == &"rename" => { - let (ser_name, de_name) = try!(get_renames(cx, meta_items)); - if ser_name.is_some() { - container_attrs.name.serialize_name = ser_name; - } - if de_name.is_some() { - container_attrs.name.deserialize_name = de_name; - } + let (ser, de) = try!(get_renames(cx, meta_items)); + ser_name.set_opt(ser); + de_name.set_opt(de); } // Parse `#[serde(deny_unknown_fields)]` ast::MetaItemKind::Word(ref name) if name == &"deny_unknown_fields" => { - container_attrs.deny_unknown_fields = true; + deny_unknown_fields.set_true(span); } // Parse `#[serde(bound="D: Serialize")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"bound" => { let where_predicates = try!(parse_lit_into_where(cx, name, lit)); - container_attrs.ser_bound = Some(where_predicates.clone()); - container_attrs.de_bound = Some(where_predicates); + ser_bound.set(span, where_predicates.clone()); + de_bound.set(span, where_predicates); } // Parse `#[serde(bound(serialize="D: Serialize", deserialize="D: Deserialize"))]` ast::MetaItemKind::List(ref name, ref meta_items) if name == &"bound" => { - let (ser_bound, de_bound) = try!(get_where_predicates(cx, meta_items)); - if ser_bound.is_some() { - container_attrs.ser_bound = ser_bound; - } - if de_bound.is_some() { - container_attrs.de_bound = de_bound; - } + let (ser, de) = try!(get_where_predicates(cx, meta_items)); + ser_bound.set_opt(ser); + de_bound.set_opt(de); } _ => { @@ -132,7 +175,16 @@ impl ContainerAttrs { } } - Ok(container_attrs) + Ok(ContainerAttrs { + name: Name { + ident: item.ident, + serialize_name: ser_name.get(), + deserialize_name: de_name.get(), + }, + deny_unknown_fields: deny_unknown_fields.get(), + ser_bound: ser_bound.get(), + de_bound: de_bound.get(), + }) } pub fn name(&self) -> &Name { @@ -160,29 +212,25 @@ pub struct VariantAttrs { impl VariantAttrs { pub fn from_variant(cx: &ExtCtxt, variant: &ast::Variant) -> Result { - let mut variant_attrs = VariantAttrs { - name: Name::new(variant.node.name), - }; + let mut ser_name = Attr::none(cx, "rename"); + let mut de_name = Attr::none(cx, "rename"); for meta_items in variant.node.attrs.iter().filter_map(get_serde_meta_items) { for meta_item in meta_items { + let span = meta_item.span; match meta_item.node { // Parse `#[serde(rename="foo")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"rename" => { let s = try!(get_str_from_lit(cx, name, lit)); - variant_attrs.name.serialize_name = Some(s.clone()); - variant_attrs.name.deserialize_name = Some(s); + ser_name.set(span, s.clone()); + de_name.set(span, s); } // Parse `#[serde(rename(serialize="foo", deserialize="bar"))]` ast::MetaItemKind::List(ref name, ref meta_items) if name == &"rename" => { - let (ser_name, de_name) = try!(get_renames(cx, meta_items)); - if ser_name.is_some() { - variant_attrs.name.serialize_name = ser_name; - } - if de_name.is_some() { - variant_attrs.name.deserialize_name = de_name; - } + let (ser, de) = try!(get_renames(cx, meta_items)); + ser_name.set_opt(ser); + de_name.set_opt(de); } _ => { @@ -197,7 +245,13 @@ impl VariantAttrs { } } - Ok(variant_attrs) + Ok(VariantAttrs { + name: Name { + ident: variant.node.name, + serialize_name: ser_name.get(), + deserialize_name: de_name.get(), + }, + }) } pub fn name(&self) -> &Name { @@ -209,8 +263,8 @@ impl VariantAttrs { #[derive(Debug)] pub struct FieldAttrs { name: Name, - skip_serializing_field: bool, - skip_deserializing_field: bool, + skip_serializing: bool, + skip_deserializing: bool, skip_serializing_if: Option, default: FieldDefault, serialize_with: Option, @@ -235,107 +289,91 @@ impl FieldAttrs { pub fn from_field(cx: &ExtCtxt, index: usize, field: &ast::StructField) -> Result { - let builder = AstBuilder::new(); + let mut ser_name = Attr::none(cx, "rename"); + let mut de_name = Attr::none(cx, "rename"); + let mut skip_serializing = BoolAttr::none(cx, "skip_serializing"); + let mut skip_deserializing = BoolAttr::none(cx, "skip_deserializing"); + let mut skip_serializing_if = Attr::none(cx, "skip_serializing_if"); + let mut default = Attr::none(cx, "default"); + let mut serialize_with = Attr::none(cx, "serialize_with"); + let mut deserialize_with = Attr::none(cx, "deserialize_with"); + let mut ser_bound = Attr::none(cx, "bound"); + let mut de_bound = Attr::none(cx, "bound"); let field_ident = match field.ident { Some(ident) => ident, - None => builder.id(index.to_string()), - }; - - let mut field_attrs = FieldAttrs { - name: Name::new(field_ident), - skip_serializing_field: false, - skip_deserializing_field: false, - skip_serializing_if: None, - default: FieldDefault::None, - serialize_with: None, - deserialize_with: None, - ser_bound: None, - de_bound: None, + None => index.to_string().to_ident(), }; for meta_items in field.attrs.iter().filter_map(get_serde_meta_items) { for meta_item in meta_items { + let span = meta_item.span; match meta_item.node { // Parse `#[serde(rename="foo")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"rename" => { let s = try!(get_str_from_lit(cx, name, lit)); - field_attrs.name.serialize_name = Some(s.clone()); - field_attrs.name.deserialize_name = Some(s); + ser_name.set(span, s.clone()); + de_name.set(span, s); } // Parse `#[serde(rename(serialize="foo", deserialize="bar"))]` ast::MetaItemKind::List(ref name, ref meta_items) if name == &"rename" => { - let (ser_name, de_name) = try!(get_renames(cx, meta_items)); - if ser_name.is_some() { - field_attrs.name.serialize_name = ser_name; - } - if de_name.is_some() { - field_attrs.name.deserialize_name = de_name; - } + let (ser, de) = try!(get_renames(cx, meta_items)); + ser_name.set_opt(ser); + de_name.set_opt(de); } // Parse `#[serde(default)]` ast::MetaItemKind::Word(ref name) if name == &"default" => { - field_attrs.default = FieldDefault::Default; + default.set(span, FieldDefault::Default); } // Parse `#[serde(default="...")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"default" => { let path = try!(parse_lit_into_path(cx, name, lit)); - field_attrs.default = FieldDefault::Path(path); + default.set(span, FieldDefault::Path(path)); } // Parse `#[serde(skip_serializing)]` ast::MetaItemKind::Word(ref name) if name == &"skip_serializing" => { - field_attrs.skip_serializing_field = true; + skip_serializing.set_true(span); } // Parse `#[serde(skip_deserializing)]` ast::MetaItemKind::Word(ref name) if name == &"skip_deserializing" => { - field_attrs.skip_deserializing_field = true; - - // Initialize field to Default::default() unless a different - // default is specified by `#[serde(default="...")]` - if field_attrs.default == FieldDefault::None { - field_attrs.default = FieldDefault::Default; - } + skip_deserializing.set_true(span); } // Parse `#[serde(skip_serializing_if="...")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"skip_serializing_if" => { let path = try!(parse_lit_into_path(cx, name, lit)); - field_attrs.skip_serializing_if = Some(path); + skip_serializing_if.set(span, path); } // Parse `#[serde(serialize_with="...")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"serialize_with" => { let path = try!(parse_lit_into_path(cx, name, lit)); - field_attrs.serialize_with = Some(path); + serialize_with.set(span, path); } // Parse `#[serde(deserialize_with="...")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"deserialize_with" => { let path = try!(parse_lit_into_path(cx, name, lit)); - field_attrs.deserialize_with = Some(path); + deserialize_with.set(span, path); } // Parse `#[serde(bound="D: Serialize")]` ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"bound" => { let where_predicates = try!(parse_lit_into_where(cx, name, lit)); - field_attrs.ser_bound = Some(where_predicates.clone()); - field_attrs.de_bound = Some(where_predicates); + ser_bound.set(span, where_predicates.clone()); + de_bound.set(span, where_predicates); } // Parse `#[serde(bound(serialize="D: Serialize", deserialize="D: Deserialize"))]` ast::MetaItemKind::List(ref name, ref meta_items) if name == &"bound" => { - let (ser_bound, de_bound) = try!(get_where_predicates(cx, meta_items)); - if ser_bound.is_some() { - field_attrs.ser_bound = ser_bound; - } - if de_bound.is_some() { - field_attrs.de_bound = de_bound; - } + let (ser, de) = try!(get_where_predicates(cx, meta_items)); + ser_bound.set_opt(ser); + de_bound.set_opt(de); } _ => { @@ -350,19 +388,39 @@ impl FieldAttrs { } } - Ok(field_attrs) + // Is skip_deserializing, initialize the field to Default::default() + // unless a different default is specified by `#[serde(default="...")]` + if let Some(Spanned { span, .. }) = skip_deserializing.0.value { + default.set_if_none(span, FieldDefault::Default); + } + + Ok(FieldAttrs { + name: Name { + ident: field_ident, + serialize_name: ser_name.get(), + deserialize_name: de_name.get(), + }, + skip_serializing: skip_serializing.get(), + skip_deserializing: skip_deserializing.get(), + skip_serializing_if: skip_serializing_if.get(), + default: default.get().unwrap_or(FieldDefault::None), + serialize_with: serialize_with.get(), + deserialize_with: deserialize_with.get(), + ser_bound: ser_bound.get(), + de_bound: de_bound.get(), + }) } pub fn name(&self) -> &Name { &self.name } - pub fn skip_serializing_field(&self) -> bool { - self.skip_serializing_field + pub fn skip_serializing(&self) -> bool { + self.skip_serializing } - pub fn skip_deserializing_field(&self) -> bool { - self.skip_deserializing_field + pub fn skip_deserializing(&self) -> bool { + self.skip_deserializing } pub fn skip_serializing_if(&self) -> Option<&ast::Path> { @@ -410,7 +468,7 @@ fn get_ser_and_de( attribute: &str, items: &[P], f: F -) -> Result<(Option, Option), Error> +) -> Result<(Option>, Option>), Error> where F: Fn(&ExtCtxt, &str, &ast::Lit) -> Result, { let mut ser_item = None; @@ -420,12 +478,12 @@ fn get_ser_and_de( match item.node { ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"serialize" => { let s = try!(f(cx, name, lit)); - ser_item = Some(s); + ser_item = Some(respan(item.span, s)); } ast::MetaItemKind::NameValue(ref name, ref lit) if name == &"deserialize" => { let s = try!(f(cx, name, lit)); - de_item = Some(s); + de_item = Some(respan(item.span, s)); } _ => { @@ -446,14 +504,14 @@ fn get_ser_and_de( fn get_renames( cx: &ExtCtxt, items: &[P], -) -> Result<(Option, Option), Error> { +) -> Result<(Option>, Option>), Error> { get_ser_and_de(cx, "rename", items, get_str_from_lit) } fn get_where_predicates( cx: &ExtCtxt, items: &[P], -) -> Result<(Option>, Option>), Error> { +) -> Result<(Option>>, Option>>), Error> { get_ser_and_de(cx, "bound", items, parse_lit_into_where) } diff --git a/serde_codegen/src/de.rs b/serde_codegen/src/de.rs index 095c9fd2..69d67bf9 100644 --- a/serde_codegen/src/de.rs +++ b/serde_codegen/src/de.rs @@ -135,7 +135,7 @@ fn build_impl_generics( // attribute specify their own bound so we do not generate one. All other fields // may need a `T: Deserialize` bound where T is the type of the field. fn needs_deserialize_bound(attrs: &attr::FieldAttrs) -> bool { - !attrs.skip_deserializing_field() + !attrs.skip_deserializing() && attrs.deserialize_with().is_none() && attrs.de_bound().is_none() } @@ -450,7 +450,7 @@ fn deserialize_seq( .enumerate() .map(|(i, &(ref field, ref attrs))| { let name = builder.id(format!("__field{}", i)); - if attrs.skip_deserializing_field() { + if attrs.skip_deserializing() { let default = expr_is_missing(cx, attrs); quote_stmt!(cx, let $name = $default; @@ -1049,7 +1049,7 @@ fn deserialize_map( // Declare each field that will be deserialized. let let_values: Vec = fields_attrs_names.iter() - .filter(|&&(_, ref attrs, _)| !attrs.skip_deserializing_field()) + .filter(|&&(_, ref attrs, _)| !attrs.skip_deserializing()) .map(|&(field, _, name)| { let field_ty = &field.ty; quote_stmt!(cx, let mut $name: Option<$field_ty> = None;).unwrap() @@ -1058,7 +1058,7 @@ fn deserialize_map( // Match arms to extract a value for a field. let value_arms = fields_attrs_names.iter() - .filter(|&&(_, ref attrs, _)| !attrs.skip_deserializing_field()) + .filter(|&&(_, ref attrs, _)| !attrs.skip_deserializing()) .map(|&(ref field, ref attrs, name)| { let deser_name = attrs.name().deserialize_name(); let name_str = builder.expr().lit().str(deser_name); @@ -1092,7 +1092,7 @@ fn deserialize_map( // Match arms to ignore value for fields that have `skip_deserializing`. // Ignored even if `deny_unknown_fields` is set. let skipped_arms = fields_attrs_names.iter() - .filter(|&&(_, ref attrs, _)| attrs.skip_deserializing_field()) + .filter(|&&(_, ref attrs, _)| attrs.skip_deserializing()) .map(|&(_, _, name)| { quote_arm!(cx, __Field::$name => { @@ -1112,7 +1112,7 @@ fn deserialize_map( }; let extract_values = fields_attrs_names.iter() - .filter(|&&(_, ref attrs, _)| !attrs.skip_deserializing_field()) + .filter(|&&(_, ref attrs, _)| !attrs.skip_deserializing()) .map(|&(_, ref attrs, name)| { let missing_expr = expr_is_missing(cx, attrs); @@ -1138,7 +1138,7 @@ fn deserialize_map( cx.span_bug(field.span, "struct contains unnamed fields") } }, - if attrs.skip_deserializing_field() { + if attrs.skip_deserializing() { expr_is_missing(cx, attrs) } else { builder.expr().id(name) @@ -1258,7 +1258,7 @@ fn check_no_str( }; for &(ref field, ref attrs) in fields { - if attrs.skip_deserializing_field() + if attrs.skip_deserializing() || attrs.deserialize_with().is_some() { continue } if let ast::TyKind::Rptr(_, ref inner) = field.ty.node { diff --git a/serde_codegen/src/ser.rs b/serde_codegen/src/ser.rs index dcc29ecb..da9f9a50 100644 --- a/serde_codegen/src/ser.rs +++ b/serde_codegen/src/ser.rs @@ -129,7 +129,7 @@ fn build_impl_generics( // attribute specify their own bound so we do not generate one. All other fields // may need a `T: Serialize` bound where T is the type of the field. fn needs_serialize_bound(attrs: &attr::FieldAttrs) -> bool { - !attrs.skip_serializing_field() + !attrs.skip_serializing() && attrs.serialize_with().is_none() && attrs.ser_bound().is_none() } @@ -749,7 +749,7 @@ fn serialize_struct_visitor( let fields_with_attrs = try!(attr::fields_with_attrs(cx, fields)); let arms: Vec = fields_with_attrs.iter() - .filter(|&&(_, ref attrs)| !attrs.skip_serializing_field()) + .filter(|&&(_, ref attrs)| !attrs.skip_serializing()) .enumerate() .map(|(i, &(ref field, ref attrs))| { let ident = field.ident.expect("struct has unnamed field"); @@ -794,7 +794,7 @@ fn serialize_struct_visitor( .build(); let len = fields_with_attrs.iter() - .filter(|&&(_, ref attrs)| !attrs.skip_serializing_field()) + .filter(|&&(_, ref attrs)| !attrs.skip_serializing()) .map(|&(ref field, ref attrs)| { let ident = field.ident.expect("struct has unnamed fields"); let mut field_expr = quote_expr!(cx, self.value.$ident); diff --git a/serde_macros/tests/compile-fail/duplicate_attributes.rs b/serde_macros/tests/compile-fail/duplicate_attributes.rs new file mode 100644 index 00000000..8d4b09a6 --- /dev/null +++ b/serde_macros/tests/compile-fail/duplicate_attributes.rs @@ -0,0 +1,25 @@ +#![feature(custom_attribute, custom_derive, plugin)] +#![plugin(serde_macros)] + +#[derive(Serialize)] +struct S { + #[serde(rename(serialize="x"))] + #[serde(rename(serialize="y"))] //~ ERROR: duplicate serde attribute `rename` + a: (), //~^ ERROR: duplicate serde attribute `rename` + //~^^ ERROR: duplicate serde attribute `rename` + + #[serde(rename(serialize="x"))] + #[serde(rename="y")] //~ ERROR: duplicate serde attribute `rename` + b: (), //~^ ERROR: duplicate serde attribute `rename` + //~^^ ERROR: duplicate serde attribute `rename` + + #[serde(rename(serialize="x"))] + #[serde(rename(deserialize="y"))] // ok + c: (), + + #[serde(rename="x")] + #[serde(rename(deserialize="y"))] //~ ERROR: duplicate serde attribute `rename` + d: (), //~^ ERROR: duplicate serde attribute `rename` +} //~^^ ERROR: duplicate serde attribute `rename` + +fn main() {} diff --git a/serde_macros/tests/compile-fail/duplicate_attributes.rs_ b/serde_macros/tests/compile-fail/duplicate_attributes.rs_ deleted file mode 100644 index 46eacee6..00000000 --- a/serde_macros/tests/compile-fail/duplicate_attributes.rs_ +++ /dev/null @@ -1,13 +0,0 @@ -#![feature(custom_attribute, custom_derive, plugin)] -#![plugin(serde_macros)] - -#[derive(Serialize, Deserialize)] -struct S { - #[serde(rename(serialize="x"))] - #[serde(rename(serialize="y"))] //~ ERROR buldternua - #[serde(rename(deserialize="y"))] // ok - #[serde(rename="y")] // error - z: i32, -} - -fn main() {}