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

Specify open dictionaries. #180

Merged
merged 20 commits into from Oct 17, 2016
Merged

Conversation

jyasskin
Copy link
Member

@jyasskin jyasskin commented Sep 30, 2016

Let me know what you think. Preview at https://rawgit.com/jyasskin/webidl/open-dictionaries/index.html.

I tried to position dictionary<K, V> as a dictionary type, but we may need to move some things from https://rawgit.com/jyasskin/webidl/open-dictionaries/index.html#idl-dictionaries to https://rawgit.com/jyasskin/webidl/open-dictionaries/index.html#idl-dictionary to really make that work.

I introduced a "mappings" term for the contents of an open dictionary, but I might be able to re-use "dictionary members".

I used <div algorithm> blocks to get better <var> checking, but that also results in boxes around the algorithms, which you might or might not like.

@bzbarsky @tobie @annevk


Preview | Diff

Per Commit Preview & Diffs

2e084f4: Preview | Diff w/ Head (2016-09-28 23:59)
2008d7f: Preview | Diff w/ Head | Diff w/ Previous (2016-09-30 09:38)
2b5f2a8: Preview | Diff w/ Head | Diff w/ Previous (2016-09-30 23:24)
3dc8ca9: Preview | Diff w/ Head | Diff w/ Previous (2016-09-30 23:29)
1a8ae76: Preview | Diff w/ Head | Diff w/ Previous (2016-09-30 23:35)
05252bf: Preview | Diff w/ Head | Diff w/ Previous (2016-09-30 23:44)
747ba1e: Preview | Diff w/ Head | Diff w/ Previous (2016-09-30 23:58)
6646a17: Preview | Diff w/ Head | Diff w/ Previous (2016-10-01 00:02)
7c5c25a: Preview | Diff w/ Head | Diff w/ Previous (2016-10-01 00:05)
756f55d: Preview | Diff w/ Head | Diff w/ Previous (2016-10-01 00:06)
399fc10: Preview | Diff w/ Head | Diff w/ Previous (2016-10-01 00:17)
59c34ac: Preview | Diff w/ Head | Diff w/ Previous (2016-10-04 01:18)
06ca8e2: Preview | Diff w/ Head | Diff w/ Previous (2016-10-04 01:48)
84d1bc3: Preview | Diff w/ Head | Diff w/ Previous (2016-10-04 21:08)
0386c5d: Preview | Diff w/ Head | Diff w/ Previous (2016-10-04 21:20)
fc878b0: Preview | Diff w/ Head | Diff w/ Previous (2016-10-06 20:30)
fe44f9c: Preview | Diff w/ Head | Diff w/ Previous (2016-10-14 18:51)
665d408: Preview | Diff w/ Head | Diff w/ Previous (2016-10-14 18:56)
9e22b41: Preview | Diff w/ Head | Diff w/ Previous (2016-10-14 20:07)
Latest (20c46cc): Preview | Diff w/ Head | Diff w/ Previous (2016-10-14 20:29)

@annevk
Copy link
Member

annevk commented Sep 30, 2016

I was wondering if the enumeration logic when converting from JavaScript should be closer to step 22, substep 3 of https://html.spec.whatwg.org/multipage/infrastructure.html#structuredclone. (I'm not really sure one way or another, but since they're both copying operations.)

@annevk
Copy link
Member

annevk commented Sep 30, 2016

Also, should we revert #13 at this point? It was added at the time as some kind of proposed replacement for this, but didn't actually address the dictionary-like problem and has since gone unused.

annevk added a commit that referenced this pull request Sep 30, 2016
This feature was introduced by #13, but the main thing we actually
needed for Headers et al was open dictionaries (see #180). As such this
feature has not seen any use.
1. If [=Type=](|O|) is not <emu-val>Object</emu-val>,
<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
1. Let |result| be a new empty instance of <code>{{dictionary}}<|K|, |V|></code>.
1. Let |entries| be [=EnumerableOwnProperties=](|O|, "key+value").
Copy link
Member Author

Choose a reason for hiding this comment

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

@annevk I think step 22, substep 3 of Structured Clone is equivalent to EnumerableOwnProperties. There is an interesting question about what order we should type-check vs [[Get]]ing the values. I see at least four options:

  1. Get all properties before checking their types (what I have now).
  2. Get and check all the keys, and then interleave getting values with checking them.
  3. Get all keys, then check a key, get its value, check its value, repeat.
  4. Get all keys, then get a key's value, check the key, check the value, repeat.

For sane objects these are the same, but for getters with side-effects they're not.

Copy link
Member

Choose a reason for hiding this comment

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

Right, I don't have an opinion here, but I think the current approach is different when it comes to getters and I don't know what is better. I just thought I'd raise it to see if anyone else had thoughts on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, StructuredClone does interleave getting with possible exceptions from the recursive StructuredClone call.

Copy link
Member

Choose a reason for hiding this comment

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

You should not use EnumerableOwnProperties here, because EnumerableOwnProperties creates an array. This mixes levels between the abstract (list/records/ES spec devices) and the concrete (Arrays), and also makes it unclear whether in subsequent steps when you do [key, value] what the exact semantics are (e.g. is it impacted by changes to Array.prototype or the iteration protocol).

Copy link
Member Author

Choose a reason for hiding this comment

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

I've switched to EnumerableOwnProperties("key"), which returns a List.

@tobie
Copy link
Collaborator

tobie commented Sep 30, 2016

I used <div algorithm> blocks to get better <var> checking, but that also results in boxes around the algorithms, which you might or might not like.

We can always fix the styling if we don't like it, but we should absolutely put all algorithm in <div algorithm> blocks. I'll fix the other algorithms in the spec shortly.

You might also want to wrap your lists in <ol class="algorithm"> as this gives us the light blue left-hand border which makes visualizing nesting in algorithms much easier (this probably should be automatic within algorithm blocks but isn't for now /cc @tabatkins).

@jyasskin
Copy link
Member Author

speced/bikeshed#759 should have made the blue border happen inside algorithm blocks, and it's working in Web Bluetooth. I'm not sure what's different here.

@@ -5171,6 +5175,7 @@ type.
"DOMException" Null
BufferRelatedType Null
"FrozenArray" "&lt;" Type "&gt;" Null
"dictionary" "&lt;" Type "," Type "&gt;" Null
Copy link
Collaborator

@tobie tobie Sep 30, 2016

Choose a reason for hiding this comment

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

You probably want to give it its own non-terminal to account for the first Type being optional:

<pre class="grammar" id="prod-OpenDictionaryType">
    OpenDictionaryType :
        "dictionary" "&lt;" Type "&gt;"
        "dictionary" "&lt;" Type "," Type "&gt;"
</pre>

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, done.

bzbarsky pushed a commit that referenced this pull request Sep 30, 2016
This feature was introduced by #13, but the main thing we actually
needed for Headers et al was open dictionaries (see #180). As such this
feature has not seen any use.
@bzbarsky
Copy link
Collaborator

I believe this proposed grammar is not LL(1) parseable. Specifically, OpenDictionaryType looks like a textbook example of https://en.wikipedia.org/wiki/LL_parser#FIRST.2FFIRST_Conflict. I'm willing to be proven wrong on this point, of course. ;) I think basic refactoring something like this:

OpenDictionaryType ::
    dictionary < Type OpenDictionaryTypeTail

OpenDictionaryTypeTail ::
    >
    , Type >

will address that.

That said, I wish we addressed the "first type must be one of these three things" bit in grammar, not prose. That, though, would cause an irreconcilable FIRST/FIRST conflict, I'm pretty sure. That part is rather annoying. I'm open to ideas for how we can have our LL(1) cake and eat our grammar-enforced sanity too. One obvious way is to make the key type be the second type. Another obvious way is to use different names for the two things... Maybe I'm just overthinking this.

Apart from that, I see the following issues:

  1. Using dictionary here could be somewhat confusing with the totally separate concept of dictionaries, IMO... We may want to come up with a different name.
  2. I'm not sure I follow the "V must be a type allowed for dictionary members." thing: all types are allowed as dictionary members.
  3. We need distinguishability information, overload resolution handling, and union conversion handling for this new type. In practice, it will behave much like "object".
  4. Do we want to allow passing null or undefined to mean "empty open dictionary" the way we do for dictionaries?
  5. I don't know whether the intent here is to be black-box identical in terms of the enumeration to structured clone step 22 substep 3, but this proposed logic is not black-box identical to that, because it calls MOP operations in a different order. It also doesn't call the exact same ones; there's no equivalent of the HasOwnProperty check from structured clone here, because the different ordering, which interleaves the [[GetOwnProperty]] and [[Get]], calls makes it less important to do that. For what it's worth, what Gecko implements for MozMap matches neither of these algorithms; I think we do structured clone step 22 substep 3, but without the extra check in substep 3 subsubstep 1. We also do our conversion to an IDL value right where structured clone would do the StructuredClone recursive call.
  6. "Repeat, for each element [key, value] of entries" is a bit confusing, because each element of entries is an ES Array. This needs to spec how to get the key and value out of the array (probably via Get() calls, not the destructuring that the current language is so suggestive of). Alternately, taking a more structured clone like approach makes this not be an issue, of course.

<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
1. Let |result| be a new empty instance of <code>{{dictionary}}<|K|, |V|></code>.
1. Let |entries| be [=EnumerableOwnProperties=](|O|, "key+value").
1. [=ReturnIfAbrupt=](|entries|).
Copy link
Member

Choose a reason for hiding this comment

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

Use ? notation instead of ReturnIfAbrupt.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

1. If [=Type=](|O|) is not <emu-val>Object</emu-val>,
<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
1. Let |result| be a new empty instance of <code>{{dictionary}}<|K|, |V|></code>.
1. Let |entries| be [=EnumerableOwnProperties=](|O|, "key+value").
Copy link
Member

Choose a reason for hiding this comment

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

You should not use EnumerableOwnProperties here, because EnumerableOwnProperties creates an array. This mixes levels between the abstract (list/records/ES spec devices) and the concrete (Arrays), and also makes it unclear whether in subsequent steps when you do [key, value] what the exact semantics are (e.g. is it impacted by changes to Array.prototype or the iteration protocol).

ECMAScript value|converted=] to an ECMAScript value as follows:

1. Let |result| be a new <emu-val>Object</emu-val> instance
created as if by the expression <code>{}</code>.
Copy link
Member

Choose a reason for hiding this comment

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

Web IDL may use this phrase elsewhere, but from now on we should be saying "Let result be ! ObjectCreate(%ObjectPrototype%)".

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

let obj = {__proto__: proto, d: 5, c: 6}
Object.defineProperty(obj, "e", {value: 7, enumerable: false});
let result = identity(obj);
assert(result.a === undefined);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I like using console.assert in these examples, since it is an actual function that exists in browsers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


Dictionary keys and values can be constrained, although keys can only be
constrained among the three string types. The following conversions all
throw {{TypeError}}s:
Copy link
Member

Choose a reason for hiding this comment

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

Is this true? I thought the conversion behavior was generally to do things like replacing surrogate pairs or returning 0. In particular I am almost certain that converting an object to a long does not throw a TypeError.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Conversion to ByteString will throw if one of the 16-bit code units in the string is larger than 255.

But yes, the other two examples here would not throw: USVString just sticks in a replacement char and converting a random object to an integer type will produce 0.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. I've changed the examples to have a "result" column which shows the coercion.

throw {{TypeError}}s:
<table class="data">
<thead><th>Value</th><th>Passed to type</th></thead>
<tr><td><code>{"😞": 1}</code></td><td><code>{{dictionary}}&lt;ByteString, long></code></td></tr>
Copy link
Member

Choose a reason for hiding this comment

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

Consider never using long in examples, per https://w3ctag.github.io/design-principles/#numeric-types. (long long would be more appropriate in this case.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I used double since I don't need the integer conversion.

@domenic
Copy link
Member

domenic commented Sep 30, 2016

IMO we should omit class=algorithm from this PR and, if people think it's desirable, do it in a separate pass all at once.

@bzbarsky
Copy link
Collaborator

because EnumerableOwnProperties creates an array

If only it did. What it returns is a List of Arrays, when used in "key+value" mode. ;)

@jyasskin
Copy link
Member Author

jyasskin commented Oct 1, 2016

I've done everything except the distinguishability information, which I'll work on now.

It looks impossible to enforce that the key is a string in an LL(1) grammar, so I've stayed with the semantic check. It would be possible if we put the key second, but I'm really reluctant to use that confusing syntax. I would be willing to make the key type required.

<ol class="algorithm">
1. Let |result| be a new empty instance of <code>{{OpenDictionary}}<|K|, |V|></code>.
1. If [=Type=](|O|) is <emu-val>Undefined</emu-val> or <emu-val>Null</emu-val>,
return |result|.
Copy link
Member

@domenic domenic Oct 1, 2016

Choose a reason for hiding this comment

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

This should throw a TypeError for consistency with closed dictionaries.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was in response to @bzbarsky's «Do we want to allow passing null or undefined to mean "empty open dictionary" the way we do for dictionaries?», and he's right that closed dictionaries treat null and undefined as empty.

Copy link
Member

Choose a reason for hiding this comment

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

Uh, what are you reading? Can you quote? I see there a very clear step 1 that says "If Type(V) is not Undefined, Null or Object, then throw a TypeError."

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wow, I totally misread the sentence that I quoted. Sorry. This seems good then.

1. If [=Type=](|O|) is <emu-val>Undefined</emu-val> or <emu-val>Null</emu-val>,
return |result|.
1. If [=Type=](|O|) is not <emu-val>Object</emu-val>,
<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
Copy link
Member

Choose a reason for hiding this comment

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

This check should not be there, for consistency with closed dictionaries.

Copy link
Member Author

Choose a reason for hiding this comment

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

Again, closed dictionaries do throw a TypeError for non-Object,Null,Undefined arguments: https://heycam.github.io/webidl/#es-to-dictionary

Copy link
Member

Choose a reason for hiding this comment

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

Again, can you quote? There's no such step that I can see; only for RegExp, which we are planning to remove (#148 #145).

Copy link
Member

Choose a reason for hiding this comment

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

As above, my bad; disregard this.

<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
1. Let |keys| be [=?=] [=EnumerableOwnProperties=](|O|, "key").
1. Repeat, for each element |key| of |keys| in [=List=] order:
1. If [=!=] [=HasOwnProperty=](|O|, |key|) is <code>true</code>, then:
Copy link
Member

Choose a reason for hiding this comment

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

Interesting. So this comes in to play if enumerating/getting causes side effects that delete the property. I don't think that's great semantics though. I think instead you want to inline the EnumerableOwnProperties algorithm here yourself, instead of separating the enumeration pass from the conversion/Get pass.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like you, @annevk, and @bzbarsky to come to some conclusion about this. So far, @annevk wondered if we should match StructuredClone(), and @bzbarsky said that MozMap is like StructuredClone but omits the HasOwnProperty() check (so, I think we'd include the key in the map but initialize it from undefined). I don't care which we do.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. @annevk, @bzbarsky, what do you think of my suggestion? I'm not sure why we do what we do in StrucuredClone, and I suspect legacy constraints.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe @ajklein can tell us? Would rather we make an informed decision here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I doubt there is much in the way of legacy stuff around structured clone here, honestly.

I think structured clone was created back before there were proxies in ES (but I could totally be wrong about that!). If that were the case, then the [[Get]] calls in that algorithm would have been the only thing that could have changed whether props exist, so it would have made sense to double-check that a previos [[Get]] has not removed the property or something.

In the proxy world, where HasOwnProperty can itself lie and remove the property or whatnot, and where [[GetOwnProperty]] can do likewise, it's not clear to me that there is any sort of sanity that can be provided in the face of a "malicious" input object that just tries to mess with you. So the only question is whether there are sane cases which would benefit from the extra double-checking.

In EnumerableOwnProperties(), the double-checking that exists is just a side-effect of checking for enumerability. That happens to also check that the property is still own, because it uses [[GetOwnProperty]]. But that looks to me like an accidental byproduct of the enumerability check.

Note that structured clone, as specified, is buggy: if one of those [[GetOwnProperty]] calls in step 22.3.2.1.1 returns undefined, what happens?

Also, it's not clear to me whether UAs actually match the structured clone spec. Might be worth checking that...

I guess all of that is to say that I see no obvious problem with removing the HasOwnProperty check in structured clone, with the understanding that this may cause the clone to have own props where the clonee doesn't have any by the time cloning is done... but that's possible now anyway, afaict.

Copy link
Member

Choose a reason for hiding this comment

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

I think what you're missing is that structured clone fails on proxies in step 19.

<tr>
<td><code>{"\uD83D": 1}</code></td>
<td><code>{{OpenDictionary}}&lt;USVString, double></code></td>
<td>[ "\uFFFD" ⇒ 1 ]</td>
Copy link
Member

Choose a reason for hiding this comment

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

It seems you are inconsistent about notation for the mappings; here you use an arrow, whereas above you use "Repeat, for each mapping (key, value) in D". Both seem reasonable, but it would be good to pick one and maybe mention it in the section that introduces the concept of mappings.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I don't want to make WebIDL's users type a non-ascii character in order to iterate over a map, so (key, value) is better for that. I can change to that here too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@tobie
Copy link
Collaborator

tobie commented Oct 3, 2016

@jyasskin wrote:

It looks impossible to enforce that the key is a string in an LL(1) grammar, so I've stayed with the semantic check. It would be possible if we put the key second, but I'm really reluctant to use that confusing syntax. I would be willing to make the key type required.

So I know just enough about grammars to be able to read them and defer to those more knowledgeable on the topic (pointers to good resources are welcomed, btw). I concur that putting the key second is a really bad idea and would suggest making both keys required. It's slightly more verbose, but also much clearer. I imagine we'd go for something like this?

OpenDictionaryType ::
    dictionary < OpenDictionaryKeyType , Type >

OpenDictionaryKeyType ::
    "DOMString"
    "USVString"
    "ByteString"

@bzbarsky
Copy link
Collaborator

If we interleave the Get()s we should interleave the to-IDL-value conversions too so we don't have to build up a temporary list of values. Imo.

1. If |desc| is not <emu-val>undefined</emu-val>
and |desc|.[=[[Enumerable]]=] is <emu-val>true</emu-val>:
1. Let |typedKey| be |key| [=converted to an IDL value=] of type |K|.
1. Let |value| be [=?=] [=Get=](|O|, |key|).
Copy link
Member Author

Choose a reason for hiding this comment

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

@domenic Done. Note that it's a little weird to call Get() when we already have the descriptor, but that's what Object.assign() does so oh well.

Copy link
Member

@domenic domenic left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, a few nits and suggestions


<ol class="algorithm">
1. Let |result| be a new empty instance of <code>{{record}}&lt;|K|, |V|></code>.
1. If [=Type=](|O|) is <emu-val>Undefined</emu-val> or <emu-val>Null</emu-val>,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: types (Undefined, Null, and Object) are not emu-vals. That is reserved for language values, not spec values.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Is there documentation for that?

Copy link
Member

Choose a reason for hiding this comment

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

https://bterlson.github.io/ecmarkup/#editorial-conventions I guess, although that's not the most obvious place to look given that we're basically only using emu-val.

1. If |typedKey| is already a key in |result|, set its value to |typedValue|.

Note: This can happen when |O| is a proxy object.
1. Otherwise, append to |result| a mapping from |typedKey| to |typedValue|.
Copy link
Member

Choose a reason for hiding this comment

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

Link mapping? And use (|typedKey|, |typedValue|) notation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

</ol>

<div class="example" id="example-es-record">
Passing the ECMAScript value <code>{b: 3, a: 4}</code> to a
Copy link
Member

Choose a reason for hiding this comment

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

s/to/as/

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

<div class="example" id="example-es-record">
Passing the ECMAScript value <code>{b: 3, a: 4}</code> to a
<code>{{record}}&lt;DOMString, double></code> argument
would result in the IDL value <code>[ ("b", 3), ("a", 4) ]</code>.
Copy link
Member

Choose a reason for hiding this comment

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

I don't know about this new array-ish notation. I'd instead say something like "a record containing the two mappings ("b", 3) and ("a", 4), in that order." Hmm, I guess it makes an appearance later in the table too, where it's more necessary for space reasons... not sure what to do. At least, remove the <code> from this one, IMO.

I think this also contradicts the statement "There is no way to represent a constant record value in IDL."

Copy link
Member Author

Choose a reason for hiding this comment

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

I think

There is no way to represent a constant record value in IDL.

means that we don't have syntax for it in the WebIDL grammar. The phrase,

a record containing the two mappings ("b", 3) and ("a", 4), in that order.

would still "represent a constant record value", but I'd like to have a more concise way to do it.

Copy link
Member

Choose a reason for hiding this comment

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

I'm still hesitant about using notation that's reminiscent of JS arrays, and is not defined anywhere. Maybe use some exotic type of bracket (such as the french double quotes that ES uses), and define it in the same place you define the mapping notation?

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, thanks!

@@ -7695,6 +7837,9 @@ represented by ECMAScript values that correspond to the union’s
1. If |types| includes a [=dictionary type=], then return the
result of [=converted to an IDL value|converting=]
|V| to that dictionary type.
1. If |types| includes a [=record type=], then return the
result of [=converted to an IDL value|converting=]
|V| to that dictionary type.
Copy link
Member

Choose a reason for hiding this comment

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

s/dictionary/record

Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, done.

return |result|.
1. If [=Type=](|O|) is not <emu-val>Object</emu-val>,
<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
1. Let |keys| be [=?=] |O|.[=[[OwnPropertyKeys]]=]().
Copy link
Member

Choose a reason for hiding this comment

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

IMO the links for the internal methods and [[enumerable]] are not good to add here. If we want to do that more generally, we can, but we shouldn't do it just in this section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member Author

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Thanks!

return |result|.
1. If [=Type=](|O|) is not <emu-val>Object</emu-val>,
<a lt="es throw">throw a <emu-val>TypeError</emu-val></a>.
1. Let |keys| be [=?=] |O|.[=[[OwnPropertyKeys]]=]().
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

1. If |typedKey| is already a key in |result|, set its value to |typedValue|.

Note: This can happen when |O| is a proxy object.
1. Otherwise, append to |result| a mapping from |typedKey| to |typedValue|.
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

</ol>

<div class="example" id="example-es-record">
Passing the ECMAScript value <code>{b: 3, a: 4}</code> to a
Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

<div class="example" id="example-es-record">
Passing the ECMAScript value <code>{b: 3, a: 4}</code> to a
<code>{{record}}&lt;DOMString, double></code> argument
would result in the IDL value <code>[ ("b", 3), ("a", 4) ]</code>.
Copy link
Member Author

Choose a reason for hiding this comment

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

I think

There is no way to represent a constant record value in IDL.

means that we don't have syntax for it in the WebIDL grammar. The phrase,

a record containing the two mappings ("b", 3) and ("a", 4), in that order.

would still "represent a constant record value", but I'd like to have a more concise way to do it.

@@ -7695,6 +7837,9 @@ represented by ECMAScript values that correspond to the union’s
1. If |types| includes a [=dictionary type=], then return the
result of [=converted to an IDL value|converting=]
|V| to that dictionary type.
1. If |types| includes a [=record type=], then return the
result of [=converted to an IDL value|converting=]
|V| to that dictionary type.
Copy link
Member Author

Choose a reason for hiding this comment

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

Whoops, done.

@tobie tobie merged commit 1814384 into whatwg:gh-pages Oct 17, 2016
@jyasskin jyasskin deleted the open-dictionaries branch October 17, 2016 15:45
@bzbarsky
Copy link
Collaborator

@tobie, @jyasskin, @domenic Are we sure we want the "is already present" behavior as checked in? That seems a bit annoying to implement in terms of performance, and it's not really clear to me why we're doing it at all.

tobie added a commit to tobie/webidl that referenced this pull request Oct 17, 2016
Fix distinguishable algorithm. Closes whatwg#190.
Fix record handling in various algorithms. Closes whatwg#191, closes whatwg#189
tobie added a commit to tobie/webidl that referenced this pull request Oct 17, 2016
Fix distinguishable algorithm. Closes whatwg#190.
Fix record handling in various algorithms. Closes whatwg#191, closes whatwg#189
tobie added a commit to tobie/webidl that referenced this pull request Oct 17, 2016
Fix distinguishable algorithm. Closes whatwg#190.
Fix record handling in various algorithms. Closes whatwg#191, closes whatwg#189.
tobie added a commit that referenced this pull request Oct 18, 2016
Fix distinguishable algorithm. Closes #190.
Fix record handling in various algorithms. Closes #191, closes #189.
@jyasskin
Copy link
Member Author

@bzbarsky I don't see "is already present" in the spec. Do you mean the "If typedKey is already a key in result," wording? That's the same operation as in Map.prototype.set, which I believe we implement efficiently. I do think we want to avoid the possibility of a record winding up with a duplicate key: it can only happen with a proxy object input, and spec authors are unlikely to consider it and handle it well.

@bzbarsky
Copy link
Collaborator

Do you mean the "If typedKey is already a key in result," wording?

Yes.

That's the same operation as in Map.prototype.set, which I believe we implement efficiently.

It's possible to do. It just requires some performance/memory/complexity tradeoffs, which it's not clear to me are valuable here.

I do think we want to avoid the possibility of a record winding up with a duplicate key

I guess the question is whether it's better to have implementation complexity/overhead to handle the rare proxy case or to have specification complexity instead.

https://dev.w3.org/html5/html-design-principles/#priority-of-constituencies would normally err on the side of specification complexity, but this is an odd case where we are likely to have more specifiers affected than implementors, so maybe this is the right tradeoff...

In any case, I just wanted to make sure we are explicitly and deliberately making this choice and understand that it's likely to cause implementation delays, in general.

@jyasskin
Copy link
Member Author

We could also throw a TypeError in the case of a duplicated key, which would allow the implementation to just keep a set of used keys, instead of needing to be able to find the value associated with a previously-used key.

@bzbarsky
Copy link
Collaborator

That makes things a bit simpler, but not really that much simpler. You still need some sort of dual-purpose data structure (O(1) lookup and fixed-order iteration).

@annevk
Copy link
Member

annevk commented Oct 19, 2016

The specification complexity would be making that duplicate key check and then throwing? Seems better handled by IDL, or would specifications handle it in other ways?

@bzbarsky
Copy link
Collaborator

The specification complexity would be making that duplicate key check and then throwing?

Or whatever they want to do in the duplicate case. @jyasskin is probably right that spec authors won't even realize they should handle it, so it probably does make sense to handle in IDL. It just makes me a bit sad that ES proxies don't enforce this themselves per spec. :(

@domenic
Copy link
Member

domenic commented Oct 19, 2016

It looks like there is talk of proxies enforcing that in tc39/ecma262#461. I am not sure whether there was consensus to change though...

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

Successfully merging this pull request may close these issues.

None yet

7 participants