-
Notifications
You must be signed in to change notification settings - Fork 165
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
Clarifications to dictionary semantics #859
Conversation
This rewrites some of the text around dictionaries to clear up the confusion in #852, as well as generally try to smooth out the reading experience and definitions. Concrete changes: * Rewrote "dictionaries are always passed by value" to be more specific and clear. * Moved "dictionaries must not be used as the type of an attribute or constant" near the top of the dictionary section, commensurate with its importance. * Removed the terms "present" and "not present"; instead we can use Infra's "exists" for maps. * Fixed several cases where the overload resolution algorithm discussed arguments being "not present", even though that was only defined for dictionaries. Instead it now correctly talks about "missing". * Rewrote the discussion of required dictionary members and dictionary members with default values to make it clear how they contribute to the dictionary's entries. * Added a note to the IDL-to-ES conversion algorithm for dictionaries explicitly pointing out that default-value dictionary members will always be present, and thus always show up in the output.
@EdgarChen and/or @annevk, would one of you be able to review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this looks okay, but @EdgarChen / @TimothyGu should probably do a final check.
index.bs
Outdated
<dfn id="dfn-dictionary-member-default-value" for="dictionary member" export>default value</dfn>, | ||
which is the value used by default when author code or specification text does not provide a value | ||
for that member. Note that [=dictionary member/required=] dictionary members only have meaning for | ||
dictionaries used as operation arguments; members should be left optional if a dictionary is only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new, right? Why is this a should and are implementers on board with enforcing this so we can ensure it doesn't end up in specifications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is new. It's just a should saying not to add pointless stuff. It's not likely to be easily enforceable on an implementation level because it would require whole-corpus analysis to find all the dictionary usage sites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not entirely clear to me if it's pointless as it's basically a specification-level assert. But since it can't be used consistently (unless you always have distinct output dictionaries, which also isn't great), I'm inclined to agree it's better not to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the "only have meaning" part isn't clear: of course the word "required" has meaning in general!
How about
Note that specifying dictionary members as required only has an observable effect when converting other representations of dictionaries (like an ECMAScript value supplied as an argument to an operation) to an IDL dictionary. Specification authors should leave members optional in all other cases, including when a dictionary type is used solely as the return type of operations.
We should also define "optional" as "not required" in the context of dictionary members.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section isn't talking about the word "required" in general. It's say "required dictionary members only have meaning...". So I think it's still fairly clear. Your rephrasing is OK too though.
Co-Authored-By: Anne van Kesteren <[email protected]>
index.bs
Outdated
Dictionaries are always passed by value: the dictionary does not retain a reference to its | ||
language-specific representation (e.g., the corresponding ECMAScript object). So for example, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like the parts before and after the colon are very different things. To me, having IDL dictionaries be passed by value means (and I'm not saying this is accurate) that in a scenario like:
- Let dictionary be an IDL dictionary.
- Perform some algorithm X given dictionary.
where "some algorithm X" modifies the given dictionary, then any modifications to dictionary will not be observable directly outside of it.
But I feel like the actual point here is that the ES-to-IDL conversion process destroys all links with the original object, unlike the case with interface types. Maybe we need some different wording to get that across.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "will not be observable directly outside of it", except for the idea that when the dictionary is converted to ES, it will not have such a link. So they seem like the same before and after the colon to me.
That said, I'd be happy to just remove "Dictionaries are always passed by value". "Pass by value" is a notoriously confusing concept and it might be better to just explicitly state what we mean, as the parts after the colon try to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removal sounds good to me.
I understood "pass by value" as passing the IDL dictionaries passed around different spec-level algorithms by value, while you understood it as passing ES objects around into IDL operations by value. See if this helps with understanding.
index.bs
Outdated
<dfn id="dfn-dictionary-member-default-value" for="dictionary member" export>default value</dfn>, | ||
which is the value used by default when author code or specification text does not provide a value | ||
for that member. Note that [=dictionary member/required=] dictionary members only have meaning for | ||
dictionaries used as operation arguments; members should be left optional if a dictionary is only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel the "only have meaning" part isn't clear: of course the word "required" has meaning in general!
How about
Note that specifying dictionary members as required only has an observable effect when converting other representations of dictionaries (like an ECMAScript value supplied as an argument to an operation) to an IDL dictionary. Specification authors should leave members optional in all other cases, including when a dictionary type is used solely as the return type of operations.
We should also define "optional" as "not required" in the context of dictionary members.
encouraged not to use <emu-val>true</emu-val> as the [=dictionary member/default value=] for | ||
{{boolean}}-typed [=dictionary members=], as this can be confusing for authors who might | ||
otherwise expect the default conversion of <emu-val>undefined</emu-val> to be used (i.e., | ||
<emu-val>false</emu-val>). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do that in a followup if desired, for both dictionaries and boolean arguments in general.
Co-Authored-By: Timothy Gu <[email protected]>
whatwg/webidl#859 removed the "present" and "not present" terms; specs are supposed to use Infra's "exists" for dictionaries instead. We had already done this in w3c#440, but there were some leftovers in the Automation section. Related to w3c#444.
This rewrites some of the text around dictionaries to clear up the confusion in #852, as well as generally try to smooth out the reading experience and definitions. Concrete changes:
Closes #852. Closes #524.
Preview | Diff