763724 - The WebIDL parser should detect interfaces that inherit from or implement themselves

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Right now I think we go into an infinite loop or something.  We should throw.  "implements" makes this a bit of a pain, possibly.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Detect cycles in the interface inheritance and implementation graph.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 632893 [details] [diff] [review]
Detect cycles in the interface inheritance and implementation graph.

It looks like my distinguishability-checking move depends on this.  :(

Comment 3

[Justin Lebar (not reading bugmail)]

>+        cycleInGraph = self.inheritsFromOrImplements(self)
>+        if cycleInGraph:
>+            raise WebIDLError("Interface %s has itself as ancestor or "
>+                              "implemented interface" % self.identifier.name,
>+                              self.location,
>+                              extraLocation=cycleInGraph.location)

I'd prefer |if self.inheritsFromOrImplements(self)|, but either way is fine.

>@@ -603,16 +610,31 @@ class IDLInterface(IDLObjectWithScope):
> 
>         # And now collect up the consequential interfaces of all of those
>         temp = set()
>         for iface in consequentialInterfaces:
>             temp |= iface.getConsequentialInterfaces()
> 
>         return consequentialInterfaces | temp
> 
>+    def inheritsFromOrImplements(self, otherInterface):

Based on its name, it seems to me that this should return a bool.  Perhaps we should rename this method, or at least document the behavior.

>+        if self.parent:
>+            if self.parent is otherInterface:
>+                return self

Why |is| and not |==|?

Comment 4

[Boris Zbarsky [:bzbarsky]]

> I'd prefer |if self.inheritsFromOrImplements(self)|

I can do that.

> Perhaps we should rename this method, or at least document the behavior.

Will try to find a better name, for sure.

> Why |is| and not |==|?

No reason.  Can do ==.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I can do that.

Actually, no, since I use cycleInGraph.location in the error message.

I renamed to findInterfaceLoopPoint and added some docs.

Comment 6

[Justin Lebar (not reading bugmail)]

> Actually, no, since I use cycleInGraph.location in the error message.

Ah, right!  Sounds like a plan.

Comment 7

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5de10f87c56e

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5de10f87c56e