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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Instead of just assuming two different interfaces are always distinguishable.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: isDistinguishableFrom should correctly check whether two interfaces can be implemented on the same object.

Comment 3

[Justin Lebar (not reading bugmail)]

Where is test_distinguishability coming from?  It's not in my inbound tree.

Comment 4

[Justin Lebar (not reading bugmail)]

> { 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 5

[Justin Lebar (not reading bugmail)]

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.

Comment 6

[Justin Lebar (not reading bugmail)]

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

Wrong.  :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[Justin Lebar (not reading bugmail)]

I like setIsConsequentialInterfaceOf more than setConsequentialOf, and have no preference between setIsConsequentialInterfaceOf and set{Is,}ConsequentialTo.

Comment 9

[Boris Zbarsky [:bzbarsky]]

setIsConsequentialInterfaceOf it is.

Comment 10

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a37c6fb970a4

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a37c6fb970a4