Have the equivalent of [TreatUndefinedAs=Missing] be the default behavior for WebIDL optional arguments

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug)

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Please cc anyone who might care whom I missed.

I don't know whether we need a [TreatUndefinedAs=Coerce] or something to produce our current behavior, but I'm going to assume for now that we do not.
(Assignee)

Comment 1

5 years ago
Created attachment 761863 [details] [diff] [review]
Still pending sorting out what to do with overloads
(Assignee)

Updated

5 years ago
Assignee: nobody → bzbarsky
(Assignee)

Comment 2

5 years ago
There's an open question about how overloads should work...
(I still don't get how foo(undefined) has its first argument missing, but I don't care enough to fight this.)
(Assignee)

Comment 4

5 years ago
The issue is that if you have script like this:

  function g(a) {
    foo(a);
  }

and then someone calls g like so:

  g();

then you get foo(undefined) in a natural way.  And having to write g like this:

  function g(a) {
    foo.apply(null, arguments);
  }

or worse yet (for cases when you don't pass through your arguments wholesale):

  function g(a) {
    if (arguments.length == 0) {
      foo();
    } else {
      foo(a);
    }
  }

is pretty uncalled for.
Isn't it a spec issue?
(Assignee)

Comment 6

5 years ago
Sure; there has been plenty of discussion about the spec end on public-script-coord.
The spec changes have been made now.  [TreatUndefinedAs=Missing] has also been removed.
The way I've handled overloads is to not consider them as being equivalent to optional arguments.  That now means that:

  void f();
  void f(long x);

is different from:

  void f(optional long x);

which is an equivalence I wanted to keep if possible, but I think we need to break it now.

When calling f(undefined), the former will select the (long) overload and convert the undefined to 0, while the latter will select the (optional long) overload and treat it as missing.
Actually overloads and optional arguments had a difference even before the change. See <https://www.w3.org/Bugs/Public/show_bug.cgi?id=19646>.
Created attachment 813210 [details] [diff] [review]
part 1.  Don't require all arguments after an optional argument to be optional.
Attachment #813210 - Flags: review?(khuey)
Created attachment 813211 [details] [diff] [review]
part 2.  Fix a bug that crept in with overloading a string an a nullable number and then passing in null, due to the number conversion being conditional on the input type in that case.   removes a footgun conversion operator on Nullable that was causing i
Attachment #813211 - Flags: review?(khuey)
Created attachment 813212 [details] [diff] [review]
part 3.  Rework the overload resolution algorithm to WebIDL spec changes in handling of optional arguments.   major changes are as follows:

1) The new algorithm no longer adjusts "argcount" based on the
   interaction of trailing undefined and [TreatUndefinedAs=Missing]
   arguments.  We never implemented this; just asserted that we didn't
   have to deal with this situation.

2) If the distinguishing argument is undefined and there is an
   overload that has an optional argument at the distinguishing
   argument index, that overload is selected.
Attachment #813212 - Flags: review?(khuey)
Created attachment 813213 [details] [diff] [review]
part 4.  Treat undefined as missing for optional WebIDL arguments.
Attachment #813213 - Flags: review?(khuey)
Created attachment 813421 [details] [diff] [review]
Part 4 with test fixes and such

See http://lists.w3.org/Archives/Public/public-script-coord/2013OctDec/0041.html for why we need the XHR webidl change.  Ms2ger, can you review the imptests changes?
Attachment #813421 - Flags: review?(khuey)
Attachment #813421 - Flags: review?(Ms2ger)
(Assignee)

Updated

5 years ago
Attachment #813213 - Attachment is obsolete: true
Attachment #813213 - Flags: review?(khuey)
Comment on attachment 813421 [details] [diff] [review]
Part 4 with test fixes and such

Review of attachment 813421 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the dom/imptests bits.

::: content/base/test/test_createHTMLDocument.html
@@ -22,1 @@
>    // Opera doesn't have a doctype: DSK-311092

We can probably remove this file.
Attachment #813421 - Flags: review?(Ms2ger) → review+
Upstreamed the test changes (and a bit more) at https://github.com/w3c/web-platform-tests/pull/361
Comment on attachment 813212 [details] [diff] [review]
part 3.  Rework the overload resolution algorithm to WebIDL spec changes in handling of optional arguments.   major changes are as follows:

Review of attachment 813212 [details] [diff] [review]:
-----------------------------------------------------------------

I managed to convince myself this is correct.
Attachment #813212 - Flags: review?(khuey) → review+

Updated

5 years ago
Depends on: 926305
See bug 926305 comment 4.  We need to fix Node.cloneNode just like we fixed XMLHttpRequest.open... and add a test.

I grepped our IDL; the only other API that has this optional-boolean-defaulting-true pattern is Element.scrollIntoView.  I'm going to fix that one as well.
Created attachment 819145 [details] [diff] [review]
To roll into part 4 when I reland
Note to self: need to fix importNode too.

Updated

5 years ago
Depends on: 933964
Depends on: 999315
You need to log in before you can comment on or make changes to this bug.