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

Arguments allow double extended attributes #691

Closed
saschanaz opened this issue Mar 19, 2019 · 8 comments
Closed

Arguments allow double extended attributes #691

saschanaz opened this issue Mar 19, 2019 · 8 comments

Comments

@saschanaz
Copy link
Member

Argument ::
    ExtendedAttributeList ArgumentRest

ArgumentRest ::
    optional TypeWithExtendedAttributes ArgumentName Default
    Type Ellipsis ArgumentName

ArgumentName ::
    ArgumentNameKeyword
    identifier

Currently the syntactic item Argument is defined as above, syntactically allowing double extended attributes only for optional arguments but not for non-optional one: [ExtAttr] optional [Clamp] short argname. Is this intended?

I think this causes ambiguity before reading the token optional, where the first extended attribute typically is for the type but not when optional.

@saschanaz saschanaz changed the title Extended attributes for arguments? Arguments allow double extended attributes Mar 19, 2019
@annevk
Copy link
Member

annevk commented Mar 19, 2019

This seems like something we should get rid of.

@bzbarsky
Copy link
Collaborator

I'm not sure I understand what the problem is.

void foo([ExtAttr] optional [Clamp] short argname);
void bar([ExtAttr, Clamp] short argname);

In the former case (foo), everything is clear: [ExtAttr] applies to the argument as a whole, while [Clamp] applies to the type.

In the latter case (bar) things are a bit more complicated because there is only one space to put extended attributes. Both [ExtAttr] and [Clamp] are initially applied to the argument as a whole, but then [Clamp] is propagated through to the type by https://heycam.github.io/webidl/#idl-type-extended-attribute-associated-with step 4. So the end result is that [Clamp] applies to the type and both extended attrs apply to the argument itself. Since the argument itself does nothing with its extended attributes for the moment, that's fine. If that were to change, whatever it does would need to ignore the extended attributes matching https://heycam.github.io/webidl/#extended-attributes-applicable-to-types

The fact that you can have extended attributes on the argument itself is intentional; while Web IDL itself does not currently define any such extended attributes, consumers of Web IDL might.

@saschanaz
Copy link
Member Author

saschanaz commented Mar 20, 2019

@bzbarsky So you mean void bar([Clamp] optional short argname); is semantically correct, because it will be propagated? @domenic previously opened https://github.com/w3c/respec/issues/1184 so I thought it was incorrect.

@bzbarsky
Copy link
Collaborator

So you mean void bar([Clamp] optional short argname); is semantically correct

No, it's not. We should probably fix the propagation to only happen for non-optional arguments...

@bzbarsky
Copy link
Collaborator

@domenic thoughts on #691 (comment) ?

@annevk annevk reopened this Mar 20, 2019
@domenic
Copy link
Member

domenic commented Mar 20, 2019

So that'd be modifying step 4 of https://heycam.github.io/webidl/#idl-type-extended-attribute-associated-with ? Seems reasonable to me.

@saschanaz
Copy link
Member Author

Note that this also applies to dictionary members:

DictionaryMember ::
    ExtendedAttributeList DictionaryMemberRest

DictionaryMemberRest ::
    required TypeWithExtendedAttributes identifier ;
    Type identifier Default ;

like [Clamp] required short membername, which is related with step 5.

@saschanaz
Copy link
Member Author

Looks like the step 4 and 5 already makes the propagation to only happen for non-optional/non-required members, as:

If type appears as part of a Type production directly within an Argument production

... a direct Type production currently means no optional or required.

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