Set correct Function.length on overloaded methods

Set correct Function.length on overloaded and variadic methods

According to WebIDL, the Function.length property should be the length of
shortest argument list in the effective overload set for argument count 0.
This means for instance that variadic arguments don't contribute to the
length.

BUG=377718
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175559

[Jens Widell, 2014-05-27 12:07:21]

PTAL.

This doesn't change CG, since the methods for which length is calculated is
still the methods with 'overload_index'==1, not the ones with 'overloads' set.
IOW, this CL is arguably quite  misleading.

When combined with the per-overload runtime enabled CL (where I change which
method is used,) this kicks in and prevents a bunch of incorrect function length
changes. Those changes that it doesn't prevent are correct per spec; i.e. cases
where the first overload in the IDL file isn't the one with the lowest number of
arguments.

[Jens Widell, 2014-05-27 12:11:21]

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:418:
{{method.number_of_required_or_variadic_arguments}}
This value is actually wrong per spec. It counts a variadic argument as one, but
if I understand the spec correctly it should count as zero. IOW,

  void f(long... l)

should have length=0. I kept it to minimize the impact of this CL, but can fix
if we want, simply by dropping the '_or_variadic' part.

[jsbell, 2014-05-27 18:10:50]

Are there any tests for function lengths? Should we add some samples to
Internals.idl (or a child object similar to TypeConversions if we don't want to
pollute Internals) and add a layout test that verifies the lengths?

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:418:
{{method.number_of_required_or_variadic_arguments}}
On 2014/05/27 12:11:21, Jens Lindström wrote:
> This value is actually wrong per spec. It counts a variadic argument as one,
but
> if I understand the spec correctly it should count as zero. IOW,
> 
>   void f(long... l)
> 
> should have length=0. I kept it to minimize the impact of this CL, but can fix
> if we want, simply by dropping the '_or_variadic' part.

IMHO, we might as well fix it. (If not, we should at least add a FIXME here) I
suspect it will break exactly zero tests.

[Jens Widell, 2014-05-27 18:27:26]

On 2014/05/27 18:10:50, jsbell wrote:
> Are there any tests for function lengths? Should we add some samples to
> Internals.idl (or a child object similar to TypeConversions if we don't want
to
> pollute Internals) and add a layout test that verifies the lengths?

I'm not aware of existing tests for it. Haven't spotted any suspicious failures
while playing around with these changes, but then they've only changed behavior
for overloaded methods so far, and that's a fairly small subset of all methods.

I'll add a layout test. I think decent coverage of interesting cases can be
achieved by just checking "real" methods, so not sure it's worth-while adding a
custom internal object just to test this. The only thing that's perhaps a bit
fragile to test that way is dependence on the order of overloads in the IDL
file; someone could break the test (i.e. make it no longer test what it was
meant to test) simple by reordering the IDL file.

[Jens Widell, 2014-06-02 08:46:43]

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:416: {{method.overloads.minarg}}
This is of course possibly incorrect if the "shortest" overload is runtime
enabled...

[Jens Widell, 2014-06-02 08:59:06]

On 2014/05/27 18:10:50, jsbell wrote:
> Are there any tests for function lengths? Should we add some samples to
> Internals.idl (or a child object similar to TypeConversions if we don't want
to
> pollute Internals) and add a layout test that verifies the lengths?

Found LayoutTests/fast/js/function-length.html which tests the .length of
various functions, and added some more cases to it to cover optional arguments,
variadic arguments and overloaded methods. (Not exactly an overwhelming amount
of additional testing done, but something at least.)

>
https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
> File Source/bindings/templates/methods.cpp (right):
> 
>
https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
> Source/bindings/templates/methods.cpp:418:
> {{method.number_of_required_or_variadic_arguments}}
> On 2014/05/27 12:11:21, Jens Lindström wrote:
> > This value is actually wrong per spec. It counts a variadic argument as one,
> but
> > if I understand the spec correctly it should count as zero. IOW,
> > 
> >   void f(long... l)
> > 
> > should have length=0. I kept it to minimize the impact of this CL, but can
fix
> > if we want, simply by dropping the '_or_variadic' part.
> 
> IMHO, we might as well fix it. (If not, we should at least add a FIXME here) I
> suspect it will break exactly zero tests.

Fixed variadics as well, in patch set 3.

[Nils Barth (inactive), 2014-06-04 07:22:02]

Looks ok, with 2 significant changes requested:
* Add a check/FIXME for when runtime features change length (substantive).
* Put the logic in Python, not Jinja.

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:416: {{method.overloads.minarg}}
On 2014/06/02 08:46:43, Jens Lindström wrote:
> This is of course possibly incorrect if the "shortest" overload is runtime
> enabled...

Good catch!
I assume this can't be fixed without generating some really ugly code for method
configuration, right?

What I'd suggest is:
* Put a FIXME at this computation (as noted, in Python).
* Check for this and raise an exception in the CG (doubt it will happen, but
it's possible): it's better to prevent people from adding unsupported cases than
to allow them to accidentally break things.

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:423: {% macro method_length(method) %}
Could you put this in Python instead, as it's logic, not textual output?

I.e., in the main display in v8_methods:
'length': method.overloads.minarg if method.overloads else
method.number_of_required_arguments,

[Nils Barth (inactive), 2014-06-04 07:30:50]

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:423: {% macro method_length(method) %}
Also, could you quote the spec?

The value of the Function object’s “length” property is a Number determined as
follows:
1. Let S be the effective overload set for regular operations (if the operation
is a regular operation) or for static operations (if the operation is a static
operation) with identifier id on interface I and with argument count 0.
2. Return the length of the shortest argument list of the entries in S.

[Jens Widell, 2014-06-04 07:38:36]

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:416: {{method.overloads.minarg}}
On 2014/06/04 07:22:02, Nils Barth wrote:
> On 2014/06/02 08:46:43, Jens Lindström wrote:
> > This is of course possibly incorrect if the "shortest" overload is runtime
> > enabled...

> I assume this can't be fixed without generating some really ugly code for
method
> configuration, right?

I guess we could generate a simple per-method helper function (when necessary)
for calculating the length, and then make {{method.length}} expand to a call to
it.

Seems overkill to do that until we find a case where we actually think it's
necessary. :-)

> What I'd suggest is:
> * Put a FIXME at this computation (as noted, in Python).
> * Check for this and raise an exception in the CG (doubt it will happen, but
> it's possible): it's better to prevent people from adding unsupported cases
than
> to allow them to accidentally break things.

Will do.

[Nils Barth (inactive), 2014-06-04 08:12:01]

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/1/Source/bindings/templates/me...
Source/bindings/templates/methods.cpp:416: {{method.overloads.minarg}}
On 2014/06/04 07:38:36, Jens Lindström wrote:
> I guess we could generate a simple per-method helper function (when necessary)
> for calculating the length, and then make {{method.length}} expand to a call
to
> it.

More seriously, the issue of implementing JS properties via constants vs.
functions does show up periodically in exactly this context, like in having
logging for constants, which requires having a function implementing them.

Philip is actually interested in this for deprecation, so this would be both
useful and a way to get started on understanding V8 implementation of Web IDL
interfaces:
https://code.google.com/p/chromium/issues/detail?id=358506

> Seems overkill to do that until we find a case where we actually think it's
> necessary. :-)

Agreed: implement when needed (or encourage the requester to implement it ;)

[Jens Widell, 2014-06-04 11:12:54]

Patch updated with moved logic, some commentary, and a check that
Function.length doesn't need to be calculated runtime.

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:423: {% macro method_length(method) %}
On 2014/06/04 07:22:02, Nils Barth wrote:
> Could you put this in Python instead, as it's logic, not textual output?
> 
> I.e., in the main display in v8_methods:
> 'length': method.overloads.minarg if method.overloads else
> method.number_of_required_arguments,

Done. Although not quite as suggested I think, since "overloads" isn't
calculated until after generate_method().

[Nils Barth (inactive), 2014-06-05 00:26:22]

LGTM with nits, thanks for a great CL!

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/296403007/diff/40001/Source/bindings/template...
Source/bindings/templates/methods.cpp:423: {% macro method_length(method) %}
On 2014/06/04 11:12:54, Jens Lindström wrote:
> Done. Although not quite as suggested I think, since "overloads" isn't
> calculated until after generate_method().

=P good point that...

https://codereview.chromium.org/296403007/diff/60001/Source/bindings/scripts/...
File Source/bindings/scripts/v8_interface.py (right):

https://codereview.chromium.org/296403007/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:268: # enabled overloads are actually
enabled.
Could you elaborate slightly, giving an example?
E.g.:
# FIXME: This calculation doesn't take into account whether runtime
# enabled overloads are actually enabled, so length may be too *long*.
# E.g., [RuntimeEnabled=Foo] void f(); void f(long x);
# has length 1 if Foo is not enabled, but length 0 if Foo is enabled.

https://codereview.chromium.org/296403007/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:269: method['length'] =
method['overloads']['minarg'] if 'overloads' in method else
method['number_of_required_arguments']
Could you wrap this line?

https://codereview.chromium.org/296403007/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:371: # Check and fail if all overloads
with the shortest acceptable arguments
Thanks, this is a great explanation!

https://codereview.chromium.org/296403007/diff/60001/Source/bindings/scripts/...
Source/bindings/scripts/v8_interface.py:378:
all(method.get('runtime_enabled_function') for method, _, _ in
effective_overloads_by_length[0][1])):
Style points:
* Could you wrap this line?
* Could you switch the order of tests so they accord with the (clear) prose: the
"not common_value" is the exceptional case.
* The [0][1] is a bit opaque; could you add an auxiliary variable, like:
shortest_overloads = effective_overloads_by_length[0][1]
?

[haraken, 2014-06-05 00:58:56]

LGTM

https://codereview.chromium.org/296403007/diff/60001/LayoutTests/fast/js/func...
File LayoutTests/fast/js/function-length.html (right):

https://codereview.chromium.org/296403007/diff/60001/LayoutTests/fast/js/func...
LayoutTests/fast/js/function-length.html:31:
shouldBe('CanvasRenderingContext2D.prototype.drawImage.length', '3');

Just to confirm: These test cases don't depend on runtime flags, right? If it
depends, I'm afraid that the test gets flaky (depending on runtime flags) and
confuse developers.

[Jens Widell, 2014-06-05 06:22:06]

On 2014/06/05 00:58:56, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/296403007/diff/60001/LayoutTests/fast/js/func...
> File LayoutTests/fast/js/function-length.html (right):
> 
>
https://codereview.chromium.org/296403007/diff/60001/LayoutTests/fast/js/func...
> LayoutTests/fast/js/function-length.html:31:
> shouldBe('CanvasRenderingContext2D.prototype.drawImage.length', '3');
> 
> Just to confirm: These test cases don't depend on runtime flags, right? If it
> depends, I'm afraid that the test gets flaky (depending on runtime flags) and
> confuse developers.

They don't. At least not today. There are runtime enabled drawImage() overloads,
but they don't affect its 'length' since there are also same-length
unconditional overloads.

Why would a runtime flag dependency make the tests flaky? Aren't layout tests
always run with the same runtime flags enabled?

[haraken, 2014-06-05 07:05:00]

On 2014/06/05 06:22:06, Jens Lindström wrote:
> On 2014/06/05 00:58:56, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/296403007/diff/60001/LayoutTests/fast/js/func...
> > File LayoutTests/fast/js/function-length.html (right):
> > 
> >
>
https://codereview.chromium.org/296403007/diff/60001/LayoutTests/fast/js/func...
> > LayoutTests/fast/js/function-length.html:31:
> > shouldBe('CanvasRenderingContext2D.prototype.drawImage.length', '3');
> > 
> > Just to confirm: These test cases don't depend on runtime flags, right? If
it
> > depends, I'm afraid that the test gets flaky (depending on runtime flags)
and
> > confuse developers.
> 
> They don't. At least not today. There are runtime enabled drawImage()
overloads,
> but they don't affect its 'length' since there are also same-length
> unconditional overloads.
> 
> Why would a runtime flag dependency make the tests flaky? Aren't layout tests
> always run with the same runtime flags enabled?

I'm not sure but probably you're right :)

[Nils Barth (inactive), 2014-06-05 07:07:01]

On 2014/06/05 07:05:00, haraken wrote:
> On 2014/06/05 06:22:06, Jens Lindström wrote:
> > Why would a runtime flag dependency make the tests flaky? Aren't layout
tests
> > always run with the same runtime flags enabled?
> 
> I'm not sure but probably you're right :)

runtime flag dependency would break tests on *certain* configurations, but not
others,
if these varied between try bots (like in Oilpan ;).

In any case, that's not an issue here, but for the record.

[Jens Widell, 2014-06-05 08:49:04]

The CQ bit was checked by jl@opera.com

[commit-bot: I haz the power, 2014-06-05 08:49:53]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jl@opera.com/296403007/80001

[commit-bot: I haz the power, 2014-06-05 11:29:00]

Change committed as 175559