From b7148708b350ac2dea9d25120dc6e0452de8acb4 Mon Sep 17 00:00:00 2001 From: Lina Cambridge Date: Fri, 2 Oct 2020 18:22:14 -0700 Subject: [PATCH 1/2] Special-case "optionals with a default value" in Gecko. These types are different than optionals without a default value. Their C++ type is just `T`, not `Optional`, since the default value is a fallback if the caller doesn't pass one. But they must be serialized as if they were `Optional`, because the Rust side of the FFI will try to deserialize them as optionals instead of instances of the value. --- .../src/bindings/gecko_js/gen_gecko_js.rs | 22 ++++---- .../gecko_js/templates/RustBufferHelper.h | 41 ++++++++++++++ .../src/bindings/gecko_js/webidl.rs | 55 ++++++++++++------- 3 files changed, 87 insertions(+), 31 deletions(-) diff --git a/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs b/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs index d90ddb21fa..e65174089b 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs +++ b/uniffi_bindgen/src/bindings/gecko_js/gen_gecko_js.rs @@ -284,7 +284,9 @@ mod filters { | WebIDLType::Flat(Type::Record(name)) | WebIDLType::Flat(Type::Object(name)) => class_name_webidl(name, context)?, WebIDLType::Nullable(inner) => format!("{}?", type_webidl(inner, context)?), - WebIDLType::Optional(inner) => type_webidl(inner, context)?, + WebIDLType::Optional(inner) | WebIDLType::OptionalWithDefaultValue(inner) => { + type_webidl(inner, context)? + } WebIDLType::Sequence(inner) => format!("sequence<{}>", type_webidl(inner, context)?), WebIDLType::Map(inner) => { format!("record", type_webidl(inner, context)?) @@ -371,6 +373,7 @@ mod filters { _ => format!("Nullable<{}>", type_cpp(inner, context)?), } } + WebIDLType::OptionalWithDefaultValue(inner) => type_cpp(inner, context)?, WebIDLType::Optional(inner) => format!("Optional<{}>", type_cpp(inner, context)?), WebIDLType::Sequence(inner) => format!("nsTArray<{}>", type_cpp(inner, context)?), WebIDLType::Map(inner) => format!("Record", type_cpp(inner, context)?), @@ -422,17 +425,11 @@ mod filters { } _ => format!("const {}&", in_arg_type_cpp(&arg.webidl_type(), context)?), }, - WebIDLType::Optional(inner) => { - let type_name = if arg.webidl_default_value().is_some() { - in_arg_type_cpp(inner.as_ref(), context)? - } else { - in_arg_type_cpp(&arg.webidl_type(), context)? - }; - format!("const {}&", type_name) - } WebIDLType::Flat(Type::Record(_)) | WebIDLType::Map(_) - | WebIDLType::Sequence(_) => { + | WebIDLType::Sequence(_) + | WebIDLType::Optional(_) + | WebIDLType::OptionalWithDefaultValue(_) => { format!("const {}&", in_arg_type_cpp(&arg.webidl_type(), context)?) } _ => in_arg_type_cpp(&arg.webidl_type(), context)?, @@ -496,6 +493,9 @@ mod filters { } WebIDLType::Flat(Type::Object(_)) => "nullptr".into(), WebIDLType::Flat(Type::String) => "EmptyString()".into(), + WebIDLType::OptionalWithDefaultValue(inner) => { + dummy_ret_value_cpp(inner.as_ref(), context)? + } WebIDLType::Optional(_) | WebIDLType::Nullable(_) | WebIDLType::Flat(Type::Record(_)) @@ -516,6 +516,7 @@ mod filters { // Since our in argument type is `nsAString`, we need to use that // to instantiate `ViaFfi`, not `nsString`. WebIDLType::Flat(Type::String) => ("nsAString".into(), false), + WebIDLType::OptionalWithDefaultValue(_) => (type_cpp(type_, context)?, true), WebIDLType::Nullable(inner) => match inner.as_ref() { WebIDLType::Flat(Type::String) => ("nsAString".into(), true), _ => (in_arg_type_cpp(type_, context)?, false), @@ -544,6 +545,7 @@ mod filters { // Out arguments are also `nsAString`, so we need to use it for the // instantiation. WebIDLType::Flat(Type::String) => ("nsAString".into(), false), + WebIDLType::OptionalWithDefaultValue(_) => (type_cpp(type_, context)?, true), WebIDLType::Nullable(inner) => match inner.as_ref() { WebIDLType::Flat(Type::String) => ("nsAString".into(), true), _ => (type_cpp(type_, context)?, false), diff --git a/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h b/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h index 5660ed6254..c805634b72 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h +++ b/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h @@ -737,4 +737,45 @@ struct ViaFfi { } }; +/// Partial specialization for all non-null types on the C++ side that should be +/// serialized as if they were nullable. This is analogous to a blanket +/// implementation of `ViaFfiUsingByteBuffer` for `Option` in Rust. + +template +struct ViaFfi { + [[nodiscard]] static bool Lift(const {{ context.ffi_rustbuffer_type() }}& aLowered, + T& aLifted) { + auto reader = Reader(aLowered); + auto hasValue = reader.ReadInt8(); + if (hasValue != 0 && hasValue != 1) { + MOZ_ASSERT(false); + return false; + } + if (!hasValue) { + return true; + } + if (!Serializable::ReadFrom(reader, aLifted)) { + return false; + } + if (reader.HasRemaining()) { + MOZ_ASSERT(false); + return false; + } + {{ context.ffi_rusterror_type() }} err = {0, nullptr}; + {{ ci.ffi_rustbuffer_free().name() }}(aLowered, &err); + if (err.mCode) { + MOZ_ASSERT(false, "Failed to free Rust buffer after lifting contents"); + return false; + } + return true; + } + + [[nodiscard]] static {{ context.ffi_rustbuffer_type() }} Lower(const T& aLifted) { + auto writer = Writer(); + writer.WriteInt8(1); + Serializable::WriteInto(writer, aLifted); + return writer.Buffer(); + } +}; + } // namespace {{ context.detail_name() }} diff --git a/uniffi_bindgen/src/bindings/gecko_js/webidl.rs b/uniffi_bindgen/src/bindings/gecko_js/webidl.rs index e0b8379f13..af0e9ed809 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/webidl.rs +++ b/uniffi_bindgen/src/bindings/gecko_js/webidl.rs @@ -50,6 +50,13 @@ pub enum WebIDLType { Nullable(Box), Optional(Box), + /// Optionals with a default value are a grab bag of special cases in Gecko. + /// In the generated C++ bindings, the type of an optional with a default + /// value is `T`, not `Optional`. However, it must be serialized as if + /// it's an `Optional`, since that's what the Rust side of the FFI + /// expects. + OptionalWithDefaultValue(Box), + /// Sequences are the same as their UniFFI counterparts. Sequence(Box), @@ -64,7 +71,19 @@ impl WebIDLType { match self { WebIDLType::Flat(Type::String) | WebIDLType::Flat(Type::Record(_)) => true, WebIDLType::Map(_) | WebIDLType::Sequence(_) => true, - WebIDLType::Optional(inner) | WebIDLType::Nullable(inner) => inner.needs_out_param(), + WebIDLType::Optional(inner) + | WebIDLType::OptionalWithDefaultValue(inner) + | WebIDLType::Nullable(inner) => inner.needs_out_param(), + _ => false, + } + } + + pub fn is_optional_record(&self) -> bool { + match self { + WebIDLType::OptionalWithDefaultValue(inner) => match inner.as_ref() { + WebIDLType::Flat(Type::Record(_)) => true, + _ => false, + }, _ => false, } } @@ -94,7 +113,9 @@ impl From for WebIDLType { panic!("[TODO: From({:?})]", type_) } Type::Optional(inner) => match *inner { - Type::Record(name) => WebIDLType::Optional(Box::new(Type::Record(name).into())), + Type::Record(name) => { + WebIDLType::OptionalWithDefaultValue(Box::new(Type::Record(name).into())) + } inner => WebIDLType::Nullable(Box::new(inner.into())), }, Type::Sequence(inner) => WebIDLType::Sequence(Box::new((*inner).into())), @@ -108,6 +129,7 @@ impl From<&WebIDLType> for FFIType { match type_ { WebIDLType::Flat(inner) => inner.into(), WebIDLType::Optional(_) + | WebIDLType::OptionalWithDefaultValue(_) | WebIDLType::Nullable(_) | WebIDLType::Sequence(_) | WebIDLType::Map(_) => FFIType::RustBuffer, @@ -310,9 +332,6 @@ impl ArgumentExt for Argument { if self.webidl_default_value().is_some() { return true; } - if let Type::Optional(inner) = self.type_() { - return matches!(inner.as_ref(), Type::Record(_)); - } false } @@ -320,15 +339,12 @@ impl ArgumentExt for Argument { if let Some(literal) = self.default_value() { return Some(literal); } - match self.type_() { - Type::Optional(inner) => match inner.as_ref() { - // Nullable UDL dictionaries must declare a default value - // in WebIDL. - Type::Record(_) => Some(Literal::EmptyMap), - _ => None, - }, - _ => None, + if self.webidl_type().is_optional_record() { + // Nullable UDL dictionaries must declare a default value + // in WebIDL. + return Some(Literal::EmptyMap); } + None } } @@ -363,15 +379,12 @@ impl FieldExt for Field { } fn webidl_default_value(&self) -> Option { - match self.type_() { - Type::Optional(inner) => match inner.as_ref() { - // Nullable UDL dictionaries must declare a default value - // in WebIDL. - Type::Record(_) => Some(Literal::EmptyMap), - _ => None, - }, - _ => None, + if self.webidl_type().is_optional_record() { + // Nullable UDL dictionaries must declare a default value + // in WebIDL. + return Some(Literal::EmptyMap); } + return None; } } From a910d3cea8f6b5f3b397194f1e308dba83af1f42 Mon Sep 17 00:00:00 2001 From: Lina Cambridge Date: Fri, 2 Oct 2020 18:31:18 -0700 Subject: [PATCH 2/2] More Gecko compilation fixes to get Nimbus building. --- .../src/bindings/gecko_js/templates/InterfaceTemplate.cpp | 2 +- .../src/bindings/gecko_js/templates/RustBufferHelper.h | 3 +-- uniffi_bindgen/src/bindings/gecko_js/templates/macros.cpp | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp b/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp index 13fb646938..f1843a15ea 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp +++ b/uniffi_bindgen/src/bindings/gecko_js/templates/InterfaceTemplate.cpp @@ -51,7 +51,7 @@ already_AddRefed<{{ obj.name()|class_name_cpp(context) }}> {{ obj.name()|class_n if (err.mCode) { {%- match cons.cpp_throw_by() %} {%- when ThrowBy::ErrorResult with (rv) %} - {{ rv }}.ThrowOperationError(err.mMessage); + {{ rv }}.ThrowOperationError(nsDependentCString(err.mMessage)); {%- when ThrowBy::Assert %} MOZ_ASSERT(false); {%- endmatch %} diff --git a/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h b/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h index c805634b72..99c9bb4b8d 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h +++ b/uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h @@ -547,14 +547,13 @@ struct Serializable> { return false; } if (!hasValue) { - aValue = dom::Optional(); return true; } T value; if (!Serializable::ReadFrom(aReader, value)) { return false; } - aValue = dom::Optional(std::move(value)); + aValue.Construct(std::move(value)); return true; }; diff --git a/uniffi_bindgen/src/bindings/gecko_js/templates/macros.cpp b/uniffi_bindgen/src/bindings/gecko_js/templates/macros.cpp index d5af13864f..10765ede2a 100644 --- a/uniffi_bindgen/src/bindings/gecko_js/templates/macros.cpp +++ b/uniffi_bindgen/src/bindings/gecko_js/templates/macros.cpp @@ -40,7 +40,7 @@ {%- when ThrowBy::ErrorResult with (rv) %} {# /* TODO: Improve error throwing. See https://github.com/mozilla/uniffi-rs/issues/295 for details. */ -#} - {{ rv }}.ThrowOperationError({{ err }}.mMessage); + {{ rv }}.ThrowOperationError(nsDependentCString({{ err }}.mMessage)); {%- when ThrowBy::Assert %} MOZ_ASSERT(false); {%- endmatch %}