Last Comment Bug 742152 - Consider moving distinguishing index determination and verification of type-identity of preceding arguments into the WebIDL data model
: Consider moving distinguishing index determination and verification of type-i...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 740069
Blocks: ParisBindings 767352
  Show dependency treegraph
 
Reported: 2012-04-03 19:48 PDT by Boris Zbarsky [:bz]
Modified: 2012-06-23 05:45 PDT (History)
3 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Move distinguishing index determination and verification of type-identity of preceding arguments out of codegen and into the WebIDL parser. (26.18 KB, patch)
2012-06-14 21:16 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
Move distinguishing index determination and verification of type-identity of preceding arguments out of codegen and into the WebIDL parser. (28.65 KB, patch)
2012-06-19 12:49 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review
With a slight tweak (30.49 KB, patch)
2012-06-20 22:18 PDT, Boris Zbarsky [:bz]
justin.lebar+bug: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-04-03 19:48:50 PDT
Right now the codegen does this business of computing the set of allowed argcounts, computing the distinguishing index for a given argcount, finding the valid overloads for a given argcount, and verifying that types of the arguments before the distinguishing index are identical for a given argcount.  Part or all of this process should move into the data model, I think.  For one thing, if we want the parser to throw whenever the WebIDL is invalid, it needs to handle this stuff itself.
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-04-03 19:50:44 PDT
I agree that this should move into the parser.
Comment 2 Boris Zbarsky [:bz] 2012-06-13 21:49:40 PDT
The plan for this is to add a validate() pass after finish() at toplevel.  We're going to need to do that because we can't really do distinguishability checking until after finish(), especially given the needs of bug 742213.
Comment 3 Boris Zbarsky [:bz] 2012-06-14 21:16:54 PDT
Created attachment 633391 [details] [diff] [review]
Move distinguishing index determination and verification of type-identity of preceding arguments out of codegen and into the WebIDL parser.

This also has way better error messages than the other used to
Comment 4 Boris Zbarsky [:bz] 2012-06-19 12:49:34 PDT
Created attachment 634554 [details] [diff] [review]
Move distinguishing index determination and verification of type-identity of preceding arguments out of codegen and into the WebIDL parser.
Comment 5 Boris Zbarsky [:bz] 2012-06-20 22:18:48 PDT
Created attachment 635187 [details] [diff] [review]
With a slight tweak
Comment 6 Justin Lebar (not reading bugmail) 2012-06-21 19:33:58 PDT
Sorry I've taken a while on this one; I'm going to have a look in the morning.
Comment 7 Justin Lebar (not reading bugmail) 2012-06-22 07:12:30 PDT
Comment on attachment 635187 [details] [diff] [review]
With a slight tweak

Can't you just define an empty |validate| method on IDLObject and override it where necessary?  (The same goes for |finish|, actually...)  Or is the idea to explicitly say "we don't care to validate this thing"?  I guess "explicit is better than implicit"...

>@@ -1819,16 +1847,19 @@ class IDLMethod(IDLInterfaceMember, IDLS
>         # REVIEW: specialType is NamedOrIndexed -- wow, this is messed up.
>         IDLInterfaceMember.__init__(self, location, identifier,
>                                     IDLInterfaceMember.Tags.Method)
> 
>         self._hasOverloads = False
> 
>         assert isinstance(returnType, IDLType)
>         self._returnType = [returnType]
>+        # We store a list of all the overload locations, matching our
>+        # signature list.
>+        self._location = [location]

Oh, I see, IDLMethod contains all the overloads!  This should be documented, and I don't like the name |_location| for multiple locations, but we can fix all this as part of bug 767352.

>@@ -1866,29 +1897,35 @@ class IDLMethod(IDLInterfaceMember, IDLS
>         for argument in arguments:
>             # Only the last argument can be variadic
>             assert not sawVariadicArgument
>             # Once we see an optional argument, there can't be any non-optional
>             # arguments.
>             if inOptionalArguments:
>                 assert argument.optional
>+            # Once we see an argument with no default value, there can
>+            # be no more default values.
>+            if sawOptionalWithNoDefault:
>+                assert not argument.defaultValue
>             inOptionalArguments = argument.optional
>             sawVariadicArgument = argument.variadic
>+            sawOptionalWithNoDefault = argument.optional and not argument.defaultValue

Why are all these assertions?  Shouldn't this whole method be raising compile
errors instead?  We can handle that as a follow-up...

>@@ -1981,29 +2020,90 @@ class IDLMethod(IDLInterfaceMember, IDLS
>                     continue
> 
>                 type = argument.type.complete(scope)
> 
>                 assert not isinstance(type, IDLUnresolvedType)
>                 assert not isinstance(type.name, IDLUnresolvedIdentifier)
>                 argument.type = type
> 
>+        # Now compute various information that will be used by the
>+        # WebIDL overload resolution algorithm.
>+        self.maxArgCount = max(len(s[1]) for s in self.signatures())

Yuck, but s[1] should become |signature.arguments| or something as part of bug 767352.

>+    def signaturesForArgCount(self, argc):
>+        return [(retval, args) for (retval, args) in self.signatures() if
>+                len(args) == argc or (len(args) > argc and args[argc].optional)]
>+
>+    def locationsForArgCount(self, argc):
>+        return [ self._location[i] for (i, args) in enumerate(self._arguments) if
>+                 len(args) == argc or
>+                 (len(args) > argc and args[argc].optional)]
>+
>+    def distinguishingIndexForArgCount(self, argc):
>+        def isValidDistinguishingIndex(idx, signatures):
>+            for (firstSigIndex, (firstRetval, firstArgs)) in enumerate(signatures[:-1]):
>+                for (secondRetval, secondArgs) in signatures[firstSigIndex+1:]:

I think it'd be clearer if you used itertools.combinations:

  for ((firstRetval, firstArgs), (secondRetval, secondArgs)) in itertools.combinations(signatures):

>+                    firstType = firstArgs[idx].type
>+                    secondType = secondArgs[idx].type
>+                    if not firstType.isDistinguishableFrom(secondType):
>+                        return False
>+            return True
>+        signatures = self.signaturesForArgCount(argc)
>+        for idx in range(argc):
>+            if isValidDistinguishingIndex(idx, signatures):
>+                return idx
>+        # No valid distinguishing index.  Time to throw
>+        locations = self.locationsForArgCount(argc)
>+        raise WebIDLError("Signatures with %d arguments for method '%s' are not "
>+                          "distinguishable" % (argc, self.identifier.name),
>+                          locations[0],
>+                          extraLocations=locations[1:])

Kind of seems like WebIDLError should take a single list of locations, the first being the "canonical" location, and the others being the "extra" ones.
Comment 8 Boris Zbarsky [:bz] 2012-06-22 09:31:37 PDT
> Or is the idea to explicitly say "we don't care to validate this thing"?

Mostly I was just following existing coding style, but yes, explicitly failing things that didn't think about whether they need validating seems like a good idea.

> Shouldn't this whole method be raising compile errors instead? 

Will do.

> I think it'd be clearer if you used itertools.combinations:

Needs python 2.6.  We have to support 2.5 at the moment if we want tinderbox to stay green...  Leaving as-is for now, but let me know if you want me to write a helper that basically reimplements itertools.combinations.

> Kind of seems like WebIDLError should take a single list of locations, the
> first being the "canonical" location, and the others being the "extra" ones.

Yeah, I'll file a followup to do that.... The list thing just kinda grew organically (first it was one extra location, etc), and I didn't want to update all the callsites when I added it.
Comment 9 Justin Lebar (not reading bugmail) 2012-06-22 09:35:05 PDT
> Needs python 2.6.

Lame!  No need to write a helper; hopefully we'll be on Python 2.6 soon.
Comment 10 Boris Zbarsky [:bz] 2012-06-22 13:24:27 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/825dfb552fd8

Filed bug 767546 on making WebIDLError take a location list.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-06-23 05:45:14 PDT
https://hg.mozilla.org/mozilla-central/rev/825dfb552fd8

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