Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Given [Constructor, Constructor(DOMString x)], new C(undefined) picks the second #351

Closed
domenic opened this issue Apr 25, 2017 · 10 comments

Comments

@domenic
Copy link
Member

domenic commented Apr 25, 2017

In particular by my reading the overload resolution algorithm, the step

Remove from S all entries whose type list is not of length argcount.

immediately disqualifies the no-arg constructor from consideration.

This seems not so great. Can we "trim trailing undefineds" before invoking the overload resolution algorithm?

/cc @bzbarsky

This is concretely an issue for DOMMatrix.

@foolip
Copy link
Member

foolip commented Apr 26, 2017

This behavior was deliberately used with alert() + alert(DOMString message) in whatwg/html@0b570cf.

Here Blink's IDL now matches the spec, but there are other cases of optional vs. override mismatches that might be observable. @mdittmer, would you be able to interrogate your dataset to find all such cases?

I think that to just "trim trailing undefineds" wouldn't work, certainly if foo(DOMString x) is the only signature, then foo(undefined) shouldn't throw because of too few arguments. Maybe it would work to drop trailing undefineds down to the minimum number of arguments in any override, but I'm not sure.

@annevk
Copy link
Member

annevk commented Apr 26, 2017

Yeah, this design is also deliberately used by XMLHttpRequest's open(). Avoid overloads if you don't want weird cases and use unions and optional instead.

@bzbarsky
Copy link
Collaborator

As previously mentioned, this is very much by design: overloads discriminate on argc first, and there are specs that depend on that. If DOMMatrix wants to treat undefined first arg as no args, it should be doing:

Constructor(optional DOMString transformList)

(or Constructor(optional sequence<unrestricted double> numberSequence)) and then handling the "not present" case.

@annevk
Copy link
Member

annevk commented Apr 26, 2017

It seems they could just have Constructor(optional (DOMString or sequence<unrestricted double>) init = []) or some such.

@bzbarsky
Copy link
Collaborator

Yes, that would be an option as well, with an extra case for "sequence has 0 elements" at https://drafts.fxtf.org/geometry/#dom-dommatrix-dommatrix-numbersequence

@bzbarsky
Copy link
Collaborator

That depends on whether you want to allow new DOMMatrix([]) with an explicit empty array.

@annevk
Copy link
Member

annevk commented Apr 26, 2017

Fair, "" is better then as they already handle that.

@annevk
Copy link
Member

annevk commented Apr 26, 2017

@zcorpan ^^

@bzbarsky
Copy link
Collaborator

There was talk of not exposing the string signature in workers, but exposing the rest, so whether to use "" depends on that a bit.

@foolip
Copy link
Member

foolip commented May 5, 2017

@mdittmer, would you be able to interrogate your dataset to find all such cases?

Mark helped me out with this offline, and among the candidates no other problematic cases were found. It didn't include constructors though, so there might be more like DOMMatrix waiting to be discovered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants