w3c / webidl2.js

WebIDL parser
https://w3c.github.io/webidl2.js/checker/
MIT License
239 stars 62 forks source link

Validate proper use of extended attributes #570

Open foolip opened 3 years ago

foolip commented 3 years ago

Web IDL has a lot of requirements about which extended attributes may be used where. For example:

There's bound to be some violations of these rules in the wild, so it would be good to validate them.

(Required [Exposed] extended attributes are already validated.)

foolip commented 3 years ago

I hacked something into webref's consistency tests to show the use for top-level definitions, which won't cover attributes and members. Here's the list of things that appear:

[Exposed] callback interface
[Exposed] dictionary
[Exposed] enum
[Exposed] interface
[Exposed] interface mixin
[Exposed] namespace
[Exposed] partial interface
[Exposed] partial namespace
[Global] interface
[HTMLConstructor] interface
[LegacyFactoryFunction] interface
[LegacyNamespace] interface
[LegacyNoInterfaceObject] interface
[LegacyOverrideBuiltIns] interface
[LegacyOverrideBuiltIns] partial interface
[LegacyTreatNonObjectAsNull] callback
[LegacyUnenumerableNamedProperties] interface
[LegacyWindowAlias] interface
[SecureContext] interface
[SecureContext] interface mixin
[SecureContext] partial interface
[Serializable] interface
[Transferable] interface

That confirms that there's some invalid use of at least [Exposed] for dictionaries and enums.

foolip commented 3 years ago

I've sent https://github.com/WICG/portals/pull/265 and https://github.com/w3c/css-houdini-drafts/pull/1023 to fix the issues I spotted.

tidoust commented 2 years ago

On top of validating the proper use of extended attributes, could there be an option to validate additional grammar constraints for extended attributes?

The Web IDL spec leaves the grammar of extended attributes fairly open: "The ExtendedAttribute grammar symbol matches nearly any sequence of tokens". The Web IDL parser rightfully accepts anything there.

However, the Web IDL spec also defines a more restricted syntax for the specific extended attributes that are practically used in web specs. I don't think that the parser should validate that syntax by default, but in practice we would typically want to validate these constraints on all web specs that define some IDL.

A recent example is the (invalid) definition of SandboxWindowProxy in WebDriver BiDi:

[Exposed=]
interface SandboxWindowProxy : WindowProxy {};

The Exposed= extended attribute is valid per the global grammar. It is invalid per the more constrained one. Code in webref currently crashes on the AST structure returned by the Web IDL parser because it takes for granted that there would be a right hand side to the definition. I'll fix the code in Webref, but we probably don't want that IDL to land in IDL extracts and Web Platform Tests in any case so we'll need to catch the invalid definition somewhere.

saschanaz commented 2 years ago

Oh, AFAIK the parser already only supports a subset of what's allowed. I'll take a look.

saschanaz commented 2 years ago

I took a look, but the parser already fails with that code.

Exception while parsing WebIDL. See JavaScript console for more details.

WebIDLParseError: Syntax error at line 1:
[Exposed=]
         ^ No right hand side to extended attribute assignment

Code in webref currently crashes on the AST structure returned by the Web IDL parser

So I'm not sure how this could happen, as the parser return nothing but an exception 🤔. Try it yourself on https://w3c.github.io/webidl2.js/checker/.

saschanaz commented 2 years ago

Arrrrrgh, actually no, I fixed it last year in #635 and forgot to publish it. 🤦‍♀️

Anyway, 24.2.1.