From 623d32f51f115d798255ae2f0e26e78b371e86fa Mon Sep 17 00:00:00 2001 From: Ryan Kelly Date: Wed, 10 Feb 2021 16:47:25 +1100 Subject: [PATCH] Add support for named alternate constructors. It's a fairly common pattern for object-oriented interfaces to have static methods that act as named "factories" for producing object instances in ways that differ from the default constructor. This commit adds basic support for such named alternate constructors: * In the UDL, declare additional constructors with a [Name=Value] attribute to tell the tool what name should be exposed under. * In the Rust code, implement a corresponding method on the struct that acts as a constructor (no `self` parameter, `Self` return type). * In the foreign-language code, expect a static method on the resulting class with name matching the one provided in Rust. We continue to reserve the name "new" to refer to the default constructor. A followup could consider annotating it with an additional attribute like [DefaultConstructor] in order to let the default constructor have a different name on the Rust side. --- docs/manual/src/udl/interfaces.md | 21 +++ examples/sprites/src/lib.rs | 6 + examples/sprites/src/sprites.udl | 2 +- .../sprites/tests/bindings/test_sprites.kts | 8 +- .../sprites/tests/bindings/test_sprites.py | 4 + .../sprites/tests/bindings/test_sprites.swift | 3 +- .../sprites/tests/test_generated_bindings.rs | 2 +- .../gecko_js/templates/InterfaceTemplate.cpp | 9 +- .../gecko_js/templates/WebIDLTemplate.webidl | 21 ++- .../kotlin/templates/ObjectTemplate.kt | 19 +- .../src/bindings/python/gen_python.rs | 2 +- .../python/templates/ObjectTemplate.py | 22 ++- .../swift/templates/ObjectTemplate.swift | 24 ++- uniffi_bindgen/src/interface/attributes.rs | 122 ++++++++++--- uniffi_bindgen/src/interface/object.rs | 162 ++++++++++++++++-- 15 files changed, 370 insertions(+), 57 deletions(-) diff --git a/docs/manual/src/udl/interfaces.md b/docs/manual/src/udl/interfaces.md index 465dab869a..1f6deade2d 100644 --- a/docs/manual/src/udl/interfaces.md +++ b/docs/manual/src/udl/interfaces.md @@ -74,6 +74,27 @@ func display(list: TodoListProtocol) { } ``` +## Alternate Named Constructors + +In addition to the default constructor connected to the `::new()` method, you can specify +alternate named constructors to create object instances in different ways. Each such constructor +must be given an explicit name, provided in the UDL with the `[Name]` attribute like so: + +```idl +interface TodoList { + // The default constructor makes an empty list. + constructor(); + // This alternate constructor makes a new TodoList from a list of string items. + [Name=new_from_items] + constructor(sequence items) + ... +``` + +For each alternate constructor, UniFFI will expose an appropriate static-method, class-method or similar +in the foreign language binding, and will connect it to the Rust method of the same name on the underlying +Rust struct. + + ## Concurrent Access Since interfaces represent mutable data, uniffi has to take extra care diff --git a/examples/sprites/src/lib.rs b/examples/sprites/src/lib.rs index 3721e37a21..2b1a6b65b3 100644 --- a/examples/sprites/src/lib.rs +++ b/examples/sprites/src/lib.rs @@ -39,6 +39,12 @@ impl Sprite { } } + fn new_relative_to(reference: Point, direction: Vector) -> Sprite { + Sprite { + current_position: translate(&reference, direction), + } + } + fn get_position(&self) -> Point { self.current_position.clone() } diff --git a/examples/sprites/src/sprites.udl b/examples/sprites/src/sprites.udl index e76c33ac08..6781c6cee5 100644 --- a/examples/sprites/src/sprites.udl +++ b/examples/sprites/src/sprites.udl @@ -14,8 +14,8 @@ dictionary Vector { }; interface Sprite { - // Should be an optional, but I had to test nullable args :) constructor(Point? initial_position); + [Name=new_relative_to] constructor(Point reference, Vector direction); Point get_position(); void move_to(Point position); void move_by(Vector direction); diff --git a/examples/sprites/tests/bindings/test_sprites.kts b/examples/sprites/tests/bindings/test_sprites.kts index a023e11ab5..42451f28dd 100644 --- a/examples/sprites/tests/bindings/test_sprites.kts +++ b/examples/sprites/tests/bindings/test_sprites.kts @@ -13,9 +13,13 @@ s.moveBy(Vector(-4.0, 2.0)) assert( s.getPosition() == Point(-3.0, 4.0) ) s.destroy() -try { +try { s.moveBy(Vector(0.0, 0.0)) assert(false) { "Should not be able to call anything after `destroy`" } } catch(e: IllegalStateException) { assert(true) -} \ No newline at end of file +} + +val srel = Sprite.newRelativeTo(Point(0.0, 1.0), Vector(1.0, 1.5)) +assert( srel.getPosition() == Point(1.0, 2.5) ) + diff --git a/examples/sprites/tests/bindings/test_sprites.py b/examples/sprites/tests/bindings/test_sprites.py index 45c130c8c9..5142c2fc42 100644 --- a/examples/sprites/tests/bindings/test_sprites.py +++ b/examples/sprites/tests/bindings/test_sprites.py @@ -11,3 +11,7 @@ s.move_by(Vector(-4, 2)) assert s.get_position() == Point(-3, 4) + +srel = Sprite.new_relative_to(Point(0, 1), Vector(1, 1.5)) +assert srel.get_position() == Point(1, 2.5) + diff --git a/examples/sprites/tests/bindings/test_sprites.swift b/examples/sprites/tests/bindings/test_sprites.swift index 8ff51ac597..d5428ac679 100644 --- a/examples/sprites/tests/bindings/test_sprites.swift +++ b/examples/sprites/tests/bindings/test_sprites.swift @@ -12,4 +12,5 @@ assert( s.getPosition() == Point(x: 1, y: 2)) s.moveBy(direction: Vector(dx: -4, dy: 2)) assert( s.getPosition() == Point(x: -3, y: 4)) - +let srel = Sprite.newRelativeTo(reference: Point(x: 0.0, y: 1.0), direction: Vector(dx: 1, dy: 1.5)) +assert( srel.getPosition() == Point(x: 1.0, y: 2.5) ) diff --git a/examples/sprites/tests/test_generated_bindings.rs b/examples/sprites/tests/test_generated_bindings.rs index d9539977de..fa4354c096 100644 --- a/examples/sprites/tests/test_generated_bindings.rs +++ b/examples/sprites/tests/test_generated_bindings.rs @@ -1,7 +1,7 @@ uniffi_macros::build_foreign_language_testcases!( "src/sprites.udl", [ - // "tests/bindings/test_sprites.py", + "tests/bindings/test_sprites.py", "tests/bindings/test_sprites.kts", "tests/bindings/test_sprites.swift", ] diff --git a/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp b/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp index f1843a15ea..ec3aa0b76a 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp +++ b/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp @@ -39,7 +39,8 @@ JSObject* {{ obj.name()|class_name_cpp(context) }}::WrapObject( return dom::{{ obj.name()|class_name_cpp(context) }}_Binding::Wrap(aCx, this, aGivenProto); } -{%- for cons in obj.constructors() %} +{%- match obj.primary_constructor() %} +{%- when Some with (cons) %} /* static */ already_AddRefed<{{ obj.name()|class_name_cpp(context) }}> {{ obj.name()|class_name_cpp(context) }}::Constructor( @@ -61,10 +62,14 @@ already_AddRefed<{{ obj.name()|class_name_cpp(context) }}> {{ obj.name()|class_n auto result = MakeRefPtr<{{ obj.name()|class_name_cpp(context) }}>(global, handle); return result.forget(); } +{%- when None %} +{%- endmatch %} + +{%- for cons in obj.alternate_constructors() %} +MOZ_STATIC_ASSERT(false, "Sorry the gecko-js backend does not yet support alternate constructors"); {%- endfor %} {%- for meth in obj.methods() %} - {% match meth.cpp_return_type() %}{% when Some with (type_) %}{{ type_|ret_type_cpp(context) }}{% else %}void{% endmatch %} {{ obj.name()|class_name_cpp(context) }}::{{ meth.name()|fn_name_cpp }}( {%- for arg in meth.cpp_arguments() %} {{ arg|arg_type_cpp(context) }} {{ arg.name() }}{%- if !loop.last %},{% endif %} diff --git a/uniffi_bindgen/src/bindings/gecko_js/templates/WebIDLTemplate.webidl b/uniffi_bindgen/src/bindings/gecko_js/templates/WebIDLTemplate.webidl index eddd93fdec..c27cb89729 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/templates/WebIDLTemplate.webidl +++ b/uniffi_bindgen/src/bindings/gecko_js/templates/WebIDLTemplate.webidl @@ -46,7 +46,9 @@ namespace {{ context.namespace()|class_name_webidl(context) }} { {%- for obj in ci.iter_object_definitions() %} [ChromeOnly, Exposed=Window] interface {{ obj.name()|class_name_webidl(context) }} { - {%- for cons in obj.constructors() %} + + {%- match obj.primary_constructor() %} + {%- when Some with (cons) %} {%- if cons.throws().is_some() %} [Throws] {% endif %} @@ -60,6 +62,23 @@ interface {{ obj.name()|class_name_webidl(context) }} { {%- if !loop.last %}, {% endif %} {%- endfor %} ); + {%- when None %} + {%- endmatch %} + + {%- for cons in obj.alternate_constructors() %} + {%- if cons.throws().is_some() %} + [Throws] + {% endif %} + {{ obj.name()|class_name_webidl(context) }} {{ cons.name()|fn_name_webidl }}( + {%- for arg in cons.arguments() %} + {% if arg.optional() -%}optional{%- else -%}{%- endif %} {{ arg.webidl_type()|type_webidl(context) }} {{ arg.name() }} + {%- match arg.webidl_default_value() %} + {%- when Some with (literal) %} = {{ literal|literal_webidl }} + {%- else %} + {%- endmatch %} + {%- if !loop.last %}, {% endif %} + {%- endfor %} + ); {%- endfor %} {% for meth in obj.methods() -%} diff --git a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt index c2ab9a223f..90e9fd4074 100644 --- a/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt +++ b/uniffi_bindgen/src/bindings/kotlin/templates/ObjectTemplate.kt @@ -12,10 +12,12 @@ class {{ obj.name()|class_name_kt }}( handle: Long ) : FFIObject(AtomicLong(handle)), {{ obj.name()|class_name_kt }}Interface { - {%- for cons in obj.constructors() %} + {%- match obj.primary_constructor() %} + {%- when Some with (cons) %} constructor({% call kt::arg_list_decl(cons) -%}) : this({% call kt::to_ffi_call(cons) %}) - {%- endfor %} + {%- when None %} + {%- endmatch %} /** * Disconnect the object from the underlying Rust object. @@ -48,12 +50,21 @@ class {{ obj.name()|class_name_kt }}( }.let { {{ "it"|lift_kt(return_type) }} } - + {%- when None -%} override fun {{ meth.name()|fn_name_kt }}({% call kt::arg_list_protocol(meth) %}) = callWithHandle { - {%- call kt::to_ffi_call_with_prefix("it", meth) %} + {%- call kt::to_ffi_call_with_prefix("it", meth) %} } {% endmatch %} {% endfor %} + + {% if obj.constructors().len() > 1 -%} + companion object { + {% for cons in obj.alternate_constructors() -%} + fun {{ cons.name()|fn_name_kt }}({% call kt::arg_list_decl(cons) %}): {{ obj.name()|class_name_kt }} = + {{ obj.name()|class_name_kt }}({% call kt::to_ffi_call(cons) %}) + {% endfor %} + } + {%- endif %} } diff --git a/uniffi_bindgen/src/bindings/python/gen_python.rs b/uniffi_bindgen/src/bindings/python/gen_python.rs index 6c086c5995..bc06e6fb84 100644 --- a/uniffi_bindgen/src/bindings/python/gen_python.rs +++ b/uniffi_bindgen/src/bindings/python/gen_python.rs @@ -183,7 +183,7 @@ mod filters { Type::Enum(name) => format!("{}({})", class_name_py(name)?, nm), Type::Object(_) => panic!("No support for lifting objects, yet"), Type::CallbackInterface(_) => panic!("No support for lifting callback interfaces, yet"), - Type::Error(_) => panic!("No support for lowering errors, yet"), + Type::Error(_) => panic!("No support for lifting errors, yet"), Type::Record(_) | Type::Optional(_) | Type::Sequence(_) | Type::Map(_) => format!( "{}.consumeInto{}()", nm, diff --git a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py index 930e670635..cec5a695c0 100644 --- a/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py +++ b/uniffi_bindgen/src/bindings/python/templates/ObjectTemplate.py @@ -1,10 +1,11 @@ class {{ obj.name()|class_name_py }}(object): - # XXX TODO: support for multiple constructors... - {%- for cons in obj.constructors() %} + {%- match obj.primary_constructor() %} + {%- when Some with (cons) %} def __init__(self, {% call py::arg_list_decl(cons) -%}): {%- call py::coerce_args_extra_indent(cons) %} - self._handle = {% call py::to_ffi_call(cons) %} - {%- endfor %} + self._handle = {% call py::to_ffi_call(cons) %}\ + {%- when None %} + {%- endmatch %} def __del__(self): rust_call_with_error( @@ -13,6 +14,19 @@ def __del__(self): self._handle ) + {% for cons in obj.alternate_constructors() -%} + @classmethod + def {{ cons.name()|fn_name_py }}(cls, {% call py::arg_list_decl(cons) %}): + {%- call py::coerce_args_extra_indent(cons) %} + # Call the (fallible) function before creating any half-baked object instances. + handle = {% call py::to_ffi_call(cons) %} + # Lightly yucky way to bypass the usual __init__ logic + # and just create a new instance with the required handle. + inst = cls.__new__(cls) + inst._handle = handle + return inst + {% endfor %} + {% for meth in obj.methods() -%} {%- match meth.return_type() -%} diff --git a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift index 672f7b23b3..3b744b6d52 100644 --- a/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift +++ b/uniffi_bindgen/src/bindings/swift/templates/ObjectTemplate.swift @@ -9,14 +9,20 @@ public protocol {{ obj.name() }}Protocol { {% endfor %} } -public class {{ obj.name() }}: {{ obj.name() }}Protocol { +public class {{ obj.name()|class_name_swift }}: {{ obj.name() }}Protocol { private let handle: UInt64 - {%- for cons in obj.constructors() %} - public init({% call swift::arg_list_decl(cons) -%}) {% call swift::throws(cons) %} { - self.handle = {% call swift::to_ffi_call(cons) %} + private init(fromRawHandle handle: UInt64) { + self.handle = handle } - {%- endfor %} + + {%- match obj.primary_constructor() %} + {%- when Some with (cons) %} + public convenience init({% call swift::arg_list_decl(cons) -%}) {% call swift::throws(cons) %} { + self.init(fromRawHandle: {% call swift::to_ffi_call(cons) %}) + } + {%- when None %} + {%- endmatch %} deinit { try! rustCall(InternalError.unknown()) { err in @@ -24,7 +30,13 @@ public class {{ obj.name() }}: {{ obj.name() }}Protocol { } } - // TODO: Maybe merge the two templates (i.e the one with a return type and the one without) + {% for cons in obj.alternate_constructors() %} + public static func {{ cons.name()|fn_name_swift }}({% call swift::arg_list_decl(cons) %}) {% call swift::throws(cons) %} -> {{ obj.name()|class_name_swift }} { + return {{ obj.name()|class_name_swift }}(fromRawHandle: {% call swift::to_ffi_call(cons) %}) + } + {% endfor %} + + {# // TODO: Maybe merge the two templates (i.e the one with a return type and the one without) #} {% for meth in obj.methods() -%} {%- match meth.return_type() -%} diff --git a/uniffi_bindgen/src/interface/attributes.rs b/uniffi_bindgen/src/interface/attributes.rs index 0ba78fb2f0..a386934612 100644 --- a/uniffi_bindgen/src/interface/attributes.rs +++ b/uniffi_bindgen/src/interface/attributes.rs @@ -27,6 +27,7 @@ use anyhow::{bail, Result}; pub(super) enum Attribute { ByRef, Error, + Name(String), Threadsafe, Throws(String), } @@ -54,20 +55,13 @@ impl TryFrom<&weedle::attribute::ExtendedAttribute<'_>> for Attribute { }, // Matches assignment-style attributes like ["Throws=Error"] weedle::attribute::ExtendedAttribute::Ident(identity) => { - if identity.lhs_identifier.0 == "Throws" { - Ok(Attribute::Throws(match identity.rhs { - weedle::attribute::IdentifierOrString::Identifier(identifier) => { - identifier.0.to_string() - } - weedle::attribute::IdentifierOrString::String(str_lit) => { - str_lit.0.to_string() - } - })) - } else { - anyhow::bail!( + match identity.lhs_identifier.0 { + "Name" => Ok(Attribute::Name(name_from_id_or_string(&identity.rhs))), + "Throws" => Ok(Attribute::Throws(name_from_id_or_string(&identity.rhs))), + _ => anyhow::bail!( "Attribute identity Identifier not supported: {:?}", identity.lhs_identifier.0 - ) + ), } } _ => anyhow::bail!("Attribute not supported: {:?}", weedle_attribute), @@ -75,6 +69,13 @@ impl TryFrom<&weedle::attribute::ExtendedAttribute<'_>> for Attribute { } } +fn name_from_id_or_string(nm: &weedle::attribute::IdentifierOrString<'_>) -> String { + match nm { + weedle::attribute::IdentifierOrString::Identifier(identifier) => identifier.0.to_string(), + weedle::attribute::IdentifierOrString::String(str_lit) => str_lit.0.to_string(), + } +} + /// Parse a weedle `ExtendedAttributeList` into a list of `Attribute`s, /// erroring out on duplicates. fn parse_attributes( @@ -154,10 +155,7 @@ impl TryFrom<&weedle::attribute::ExtendedAttributeList<'_>> for FunctionAttribut ) -> Result { let attrs = parse_attributes(weedle_attributes, |attr| match attr { Attribute::Throws(_) => Ok(()), - _ => bail!(format!( - "{:?} not supported for functions, methods or constructors", - attr - )), + _ => bail!(format!("{:?} not supported for functions or methods", attr)), })?; Ok(Self(attrs)) } @@ -218,10 +216,44 @@ impl TryFrom<&weedle::attribute::ExtendedAttributeList<'_>> for InterfaceAttribu // but not yet. pub(super) type MethodAttributes = FunctionAttributes; -// Similarly, currently Constructors only support the same attributes as Functions. -// When this changes, (e.g. https://github.com/mozilla/uniffi-rs/issues/37 to support multiple -// named constructors), ConstructorAttributes will need its own implementation. -pub(super) type ConstructorAttributes = FunctionAttributes; +// Constructors can have a `Name` attribute, so need their own implementation. +/// Represents UDL attributes that might appear on a function. +/// +/// This supports the `[Throws=ErrorName]` attribute for functions that +/// can produce an error. +#[derive(Debug, Clone, Hash, Default)] +pub(super) struct ConstructorAttributes(Vec); + +impl ConstructorAttributes { + pub(super) fn get_throws_err(&self) -> Option<&str> { + self.0.iter().find_map(|attr| match attr { + // This will hopefully return a helpful compilation error + // if the error is not defined. + Attribute::Throws(inner) => Some(inner.as_ref()), + _ => None, + }) + } + pub(super) fn get_name(&self) -> Option<&str> { + self.0.iter().find_map(|attr| match attr { + Attribute::Name(inner) => Some(inner.as_ref()), + _ => None, + }) + } +} + +impl TryFrom<&weedle::attribute::ExtendedAttributeList<'_>> for ConstructorAttributes { + type Error = anyhow::Error; + fn try_from( + weedle_attributes: &weedle::attribute::ExtendedAttributeList<'_>, + ) -> Result { + let attrs = parse_attributes(weedle_attributes, |attr| match attr { + Attribute::Throws(_) => Ok(()), + Attribute::Name(_) => Ok(()), + _ => bail!(format!("{:?} not supported for constructors", attr)), + })?; + Ok(Self(attrs)) + } +} #[cfg(test)] mod test { @@ -245,6 +277,22 @@ mod test { Ok(()) } + #[test] + fn test_name() -> Result<()> { + let (_, node) = weedle::attribute::ExtendedAttribute::parse("Name=Value").unwrap(); + let attr = Attribute::try_from(&node)?; + assert!(matches!(attr, Attribute::Name(nm) if nm == "Value")); + + let (_, node) = weedle::attribute::ExtendedAttribute::parse("Name").unwrap(); + let err = Attribute::try_from(&node).unwrap_err(); + assert_eq!( + err.to_string(), + "ExtendedAttributeNoArgs not supported: \"Name\"" + ); + + Ok(()) + } + #[test] fn test_threadsafe() -> Result<()> { let (_, node) = weedle::attribute::ExtendedAttribute::parse("Threadsafe").unwrap(); @@ -317,11 +365,43 @@ mod test { let err = FunctionAttributes::try_from(&node).unwrap_err(); assert_eq!( err.to_string(), - "ByRef not supported for functions, methods or constructors" + "ByRef not supported for functions or methods" ); Ok(()) } + #[test] + fn test_constructor_attributes() -> Result<()> { + let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[Throws=Error]").unwrap(); + let attrs = ConstructorAttributes::try_from(&node).unwrap(); + assert!(matches!(attrs.get_throws_err(), Some("Error"))); + assert!(matches!(attrs.get_name(), None)); + + let (_, node) = + weedle::attribute::ExtendedAttributeList::parse("[Name=MyFactory]").unwrap(); + let attrs = ConstructorAttributes::try_from(&node).unwrap(); + assert!(matches!(attrs.get_throws_err(), None)); + assert!(matches!(attrs.get_name(), Some("MyFactory"))); + + let (_, node) = + weedle::attribute::ExtendedAttributeList::parse("[Throws=Error, Name=MyFactory]") + .unwrap(); + let attrs = ConstructorAttributes::try_from(&node).unwrap(); + assert!(matches!(attrs.get_throws_err(), Some("Error"))); + assert!(matches!(attrs.get_name(), Some("MyFactory"))); + + Ok(()) + } + + #[test] + fn test_other_attributes_not_supported_for_constructors() -> Result<()> { + let (_, node) = + weedle::attribute::ExtendedAttributeList::parse("[Throws=Error, ByRef]").unwrap(); + let err = ConstructorAttributes::try_from(&node).unwrap_err(); + assert_eq!(err.to_string(), "ByRef not supported for constructors"); + Ok(()) + } + #[test] fn test_byref_attribute() -> Result<()> { let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[ByRef]").unwrap(); diff --git a/uniffi_bindgen/src/interface/object.rs b/uniffi_bindgen/src/interface/object.rs index 940de7045d..1b385691c6 100644 --- a/uniffi_bindgen/src/interface/object.rs +++ b/uniffi_bindgen/src/interface/object.rs @@ -45,6 +45,7 @@ //! # Ok::<(), anyhow::Error>(()) //! ``` +use std::collections::HashSet; use std::convert::TryFrom; use std::hash::{Hash, Hasher}; @@ -98,6 +99,19 @@ impl Object { self.constructors.iter().collect() } + pub fn primary_constructor(&self) -> Option<&Constructor> { + self.constructors + .iter() + .find(|cons| cons.is_primary_constructor()) + } + + pub fn alternate_constructors(&self) -> Vec<&Constructor> { + self.constructors + .iter() + .filter(|cons| !cons.is_primary_constructor()) + .collect() + } + pub fn methods(&self) -> Vec<&Method> { self.methods.iter().collect() } @@ -146,29 +160,38 @@ impl APIConverter for weedle::InterfaceDefinition<'_> { if self.inheritance.is_some() { bail!("interface inheritence is not supported"); } + let mut object = Object::new(self.identifier.0.to_string()); let attributes = match &self.attributes { Some(attrs) => InterfaceAttributes::try_from(attrs)?, None => Default::default(), }; - let mut object = Object::new(self.identifier.0.to_string()); + object.threadsafe = attributes.threadsafe(); + // Convert each member into a constructor or method, guarding against duplicate names. + let mut member_names = HashSet::new(); for member in &self.members.body { match member { weedle::interface::InterfaceMember::Constructor(t) => { - object.constructors.push(t.convert(ci)?); + let cons = t.convert(ci)?; + if !member_names.insert(cons.name.clone()) { + bail!("Duplicate interface member name: \"{}\"", cons.name()) + } + object.constructors.push(cons); } weedle::interface::InterfaceMember::Operation(t) => { let mut method = t.convert(ci)?; + if !member_names.insert(method.name.clone()) { + bail!("Duplicate interface member name: \"{}\"", method.name()) + } method.object_name.push_str(object.name.as_str()); object.methods.push(method); } _ => bail!("no support for interface member type {:?} yet", member), } } - if object.constructors.is_empty() { + // Everyone gets a primary constructor, even if not declared explicitly. + if object.primary_constructor().is_none() { object.constructors.push(Default::default()); } - - object.threadsafe = attributes.threadsafe(); Ok(object) } } @@ -212,6 +235,10 @@ impl Constructor { self.ffi_func.return_type = Some(FFIType::UInt64); Ok(()) } + + fn is_primary_constructor(&self) -> bool { + self.name == "new" + } } impl Hash for Constructor { @@ -241,14 +268,15 @@ impl Default for Constructor { impl APIConverter for weedle::interface::ConstructorInterfaceMember<'_> { fn convert(&self, ci: &mut ComponentInterface) -> Result { + let attributes = match &self.attributes { + Some(attr) => ConstructorAttributes::try_from(attr)?, + None => Default::default(), + }; Ok(Constructor { - name: String::from("new"), // TODO: get the name from an attribute maybe? + name: String::from(attributes.get_name().unwrap_or("new")), arguments: self.args.body.list.convert(ci)?, ffi_func: Default::default(), - attributes: match &self.attributes { - Some(attr) => ConstructorAttributes::try_from(attr)?, - None => Default::default(), - }, + attributes, }) } } @@ -337,8 +365,8 @@ impl APIConverter for weedle::interface::OperationInterfaceMember<'_> { if self.special.is_some() { bail!("special operations not supported"); } - if let Some(weedle::interface::StringifierOrStatic::Stringifier(_)) = self.modifier { - bail!("stringifiers are not supported"); + if self.modifier.is_some() { + bail!("method modifiers are not supported") } let return_type = ci.resolve_return_type_expression(&self.return_type)?; if let Some(Type::Object(_)) = return_type { @@ -347,7 +375,13 @@ impl APIConverter for weedle::interface::OperationInterfaceMember<'_> { Ok(Method { name: match self.identifier { None => bail!("anonymous methods are not supported {:?}", self), - Some(id) => id.0.to_string(), + Some(id) => { + let name = id.0.to_string(); + if name == "new" { + bail!("the method name \"new\" is reserved for the default constructor"); + } + name + } }, // We don't know the name of the containing `Object` at this point, fill it in later. object_name: Default::default(), @@ -413,4 +447,106 @@ mod test { Ok(()) } + + #[test] + fn test_alternate_constructors() -> Result<()> { + const UDL: &str = r#" + namespace test{}; + interface Testing { + constructor(); + [Name=new_with_u32] + constructor(u32 v); + }; + "#; + let ci = ComponentInterface::from_webidl(UDL).unwrap(); + assert_eq!(ci.iter_object_definitions().len(), 1); + + let obj = ci.get_object_definition("Testing").unwrap(); + assert!(obj.primary_constructor().is_some()); + assert_eq!(obj.alternate_constructors().len(), 1); + assert_eq!(obj.methods().len(), 0); + + let cons = obj.primary_constructor().unwrap(); + assert_eq!(cons.name(), "new"); + assert_eq!(cons.arguments.len(), 0); + assert_eq!(cons.ffi_func.arguments.len(), 0); + + let cons = obj.alternate_constructors()[0]; + assert_eq!(cons.name(), "new_with_u32"); + assert_eq!(cons.arguments.len(), 1); + assert_eq!(cons.ffi_func.arguments.len(), 1); + + Ok(()) + } + + #[test] + fn test_the_name_new_identifies_the_primary_constructor() -> Result<()> { + const UDL: &str = r#" + namespace test{}; + interface Testing { + [Name=newish] + constructor(); + [Name=new] + constructor(u32 v); + }; + "#; + let ci = ComponentInterface::from_webidl(UDL).unwrap(); + assert_eq!(ci.iter_object_definitions().len(), 1); + + let obj = ci.get_object_definition("Testing").unwrap(); + assert!(obj.primary_constructor().is_some()); + assert_eq!(obj.alternate_constructors().len(), 1); + assert_eq!(obj.methods().len(), 0); + + let cons = obj.primary_constructor().unwrap(); + assert_eq!(cons.name(), "new"); + assert_eq!(cons.arguments.len(), 1); + + let cons = obj.alternate_constructors()[0]; + assert_eq!(cons.name(), "newish"); + assert_eq!(cons.arguments.len(), 0); + assert_eq!(cons.ffi_func.arguments.len(), 0); + + Ok(()) + } + + #[test] + fn test_the_name_new_is_reserved_for_constructors() { + const UDL: &str = r#" + namespace test{}; + interface Testing { + constructor(); + void new(u32 v); + }; + "#; + let err = ComponentInterface::from_webidl(UDL).unwrap_err(); + assert_eq!( + err.to_string(), + "the method name \"new\" is reserved for the default constructor" + ); + } + + #[test] + fn test_duplicate_primary_constructors_not_allowed() { + const UDL: &str = r#" + namespace test{}; + interface Testing { + constructor(); + constructor(u32 v); + }; + "#; + let err = ComponentInterface::from_webidl(UDL).unwrap_err(); + assert_eq!(err.to_string(), "Duplicate interface member name: \"new\""); + + const UDL2: &str = r#" + namespace test{}; + interface Testing { + constructor(); + [Name=new] + constructor(u32 v); + }; + "#; + let err = ComponentInterface::from_webidl(UDL2).unwrap_err(); + assert_eq!(err.to_string(), "Duplicate interface member name: \"new\""); + } }