whatwg / webidl

Web IDL Standard
https://webidl.spec.whatwg.org/
Other
405 stars 162 forks source link

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

Open bzbarsky opened 8 years ago

bzbarsky commented 8 years ago

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 commented 8 years ago

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

jan-ivar commented 8 years ago

See https://bugzilla.mozilla.org/show_bug.cgi?id=1226475#c6

domenic commented 8 years ago

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 commented 8 years ago

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.
bzbarsky commented 7 years ago

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

jan-ivar commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

(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 commented 6 years ago

(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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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"?

jan-ivar commented 6 years ago

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

What about records? Would they work the same?

jan-ivar commented 6 years ago

I mentioned the two specs not as examples of great APIs, but as evidence the webidl semantics here are unintuitive.

bzbarsky commented 6 years ago

I pointed out that the syntax itself is counter intuitive.

Which particular syntax?

Having things labeled optional which are effectively required, seem like semantic footguns

That seems like a fair criticism. When we added "required", we allowed making dictionaries with a required member non-optional arguments even in trailing position. Maybe we should just disallow them from being marked optional, period, because that's never a sensible thing to do. That seems tangential to the issue at hand, though.

It's also not to spec

What spec, exactly?

as an "actual JS developer"

My point was that if we're trying to figure out what a "good" JS API is, the right way to do that is to examine APIs designed by people who write JS all the time for use by other people who write JS all the time. I'm not quite sure what you're asking here...

What about records?

Again, conceptually, an empty record and a missing record mean the same thing: empty list of stuff. What would be a use case where you'd want to tell apart the two cases? That case seems a lot more clear-cut than the dictionary case, because there is no equivalent of required, right?

I keep thinking that adding required to dictionaries was a mistake. :(

annevk commented 6 years ago

If we did not have required folks would implement that semantic in prose and we'd have much more inconsistency than we'd have now. E.g., type of exceptions, exception ordering, exception or ignored.

emlun commented 6 years ago

I agree with @jan-ivar that defaulting optional dictionary members to the empty dictionary is a violation of the principle of least astonishment. As noted in https://github.com/w3c/webauthn/issues/750#issuecomment-401752095, as an API author I expect to be able to express parameter structures where "this thing is optional, but if it's present then there's a required thing inside it".

Reading through this thread, though, I note that the case of nested dictionaries seems very different from the case of dictionaries as function arguments. I'm willing to agree that as function arguments, it's reasonable that optional dictionary arguments default to the empty dictionary - so that "no bag of arguments" is equivalent to "empty bag of arguments" as noted by @bzbarsky. However, dictionaries as optional dictionary members should default to undefined (unless an explicit default is given), because having those default to {} would unnecessarily make WebIDL much less expressive.

equalsJeffH commented 6 years ago

@emlun

I note that the case of nested dictionaries seems very different from the case of dictionaries as function arguments.

are not those two cases related since (as we do in webauthn) one can have an "options dictionary", itself containing dictionary members, that is passed as a function argument? e.g.:

var publicKey = {
  [...]
  // Relying Party:
  rp: {
    name: "ACME Corporation"
  },

  // User:
  user: {
    id:[...],
    name: "alex.p.mueller@example.com",
    displayName: "Alex P. Müller",
    icon: "https://pics.example.com/00/p/aBjjjpqPb.png"
  },
 [...]

navigator.credentials.create({ publicKey })
  .then....;

..?

emlun commented 6 years ago

Yes, the two cases are indeed related, but semantically different (IMO). As @bzbarsky points out,

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). [...]

In this sense, I can agree that undefined, null and {} should all be equivalent - but I do not agree that the same rule should apply for the members inside that root dictionary, as I explain in https://github.com/w3c/webauthn/issues/750#issuecomment-401752095.

So under my proposition, with the definitions

function foo(Arguments args) {
}

dictionary Arguments {
  Thing aThing;
}

dictionary Thing {
  required DOMString id;
}

these invocations would all be equivalent and valid:

but this would be invalid due to the missing id member:

...whereas if aThing also implicitly defaults to {}, then all of the above invocations would be equivalent, and therefore invalid since the last one is invalid.

bzbarsky commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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.

emlun commented 6 years ago

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 https://github.com/heycam/webidl/issues/76#issuecomment-401766639 is that null be equivalent to {} for function arguments but not for dictionary members.

bzbarsky commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

bzbarsky commented 6 years ago

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 commented 6 years ago

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

bzbarsky commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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.