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

Sort out when dictionaries and records should have default values #76

Open
bzbarsky opened this issue Nov 20, 2015 · 48 comments
Open

Sort out when dictionaries and records should have default values #76

bzbarsky opened this issue Nov 20, 2015 · 48 comments
Labels
☕☕ difficulty:medium Hard to fix ⌛⌛ duration:medium Shouldn't be too long to fix

Comments

@bzbarsky
Copy link
Collaborator

Right now optional dictionary arguments in trailing position have a default value (empty dictionary) but other optional dictionaries (non-trailing arguments, dictionary members) do not.

This allows spec authors to create APIs in which undefined and empty dictionary have different behavior, which seems suboptimal.

So I'd like to propose that optional dictionaries always have empty dictionary as default value. What that means in practice is not clear. I guess they should use null as the default value. Sadly that's observably different from using {} if someone spews things on Object.prototype...

Anyway, if we do that, then we have one problem: dictionary-to-JS conversion can give ugly results (as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1226475#c0 for example).

To solve that, I propose the following:

  1. A dictionary is "elidable" (better-naming-wanted) if it is either empty or only has members whose values are themselves elidable dictionaries.
  2. When converting a dictionary to a JS value, skip over members whose values are elidable dictionaries, just like we skip over members that are not present.

For dictionaries that have default values that will still cause them to appear (with those default values), but I can live with that, I think.

@domenic/@heycam Thoughts?

@bzbarsky
Copy link
Collaborator Author

Er, trying to actually get @domenic and @heycam on here, since github seems unhappy with the slash there...

@jan-ivar
Copy link
Contributor

@domenic
Copy link
Member

domenic commented Nov 20, 2015

So I'd like to propose that optional dictionaries always have empty dictionary as default value. What that means in practice is not clear. I guess they should use null as the default value. Sadly that's observably different from using {} if someone spews things on Object.prototype...

Would it be possible to say something like "when an ECMAScript value is not supplied for the argument, the corresponding Web IDL value is the empty dictionary"? I don't quite understand why you need a default value.

Anyway, if we do that, then we have one problem: dictionary-to-JS conversion can give ugly results (as described in https://bugzilla.mozilla.org/show_bug.cgi?id=1226475#c0 for example).

I guess this doesn't seem like a big deal to me; having a canonical form with the same keys every time seems like a plus, even if their values are not terribly useful. But I don't feel strongly, so we can change it if you want.

For dictionaries that have default values that will still cause them to appear (with those default values), but I can live with that, I think.

Indeed, this seems like a good thing in fact.

Thoughts?

Overall sounds sane, although if there are cases where {} and undefined are treated differently, then I guess we are in trouble. It sounds like the Bugzilla comment 6 concern was aleviated in that regard though? Will we never have such a case?

@bzbarsky
Copy link
Collaborator Author

Would it be possible to say something like "when an ECMAScript value is not supplied for the
argument, the corresponding Web IDL value is the empty dictionary"?

Sure. We already say something like that that for dictionaries in trailing position, as I noted.

although if there are cases where {} and undefined are treated differently, then I guess we are in trouble

The only cases where that happens are:

  1. A union which contains a dictionary and the default value is a value for one of the other types in the union. But then conversion from IDL to JS would produce that default value, not undefined.
  2. If someone adds properties on Object.prototype, because undefined always produces an empty dictionary while {} would then possibly produce a nonempty one. But that would be a problem for arguments too; people should just not add properties on Object.prototype because that kills puppies.

@tobie tobie added ⌛⌛ duration:medium Shouldn't be too long to fix ☕☕ difficulty:medium Hard to fix labels Jun 19, 2016
@bzbarsky bzbarsky changed the title Sort out when dictionaries should have default values Sort out when dictionaries and records should have default values Feb 3, 2017
@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Feb 3, 2017

This needs to be sorted out for records too...

@jan-ivar
Copy link
Contributor

jan-ivar commented Mar 15, 2018

Automatically eliding empty dictionaries might make it harder to provide invariants to JS, like:

if (pc.getParameters().rtcp.cname) // TypeError: pc.getParameters(...).rtcp is undefined

That might be correct in this particular case, but as a general rule, I could see it causing edges if a returned dictionary is non-empty most of the time (but not always).

@jan-ivar
Copy link
Contributor

jan-ivar commented Mar 15, 2018

The workaround of marking rtcp as required, might mean we couldn't reuse the dictionary for pc.setParameters() (Again, a hypothetical example, as we might actually want rtcp to be absent until rtcp-related data is available).

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 1, 2018

@domenic @tobie @annevk This is causing problems because specs are coming up with APIs that involve optional dictionaries that have required members: something that never works as an argument but technically works as a dictionary member right now, breaking the symmetry between "list of arguments" and "a dictionary with some members".

So we need to decide whether we want to allow breaking this symmetry and if not we need to change IDL accordingly so people stop trying to do that.

@annevk
Copy link
Member

annevk commented Jun 1, 2018

I think I'd argue that a dictionary member that is a dictionary should have a default value as well.

dictionary Foo { required long foo1; };
dictionary Bar { Foo myFoo; };
...
  void doStuff(optional Bar myBar);

Invoking doStuff() should throw. Invoking doStuff({ myBar: { foo1: 42 }}) would be okay, as far as IDL is concerned.

I don't think there's a good reason to make dictionary handling inconsistent between it being an argument and a member of another dictionary.

(According to bz on IRC Firefox already does this.)

@jan-ivar
Copy link
Contributor

jan-ivar commented Jun 1, 2018

@annevk Semantically, doesn't that seem odd, since if someone wanted that they can already use:

dictionary Foo { required long foo1; };
dictionary Bar { required Foo myFoo; }; 

?

There's also the issue of at least two specs doing this already, and probably not meaning what I wrote. See https://bugzilla.mozilla.org/show_bug.cgi?id=1368949

@annevk
Copy link
Member

annevk commented Jun 1, 2018

@jan-ivar similarly to how we require dictionaries to always be optional arguments, we should probably also forbid the combination with required. Requiring a dictionary doesn't make much sense, unless you wanted to distinguish between passing undefined and {}, which we've decided is not something we want to allow.

@jan-ivar
Copy link
Contributor

jan-ivar commented Jun 1, 2018

similarly to how we require dictionaries to always be optional arguments

But we don't. We can write:

dictionary Foo { required long foo1; };
dictionary Bar { required Foo myFoo; }; 

void doFoo(Foo myFoo);
void doStuff(Bar myBar);

doFoo() throws TypeError: Not enough arguments to Jib.doFoo.
doFoo({}) throws TypeError: Missing required 'foo1' member of Foo.
doStuff() throws TypeError: Not enough arguments to Jib.doStuff.
doStuff({}) throws TypeError: Missing required 'myFoo' member of Bar.
doStuff({myFoo: {}}) throws TypeError: Missing required 'foo1' member of Foo.
doStuff({myFoo: {foo1: 1}}) succeeds.

I don't think there's a good reason to make dictionary handling inconsistent between it being an argument and a member of another dictionary.

Function arguments are required by default whereas dictionary members are optional by default, so I don't see what consistency we'd be upholding.

At least two specs interpret:

dictionary Foo { required long foo1; };
dictionary Bar { Foo myFoo; };

to mean doStuff() should succeed. Exhibit A: PaymentDetailsModifier, Exhibit B: MediaConfiguration:

dictionary MediaConfiguration {
  VideoConfiguration video;
  AudioConfiguration audio;
};

says:

"If configuration.video is present and is not a valid video configuration, return a Promise rejected with a TypeError. "

I.e. they expect { audio: {contentType: 'audio/webm; codecs=opus'}} to not throw TypeError,
and { audio: {contentType: 'audio/webm; codecs=opus'}, video: {}} to throw TypeError.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 2, 2018

But we don't. We can write:

Specifically for dictionaries with a required member, because those correspond to a set of arguments at least one of which is required.

Function arguments are required by default whereas dictionary members are optional by default

Dictionaries as initially designed are meant to be equivalent to a bunch of trailing optional arguments, but easier to use because you can specify only the arguments you want instead of all the ones before the last one you want (with undefined for the ones you don't want). If JS supported named-argument passing like Python, this pattern likely would never have been invented in the first place; it's there to work around the fact that JS only has positional arguments.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 2, 2018

And arguably adding required members to dictionaries was a weird decision: now people have to decide when to make an argument an actual (non-optional) argument and when to make it a required dictionary member...

@jyavenard
Copy link
Member

@bzbarsky > Dictionaries as initially designed are meant to be equivalent to a bunch of trailing optional arguments

Does that still needs to hold though?

Like @jan-ivar I find it hard to justify that consistency especially when it's not consistent now. Why would one inconsistency preferred over another?

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 2, 2018

Does that still needs to hold though?

Does what need to hold? Dictionaries are conceptually a bag of arguments. Adding required doesn't really change this, apart from allowing required arguments in the bag.

Some specs are sort of trying to have nested bags of arguments (or more precisely to represent actual data structures of some sort, not just bags of arguments). That's where the issues come in...

@jyavenard
Copy link
Member

That "Dictionaries as initially designed are meant to be equivalent to a bunch of trailing optional arguments"
Exactly, optional arguments.
I understand where you're coming from but what's wrong in extending that earlier concept? There are certainly advantages to it.

What the child or is define does should have no bearing on its parent definition. like any OOP.

@annevk
Copy link
Member

annevk commented Jun 2, 2018

The problem is that you want to distinguish between {} and undefined, which would be a new unprecedented thing for dictionaries. The idea is that if something takes a dictionary, the algorithm is always handed a dictionary. You propose breaking that invariant.

@jan-ivar
Copy link
Contributor

jan-ivar commented Jun 2, 2018

(In response to bz) I think the weird decision would be to say all required arguments must be positional.

I think we left out the advent of the nested dictionary. Arguably, that's when we invited complexity.

At least two specs have shown they want rules about when arguments are required based on (grouping of) other arguments. The question is whether WebIDL can express it.

@jan-ivar
Copy link
Contributor

jan-ivar commented Jun 2, 2018

(In response to annevk) {} is not falsy, so I think that's fine semantically. As to invariants, I like those. Would we need to change them all? Minimally only for nested optional dictionaries with required members, right?

@jan-ivar
Copy link
Contributor

jan-ivar commented Jun 2, 2018

Also, to the consistency argument:

Regardless of history, the semantics chosen are not POLA and seem internally inconsistent. E.g.:

dictionary Foo { required long foo1; };
dictionary Bar { Foo myFoo = null; };

void doStuff(optional Bar myBar);
Jib.doStuff(); // TypeError: Missing required 'foo1' member of Foo.

Note how we don't get “Not enough arguments” this time. Not only is our default allowed but ignored, if I remove the optional keyword, the compiler says:

WebIDL.WebIDLError: error: Dictionary argument without any required fields or union argument containing such dictionary not followed by a required argument must be optional, /Users/Jan/moz/mozilla-central/dom/webidl/Jib.webidl line 18:19
0:05.91 void doStuff(Bar myBar);

So it literally says the argument to doStuff is optional, and the compiler agrees. None of that adds up to foo1 (and therefore myBar) being required by doStuff(), not even the WebIDL spec.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 2, 2018

Note how we don't get “Not enough arguments” this time.

Sure, because of the details of exactly how the overload algorithm works. The argument is flagged as optional, technically, but if you don't pass it it's defaulting to a value that's not actually valid based on the other IDL there... I'm not quite sure what your argument is, honestly.

Not only is our default allowed but ignored

Um... what's ignored? The point is that null is already the default value in Firefox. And the default for the optional Bar myBar is also null; everyone agrees on that.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jun 2, 2018

I understand where you're coming from but what's wrong in extending that earlier concept?

I think it's worth taking a step back for a second.

WebIDL consumers can always define arbitrary processing of things as desired; any and object are escape hatches that then let you do whatever.

Actual WebIDL syntax is meant to cover common patterns and promote best practices. This is why dictionary arguments treat undefined, null, and {} identically: designing APIs that treat "no options object" and "empty options object" differently is not a best practice.

Now the question is what should be the best practice for this "nested dictionary" thing. Are there examples of such APIs in JS libraries, designed by actual JS developers? Because all the examples in specs pointed to above were created by C++ developers, and historically those are not very well clued-in to how JS APIs should look and act.

@jan-ivar
Copy link
Contributor

jan-ivar commented Jun 2, 2018

Sounds like a true scotsman argument. I pointed out that the syntax itself is counter intuitive. That seems true regardless of who wields the tool. Having things labeled optional which are effectively required, seem like semantic footguns, even for the most experienced. It's also not to spec, so where would someone read and learn this logic, in order to program, as you say, as an "actual JS developer"?

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 2, 2018

@emlun under your proposal, foo({ aThing: null }) would also be invalid, right? Just making sure we're all on the same page in terms of implications.

@emlun
Copy link

emlun commented Jul 3, 2018

Yes, I suppose so - unless null is a valid value for a Thing, which I suppose it's not unless it's declared as Thing? aThing?

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 3, 2018

null is a valid value for a Thing. It means "empty dictionary".

You can't do Thing? aThing to start with. See https://heycam.github.io/webidl/#idl-dictionaries where it says:

If the type of the dictionary member, after resolving typedefs, is a nullable type, its inner type must not be a dictionary type.

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 3, 2018
…onal dictionary. r=bz

Summary:
In order to allow for optional dictionaries with required members
See whatwg/webidl#76 for more information.

Reviewers: bzbarsky

Tags: #secure-revision

Bug #: 1471165

Differential Revision: https://phabricator.services.mozilla.com/D1833
@emlun
Copy link

emlun commented Jul 4, 2018

You can't do Thing? aThing to start with.

Ah, thanks, my mistake.

null is a valid value for a Thing. It means "empty dictionary".

Is this the way it currently is, or what you're suggesting in OP? My suggestion in #76 (comment) is that null be equivalent to {} for function arguments but not for dictionary members.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 4, 2018

Is this the way it currently is, or what you're suggesting in OP?

This is the way it currently is. To be clear, in the current spec for function arguments, undefined and null are equivalent to each other (and mean "empty dictionary") while {} is a bit more complicated because it involved property gets but generally means "empty dictionary" if people haven't modified Object.prototype.

For other dictionaries, still in the current spec, null means empty dictionary while {} means you start getting properties off it, etc.

This is why I wanted to double-check what your actual suggestion is: it sounds like you want to treat undefined and null interchangeably to mean "not present", which is not how it works anywhere in webidl.

@emlun
Copy link

emlun commented Jul 4, 2018

Ok. Yeah, my inexperience with WebIDL is probably obvious at this point. I suppose the exact semantics of explicit nulls and undefineds is of lesser importance, but I believe that treating them as equivalent to an empty dictionary everywhere will keep causing confusion by implicitly eliminating a very natural design pattern.

@bzbarsky
Copy link
Collaborator Author

bzbarsky commented Jul 4, 2018

Explicit null is the only reliable way to get an empty dictionary, as I said above. Furthermore, it's not treated as "missing" for any data type in webidl. So while there is a case to be made for treating explicit/implicit (should behave the same) undefined as missing, explicit null should in fact be treated as empty dictionary, period.

@alvestrand
Copy link

If explicit null means empty (truly empty, no members) dictionary, then null is an illegal value for any argument whose type is a dictionary with required members.
So such arguments aren't nullable, no matter how you try to decorate them.

I'm OK with that. It probably is worth calling out explicitly.

@emlun
Copy link

emlun commented Jul 12, 2018

Yes, null would be an illegal value for a member whose type is a dictionary with required members - but (if I understand @bzbarsky correctly) undefined could be a legal value for such a member unless that member is required.

@bzbarsky
Copy link
Collaborator Author

OK. So if we stick to just arguments... Currently the spec only defaults trailing optional dictionary arguments to empty dictionary. Is there a reason not to do this for non-trailing ones?

@annevk
Copy link
Member

annevk commented Sep 28, 2018

You mean in the event that someone passes undefined? Would that currently pass undefined through to the algorithm?

@bzbarsky
Copy link
Collaborator Author

You mean in the event that someone passes undefined?

Yes. So given this IDL:

dictionary Foo  {
  long x = 5;
};
[Constructor]
interface Bar {
  void myMethod(optional Foo foo, long baz);
};

and this JS:

new Bar().myMethod(undefined, 3);

is there a reason to support the "foo" arg being missing as opposed to being an empty dictionary (or more precisely a dictionary with an x member whose value is 5)? And yes, this is a pretty weird API and people shouldn't really create APIs like this.

@annevk
Copy link
Member

annevk commented Sep 28, 2018

If we can I'd prefer forbidding such APIs, but otherwise defaulting makes sense I suppose.

@bzbarsky
Copy link
Collaborator Author

We can certainly forbid it. I'm not sure when the ability to add non-trailing optional arguments was added. https://www.w3.org/Bugs/Public/show_bug.cgi?id=10336 was asking for it but was wontfixed. None of the other issues or bugzilla tickets seem obviously relevant, and digging through blame with all the mass-reformattings is hard... I can do it if really needed, though; I suspect it's blame on the overload resolution algorithm and its handling of undefined that matters here. It might be helpful to understand what the use cases were for non-trailing optionals to decide what to do with dictionaries that fit that description.

@Manishearth
Copy link

Perhaps as a stopgap we could allow setting dictionary members as nullable if they're required dictionaries?

e.g.

dictionary Foo {required bool thing;};
dictionary Bar {Foo? foo;}

stumbling across this while designing XRTest, which isn't a web-exposed API but it doesn't seem like an uncommon pattern

@bzbarsky
Copy link
Collaborator Author

@Manishearth What is the exact usecase? Note that you can already write:

dictionary Foo { required bool thing;};
dictionary Bar { Foo foo; }

and this will do the right thing if you initialize Bar with an object with no foo property. The question is what should happen if someone explicitly initializes Bar with { foo: null } , right? But why are people doing that, exactly?

@Manishearth
Copy link

and this will do the right thing if you initialize Bar with an object with no foo property

Hmm. I might be confused as to the current state of things.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 3, 2019
…onal dictionary. r=bz

Summary:
In order to allow for optional dictionaries with required members
See whatwg/webidl#76 for more information.

Reviewers: bzbarsky

Tags: #secure-revision

Bug #: 1471165

Differential Revision: https://phabricator.services.mozilla.com/D1833

UltraBlame original commit: 3f1d5e7ef2449764526ec086f27c1550516dcdc6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 3, 2019
…onal dictionary. r=bz

Summary:
In order to allow for optional dictionaries with required members
See whatwg/webidl#76 for more information.

Reviewers: bzbarsky

Tags: #secure-revision

Bug #: 1471165

Differential Revision: https://phabricator.services.mozilla.com/D1833

UltraBlame original commit: 3f1d5e7ef2449764526ec086f27c1550516dcdc6
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 3, 2019
…onal dictionary. r=bz

Summary:
In order to allow for optional dictionaries with required members
See whatwg/webidl#76 for more information.

Reviewers: bzbarsky

Tags: #secure-revision

Bug #: 1471165

Differential Revision: https://phabricator.services.mozilla.com/D1833

UltraBlame original commit: 3f1d5e7ef2449764526ec086f27c1550516dcdc6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
☕☕ difficulty:medium Hard to fix ⌛⌛ duration:medium Shouldn't be too long to fix
Development

No branches or pull requests

10 participants