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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Still pending sorting out what to do with overloads

Comment 2

[Boris Zbarsky [:bzbarsky]]

There's an open question about how overloads should work...

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(I still don't get how foo(undefined) has its first argument missing, but I don't care enough to fight this.)

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Masatoshi Kimura [:emk]]

Isn't it a spec issue?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sure; there has been plenty of discussion about the spec end on public-script-coord.

Comment 7

[Cameron McCormack (:heycam)]

The spec changes have been made now.  [TreatUndefinedAs=Missing] has also been removed.

Comment 8

[Cameron McCormack (:heycam)]

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.

Comment 9

[Masatoshi Kimura [:emk]]

Actually overloads and optional arguments had a difference even before the change. See <https://www.w3.org/Bugs/Public/show_bug.cgi?id=19646>.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Don't require all arguments after an optional argument to be optional.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: 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

Comment 12

[Boris Zbarsky [:bzbarsky]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Treat undefined as missing for optional WebIDL arguments.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: 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?

Comment 15

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Upstreamed the test changes (and a bit more) at https://github.com/w3c/web-platform-tests/pull/361

Comment 17

[Boris Zbarsky [:bzbarsky]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=ce24ed23155a

Comment 18

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/877a227c502f
   https://hg.mozilla.org/integration/mozilla-inbound/rev/d8636e485e85
   https://hg.mozilla.org/integration/mozilla-inbound/rev/61092280cb2a
   https://hg.mozilla.org/integration/mozilla-inbound/rev/ccf11ae08ba2

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/877a227c502f
https://hg.mozilla.org/mozilla-central/rev/d8636e485e85
https://hg.mozilla.org/mozilla-central/rev/61092280cb2a
https://hg.mozilla.org/mozilla-central/rev/ccf11ae08ba2

Comment 21

[Ed Morley [:emorley]]

Backed out for bug 926305:
remote:   https://hg.mozilla.org/mozilla-central/rev/f313d33bdbc4
remote:   https://hg.mozilla.org/mozilla-central/rev/8a2bb2622978
remote:   https://hg.mozilla.org/mozilla-central/rev/b5dc6d9578e7
remote:   https://hg.mozilla.org/mozilla-central/rev/fcddc11fb2ef

Comment 22

[Boris Zbarsky [:bzbarsky]]

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.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: To roll into part 4 when I reland

Comment 24

[Boris Zbarsky [:bzbarsky]]

Note to self: need to fix importNode too.

Comment 25

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/dfaf4e97dc29
   https://hg.mozilla.org/integration/mozilla-inbound/rev/e052d7967418
   https://hg.mozilla.org/integration/mozilla-inbound/rev/cd1cfd84378d
   https://hg.mozilla.org/integration/mozilla-inbound/rev/250ae9645c79

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dfaf4e97dc29
https://hg.mozilla.org/mozilla-central/rev/e052d7967418
https://hg.mozilla.org/mozilla-central/rev/cd1cfd84378d
https://hg.mozilla.org/mozilla-central/rev/250ae9645c79