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

Should nullable optional dictionary argument have a default value? #774

Closed
saschanaz opened this issue Aug 23, 2019 · 10 comments
Closed

Should nullable optional dictionary argument have a default value? #774

saschanaz opened this issue Aug 23, 2019 · 10 comments

Comments

@saschanaz
Copy link
Member

saschanaz commented Aug 23, 2019

The Media Session spec has the following IDL:

dictionary MediaPositionState {
  double duration;
  double playbackRate;
  double position;
};

[Exposed=Window]
interface MediaSession {
  // -- snip --
  void setPositionState(optional MediaPositionState? state);
};

Currently the prose says:

If the type of an argument is a dictionary type or a union type that has a dictionary type as one of its flattened member types, and that dictionary type and its ancestors have no required members, and the argument is either the final argument or is followed only by optional arguments, then the argument must be specified as optional and have a default value provided.

Per my understanding the spec requires a default value for optional nullable dictionaries, but is it intended?

@annevk
Copy link
Member

annevk commented Aug 23, 2019

Why is this dictionary made nullable? (It's not clear to me we should allow that at all.)

@saschanaz
Copy link
Member Author

AFAIK somehow an author decided to do that without any real benefit, per my reading.

@annevk
Copy link
Member

annevk commented Aug 23, 2019

Filed w3c/mediasession#231. Perhaps we should look into forbidding this? At least when there's no required members? And even then it seems like a poor pattern, but I vaguely recall it being too late for nested dictionaries...

@Ms2ger
Copy link
Member

Ms2ger commented Aug 23, 2019

I don't think the prose you quote actually requires a default here (it's not a dictionary type or a union type), but agree that the API doesn't make sense as specced.

@bzbarsky
Copy link
Collaborator

void setPositionState(optional MediaPositionState? state);

This is not valid IDL. This is mentioned in the note under https://heycam.github.io/webidl/#idl-nullable-type and in the following normative text under https://heycam.github.io/webidl/#idl-operations>:

If the Type of an operation argument is an identifier followed by ?, then the identifier must identify an interface, enumeration, callback function, callback interface, or typedef.

which explicitly excludes "dictionary" from the list, and the following paragraph, which says:

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

@annevk
Copy link
Member

annevk commented Aug 26, 2019

Thanks, then we can close this I think as there's already a downstream issue.

@saschanaz
Copy link
Member Author

👍 Thanks for the details!

@bzbarsky
Copy link
Collaborator

I should note that the IDL parser in Firefox already catches this problem with the IDL, though our error reporting there could be better. I'll file a bug on that later today when I can log into bugzilla again.

@foolip This is another instance of Blink's binding generator not enforcing Web IDL constraints and that causing Google engineers to write invalid spec IDL... I didn't see an existing issue on this, so filed https://bugs.chromium.org/p/chromium/issues/detail?id=997708

@saschanaz
Copy link
Member Author

Note that I found this when I run my webidl2.js validator (which currently incorrectly suggests adding a default value for an invalid nullable dictionary argument) against WPT, maybe we can still catch this type of problems there.

@bzbarsky
Copy link
Collaborator

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1577016 on the Gecko side.

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

No branches or pull requests

4 participants