void -> undefined, and allow it anywhere. #906

tabatkins · 2020-08-05T21:59:10Z

Fixes #60

saschanaz · 2020-08-05T22:07:33Z

index.bs

@@ -2520,7 +2525,6 @@ steps are:” followed by a list, or “The <code>|operation|(<var ignore>arg1</
 <pre class="grammar" id="prod-ReturnType">
    ReturnType :
        Type


Is this needed anymore?

Not really, but I left it in because maybe it's useful to distinguish that in the future? I can just fold it away if y'all think that would be better.

Let's fold it away; I think a big win of this PR is unifying the two value spaces.

styfle · 2020-08-05T22:56:41Z

index.bs

@@ -14547,7 +14549,7 @@ are passed to it or what kind of value is returned from it.

 <h3 id="VoidFunction" callback>VoidFunction</h3>

-<pre class="idl">callback VoidFunction = void ();</pre>
+<pre class="idl">callback VoidFunction = undefined ();</pre>


Should this be UndefinedFunction?

I didn't think it was worth changing the name there, and requiring all users of VoidFunction to update. It's still a meaningful name post-update.

Note that 4 specs in reffy-reports use VoidFunction: CSS Layout, CSS Paint, HTML, and WebRTC.

domenic

Wow, this turned out simpler than I expected. I guess the actual change, e.g. allowing it in unions, falls out of changing the grammar?

Let's fold in Type into ReturnType. Then we should make sure we're ready for a big spec update of this magnitude before merging:

File issues on all the places located in https://github.com/heycam/webidl#filing-issues-elsewhere, pointing to this about-to-be-merged PR.
@saschanaz, can you get your bot ready to pull request all the specs?
/cc @foolip and @stephenmcgruer who has been doing some IDL stuff, I think related to reffy reports. (Maybe reffy should also go in that list of places to file issues?)

domenic · 2020-08-06T19:18:03Z

index.bs

@@ -2520,7 +2525,6 @@ steps are:” followed by a list, or “The <code>|operation|(<var ignore>arg1</
 <pre class="grammar" id="prod-ReturnType">
    ReturnType :
        Type


Let's fold it away; I think a big win of this PR is unifying the two value spaces.

saschanaz · 2020-08-06T19:32:41Z

@saschanaz, can you get your bot ready to pull request all the specs?

Sure, but the code currently does not support HTML and WebGL, as those specs manually decorate their IDL while others have pure IDL. Patching those two should thus be manual.

tabatkins · 2020-08-06T21:26:36Z

I guess the actual change, e.g. allowing it in unions, falls out of changing the grammar?

Yup, just had to make it a PrimitiveType, rather than a special ReturnType only.

Let's fold in Type into ReturnType.

Two votes for, so new commit coming shortly.

tabatkins · 2020-08-06T21:37:32Z

File issues on all the places located in [...]

Will get to that shortly, thanks for the pointer.

saschanaz · 2020-08-06T21:54:30Z

index.bs

@@ -1791,11 +1791,17 @@ The following extended attributes are applicable to constants:

 <pre class="grammar" id="prod-ConstValue">
    ConstValue :
+        UndefinedLiteral


Is this for #905?

No, unrelated. It just looked correct, since I was adding a new literal, that it should be added to the Const productions as well.

Does this PR uses the new literal elsewhere? I think this is the only use.

Perhaps this should be deferred to #905?

Does this PR uses the new literal elsewhere? I think this is the only use.

No, this is it. But the other primitives all have a literal, except null (which is a little weird, but it's handled as a type annotation rather than a type on its own, so eh).

I could remove this if necessary, it just seemed nice.

It'd probably be better to avoid unnecessary indirections in the grammar.

saschanaz · 2020-08-06T23:22:01Z

Does an argument typed undefined make sense? Should it be prevented?

tabatkins · 2020-08-06T23:55:42Z

Does an argument typed undefined make sense? Should it be prevented?

By itself, like DOMString foo(undefined bar)? Probably not. In a union, tho, definitely yes. I don't think it's worth trying to grammar-finesse it out of existence; doing so wouldn't be avoiding any footguns, just complexifying the grammar.

saschanaz · 2020-08-07T12:40:33Z

Probably not. In a union, tho, definitely yes.

I think optional or = undefined should be used instead of making a union in an argument, unless I missed something. Probably return type is the only place that a undefined union makes sense.

domenic · 2020-08-07T14:36:50Z

I think optional or = undefined should be used instead of making a union in an argument, unless I missed something. Probably return type is the only place that a undefined union makes sense.

Good point... Hmm. Is that something that the grammar can prevent? Otherwise we should add a prose restriction.

tabatkins · 2020-08-07T15:05:48Z

It is, but it would require adding ReturnType back in and making it more complex, duplicating the Union grammars into Return* variants that allow undefined.

Like I said, tho, I don't think this is a footgun that we actually need to protect anyone against. We can put in a prose restriction if y'all feel it's absolutely necessary, but I really don't want to enforce this in the grammar.

domenic · 2020-08-07T15:06:49Z

I think this is pretty important to enforce in the grammar.

saschanaz · 2020-08-07T15:08:54Z

Agreed, it would be very verbose to force it in the grammar. It should probably be okay to just write a prose, as WPT has a validator that should prevent authors to do that.

domenic · 2020-08-07T15:11:59Z

I don't understand why it's important to avoid verbosity in the grammar here. What else is the grammar for, if not to enforce restrictions of this sort?

tabatkins · 2020-08-07T15:33:42Z

Oh dang, upon review it would actually be even worse than I thought to handle in the grammar - I'd have to split the typedef production as well so that typedefs with an undefined in them are only usable in return types. :( Basically every single place where a type is mentioned needs to split into a Return* variant to enforce this.

I don't understand why it's important to avoid verbosity in the grammar here.

Overall, simple grammars are better because they're easier to read and understand, and easier to maintain over time without mistakes or oversights. Grammar simplicity is mostly a "spec author" convenience in the priority of constituencies, slightly a "UA author" issue.

What else is the grammar for, if not to enforce restrictions of this sort?

"Of this sort", sure. My objection is that this isn't actually a restriction worth enforcing in the first place, so it's not worth adding a bunch of complexity for. We should add restrictions when we think spec authors will actually make a mistake that they'd appreciate being informed about.

If we're really concerned with people doing = undefined defaults before we make a final decision about optional, I'll just remove that part of the patch. But banning undefined from argument types seems like something we really don't need to care about, imo.

domenic · 2020-08-07T15:42:18Z

I think this is a very important that people not use undefined in argument types, for the reasons listed above. If we can't enforce that via grammar, fine, but we need a prose restriction at the very least.

tabatkins · 2020-08-07T15:56:09Z

Ah yeah, and on further consideration, even if I split the grammar it wouldn't help, because doing so would also strongly break the LL(1) nature of the grammar, instead now requiring infinite lookahead to parse (because an undefined in the very last token of a very large union would affect which grammar you're parsing with). I'm pretty sure that's a no-go right away, then.

I'll remove the const bit and add a prose restriction, one sec.

…as a const value and forbid it from being an argument type.

saschanaz · 2020-08-07T17:58:40Z

I guess it might not make sense on dictionaries either. I wonder we should allow it only at return type position and some template parameters including Promise.

tabatkins · 2020-08-07T21:40:55Z

(Sorry, deleted previous comment, I was misreading.)

I guess it might not make sense on dictionaries either.

Hm, because dictionary entries can be marked as "optional" too?

saschanaz · 2020-08-07T21:43:55Z

Hm, because dictionary entries can be marked as "optional" too?

Yeah, the entries are optional by default so undefined means nothing. And attributes, we probably don't want to define attributes that return undefined. Could we narrow the scope here?

domenic · 2020-08-07T21:44:36Z

Well, we definitely want attributes which return undefined OR something else. But I agree restricting dictionaries is a good idea.

(In particular, if we didn't restrict dictionaries, then spec authors would have to choose between setting their dictionary members to undefined vs. leaving them absent... ugh.)

This change to the parser at the moment is only internal, and in this commit an affected override is corrected accordingly. IDL files still cal use `void` at the moment. Chromium change: https://chromium.googlesource.com/chromium/src/+/5e8a1af4b7b9bb9a7d2948758b41ef3bf9abc36f commit 5e8a1af4b7b9bb9a7d2948758b41ef3bf9abc36f Author: Yuki Shiino <[email protected]> Date: Wed Nov 1 15:08:24 2023 +0000 bindings: Rename 'void' to 'undefined' in bindings/scripts/ This patch renames only the internal implementation code in web_idl/ and bind_gen/, and doesn't rename any IDL code in *.idl files. Web IDL 2.13.2. undefined https://webidl.spec.whatwg.org/#idl-undefined See also whatwg/webidl#906 Bug: 1116522

void -> undefined, and allow it anywhere. whatwg#60

ca6198e

tabatkins changed the title ~~void -> undefined, and allow it anywhere. https://github.com/heycam/w…~~ void -> undefined, and allow it anywhere. Aug 5, 2020

tabatkins mentioned this pull request Aug 5, 2020

add a type to represent the undefined value #60

Closed

saschanaz reviewed Aug 5, 2020

View reviewed changes

styfle reviewed Aug 5, 2020

View reviewed changes

domenic reviewed Aug 6, 2020

View reviewed changes

Now that ReturnType has collapsed to a single production, fold it away.

3f5d774

domenic approved these changes Aug 6, 2020

View reviewed changes

saschanaz reviewed Aug 6, 2020

View reviewed changes

saschanaz mentioned this pull request Aug 6, 2020

feat: support undefined type w3c/webidl2.js#486

Merged

2 tasks

Pending final decision on optional in whatwg#905, remove 'undefined' …

1695d3a

…as a const value and forbid it from being an argument type.

domenic approved these changes Aug 7, 2020

View reviewed changes

nektro mentioned this pull request Jun 27, 2024

Remove deprecated void support w3c/webidl2.js#784

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

void -> undefined, and allow it anywhere. #906

void -> undefined, and allow it anywhere. #906

tabatkins commented Aug 5, 2020 •

edited

Loading

saschanaz Aug 5, 2020

tabatkins Aug 5, 2020

domenic Aug 6, 2020

styfle Aug 5, 2020 •

edited

Loading

tabatkins Aug 5, 2020

saschanaz Aug 5, 2020 •

edited

Loading

domenic left a comment

domenic Aug 6, 2020

saschanaz commented Aug 6, 2020

tabatkins commented Aug 6, 2020

tabatkins commented Aug 6, 2020

saschanaz Aug 6, 2020

tabatkins Aug 6, 2020

saschanaz Aug 6, 2020 •

edited

Loading

tabatkins Aug 6, 2020

domenic Aug 7, 2020

saschanaz commented Aug 6, 2020

tabatkins commented Aug 6, 2020

saschanaz commented Aug 7, 2020

domenic commented Aug 7, 2020

tabatkins commented Aug 7, 2020

domenic commented Aug 7, 2020

saschanaz commented Aug 7, 2020

domenic commented Aug 7, 2020

tabatkins commented Aug 7, 2020 •

edited

Loading

domenic commented Aug 7, 2020

tabatkins commented Aug 7, 2020

saschanaz commented Aug 7, 2020 •

edited

Loading

tabatkins commented Aug 7, 2020

saschanaz commented Aug 7, 2020

domenic commented Aug 7, 2020 •

edited

Loading

void -> undefined, and allow it anywhere. #906

void -> undefined, and allow it anywhere. #906

Conversation

tabatkins commented Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

styfle Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz Aug 5, 2020 • edited Loading

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz commented Aug 6, 2020

tabatkins commented Aug 6, 2020

tabatkins commented Aug 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz Aug 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschanaz commented Aug 6, 2020

tabatkins commented Aug 6, 2020

saschanaz commented Aug 7, 2020

domenic commented Aug 7, 2020

tabatkins commented Aug 7, 2020

domenic commented Aug 7, 2020

saschanaz commented Aug 7, 2020

domenic commented Aug 7, 2020

tabatkins commented Aug 7, 2020 • edited Loading

domenic commented Aug 7, 2020

tabatkins commented Aug 7, 2020

saschanaz commented Aug 7, 2020 • edited Loading

tabatkins commented Aug 7, 2020

saschanaz commented Aug 7, 2020

domenic commented Aug 7, 2020 • edited Loading

tabatkins commented Aug 5, 2020 •

edited

Loading

styfle Aug 5, 2020 •

edited

Loading

saschanaz Aug 5, 2020 •

edited

Loading

saschanaz Aug 6, 2020 •

edited

Loading

tabatkins commented Aug 7, 2020 •

edited

Loading

saschanaz commented Aug 7, 2020 •

edited

Loading

domenic commented Aug 7, 2020 •

edited

Loading