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.

Sign up for GitHub

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

Jump to bottom

<dfn data-dfn-type=idl> is incorrectly allowed without error #3644

Closed
1 task done
tabatkins opened this issue Jun 18, 2021 · 8 comments · Fixed by #3646
Closed
1 task done

<dfn data-dfn-type=idl> is incorrectly allowed without error #3644

tabatkins opened this issue Jun 18, 2021 · 8 comments · Fixed by #3646
Labels
bug

Comments

@tabatkins
Copy link

Important info

Description of problem

What happened (e.g., it crashed)?: The [[addressLine]] slot definition is marked as data-dfn-type=idl, and no error shows up in the console.

Expected behavior (e.g., it shouldn't crash): Error should be emitted, as that's not a valid definition type. (It's a linking supertype that encompasses all the IDL-related types.)

You can see this dfn show up in webref with the invalid type at https://github.com/w3c/webref/blob/master/tr/dfns/payment-request.json#L1992.

(This particular definition should actually be type=attribute, but the markup error isn't the significant issue here.)
@tabatkins tabatkins added the bug label Jun 18, 2021
@tabatkins tabatkins mentioned this issue Jun 18, 2021
Closed
@tidoust
Copy link
Contributor

tidoust commented Jun 19, 2021 •
edited
Loading

Is attribute the right type to use for internal slots? Would it be worth having a specific type such as slot?

FYI, here is the list of dfns in webref that have an `idl` type. They are all for internal slots:

@marcoscaceres
Copy link
Contributor

Thanks @tidoust for the complete list.

Is attribute the right type to use for internal slots?

I think attribute is correct type, as they are just private properties not accessible by user-land code.

Would it be worth having a specific type such as slot?

Probably not. If anything, we could add some accompanying metadata ("isSlot") if we need it later.

@dontcallmedom
Copy link
Member

One reason why attribute may not be right, is that attribute definitions are exported by default, which I'm not sure is a desirable outcome for internal slots - it feels like they should be done so on an exceptional basis.

@marcoscaceres marcoscaceres mentioned this issue Jun 21, 2021
Merged
@dontcallmedom dontcallmedom mentioned this issue Jun 21, 2021
Merged
@marcoscaceres
Copy link
Contributor

it feels like they should be done so on an exceptional basis.

Looks like Bikeshed exports them by default... for example:
https://github.com/w3c/webcodecs/blob/main/index.src.html#L522

They are just "attributes".

I've be inclined not to export them by default, but it might be too late as too many are exported now? @tabatkins, wdyt?

@dontcallmedom
Copy link
Member

dontcallmedom commented Jun 21, 2021 •
edited
Loading

attribute definitions get exported by default, and that's good (after all, something exposed to Web developers should always also be exposed to other specs).

I wouldn't based our approach to this based WebCodecs marking its internal slots as attributes, esp given that there has been no type available for those so far.

If we were to push for a change in the taxonomy, there are are number of specs that would need patching though. It shows that internal slots are used for methods too; WHATWG Streams marks those as abstract-op.

Here is the full list of exported definitions starting with ̀[[` (beyond those bogusly marked as `idl`):

(generated by

jq '.spec as $spec|.dfns[]|select(.access == "public" and (.linkingText[0]|startswith("[[")) )|("* [`" + .linkingText[0] + "` " + .type + " in «" + $spec.title +"»](" + .href + ")")' *.json|tr -d '"' |grep -v "idl"

in webref/ed/dfns)

@sidvishnoi
Copy link
Member

Slightly unrelated: For WebRef data used by ReSpec, run the following query at https://respec.org/xref/filter/: term.startsWith("[[").

@dontcallmedom dontcallmedom mentioned this issue Jun 21, 2021
Open
@tabatkins
Copy link
Author

Yes, attribute is the right type.

I don't see a good reason not to export them; that just makes it harder to link them, and I don't see a reason to do that. (The only reason to not export something is just because it's not a very useful term to link to, and you don't want to potentially cause linking conflicts that need to be resolved between a useful term and your spec-internal non-useful term. IDL terms are always namespaced to their construct, tho, so that's not an issue, and we can and should safely default to exporting.)

(Sorry for the delay in responding; between vacation and returning from vacation, I'm quite behind.)

@dontcallmedom
Copy link
Member

re exporting, the vision I've been working towards is that things that get exported are fine to interact with from other specs; this seems natural for many of the typed definition types (since they end up being exposed to developers, exposing them to spec writers make sense). Internal slots feel like they should be exposed on an explicit basis, as a way to keep monkey patching in check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(core/dfn): treat internal slots as IDL attributes speced/respec
5 participants
@dontcallmedom @tabatkins @marcoscaceres @tidoust @sidvishnoi