Bug 1471726 - Part 1: Correct codegen for XPIDL arrays of JSVals, r=mccr8
☠☠ backed out by 113e9ca0b5bc ☠ ☠
authorNika Layzell <nika@thelayzells.com>
Fri, 06 Jul 2018 16:47:22 -0400
changeset 428495 da79e75b9cb35daeb80fe000a6f0f9ce339aedac
parent 428494 c23440f7dd0ba9646b7f39456c62abb4d346b5e0
child 428496 40885fbf99c60efa95765d764d21c92de9846a6d
push id34337
push userncsoregi@mozilla.com
push dateThu, 26 Jul 2018 21:58:45 +0000
treeherdermozilla-central@8f2f847b2f9d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1471726
milestone63.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1471726 - Part 1: Correct codegen for XPIDL arrays of JSVals, r=mccr8
xpcom/idl-parser/xpidl/xpidl.py
xpcom/rust/xpcom/src/reexports.rs
xpcom/rust/xpcom/xpcom_macros/src/lib.rs
--- a/xpcom/idl-parser/xpidl/xpidl.py
+++ b/xpcom/idl-parser/xpidl/xpidl.py
@@ -15,17 +15,17 @@ from ply import yacc
 
 """A type conforms to the following pattern:
 
     def isScriptable(self):
         'returns True or False'
 
     def nativeType(self, calltype):
         'returns a string representation of the native type
-        calltype must be 'in', 'out', or 'inout'
+        calltype must be 'in', 'out', 'inout', or 'element'
 
 Interface members const/method/attribute conform to the following pattern:
 
     name = 'string'
 
     def toIDL(self):
         'returns the member signature as IDL'
 """
@@ -139,17 +139,17 @@ class Builtin(object):
             const = 'const '
         elif shared:
             if not self.isPointer():
                 raise IDLError("[shared] not applicable to non-pointer types.", self.location)
             const = 'const '
         else:
             const = ''
         return "%s%s %s" % (const, self.nativename,
-                            calltype != 'in' and '*' or '')
+                            '*' if 'out' in calltype else '')
 
     def rustType(self, calltype, shared=False, const=False):
         # We want to rewrite any *mut pointers to *const pointers if constness
         # was requested.
         const = const or (calltype == 'in' and self.isPointer()) or shared
         rustname = self.rustname
         if const and self.isPointer():
             rustname = self.rustname.replace("*mut", "*const")
@@ -395,18 +395,17 @@ class Typedef(object):
     def resolve(self, parent):
         parent.setName(self)
         self.realtype = parent.getName(self.type, self.location)
 
     def isScriptable(self):
         return self.realtype.isScriptable()
 
     def nativeType(self, calltype):
-        return "%s %s" % (self.name,
-                          calltype != 'in' and '*' or '')
+        return "%s %s" % (self.name, '*' if 'out' in calltype else '')
 
     def rustType(self, calltype):
         return "%s%s" % (calltype != 'in' and '*mut ' or '',
                          self.name)
 
     def __str__(self):
         return "typedef %s %s\n" % (self.type, self.name)
 
@@ -435,18 +434,17 @@ class Forward(object):
                     break
 
         parent.setName(self)
 
     def isScriptable(self):
         return True
 
     def nativeType(self, calltype):
-        return "%s %s" % (self.name,
-                          calltype != 'in' and '* *' or '*')
+        return "%s *%s" % (self.name, '*' if 'out' in calltype else '')
 
     def rustType(self, calltype):
         if rustBlacklistedForward(self.name):
             raise RustNoncompat("forward declaration %s is unsupported" % self.name)
         return "%s*const %s" % (calltype != 'in' and '*mut ' or '',
                                 self.name)
 
     def __str__(self):
@@ -454,39 +452,29 @@ class Forward(object):
 
 
 class Native(object):
     kind = 'native'
 
     modifier = None
     specialtype = None
 
+    # A tuple type here means that a custom value is used for each calltype:
+    #   (in, out/inout, array element) respectively.
+    # A `None` here means that the written type should be used as-is.
     specialtypes = {
         'nsid': None,
-        'domstring': 'nsAString',
-        'utf8string': 'nsACString',
-        'cstring': 'nsACString',
-        'astring': 'nsAString',
-        'jsval': 'JS::Value',
+        'domstring': ('const nsAString&', 'nsAString&', 'nsString'),
+        'utf8string': ('const nsACString&', 'nsACString&', 'nsCString'),
+        'cstring': ('const nsACString&', 'nsACString&', 'nsCString'),
+        'astring': ('const nsAString&', 'nsAString&', 'nsString'),
+        'jsval': ('JS::HandleValue', 'JS::MutableHandleValue', 'JS::Value'),
         'promise': '::mozilla::dom::Promise',
     }
 
-    # Mappings from C++ native name types to rust native names. Types which
-    # aren't listed here are incompatible with rust code.
-    rust_nativenames = {
-        'void': "libc::c_void",
-        'char': "u8",
-        'char16_t': "u16",
-        'nsID': "nsID",
-        'nsIID': "nsIID",
-        'nsCID': "nsCID",
-        'nsAString': "::nsstring::nsAString",
-        'nsACString': "::nsstring::nsACString",
-    }
-
     def __init__(self, name, nativename, attlist, location):
         self.name = name
         self.nativename = nativename
         self.location = location
 
         for name, value, aloc in attlist:
             if value is not None:
                 raise IDLError("Unexpected attribute value", aloc)
@@ -531,54 +519,67 @@ class Native(object):
         return self.modifier == 'ref'
 
     def nativeType(self, calltype, const=False, shared=False):
         if shared:
             if calltype != 'out':
                 raise IDLError("[shared] only applies to out parameters.")
             const = True
 
-        if self.specialtype not in [None, 'promise'] and calltype == 'in':
+        if isinstance(self.nativename, tuple):
+            if calltype == 'in':
+                return self.nativename[0] + ' '
+            elif 'out' in calltype:
+                return self.nativename[1] + ' '
+            else:
+                return self.nativename[2] + ' '
+
+        # 'in' nsid parameters should be made 'const'
+        if self.specialtype == 'nsid' and calltype == 'in':
             const = True
 
-        if self.specialtype == 'jsval':
-            if calltype == 'out' or calltype == 'inout':
-                return "JS::MutableHandleValue "
-            return "JS::HandleValue "
-
         if self.isRef(calltype):
-            m = '& '
-        elif self.isPtr(calltype):
-            m = '*' + ((self.modifier == 'ptr' and calltype != 'in') and '*' or '')
+            m = '& '  # [ref] is always passed with a single indirection
         else:
-            m = calltype != 'in' and '*' or ''
+            m = '* ' if 'out' in calltype else ''
+            if self.isPtr(calltype):
+                m += '* '
         return "%s%s %s" % (const and 'const ' or '', self.nativename, m)
 
     def rustType(self, calltype, const=False, shared=False):
-        if shared:
-            if calltype != 'out':
-                raise IDLError("[shared] only applies to out parameters.")
-            const = True
+        # For the most part, 'native' types don't make sense in rust, as they
+        # are native C++ types. However, we can support a few types here, as
+        # they're important.
+        #
+        # NOTE: This code doesn't try to perfectly match C++ constness, as
+        # constness doesn't affect ABI, and raw pointers are already unsafe.
 
-        if self.specialtype is not None and calltype == 'in':
-            const = True
+        if self.modifier not in ['ptr', 'ref']:
+            raise RustNoncompat('Rust only supports [ref] / [ptr] native types')
+
+        prefix = '*mut ' if 'out' in calltype else '*const '
+        if 'out' in calltype and self.modifier == 'ptr':
+            prefix += '*mut '
 
-        if self.nativename not in self.rust_nativenames:
-            raise RustNoncompat("native type %s is unsupported" % self.nativename)
-        name = self.rust_nativenames[self.nativename]
+        if self.specialtype == 'nsid':
+            return prefix + self.nativename
+        if self.specialtype in ['cstring', 'utf8string']:
+            if calltype == 'element':
+                return '::nsstring::nsCString'
+            return prefix + '::nsstring::nsACString'
+        if self.specialtype in ['astring', 'domstring']:
+            if calltype == 'element':
+                return '::nsstring::nsString'
+            return prefix + '::nsstring::nsAString'
+        if self.nativename == 'void':
+            return prefix + 'libc::c_void'
 
-        if self.isRef(calltype):
-            m = const and '&' or '&mut '
-        elif self.isPtr(calltype):
-            m = (const and '*const ' or '*mut ')
-            if self.modifier == 'ptr' and calltype != 'in':
-                m += '*mut '
-        else:
-            m = calltype != 'in' and '*mut ' or ''
-        return "%s%s" % (m, name)
+        if self.specialtype:
+            raise RustNoncompat("specialtype %s unsupported" % self.specialtype)
+        raise RustNoncompat("native type %s unsupported" % self.nativename)
 
     def __str__(self):
         return "native %s(%s)\n" % (self.name, self.nativename)
 
 
 class WebIDL(object):
     kind = 'webidl'
 
@@ -608,21 +609,21 @@ class WebIDL(object):
             self.headerFile = self.native.replace('::', '/') + '.h'
 
         parent.setName(self)
 
     def isScriptable(self):
         return True  # All DOM objects are script exposed.
 
     def nativeType(self, calltype):
-        return "%s %s" % (self.native, calltype != 'in' and '* *' or '*')
+        return "%s *%s" % (self.native, '*' if 'out' in calltype else '')
 
     def rustType(self, calltype):
         # Just expose the type as a void* - we can't do any better.
-        return "%s*const libc::c_void" % (calltype != 'in' and '*mut ' or '')
+        return "%s*const libc::c_void" % ('*mut ' if 'out' in calltype else '')
 
     def __str__(self):
         return "webidl %s\n" % self.name
 
 
 class Interface(object):
     kind = 'interface'
 
@@ -708,22 +709,21 @@ class Interface(object):
 
     def isScriptable(self):
         # NOTE: this is not whether *this* interface is scriptable... it's
         # whether, when used as a type, it's scriptable, which is true of all
         # interfaces.
         return True
 
     def nativeType(self, calltype, const=False):
-        return "%s%s %s" % (const and 'const ' or '',
-                            self.name,
-                            calltype != 'in' and '* *' or '*')
+        return "%s%s *%s" % ('const ' if const else '', self.name,
+                             '*' if 'out' in calltype else '')
 
-    def rustType(self, calltype):
-        return "%s*const %s" % (calltype != 'in' and '*mut ' or '',
+    def rustType(self, calltype, const=False):
+        return "%s*const %s" % ('*mut ' if 'out' in calltype else '',
                                 self.name)
 
     def __str__(self):
         l = ["interface %s\n" % self.name]
         if self.base is not None:
             l.append("\tbase %s\n" % self.base)
         l.append(str(self.attributes))
         if self.members is None:
@@ -1214,22 +1214,29 @@ class Param(object):
 class Array(object):
     def __init__(self, basetype):
         self.type = basetype
 
     def isScriptable(self):
         return self.type.isScriptable()
 
     def nativeType(self, calltype, const=False):
-        return "%s%s*" % (const and 'const ' or '',
-                          self.type.nativeType(calltype))
+        # For legacy reasons, we have to add a 'const ' to builtin pointer array
+        # types. (`[array] in string` and `[array] in wstring` parameters)
+        if calltype == 'in' and isinstance(self.type, Builtin) and self.type.isPointer():
+            const = True
+
+        return "%s%s*%s" % ('const ' if const else '',
+                            self.type.nativeType('element'),
+                            '*' if 'out' in calltype else '')
 
     def rustType(self, calltype, const=False):
-        return "%s %s" % (const and '*const' or '*mut',
-                          self.type.rustType(calltype))
+        return "%s%s%s" % ('*mut ' if 'out' in calltype else '',
+                           '*const ' if const else '*mut ',
+                           self.type.rustType('element'))
 
 
 class IDLParser(object):
     keywords = {
         'const': 'CONST',
         'interface': 'INTERFACE',
         'in': 'IN',
         'inout': 'INOUT',
--- a/xpcom/rust/xpcom/src/reexports.rs
+++ b/xpcom/rust/xpcom/src/reexports.rs
@@ -6,13 +6,13 @@
 /// which are defined in other libraries which `xpcom` depends on, but which may
 /// not be `extern crate`-ed into the crate the macros are expanded into. This
 /// module re-exports those types from `xpcom` so that they can be used from the
 /// macro.
 
 // re-export libc so it can be used by the procedural macro.
 pub extern crate libc;
 
-pub use nsstring::{nsACString, nsAString};
+pub use nsstring::{nsACString, nsAString, nsCString, nsString};
 
 pub use nserror::{nsresult, NsresultExt, NS_ERROR_NO_INTERFACE, NS_OK};
 
 pub use std::ops::Deref;
--- a/xpcom/rust/xpcom/xpcom_macros/src/lib.rs
+++ b/xpcom/rust/xpcom/xpcom_macros/src/lib.rs
@@ -558,17 +558,19 @@ fn xpcom(init: DeriveInput) -> Result<To
         impl #name {
             /// This method is used for
             fn allocate(__init: #name_init) -> ::xpcom::RefPtr<Self> {
                 #[allow(unused_imports)]
                 use ::xpcom::*;
                 #[allow(unused_imports)]
                 use ::xpcom::interfaces::*;
                 #[allow(unused_imports)]
-                use ::xpcom::reexports::{libc, nsACString, nsAString, nsresult};
+                use ::xpcom::reexports::{
+                    libc, nsACString, nsAString, nsCString, nsString, nsresult
+                };
 
                 unsafe {
                     // NOTE: This is split into multiple lines to make the
                     // output more readable.
                     let value = #name {
                         #(#vtables)*
                         __refcnt: #refcnt_ty::new(),
                         #(#inits)*