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

Define a way to specify a default value for dictionaries (the literal "{}") and require it to be specified for the dictionary arguments that are required to be optional. #750

Merged
merged 1 commit into from
Jul 1, 2019

Conversation

bzbarsky
Copy link
Collaborator

@bzbarsky bzbarsky commented Jun 28, 2019

@bzbarsky
Copy link
Collaborator Author

Note that this will require a ton of spec updates, by the way, to add all the now-required = {}.

Copy link
Member

@Ms2ger Ms2ger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine. (My first thought was to use = null, but maybe this is clearer.)

index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
@annevk
Copy link
Member

annevk commented Jul 1, 2019

Created PRs for whatwg/dom#771 and whatwg/html#4745. This will require Bikeshed changes too, tracked at speced/bikeshed#1491 and plinss/widlparser#39 for the IDL parser backend.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 1, 2019

My first thought was to use = null

Mine was too, and is in fact what Gecko has long implemented, but @annevk convinced me that this is more what people are likely to expect.

"{}") and require it to be specified for the dictionary arguments that
are required to be optional.

Fixes whatwg#585
Fixes whatwg#602
@bzbarsky bzbarsky force-pushed the better-dictionary-defaults branch from a740e17 to 5d249a7 Compare July 1, 2019 16:31
@bzbarsky bzbarsky merged commit 7329e8c into whatwg:master Jul 1, 2019
@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 1, 2019

@annevk thank you for filing all those!

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 1, 2019

https://bugzilla.mozilla.org/show_bug.cgi?id=1562680 tracks Gecko implementing this.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 5, 2019

We probably need to get a bunch of other spec issues filed... Can we hope the bikeshed changes will drive that, or should we go through the list of changes I needed to make to IDLs in Gecko and try to file issues based on that?

@annevk
Copy link
Member

annevk commented Jul 5, 2019

If you can dump a file list in a new issue for triage that'd be good I think. The other problem with updating IDL in the specifications is that we also need testharness support as it automatically gets the IDL from the specifications.

@Ms2ger
Copy link
Member

Ms2ger commented Jul 8, 2019

webidl2.js is fixed in w3c/webidl2.js#338, but the copy in wpt isn't updated yet.

@marcoscaceres
Copy link
Member

Just recording that the relevant bug on the Blink side is: https://bugs.chromium.org/p/chromium/issues/detail?id=948139

Do we know if the WebKit folks are aware of this change?

@kainino0x
Copy link

kainino0x commented Sep 29, 2019

In both webidl2.js and bikeshed right now, this code is accepted:

dictionary A {
  required long x;
};

dictionary B {
  A a = {};
};

but it seems like this ought to be invalid since {} is not a valid A. = null seems like it should equally not work.

Semi-related, I'm not sure I know what dictionary C { A a; } means. Does this allow a to be "A or undefined"? It seems that way in Chrome's bindings generator right now. We want it to work this way if possible in WebGPU. [edit: here]

gpuweb/gpuweb#420

@bzbarsky
Copy link
Collaborator Author

but it seems like this ought to be invalid since {} is not a valid A

That would be an option, yes.

= null seems like it should equally not work.

Per spec that does not work, right.

Does this allow a to be "A or undefined"?

It allows a to be not present (that is the Get() on the JS object returns undefined), but if it's present (any other value is returned) then it only allows values that can be converted to A without failing.

Chrome's bindings generator has all sorts of bugs around dictionaries, so I wouldn't base too much on what it does, but the desire for the dictionary C { A a; } case where A has required members and allowing {} to be passed for a C instance was present in a number of specs, so the IDL spec explicitly allows that.

@kainino0x
Copy link

That's really helpful, thanks for the info!

= null seems like it should equally not work.

Per spec that does not work, right.

(fwiw, webidl2.js and bikeshed do allow this, too.)

@saschanaz
Copy link
Member

(fwiw, webidl2.js and bikeshed do allow this, too.)

I neither wouldn't base too much on what they allow, because both have been just parsers without semantics checks, while only recently webidl2.js started providing some.

@rniwa
Copy link

rniwa commented Sep 29, 2019

@cdumez

@bzbarsky
Copy link
Collaborator Author

(fwiw, webidl2.js and bikeshed do allow this, too.)

Right, it arguably used to be OK (and was supported in Gecko) until the changes in this PR. At this point it's clearly not OK, though. IDL null (not to be confused with JS null!) is not a valid value for a dictionary type.

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