Avoid extra arity and type check on distinguishing argument

Avoid extra arity and type check on distinguishing argument

For wrapper type distinguishing arguments (at least) we don't need to
check the type in the method implementation, since the same check is part
of the overload resolution that lead to the method being called at all.

BUG=293561

[Jens Widell, 2014-05-15 11:24:02]

Please review when convenient.

A pretty simple fix, removing a bit of redundant code for some overloaded
methods.

[Jens Widell, 2014-05-15 12:25:12]

On 2014/05/15 11:24:02, Jens Lindström wrote:
> A pretty simple fix, [...]

But not as simple as I thought... :-/

A single method can of course have multiple distinguishing arguments. This
implementation is guaranteed to set method['distinguishing_argument_index'] to
the highest one since effective overload sets are processed in length order, but
that dependency is a bit unfortunate in the first place. (The code could be
modified to make it more explicit that it is always set to the highest index.)

Given that, all other possible distinguishing arguments (at lower indexes) are
still type checked. And the highest one, for which we drop the type check, must
be optional currently (as long as we don't support variadics) which means that
if the function was called with fewer arguments, we would never have reached the
dropped type check anyway, since optional arguments (of the types for which we
drop the check) are implemented by calling the implementation early with fewer
arguments and return.

So to summarize: I think this patch still works as intended, but mostly by
chance, and it has too many implicit dependencies for me to be particularly
happy about it.

Patch set 3 adds another set of overloaded methods where one has two different
distinguishing arguments, to show what happens in that case.

[haraken, 2014-05-15 17:11:42]

I might want to hear Nils' thoughts first :)

[Nils Barth (inactive), 2014-05-16 02:15:38]

not LGTM: agreed, this is too fragile, and this is a rare corner case anyway.
(I think you reached the same conclusion.)

More fundamentally, I think it's better to implement union type arguments and
optimize those: that should cover most use cases.

Currently we don't use overloaded methods much, and most use case are better
implemented as union types instead: I don't think this is worth the complexity.

Performance-wise, we'd need something that is performance-sensitive and has
overloading based on distinguishing argument that has a test:
mustBeFast(T t);
mustBeFast(U u);
...but this would normally be written as:
mustBeFast((T or U) tOrU);
...and then we don't need overload resolution, and optimizing union type
arguments (e.g., passing the argument to Blink as a tagged union) would have a
bigger impact and be safe.


== Details ==
I'm concerned that this is already complicated code, and it's dangerous to try
to optimize it too much unless there's evidence that this is a performance
bottleneck: any bugs this causes are going to be *very* confusing, especially
since test coverage of type checking is pretty patchy.
We could have really bad cases where adding an overload accidentally turns off
type checking and then causes crashers, or even security bugs.

The idea is to remove redundancy between overload resolution and type checking,
both of which are complicated algorithms, so any such optimizations are going to
be fragile.

I've done a little in resolution_tests_methods(), and that's at least all in one
place (just overload resolution method, and one Python function), so it's easier
to understand and more robust, and failures are obvious, not subtle (fails to
resolve, indeed causing compile failure if particular overload can't be
reached!).
(Currently skip an IsNumber() check if it comes before an unconditional one:
only used when distinguishing primitive types.)

Another one to skip would be to remove the IsDate or IsRegExp check if we've
already resolved it;
I've added a FIXME for that.
I.e., skip duplicate tests due to:
5. Otherwise: if V is a Date object and ...
6. Otherwise: if V is a RegExp object and ...
8. Otherwise: if V is any kind of object except for a native Date object, a
native RegExp object, and ...

Looking at the overload resolution algorithm, the tests that seem duplicated
between resolution and typechecking are:
3. V8*::hasInstance (implements interface),
5. IsDate
6. IsRegExp
7. IsCallable
8. IsObject
OTOH, primitive types aren't type checked, b/c they can be converted.


> A single method can of course have multiple distinguishing arguments.

This is just because of optionality (and variadic), right?
For a given length, there's a single distinguishing argument index.
http://heycam.github.io/webidl/#dfn-distinguishing-argument-index

E.g., for:
f(T t);
f(U u, optional T t);
f(U u, U u2);

f2 expands to:
f(U u);
f(U u, T t);

...and in the first signature, we've already checked the 1st arg,
b/c distinguishing:
f(T t);
f(U u);

...while in the second signature, we've already checked the 2nd arg:
f(U u, T t);
f(U u, U u2);


So I believe this code works, but it's fragile and dangerous, and the benefits
are small: we've got bigger fish to fry.

[Nils Barth (inactive), 2014-05-16 02:15:45]

https://codereview.chromium.org/289843006/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/289843006/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestObject.idl:415: [TypeChecking=Interface] void
overloadedMethodN(double a, TestInterface b);
You don't need the double here do you?
...and this is subtle enough that some comment on what's being tested would be
good:
// 2 distinguishing argument indices in second method, due to optionality

[Jens Widell, 2014-05-16 05:58:43]

On 2014/05/16 02:15:38, Nils Barth wrote:
> not LGTM: agreed, this is too fragile, and this is a rare corner case anyway.
> (I think you reached the same conclusion.)

Yes. Clearly, skipped checks needs to be easily proven correct (or have a
profound effect on performance or code size, I guess.) This ended up neither, so
not worth the cost.

Will close this CL.

> > A single method can of course have multiple distinguishing arguments.
> 
> This is just because of optionality (and variadic), right?
> For a given length, there's a single distinguishing argument index.
> http://heycam.github.io/webidl/#dfn-distinguishing-argument-index

Yes, an affected method must appear in the effective overload set for multiple
number of arguments, i.e. have some optional or variadic arguments.

[Jens Widell, 2014-05-16 05:59:17]

https://codereview.chromium.org/289843006/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/289843006/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestObject.idl:415: [TypeChecking=Interface] void
overloadedMethodN(double a, TestInterface b);
On 2014/05/16 02:15:46, Nils Barth wrote:
> You don't need the double here do you?

Don't think I do, no. Unclear why I thought I did.

> ...and this is subtle enough that some comment on what's being tested would be
> good:
> // 2 distinguishing argument indices in second method, due to optionality

Yeah. By the time I added this, I had sort of already abandoned this CL in my
mind anyway, so wasn't being too thorough here.

Do you think it meaningful to add these methods on their own?

[Nils Barth (inactive), 2014-05-16 06:05:31]

https://codereview.chromium.org/289843006/diff/40001/Source/bindings/tests/id...
File Source/bindings/tests/idls/TestObject.idl (right):

https://codereview.chromium.org/289843006/diff/40001/Source/bindings/tests/id...
Source/bindings/tests/idls/TestObject.idl:415: [TypeChecking=Interface] void
overloadedMethodN(double a, TestInterface b);
On 2014/05/16 05:59:17, Jens Lindström wrote:
> Do you think it meaningful to add these methods on their own?

I don't think that's necessary, but thanks for asking!
Generally we only add test cases when there's an actual use case that fails
otherwise, not just for internal correctness tests.

(We try to keep CG and test cases close to actual usage: that's why I've been
avoiding adding correct CG code that's unused: dead code rots quickly around
here.)