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

Drop IDL extended attributes from data-type values #2790

Merged
merged 2 commits into from
Feb 9, 2024

Conversation

tidoust
Copy link
Contributor

@tidoust tidoust commented Feb 9, 2024

This is a fix for #2705, where the problem is that Bikeshed dies on argumentdef tables when a spec defines an IDL interface attribute or a dictionary member that comes with extended attributes, something like

dictionary test {
	required [EnforceRange] short member1;
}

To produce the ", of type foo" content that appears next to the term in the table, Bikeshed relies on the data-type attribute set in the IDL block. The value of that attribute comes from a type field returned by widlparser. The code in Bikeshed expected that value to be a real IDL type (a simple type or a union) but that construct in widlparser is a "type with extended attributes": https://github.com/plinss/widlparser/blob/6c91beb68806fd6c206c57cecd15e455b0b6c1c1/widlparser/productions.py#L1294

As a result, the type Bikeshed sees is [EnforceRange] short in the above example, which cannot be directly associated with a type in Web IDL.

This update gets back to actual IDL types in the data-type attribute, which effectively solves the problem of the ", of type foo" content generation. A new idl008 test was added to check extended attributes.

One side effect of that update is that data-type attribute values no longer contain trailing spaces. That seems a good thing (spaces where already stripped for attributes, but not for dictionary members), but required re-generating a bunch of reference tests.

Note: If the inclusion of extended attributes in the data-type attribute was actually intended, then the fix would rather need to be done in attributeInfo.py.

This is a fix for speced#2705, where the problem is that Bikeshed dies on argumentdef
tables when a spec defines an IDL interface attribute or a dictionary member
that comes with extended attributes, something like

```
dictionary test {
	required [EnforceRange] short member1;
}
```

To produce the ", of type foo" content that appears next to the term in the
table, Bikeshed relies on the `data-type` attribute set in the IDL block. The
value of that attribute comes from a `type` field returned by widlparser. The
code in Bikeshed expected that value to be a real IDL type (a simple type or
a union) but that construct in widlparser is a "type with extended attributes":
https://github.com/plinss/widlparser/blob/6c91beb68806fd6c206c57cecd15e455b0b6c1c1/widlparser/productions.py#L1294

As a result, the type Bikeshed sees is `[EnforceRange] short` in the above
example, which cannot be directly associated with a type in Web IDL.

This update gets back to actual IDL types in the `data-type` attribute, which
effectively solves the problem of the ", of type foo" content generation. A
new idl008 test was added to check extended attributes.

One side effect of that update is that `data-type` attribute values no longer
contain trailing spaces. That seems a good thing (spaces where already stripped
for attributes, but not for dictionary members), but required re-generating a
bunch of reference tests.
@tabatkins tabatkins merged commit a347c59 into speced:main Feb 9, 2024
4 of 9 checks passed
@tabatkins
Copy link
Collaborator

Hrm, I wish there was an easier way to just obtain the type without the attributes, but the design of widlparser means that I can't just construct a Type without giving it tokens to build itself out of. I'd have to either do a hacky mock tokenizer, or significantly refactor to make all the __init__ methods just simple constructors, and swap the existing parser-based ones to static functions.

Thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants