Bug 768537 part 1. Update parser support for dictionaries to spec changes. r=jlebar
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 17 Jul 2012 12:18:53 -0400
changeset 105024 0cad1e553bead63f29919ee9220542cb4e6debd5
parent 105023 cf567772a2f7c1174ed469a01c304cf83e0aa61c
child 105025 ea5a243a60f1fc3127527f40af50ea3e8d4736d1
push idunknown
push userunknown
push dateunknown
reviewersjlebar
bugs768537
milestone17.0a1
Bug 768537 part 1. Update parser support for dictionaries to spec changes. r=jlebar There are several parts here: 1) Enforce the requirement that dictionary arguments not followed by a required argument are optional. 2) Make dictionaries no longer be distinguishable from nullable types. 3) Disallow dictionaries or unions containing dictionaries inside a nullable type. 4) Force optional dictionaries to have a default value of null so that codegen doesn't have to worry about dealing with optional arguments that have no default value in the IDL but need to be treated as if they were null.
content/base/src/nsXMLHttpRequest.h
dom/bindings/parser/WebIDL.py
dom/bindings/parser/tests/test_dictionary.py
dom/bindings/parser/tests/test_distinguishability.py
dom/bindings/test/TestBindingHeader.h
dom/bindings/test/TestCodeGen.webidl
dom/webidl/XMLHttpRequest.webidl
dom/workers/XMLHttpRequest.cpp
dom/workers/XMLHttpRequest.h
--- a/content/base/src/nsXMLHttpRequest.h
+++ b/content/base/src/nsXMLHttpRequest.h
@@ -176,32 +176,29 @@ public:
   {
     return GetOwner();
   }
 
   // The WebIDL constructor.
   static already_AddRefed<nsXMLHttpRequest>
   Constructor(JSContext* aCx,
               nsISupports* aGlobal,
-              const mozilla::dom::Nullable<mozilla::dom::MozXMLHttpRequestParameters>& aParams,
+              const mozilla::dom::MozXMLHttpRequestParameters& aParams,
               ErrorResult& aRv)
   {
     nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aGlobal);
     nsCOMPtr<nsIScriptObjectPrincipal> principal = do_QueryInterface(aGlobal);
     if (!window || ! principal) {
       aRv.Throw(NS_ERROR_FAILURE);
       return NULL;
     }
 
     nsRefPtr<nsXMLHttpRequest> req = new nsXMLHttpRequest();
     req->Construct(principal->GetPrincipal(), window);
-    if (!aParams.IsNull()) {
-      const mozilla::dom::MozXMLHttpRequestParameters& params = aParams.Value();
-      req->InitParameters(params.mozAnon, params.mozSystem);
-    }
+    req->InitParameters(aParams.mozAnon, aParams.mozSystem);
     return req.forget();
   }
 
   void Construct(nsIPrincipal* aPrincipal,
                  nsPIDOMWindow* aOwnerWindow,
                  nsIURI* aBaseURI = NULL)
   {
     MOZ_ASSERT(aPrincipal);
--- a/dom/bindings/parser/WebIDL.py
+++ b/dom/bindings/parser/WebIDL.py
@@ -702,21 +702,19 @@ class IDLDictionary(IDLObjectWithScope):
                                   [oldParent.location, self.parent.location])
 
             # Make sure the parent resolves all its members before we start
             # looking at them.
             self.parent.finish(scope)
 
         for member in self.members:
             member.resolve(self)
-            if not member.type.isComplete():
-                type = member.type.complete(scope)
-                assert not isinstance(type, IDLUnresolvedType)
-                assert not isinstance(type.name, IDLUnresolvedIdentifier)
-                member.type = type
+            if not member.isComplete():
+                member.complete(scope)
+                assert member.type.isComplete()
 
         # Members of a dictionary are sorted in lexicographic order
         self.members.sort(cmp=cmp, key=lambda x: x.identifier.name)
 
         inheritedMembers = []
         ancestor = self.parent
         while ancestor:
             if ancestor == self:
@@ -1014,28 +1012,42 @@ class IDLNullableType(IDLType):
         assert isinstance(parentScope, IDLScope)
         self.inner.resolveType(parentScope)
 
     def isComplete(self):
         return self.inner.isComplete()
 
     def complete(self, scope):
         self.inner = self.inner.complete(scope)
-        if self.inner.isUnion() and self.inner.hasNullableType:
+        if self.inner.isUnion():
+            if self.inner.hasNullableType:
+                raise WebIDLError("The inner type of a nullable type must not "
+                                  "be a union type that itself has a nullable "
+                                  "type as a member type", [self.location])
+            # Check for dictionaries in the union
+            for memberType in self.inner.flatMemberTypes:
+                if memberType.isDictionary():
+                    raise WebIDLError("The inner type of a nullable type must "
+                                      "not be a union type containing a "
+                                      "dictionary type",
+                                      [self.location, memberType.location])
+                    
+        if self.inner.isDictionary():
             raise WebIDLError("The inner type of a nullable type must not be a "
-                              "union type that itself has a nullable type as a "
-                              "member type", [self.location])
+                              "dictionary type", [self.location])
+
         self.name = self.inner.name
         return self
 
     def unroll(self):
         return self.inner.unroll()
 
     def isDistinguishableFrom(self, other):
-        if other.nullable() or (other.isUnion() and other.hasNullableType):
+        if (other.nullable() or (other.isUnion() and other.hasNullableType) or
+            other.isDictionary()):
             # Can't tell which type null should become
             return False
         return self.inner.isDistinguishableFrom(other)
 
 class IDLSequenceType(IDLType):
     def __init__(self, location, parameterType):
         assert not parameterType.isVoid()
 
@@ -1400,18 +1412,19 @@ class IDLWrapperType(IDLType):
         if self.isEnum():
             return (other.isInterface() or other.isObject() or
                     other.isCallback() or other.isDictionary() or
                     other.isSequence() or other.isArray() or
                     other.isDate())
         if other.isPrimitive() or other.isString() or other.isEnum() or other.isDate():
             return True
         if self.isDictionary():
-            return (other.isNonCallbackInterface() or other.isSequence() or
-                    other.isArray())
+            return (not other.nullable() and
+                    (other.isNonCallbackInterface() or other.isSequence() or
+                     other.isArray()))
 
         assert self.isInterface()
         # XXXbz need to check that the interfaces can't be implemented
         # by the same object
         if other.isInterface():
             if other.isSpiderMonkeyInterface():
                 # Just let |other| handle things
                 return other.isDistinguishableFrom(self)
@@ -1721,17 +1734,19 @@ class IDLValue(IDLObject):
 
 class IDLNullValue(IDLObject):
     def __init__(self, location):
         IDLObject.__init__(self, location)
         self.type = None
         self.value = None
 
     def coerceToType(self, type, location):
-        if not isinstance(type, IDLNullableType) and not (type.isUnion() and type.hasNullableType):
+        if (not isinstance(type, IDLNullableType) and
+            not (type.isUnion() and type.hasNullableType) and
+            not type.isDictionary()):
             raise WebIDLError("Cannot coerce null value to type %s." % type,
                               [location])
 
         nullValue = IDLNullValue(self.location)
         nullValue.type = type
         return nullValue
         
 
@@ -1857,40 +1872,64 @@ class IDLAttribute(IDLInterfaceMember):
 
 class IDLArgument(IDLObjectWithIdentifier):
     def __init__(self, location, identifier, type, optional=False, defaultValue=None, variadic=False, dictionaryMember=False):
         IDLObjectWithIdentifier.__init__(self, location, None, identifier)
 
         assert isinstance(type, IDLType)
         self.type = type
 
-        if defaultValue:
-            defaultValue = defaultValue.coerceToType(type, location)
-            assert defaultValue
-
         self.optional = optional
         self.defaultValue = defaultValue
         self.variadic = variadic
         self.dictionaryMember = dictionaryMember
+        self._isComplete = False
 
         assert not variadic or optional
 
     def addExtendedAttributes(self, attrs):
         if self.dictionaryMember:
             for (attr, value) in attrs:
                 if attr == "TreatUndefinedAs":
                     raise WebIDLError("[TreatUndefinedAs] is not allowed for "
                                       "dictionary members", [self.location])
                 elif attr == "TreatNullAs":
                     raise WebIDLError("[TreatNullAs] is not allowed for "
                                       "dictionary members", [self.location])
 
         # But actually, we can't handle this at all, so far.
         assert len(attrs) == 0
 
+    def isComplete(self):
+        return self._isComplete
+
+    def complete(self, scope):
+        if self._isComplete:
+            return
+
+        self._isComplete = True
+
+        if not self.type.isComplete():
+            type = self.type.complete(scope)
+            assert not isinstance(type, IDLUnresolvedType)
+            assert not isinstance(type.name, IDLUnresolvedIdentifier)
+            self.type = type
+
+        if self.type.isDictionary() and self.optional and not self.defaultValue:
+            # Default optional dictionaries to null, for simplicity,
+            # so the codegen doesn't have to special-case this.
+            self.defaultValue = IDLNullValue(self.location)
+
+        # Now do the coercing thing; this needs to happen after the
+        # above creation of a default value.
+        if self.defaultValue:
+            self.defaultValue = self.defaultValue.coerceToType(self.type,
+                                                               self.location)
+            assert self.defaultValue
+
 class IDLCallbackType(IDLType, IDLObjectWithScope):
     def __init__(self, location, parentScope, identifier, returnType, arguments):
         assert isinstance(returnType, IDLType)
 
         IDLType.__init__(self, location, identifier.name)
 
         self._returnType = returnType
         # Clone the list
@@ -2033,44 +2072,16 @@ class IDLMethod(IDLInterfaceMember, IDLS
             assert not arguments[1].optional and not arguments[1].variadic
 
         if self._stringifier:
             assert len(self._overloads) == 1
             overload = self._overloads[0]
             assert len(overload.arguments) == 0
             assert overload.returnType == BuiltinTypes[IDLBuiltinType.Types.domstring]
 
-        inOptionalArguments = False
-        variadicArgument = None
-        sawOptionalWithNoDefault = False
-
-        assert len(self._overloads) == 1
-        arguments = self._overloads[0].arguments
-
-        for argument in arguments:
-            # Only the last argument can be variadic
-            if variadicArgument:
-                raise WebIDLError("Variadic argument is not last argument",
-                                  [variadicArgument.location])
-            # Once we see an optional argument, there can't be any non-optional
-            # arguments.
-            if inOptionalArguments and not argument.optional:
-                raise WebIDLError("Non-optional argument after optional arguments",
-                                  [argument.location])
-            # Once we see an argument with no default value, there can
-            # be no more default values.
-            if sawOptionalWithNoDefault and argument.defaultValue:
-                raise WebIDLError("Argument with default value after optional "
-                                  "arguments with no default values",
-                                  [argument.location])
-            inOptionalArguments = argument.optional
-            if argument.variadic:
-                variadicArgument = argument
-            sawOptionalWithNoDefault = argument.optional and not argument.defaultValue
-
     def isStatic(self):
         return self._static
 
     def isGetter(self):
         return self._getter
 
     def isSetter(self):
         return self._setter
@@ -2143,25 +2154,59 @@ class IDLMethod(IDLInterfaceMember, IDLS
         return self
 
     def signatures(self):
         return [(overload.returnType, overload.arguments) for overload in
                 self._overloads]
 
     def finish(self, scope):
         for overload in self._overloads:
-            for argument in overload.arguments:
-                if argument.type.isComplete():
+            inOptionalArguments = False
+            variadicArgument = None
+            sawOptionalWithNoDefault = False
+
+            arguments = overload.arguments
+            for (idx, argument) in enumerate(arguments):
+                if argument.isComplete():
                     continue
 
-                type = argument.type.complete(scope)
-
-                assert not isinstance(type, IDLUnresolvedType)
-                assert not isinstance(type.name, IDLUnresolvedIdentifier)
-                argument.type = type
+                argument.complete(scope)
+                assert argument.type.isComplete()
+
+                if argument.type.isDictionary():
+                    # Dictionaries at the end of the list or followed by
+                    # optional arguments must be optional.
+                    if (not argument.optional and
+                        (idx == len(arguments) - 1 or arguments[idx+1].optional)):
+                        raise WebIDLError("Dictionary argument not followed by "
+                                          "a required argument must be "
+                                          "optional", [argument.location])
+
+                # Only the last argument can be variadic
+                if variadicArgument:
+                    raise WebIDLError("Variadic argument is not last argument",
+                                      [variadicArgument.location])
+                # Once we see an optional argument, there can't be any non-optional
+                # arguments.
+                if inOptionalArguments and not argument.optional:
+                    raise WebIDLError("Non-optional argument after optional "
+                                      "arguments",
+                                      [argument.location])
+                # Once we see an argument with no default value, there can
+                # be no more default values.
+                if sawOptionalWithNoDefault and argument.defaultValue:
+                    raise WebIDLError("Argument with default value after "
+                                      "optional arguments with no default "
+                                      "values",
+                                      [argument.location])
+                inOptionalArguments = argument.optional
+                if argument.variadic:
+                    variadicArgument = argument
+                sawOptionalWithNoDefault = (argument.optional and
+                                            not argument.defaultValue)
 
             returnType = overload.returnType
             if returnType.isComplete():
                 continue
 
             type = returnType.complete(scope)
 
             assert not isinstance(type, IDLUnresolvedType)
--- a/dom/bindings/parser/tests/test_dictionary.py
+++ b/dom/bindings/parser/tests/test_dictionary.py
@@ -116,8 +116,83 @@ def WebIDLTest(parser, harness):
               [TreatUndefinedAs=EmptyString] DOMString foo;
             };
         """)
         results = parser.finish()
     except:
         threw = True
 
     harness.ok(threw, "Should not allow [TreatUndefinedAs] on dictionary members");
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            dictionary A {
+            };
+            interface X {
+              void doFoo(A arg);
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Trailing dictionary arg must be optional")
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            dictionary A {
+            };
+            interface X {
+              void doFoo(A arg1, optional long arg2);
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Dictionary arg followed by optional arg must be optional")
+
+    parser = parser.reset()
+    parser.parse("""
+            dictionary A {
+            };
+            interface X {
+              void doFoo(A arg1, long arg2);
+            };
+        """)
+    results = parser.finish()
+    harness.ok(True, "Dictionary arg followed by required arg can be required")
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            dictionary A {
+            };
+            interface X {
+              void doFoo(optional A? arg1);
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Dictionary arg must not be nullable")
+
+    parser = parser.reset()
+    threw = False
+    try:
+        parser.parse("""
+            dictionary A {
+            };
+            interface X {
+              void doFoo((A or long)? arg1);
+            };
+        """)
+        results = parser.finish()
+    except:
+        threw = True
+
+    harness.ok(threw, "Dictionary arg must not be in a nullable union")
--- a/dom/bindings/parser/tests/test_distinguishability.py
+++ b/dom/bindings/parser/tests/test_distinguishability.py
@@ -4,17 +4,17 @@ def firstArgType(method):
 def WebIDLTest(parser, harness):
     parser.parse("""
       dictionary Dict {
       };
       callback interface Foo {
       };
       interface Bar {
         // Bit of a pain to get things that have dictionary types
-        void passDict(Dict arg);
+        void passDict(optional Dict arg);
         void passFoo(Foo arg);
         void passNullableUnion((object? or DOMString) arg);
         void passNullable(Foo? arg);
       };
     """)
     results = parser.finish()
 
     iface = results[2]
--- a/dom/bindings/test/TestBindingHeader.h
+++ b/dom/bindings/test/TestBindingHeader.h
@@ -341,19 +341,16 @@ public:
   void MethodRenamedTo(ErrorResult&);
   void MethodRenamedTo(int8_t, ErrorResult&);
   int8_t GetAttributeGetterRenamedTo(ErrorResult&);
   int8_t GetAttributeRenamedTo(ErrorResult&);
   void SetAttributeRenamedTo(int8_t, ErrorResult&);
 
   // Dictionary tests
   void PassDictionary(const Dict&, ErrorResult&);
-  void PassOptionalDictionary(const Optional<Dict>&, ErrorResult&);
-  void PassNullableDictionary(const Nullable<Dict>&, ErrorResult&);
-  void PassOptionalNullableDictionary(const Optional<Nullable<Dict> >&, ErrorResult&);
   void PassOtherDictionary(const GrandparentDict&, ErrorResult&);
   void PassSequenceOfDictionaries(const Sequence<Dict>&, ErrorResult&);
 
   // Methods and properties imported via "implements"
   bool GetImplementedProperty(ErrorResult&);
   void SetImplementedProperty(bool, ErrorResult&);
   void ImplementedMethod(ErrorResult&);
   bool GetImplementedParentProperty(ErrorResult&);
--- a/dom/bindings/test/TestCodeGen.webidl
+++ b/dom/bindings/test/TestCodeGen.webidl
@@ -256,21 +256,18 @@ interface TestInterface {
   //void passUnionWithDict((Dict or long) arg);
 
   // binaryNames tests
   void methodRenamedFrom();
   void methodRenamedFrom(byte argument);
   readonly attribute byte attributeGetterRenamedFrom;
   attribute byte attributeRenamedFrom;
 
-  void passDictionary(Dict x);
-  void passOptionalDictionary(optional Dict x);
-  void passNullableDictionary(Dict? x);
-  void passOptionalNullableDictionary(optional Dict? x);
-  void passOtherDictionary(GrandparentDict x);
+  void passDictionary(optional Dict x);
+  void passOtherDictionary(optional GrandparentDict x);
   void passSequenceOfDictionaries(sequence<Dict> x);
 };
 
 interface TestNonWrapperCacheInterface {
 };
 
 interface ImplementedInterfaceParent {
   void implementedParentMethod();
--- a/dom/webidl/XMLHttpRequest.webidl
+++ b/dom/webidl/XMLHttpRequest.webidl
@@ -46,17 +46,17 @@ dictionary MozXMLHttpRequestParameters
   boolean mozAnon = false;
 
   /**
    * If true, the same origin policy will not be enforced on the request.
    */
   boolean mozSystem = false;
 };
 
-[Constructor(optional MozXMLHttpRequestParameters? params = null)]
+[Constructor(optional MozXMLHttpRequestParameters params)]
 interface XMLHttpRequest : XMLHttpRequestEventTarget {
   // event handler
   [TreatNonCallableAsNull, GetterInfallible=MainThread]
   attribute Function? onreadystatechange;
 
   // states
   const unsigned short UNSENT = 0;
   const unsigned short OPENED = 1;
--- a/dom/workers/XMLHttpRequest.cpp
+++ b/dom/workers/XMLHttpRequest.cpp
@@ -1461,17 +1461,17 @@ XMLHttpRequest::_finalize(JSFreeOp* aFop
   ReleaseProxy(XHRIsGoingAway);
   XMLHttpRequestEventTarget::_finalize(aFop);
 }
 
 // static
 XMLHttpRequest*
 XMLHttpRequest::Constructor(JSContext* aCx,
                             JSObject* aGlobal,
-                            const Nullable<MozXMLHttpRequestParametersWorkers>& aParams,
+                            const MozXMLHttpRequestParametersWorkers& aParams,
                             ErrorResult& aRv)
 {
   WorkerPrivate* workerPrivate = GetWorkerPrivateFromContext(aCx);
   MOZ_ASSERT(workerPrivate);
 
   nsRefPtr<XMLHttpRequest> xhr = new XMLHttpRequest(aCx, workerPrivate);
 
   if (!Wrap(aCx, aGlobal, xhr)) {
--- a/dom/workers/XMLHttpRequest.h
+++ b/dom/workers/XMLHttpRequest.h
@@ -67,17 +67,17 @@ public:
   virtual void
   _trace(JSTracer* aTrc) MOZ_OVERRIDE;
 
   virtual void
   _finalize(JSFreeOp* aFop) MOZ_OVERRIDE;
 
   static XMLHttpRequest*
   Constructor(JSContext* aCx, JSObject* aGlobal,
-              const Nullable<MozXMLHttpRequestParametersWorkers>& aParams,
+              const MozXMLHttpRequestParametersWorkers& aParams,
               ErrorResult& aRv);
   void
   Unpin();
 
   bool
   Notify(JSContext* aCx, Status aStatus) MOZ_OVERRIDE;
 
 #define IMPL_GETTER_AND_SETTER(_type)                                          \