Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Clarifications to dictionary semantics #859
Changes from 1 commit
da4a89f
882f3c8
bf042d5
92a21a0
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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:
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.
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.
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.
Cite https://w3ctag.github.io/design-principles/ (https://w3ctag.github.io/design-principles/#prefer-dict-to-bool in particular)?
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.