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

Type association restrictions for extended attributes break with unions #827

Open
bzbarsky opened this issue Dec 12, 2019 · 12 comments
Open

Comments

@bzbarsky
Copy link
Collaborator

bzbarsky commented Dec 12, 2019

Consider IDL that says:

  void foo([AllowShared] BufferSource);

Per spec as written right now, this is invalid, because https://heycam.github.io/webidl/#AllowShared says:

A type that is not a buffer source type must not be associated with the [AllowShared] extended attribute.

and ArrayBufferView is in fact not a buffer source type. The rules in https://heycam.github.io/webidl/#idl-type-extended-attribute-associated-with propate the extended attribute to all the things in the union (ignoring for the moment the typedef complications), but don't remove the currently-invalid association with the outer union type...

We should really fix both this and #670, presumably, but it's not clear to me what the goals of these restrictions really were and hence how they should be loosened while preserving those goals.

@domenic @annevk

@annevk
Copy link
Member

annevk commented Dec 12, 2019

Would changing it to

A type that is not a union or a buffer source type must not be associated with the [AllowShared] extended attribute.

work in that we don't really care about unions as type other than using them as a grouping mechanism?

That would forbid [AllowShared] (ArrayBuffer or DOMString), but that seems okay?

@bzbarsky
Copy link
Collaborator Author

Hmm. That would be one option. The other would be to:

  1. Define a concept of "type A that may be type B" which would then be defined to do sane things for unions and nullables.
  2. Allow [AllowShared] on any type that may be a buffer source type.

That's equivalent to the proposal in #827 (comment) as far as observable behavior for [AllowShared] ArrayBufferView and also allows [AllowShared] ArrayBufferView?, though we would need some changes to https://heycam.github.io/webidl/#idl-type-extended-attribute-associated-with to propagate through nullables. It seems like we should do that, since fundamentally ? is pretty much like a union type but with different syntax...

If we wanted to also allow [AllowShared] (ArrayBuffer or DOMString) we could fix union propagation to only propagate if the extended attr applies to the inner type, right? I don't really see a reason to forbid it...

@bzbarsky
Copy link
Collaborator Author

In particular, do we want to have [AllowShared] on things like the arg to XHR send()?

@domenic
Copy link
Member

domenic commented Dec 12, 2019

In particular, do we want to have [AllowShared] on things like the arg to XHR send()?

I don't think we do? Instead you should annotate the buffer-source types specifically, i.e. it should change to

typedef (Blob or [AllowShared] BufferSource or FormData or URLSearchParams or ReadableStream or USVString) BodyInit;

If we wanted to also allow [AllowShared] (ArrayBuffer or DOMString) we could fix union propagation to only propagate if the extended attr applies to the inner type, right? I don't really see a reason to forbid it...

I think we should forbid it because it's significantly less clear than ([AllowShared] ArrayBuffer or DOMString)

@bzbarsky
Copy link
Collaborator Author

OK. And similar for [Clamp] and the like?

So it sounds like we should allow an extended attribute on a union only if it applies to everything inside the union, is the thinking?

@domenic
Copy link
Member

domenic commented Dec 12, 2019

That makes the most sense to me, at least.

@annevk
Copy link
Member

annevk commented Dec 12, 2019

My intuition is similar to that of @domenic, FWIW.

The nullable variant is interesting though, i.e., is [AllowShared] Uint8Array? okay? I don't have any use cases for it at the moment, but it seems like that has to work as you cannot write ([AllowShared] Uint8Array or null).

(Aside: now that we have unions maybe or null is a better syntax.)

@bzbarsky
Copy link
Collaborator Author

For the nullable case you could do:

    typedef [AllowShared] Uint8Array Something;

and then

  void foo(Something? arg);

to force the [AllowShared] to bind to the inner type, but it's kinda annoying...

@annevk
Copy link
Member

annevk commented Dec 12, 2019

I guess I'd say that for this case and the [Clamp] long? case the intermediate solution is to allow (and mostly ignore) extended attributes to apply to the null case of a nullable type and a potential longer term solution (especially now that Kagami has a tool to rewrite IDL in specifications) is to introduce a null type and get rid of nullables.

@bzbarsky
Copy link
Collaborator Author

And yes, we could try to move nullables over to union syntax, with the complication that null would not be a type you can ever really use standalone, presumably... and it would still need to get exempted from this checking so people can do [AllowShared] (BufferSource or null) (which WebGL wants to do; they are doing [AllowShared] BufferSource? right now).

@annevk
Copy link
Member

annevk commented Dec 12, 2019

Could they not do ([AllowShared] BufferSource or null) in this magical future?

@bzbarsky
Copy link
Collaborator Author

Hmm. Yes, I think they could.

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

3 participants