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

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Right now I think we go into an infinite loop or something.  We should throw.  "implements" makes this a bit of a pain, possibly.
Created attachment 632893 [details] [diff] [review]
Detect cycles in the interface inheritance and implementation graph.
Attachment #632893 - Flags: review?(khuey)
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.  :(
Attachment #632893 - Flags: review?(khuey) → review?(justin.lebar+bug)
>+        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 |==|?
Attachment #632893 - Flags: review?(justin.lebar+bug) → review+
> 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 ==.
> I can do that.

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

I renamed to findInterfaceLoopPoint and added some docs.
> Actually, no, since I use cycleInGraph.location in the error message.

Ah, right!  Sounds like a plan.
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de10f87c56e
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/5de10f87c56e
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.