Merge pull request #314 from mozilla/nimbus-in-firefox

Special-case "optionals with a default value" and fix compilation errors in Gecko JS, r=@eoger

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<DOMString, {}>", 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<nsString, {}>", 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),

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 %}

uniffi_bindgen/src/bindings/gecko_js/templates/RustBufferHelper.h

@@ -547,14 +547,13 @@ struct Serializable<dom::Optional<T>> {
       return false;
     }
     if (!hasValue) {
-      aValue = dom::Optional<T>();
       return true;
     }
     T value;
     if (!Serializable<T>::ReadFrom(aReader, value)) {
       return false;
     }
-    aValue = dom::Optional<T>(std::move(value));
+    aValue.Construct(std::move(value));
     return true;
   };
 
@@ -737,4 +736,45 @@ struct ViaFfi<nsAString, {{ context.ffi_rustbuffer_type() }}, true> {
   }
 };
 
+/// 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<T>` in Rust.
+
+template <typename T>
+struct ViaFfi<T, {{ context.ffi_rustbuffer_type() }}, true> {
+  [[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<T>::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<T>::WriteInto(writer, aLifted);
+    return writer.Buffer();
+  }
+};
+
 }  // namespace {{ context.detail_name() }}

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 %}

uniffi_bindgen/src/bindings/gecko_js/webidl.rs

@@ -50,6 +50,13 @@ pub enum WebIDLType {
     Nullable(Box<WebIDLType>),
     Optional(Box<WebIDLType>),
 
+    /// 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<T>`. However, it must be serialized as if
+    /// it's an `Optional<T>`, since that's what the Rust side of the FFI
+    /// expects.
+    OptionalWithDefaultValue(Box<WebIDLType>),
+
     /// Sequences are the same as their UniFFI counterparts.
     Sequence(Box<WebIDLType>),
 
@@ -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<Type> for WebIDLType {
                 panic!("[TODO: From<Type>({:?})]", 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,25 +332,19 @@ 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
     }
 
     fn webidl_default_value(&self) -> Option<Literal> {
         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<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);
         }
+        return None;
     }
 }