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

undefined can't be a member of a union type #962

Closed
petervanderbeken opened this issue Mar 9, 2021 · 17 comments
Closed

undefined can't be a member of a union type #962

petervanderbeken opened this issue Mar 9, 2021 · 17 comments

Comments

@petervanderbeken
Copy link

I think one of the goals of #60 was to allow union types with undefined as a member type for return values. It did so by just adding undefined to the grammar for Type. Union types currently require member types to be distinguishable (Each pair of flattened member types in a union type, T and U, must be distinguishable). I don't see any changes to that in #906, or any changes to add rules for undefined in the distinguishability algorithm, so right now undefined cannot actually be a member type of a union type.

The prose for union type conversion to an ECMAScript value also still contains the following sentence: If the value is an object reference to a special object that represents an ECMAScript undefined value, then it is converted to the ECMAScript undefined value. I'm not sure we should keep that, there is a rule for conversion of the undefined type, so it should be possible to rely on the generic algorithm there.

@petervanderbeken
Copy link
Author

It might work to make undefined distinguishable from all other types.

@annevk
Copy link
Member

annevk commented Mar 9, 2021

cc @tabatkins

@tabatkins
Copy link
Contributor

Oooh, I didn't realize additional spec work had to be done there.

Yes, both of these changes are reasonable, I'll write a PR.

@tabatkins
Copy link
Contributor

tabatkins commented Mar 11, 2021

Looking over the remaining spots where undefined is mentioned, I'm not entirely sure what to do about the "convert an ES value to an IDL union type" part, and I think it raises questions about distinguishability too:

If V is null or undefined, then:

  • If types includes a dictionary type, then return the result of converting V to that dictionary type.

So this implies that the current behavior is that a JS undefined can always be converted into a dictionary type. I presume this is fallout from the fact that dictionary arguments are required to be optional. Does this imply that undefined and dictionary types should not be distinguishable, tho? Or is it okay for them to remain distinguishable, so I'll add a new first step that checks if the union type contains undefined, and then the dictionary conversion is a fallback?

(Also, hm, all of the 3.2.24 algo confuses me, since it's composed of nested conditionals where the inner conditionals don't have "else" clauses. Is the implication that you continue to next top-level step if you fail all the conditions?)

tabatkins added a commit to tabatkins/webidl that referenced this issue Mar 11, 2021
@annevk
Copy link
Member

annevk commented Mar 12, 2021

It might be okay for them to be distinguishable in theory, but I don't think we'd ever want to have APIs use that.

And yeah, if the algorithm doesn't return, it continues, ultimately throwing.

@petervanderbeken
Copy link
Author

When converting from ES to IDL union there should never be undefined in the union, since IDL undefined is forbidden in arguments and in dictionaries? At least that's how I read "undefined must not be used as the type of an argument in any circumstance …, or as the type of a dictionary member".

@domenic
Copy link
Member

domenic commented Mar 12, 2021

I would err on the side of them not being distinguishable.

@tabatkins
Copy link
Contributor

since IDL undefined is forbidden in arguments and in dictionaries

Yes, it's forbidden only in those places. It's allowed in return types, attribute types, etc.

(And it's only forbidden in those places because we have optional filling the same purpose.)

I would err on the side of them not being distinguishable.

Works for me. I've edited the distinguishability table, and that means I can still add a rule checking for undefined in the union.


New question now that I've reviewing more here - currently, the spec says that undefined satisfies a nullable type (first step of https://heycam.github.io/webidl/#es-union).

So, per current spec, you can satisfy an IDL Foo? type with a JS undefined. That implies that (Foo? or undefined) should be invalid due to lack of distinguishability, right? (So I need to augment step 1 of https://heycam.github.io/webidl/#dfn-distinguishable to also mention undefined.)

But the opposite is not true - you can't satisfy a IDL undefined type with a JS null, so an IDL undefined? type still makes sense, right? (And so I do not need to modify https://heycam.github.io/webidl/#dfn-nullable-type to exclude undefined.)

(This didn't come up in the previous void case, since void couldn't be combined with things in this way...)

@tabatkins
Copy link
Contributor

Hm, no, dictionary types are explicitly excluded from being nullable, so undefined should be too.

@tabatkins
Copy link
Contributor

Okay, chasing the blame backwards, dictionary types are excluded from nullability specifically because the spec has always considered JS null to be capable of satisfying a dictionary type. (I presume this is legacy behavior from it being hard, in IDL-ese, to talk about JS undefined.)

But I don't want that to be the case for undefined, right? Like, an attribute defined with (undefined or FooInterface) attr; should throw if you try to assign null to it, right? If so, then I do want to allow undefined to be nullable.

Or do we want to maintain the old IDL idea that JS's null and undefined were the same value?

@annevk
Copy link
Member

annevk commented Mar 13, 2021

I think it makes sense for undefined to be nullable. We don't want to perpetuate null and undefined being the same.

(It might make even more sense if T? became a shorthand for T or null or undefined. I had completely forgotten undefined satisfied T? (at least on the input side; the other annoyance (but also nice sometimes) with IDL is it conflating inputs and outputs).)

@tabatkins
Copy link
Contributor

Yeah, this accords with my preference as well. I'll need to do a more thoro eye-check to make sure that nullable and dictionary types are defined to play well with the undefined type everywhere.

@petervanderbeken
Copy link
Author

I had totally missed that writable attributes can take undefined in a union.

What would be the use for undefined being nullable? I don't think that would make a lot of sense outside of unions.

I like T? being shorthand for T or null or undefined (sidenote: there is no null type), but if that's a real union, wouldn't that make it impossible to distinguish null and undefined in a union? T? or undefined would be invalid, and existing APIs would expect the current conversion from ES nullor undefined to IDL null, so it seems like we wouldn't be able to fix it on that level.

@annevk
Copy link
Member

annevk commented Mar 15, 2021

Thanks, I guess it should be a shorthand for T or [LegacyUndefinedToNull] null. I suppose that's a reason to get rid of the shorthand.

@petervanderbeken
Copy link
Author

If you insert a rule at the top of the union conversion that If the union type includes the undefined type and V is undefined, then return the IDL value undefined, then that allows unions that treat null and undefined as separate values (just need to also include a nullable type in the union). If one wants null and undefined to be treated the same, then just don't include undefined in the union. The algorithm then falls back to If the union type includes a nullable type and V is null or undefined, then return the IDL value null. It's not great for distinguishability (null and undefined are distinguishable but also not?), but I think that because of the existing conversion behaviour it's hard to reconcile both. Unless you hide that behind something like [LegacyUndefinedToNull], but that seems pretty daunting.

@tabatkins
Copy link
Contributor

Yeah, planning to let an explicit undefined in a union win out over both nullable and dictionary conversions, so it would be meaningful to have (undefined or Foo?) and (undefined or MyDict); passing a JS undefined would yield the IDL undefined in both cases, rather than a null'd Foo or defaulted MyDict.

@tabatkins
Copy link
Contributor

Okay, #965 should close this now.

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

5 participants
@domenic @tabatkins @annevk @petervanderbeken and others