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

Dictionary types being nullable doesn't make sense #967

Open
tabatkins opened this issue Mar 13, 2021 · 19 comments
Open

Dictionary types being nullable doesn't make sense #967

tabatkins opened this issue Mar 13, 2021 · 19 comments

Comments

@tabatkins
Copy link
Contributor

Per https://heycam.github.io/webidl/#idl-nullable-type, dictionary types are allowed to be nullable (they're not excluded by any of the conditions), but unions containing dictionary types can't be nullable.

That is, MyDict? is valid, but (MyDict or DOMString)? is not.

This doesn't seem to make sense. Per https://heycam.github.io/webidl/#es-dictionary, JS null is always acceptable to convert to a dictionary type; nullability doesn't enter into the equation at all. In other words, dictionary types are implicitly nullable. You can also see this more easily in https://heycam.github.io/webidl/#es-union, where both null and undefined can be turned into a dictionary type if a union includes one.

Tracing the blame back, this change occurred in 6ac261b, with the reasoning given being "Disallow dictionaries from being nullable only as operation argument types.". But that edit preserved the fact that JS null is always convertable to a dictionary type, so I'm not sure what it was actually trying to do.

I suggest that we revert that change and just always disallow dictionary types from being nullable (since they're always implicitly nullable already).

@annevk
Copy link
Member

annevk commented Mar 13, 2021

I think this might be a thing some people do with nested dictionaries. Or perhaps return values? (I agree with you that it's a bad pattern.)

@saschanaz
Copy link
Member

Do we currently have a use for nested nullable dictionaries?

@annevk
Copy link
Member

annevk commented Mar 15, 2021

I'm not sure.

There is some related discussion in #76 and one thing bz mentions there that null always means empty dictionary, but I'm not sure if that's applicable to a dictionary in a union as well.

@tabatkins
Copy link
Contributor Author

I may have hidden my point in some of the details.

MyDict and MyDict? are identical in terms of what values satisfy them, but both are allowed. (DOMString or MyDict), (DOMString or MyDict?), and (DOMString or MyDict)? are identical as well, but the last one is disallowed.

6ac261b was the commit that happened to make the change that allowed MyDict?, but (I assume by accident?) left the prohibition against (DOMString or MyDict?) alone, resulting in today's confusing world where dictionaries are explicitly nullable only sometimes (but implicitly nullable all the time).

My suggestion is that we return to the prohibition against explicitly nullable dictionaries everywhere, so MyDict? and (DOMString or MyDict?) both go back to being invalid syntax. This won't result in any observable API change, since a JS null always satisfies a dict type anyway.

@tabatkins
Copy link
Contributor Author

one thing bz mentions there that null always means empty dictionary, but I'm not sure if that's applicable to a dictionary in a union as well.

It is. Note step 3 in https://heycam.github.io/webidl/#es-union, where if the JS value is null or undefined, and the union contains a dictionary type, the JS value gets converted into the dictionary type.

@annevk
Copy link
Member

annevk commented Mar 15, 2021

That's only if the union type does not include a nullable type, due to step 1:

If the union type includes a nullable type and V is null or undefined, then return the IDL value null.

Looking at https://heycam.github.io/webidl/#es-dictionary I don't think nested dictionaries are a concern.

So I think I agree that we can disallow explicitly nullable dictionaries.

Is (DOMString? or MyDict) disallowed as well? I guess it should be?

@saschanaz
Copy link
Member

I wonder we should check whether any existing spec will be affected, since the resulting value would be different anyway (null vs. an empty dictionary).

@annevk
Copy link
Member

annevk commented Mar 15, 2021

Ah yes, we should check return values.

@saschanaz
Copy link
Member

saschanaz commented Mar 15, 2021

Also for non-return-value cases, because

undefined foo(Bar? bar);

here foo(null) will give null for bar whereas it would be an empty dictionary without ?. Changing prose would be enough in such case, though. (Unless anyone is differentiating null and empty dictionaries.)

@tabatkins
Copy link
Contributor Author

tabatkins commented Mar 15, 2021

Is (DOMString? or MyDict) disallowed as well? I guess it should be?

Already disallowed, check the paragraph a little below the list at https://heycam.github.io/webidl/#dfn-number-of-nullable-member-types:

The number of nullable member types of a union type must be 0 or 1, and if it is 1 then the union type must also not have a dictionary type in its flattened member types.

That's only if the union type does not include a nullable type, due to step 1:

Ahhhh, I'd skipped over that somehow, right. So while the set of input values accepted is identical, the IDL value that comes out of the conversion is different. I don't think there's a great reason to change that, particularly if there's any existing specs depending on it.

Okay, so my real concern is just making the rules consistent rather than arbitrary (so I can write parallel consistent rules for undefined); I don't much care which side of the line we land on. So instead, then, I suggest we just allow dictionaries in nullable unions.

That is, taking the cases mentioned earlier:

  • MyDict: allowed today, will still be allowed, JS null gives default dict
  • MyDict?: allowed today, will still be allowed, JS null gives IDL null
  • (DOMString or MyDict): allowed today, will still be allowed, JS null gives default dict
  • (DOMString or MyDict?): allowed today, will still be allowed, JS null gives IDL null
  • (DOMString or MyDict)?: disallowed today, will be allowed, JS null gives IDL null
  • (DOMString? or MyDict): disallowed today, will still be disallowed (unclear which type should eat a JS null)

@annevk
Copy link
Member

annevk commented Mar 15, 2021

@saschanaz per

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. If the operation argument type is an identifier not followed by ?, then the identifier must identify any one of those definitions or a dictionary.

that is disallowed. I would be curious about the cases where we still allow T? with T being a dictionary and if they occur in practice. It seems like that would be return values and attributes? (It's also explicitly disallowed for nested dictionaries.)

If we can completely disallow it I think that would be better.

@saschanaz
Copy link
Member

@saschanaz per

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. If the operation argument type is an identifier not followed by ?, then the identifier must identify any one of those definitions or a dictionary.

that is disallowed. I would be curious about the cases where we still allow T? with T being a dictionary and if they occur in practice. It seems like that would be return values and attributes? (It's also explicitly disallowed for nested dictionaries.)

If we can completely disallow it I think that would be better.

Ah right, but not everywhere though.

@tabatkins
Copy link
Contributor Author

As far as I can tell, while MyDict? foo() is disallowed by that text, (DOMString or MyDict?) foo() is not. There's some strange mixing of abstraction levels here, where it's explicitly talking about syntax ("identifier followed by ?") while also talking about types ("nullable type", slightly later). Due to this, I think it ends up missing the "nullable dict in a union" case.

Looking over the rest of the sections...

  • consts are moot, as they're restricted to a small set of types
  • attributes explicitly prevent dicts from appearing at all
  • operations (including the special operations) have the weird restrictions that I think don't quite work, but clearly want to disallow nullable dicts as their return type
  • value iterators are implicitly bound by the same restrictions as the indexed property getter, but pair iterators aren't, and can include nullable dicts
  • maplikes and setlikes can include nullable dicts in their types
  • callback functions can return nullable dicts

So aside from callback functions (which are fairly rare and I doubt use nullable dicts anywhere), only the newest sorts of constructs can return a nullable dict, and it should be safe (not to mention consistent) to ban them there as well.

That leaves us with just arguments that can still potentially take nullable dicts. @saschanaz any reasonable way to search for instances of this?

@domenic
Copy link
Member

domenic commented Mar 15, 2021

Chiming in from the sidelines, it looks like you're starting to run into the general incoherence we have around union types and typedefs: #649, especially #827, #670.

@saschanaz
Copy link
Member

The best way I can think of is to play with webidl2.js and webref with some added checks.

@tabatkins
Copy link
Contributor Author

it looks like you're starting to run into the general incoherence we have around union types and typedefs

YUP

@domenic
Copy link
Member

domenic commented Apr 22, 2021

@tabatkins any interest in fixing this one too since we finally merged your undefined PR? We can promise quicker reviews this time :)

@tabatkins
Copy link
Contributor Author

Yeah I'm happy to, I'll try to get to it next week.

@saschanaz
Copy link
Member

saschanaz commented Apr 23, 2021

That leaves us with just arguments that can still potentially take nullable dicts. @saschanaz any reasonable way to search for instances of this?

https://heycam.github.io/webidl/#idl-operations

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

So I think it also wants to prevent dictionaries from being nullable there, although (DOMString or MyDict?) is not prevented. webidl2.js already validates it under no-nullable-dict-arg rule.

Edit:

I tweaked webidl2.js, ran it on webref, and found no argument with nullable dictionaries including ones in unions. 🎉

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