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.

This also has way better error messages than the other used to

Sorry I've taken a while on this one; I'm going to have a look in the morning.

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.

> 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.