Consider moving distinguishing index determination and verification of type-identity of preceding arguments into the WebIDL data model

RESOLVED FIXED in mozilla16

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla16
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
I agree that this should move into the parser.
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.
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
Attachment #633391 - Flags: review?(khuey)
Assignee: khuey → bzbarsky
Whiteboard: [need review]
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.
Attachment #634554 - Flags: review?(justin.lebar+bug)
Attachment #633391 - Attachment is obsolete: true
Attachment #633391 - Flags: review?(khuey)
Created attachment 635187 [details] [diff] [review]
With a slight tweak
Attachment #635187 - Flags: review?(justin.lebar+bug)
Attachment #634554 - Attachment is obsolete: true
Attachment #634554 - Flags: review?(justin.lebar+bug)
Sorry I've taken a while on this one; I'm going to have a look in the morning.
Blocks: 767352
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.
Attachment #635187 - Flags: review?(justin.lebar+bug) → review+
> 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.
> Needs python 2.6.

Lame!  No need to write a helper; hopefully we'll be on Python 2.6 soon.
https://hg.mozilla.org/integration/mozilla-inbound/rev/825dfb552fd8

Filed bug 767546 on making WebIDLError take a location list.
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/825dfb552fd8
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.