Last Comment Bug 763724 - The WebIDL parser should detect interfaces that inherit from or implement themselves
: The WebIDL parser should detect interfaces that inherit from or implement the...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: ParisBindings
  Show dependency treegraph
 
Reported: 2012-06-11 15:13 PDT by Boris Zbarsky [:bz] (still a bit busy)
Modified: 2012-06-22 03:47 PDT (History)
3 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Detect cycles in the interface inheritance and implementation graph. (8.79 KB, patch)
2012-06-13 14:51 PDT, Boris Zbarsky [:bz] (still a bit busy)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] (still a bit busy) 2012-06-11 15:13:02 PDT
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 [:bz] (still a bit busy) 2012-06-13 14:51:08 PDT
Created attachment 632893 [details] [diff] [review]
Detect cycles in the interface inheritance and implementation graph.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-06-19 12:17:28 PDT
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) 2012-06-20 11:49:28 PDT
>+        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 [:bz] (still a bit busy) 2012-06-20 11:51:58 PDT
> 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 [:bz] (still a bit busy) 2012-06-20 21:57:17 PDT
> 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) 2012-06-20 21:59:25 PDT
> Actually, no, since I use cycleInGraph.location in the error message.

Ah, right!  Sounds like a plan.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-06-21 09:30:02 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/5de10f87c56e
Comment 8 Ed Morley [:emorley] 2012-06-22 03:47:02 PDT
https://hg.mozilla.org/mozilla-central/rev/5de10f87c56e

Note You need to log in before you can comment on or make changes to this bug.