The default bug view has changed. See this FAQ.

isDistinguishableFrom should check whether two interfaces can be implemented by the same object

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Instead of just assuming two different interfaces are always distinguishable.
Implementation plan:

On each IDLInterface C, store the following set of interfaces:

  { C, interfaces C is ancestor of, interfaces C is consequential interface of }

then A and B are distinguishable (ignoring typed array stuff and callbacks) if and only if A.getThisSet() intersect B.getThisSet() is empty.
Created attachment 633224 [details] [diff] [review]
isDistinguishableFrom should correctly check whether two interfaces can be implemented on the same object.
Attachment #633224 - Flags: review?(khuey)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Attachment #633224 - Flags: review?(khuey) → review?(justin.lebar+bug)
Where is test_distinguishability coming from?  It's not in my inbound tree.
> { C, interfaces C is ancestor of, interfaces C is consequential interface of }

This is the same as

  { C, interfaces C is a consequential interface of },

because if C is an ancestor of X, then C is a consequential interface of X, right?  So this is really just the converse (I don't like "inverse" here) of the consequential relation (union {C}, anyway).
Comment on attachment 633224 [details] [diff] [review]
isDistinguishableFrom should correctly check whether two interfaces can be implemented on the same object.

>diff --git a/dom/bindings/parser/WebIDL.py b/dom/bindings/parser/WebIDL.py
>--- a/dom/bindings/parser/WebIDL.py
>+++ b/dom/bindings/parser/WebIDL.py
>@@ -367,16 +367,17 @@ class IDLInterface(IDLObjectWithScope):
>         assert not parent or isinstance(parent, IDLIdentifierPlaceholder)
> 
>         self.parent = parent
>         self._callback = False
>         self._finished = False
>         self.members = list(members) # clone the list
>         self.implementedInterfaces = set()
>         self._consequential = False
>+        self.interfacesImplyingSelf = set([self])

I struggled with this for a while, and I think a simple explanation for this field is, the field contains the interfaces that |self| is a part of.  I think it's worth documenting that.  I also think the name could be improved, too; maybe interfacesBasedOnSelf?

>@@ -508,18 +529,19 @@ class IDLInterface(IDLObjectWithScope):
>             specialMembersSeen.add(memberType)
> 
>     def isInterface(self):
>         return True
> 
>     def isExternal(self):
>         return False
> 
>-    def setConsequential(self):
>+    def setConsequential(self, other):
>         self._consequential = True
>+        self.interfacesImplyingSelf.add(other)

Maybe now |setConsequentialTo|?

>+            if other.isSpiderMonkeyInterface():
>+                # Just let |other| handle things
>+                return other.isDistinguishableFrom(self)
>+            assert self.isGeckoInterface() and other.isGeckoInterface()
>+            if self.inner.isExternal() or other.unroll().inner.isExternal():
>+                return self != other
>+            return (len(self.inner.interfacesImplyingSelf &
>+                        other.unroll().inner.interfacesImplyingSelf) == 0 and

So two interfaces A and B are distinguishable if no object implements or inherits-from both A and B.

This creates really tight coupling -- my union or override can break if some bozo implements an object in some completely irrelevant spec.  But I guess we have to do this because we have no concept of an explicit cast.
Attachment #633224 - Flags: review?(justin.lebar+bug) → review+
> because if C is an ancestor of X, then C is a consequential interface of X, right?

Wrong.  :)
> Where is test_distinguishability coming from? 

Bug 764698.

> because if C is an ancestor of X, then C is a consequential interface of X, right?

As you discovered, wrong.

> I also think the name could be improved, too; maybe interfacesBasedOnSelf?

Will do.  And will document.

> Maybe now |setConsequentialTo|?

Hmm.  Or setConsequentialOf?  Preference?  Or even setIsConsequentialInterfaceOf?

Fully agreed with the last two paragraphs of comment 5.
I like setIsConsequentialInterfaceOf more than setConsequentialOf, and have no preference between setIsConsequentialInterfaceOf and set{Is,}ConsequentialTo.
setIsConsequentialInterfaceOf it is.
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37c6fb970a4
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/a37c6fb970a4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.