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

fix(core/dfn): treat internal slots as IDL attributes #3646

Merged
merged 23 commits into from
Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 33 additions & 11 deletions src/core/dfn.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,15 @@
// Module core/dfn
// - Finds all <dfn> elements and populates definitionMap to identify them.

import { getDfnTitles, norm, showError } from "./utils.js";
import {
codedJoinOr,
docLink,
getDfnTitles,
norm,
showError,
} from "./utils.js";
import { registerDefinition } from "./dfn-map.js";
import { slotRegex } from "./inline-idl-parser.js";

export const name = "core/dfn";

Expand All @@ -12,9 +19,11 @@ export function run() {
const titles = getDfnTitles(dfn);
registerDefinition(dfn, titles);

const [firstTitle] = titles;

associateInternalSlotWithIDL(firstTitle, dfn);
const [linkingText] = titles;
// Matches attributes and methods, like [[some words]](with, optional, arguments)
if (slotRegex.test(linkingText)) {
processAsInternalSlot(linkingText, dfn);
}

// Per https://tabatkins.github.io/bikeshed/#dfn-export, a dfn with dfnType
// other than dfn and not marked with data-no-export is to be exported.
Expand All @@ -25,7 +34,7 @@ export function run() {
}

// Only add `lt`s that are different from the text content
if (titles.length === 1 && firstTitle === norm(dfn.textContent)) {
if (titles.length === 1 && linkingText === norm(dfn.textContent)) {
return;
}
dfn.dataset.lt = titles.join("|");
Expand All @@ -36,9 +45,8 @@ export function run() {
* @param {string} title
* @param {HTMLElement} dfn
*/
function associateInternalSlotWithIDL(title, dfn) {
if (!/^\[\[\w+\]\]$/.test(title)) return;
if (!dfn.dataset.idl) {
function processAsInternalSlot(title, dfn) {
if (!dfn.dataset.hasOwnProperty("idl")) {
dfn.dataset.idl = "";
}

Expand All @@ -52,9 +60,23 @@ function associateInternalSlotWithIDL(title, dfn) {
// Assure that it's data-dfn-for= something.
if (!dfn.dataset.dfnFor) {
const msg = `Internal slot "${title}" must be associated with a WebIDL interface.`;
const hint =
"Use a `data-dfn-for` attribute to associate this dfn with a WebIDL interface.";
const hint = docLink`Use a ${"[data-dfn-for]"} attribute to associate this dfn with a WebIDL interface.`;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, interesting!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will eventually need a way to actually verify this... but pretty cool if we can do that!

showError(msg, name, { hint, elements: [dfn] });
}

// If it ends with a ), then it's method. Attribute otherwise.
if (!dfn.dataset.dfnType) {
dfn.dataset.dfnType = title.endsWith(")") ? "method" : "attribute";
return;
}

// Perform validation on the dfn's type type.
const allowedSlotTypes = ["attribute", "method"];
if (!allowedSlotTypes.some(type => dfn.dataset.dfnType === type)) {
const msg = docLink`Invalid ${"[data-dfn-for]"} for defining an internal slot.`;
const hint = `The only allowed types are: ${codedJoinOr(allowedSlotTypes, {
quotes: true,
})}.`;
showError(msg, name, { hint, elements: [dfn] });
}
dfn.dataset.dfnType = "attribute";
}
48 changes: 36 additions & 12 deletions src/core/inline-idl-parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,18 @@ import { html } from "./import-maps.js";
const idlPrimitiveRegex = /^[a-z]+(\s+[a-z]+)+\??$/; // {{unrestricted double?}} {{ double }}
const exceptionRegex = /\B"([^"]*)"\B/; // {{ "SomeException" }}
const methodRegex = /(\w+)\((.*)\)$/;
const slotRegex = /^\[\[(\w+)\]\]$/;

export const slotRegex = /\[\[(\w+(?: +\w+)*)\]\](\([^)]*\)(\.[^)]*\))?)*$/;
// matches: `value` or `[[value]]`
// NOTE: [[value]] is actually a slot, but database has this as type="attribute"
const attributeRegex = /^((?:\[\[)?(?:\w+)(?:\]\])?)$/;
const attributeRegex = /^((?:\[\[)?(?:\w+(?: +\w+)*)(?:\]\])?)$/;
const baseRegex = /^(?:\w+)\??$/;
const enumRegex = /^(\w+)\["([\w- ]*)"\]$/;
// TODO: const splitRegex = /(?<=\]\]|\b)\./
// https://github.com/w3c/respec/pull/1848/files#r225087385
const methodSplitRegex = /\.?(\w+\(.*\)$)/;

const slotSplitRegex = /\.?\/(.+)/;
const isProbablySlotRegex = /\[\[.+\]\]/;
/**
* @typedef {object} IdlBase
* @property {"base"} type
Expand All @@ -34,8 +36,10 @@ const methodSplitRegex = /\.?(\w+\(.*\)$)/;
* @typedef {object} IdlInternalSlot
* @property {"internal-slot"} type
* @property {string} identifier
* @property {string[]} [args]
* @property {boolean} renderParent
* @property {InlineIdl | null} [parent]
* @property {"attribute"|"method"} slotType
*
* @typedef {object} IdlMethod
* @property {"method"} type
Expand Down Expand Up @@ -71,7 +75,11 @@ const methodSplitRegex = /\.?(\w+\(.*\)$)/;
* @returns {InlineIdl[]}
*/
function parseInlineIDL(str) {
const [nonMethodPart, methodPart] = str.split(methodSplitRegex);
// If it's got [[ string ]], then split as an internal slot
const splitter = isProbablySlotRegex.test(str)
? slotSplitRegex
: methodSplitRegex;
const [nonMethodPart, methodPart] = str.split(splitter);
const tokens = nonMethodPart
.split(/[./]/)
.concat(methodPart)
Expand Down Expand Up @@ -108,8 +116,19 @@ function parseInlineIDL(str) {
}
// internal slot
if (slotRegex.test(value)) {
const [, identifier] = value.match(slotRegex);
results.push({ type: "internal-slot", identifier, renderParent });
const [, identifier, allArgs] = value.match(slotRegex);
const slotType = allArgs ? "method" : "attribute";
const args = allArgs
?.slice(1, -1)
.split(/,\s*/)
.filter(arg => arg);
results.push({
type: "internal-slot",
slotType,
identifier,
args,
renderParent,
});
continue;
}
// attribute
Expand Down Expand Up @@ -167,15 +186,20 @@ function renderBase(details) {
* @param {IdlInternalSlot} details
*/
function renderInternalSlot(details) {
const { identifier, parent, renderParent } = details;
const { identifier, parent, slotType, renderParent, args } = details;
const { identifier: linkFor } = parent || {};
const lt = `[[${identifier}]]`;
const isMethod = slotType === "method";
const argsHtml = isMethod
? htmlJoinComma(args, arg => html`<var>${arg}</var>`)
: null;
const textArgs = isMethod ? `(${args.join(", ")})` : "";
const lt = `[[${identifier}]]${textArgs}`;
const element = html`${parent && renderParent ? "." : ""}<a
data-xref-type="attribute"
data-link-for=${linkFor}
data-xref-for=${linkFor}
data-xref-type="${slotType}"
data-link-for="${linkFor}"
data-xref-for="${linkFor}"
data-lt="${lt}"
><code>[[${identifier}]]</code></a
><code>[[${identifier}]]${isMethod ? html`(${argsHtml})` : null}</code></a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably merge this into argsHtml above.

>`;
return element;
}
Expand Down
34 changes: 26 additions & 8 deletions tests/spec/core/dfn-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,8 +163,13 @@ describe("Core — Definitions", () => {
interface Foo{};
</pre>
<p>
<dfn id="simple">[[\\slot]]</dfn>
<dfn id="parent" data-dfn-for="Window">[[\\slot]]</dfn>
<dfn id="attribute">
[[\\internal slot]]
</dfn>
<dfn id="method">
[[\\I am_a method]](I, really, ...am)
</dfn>
<dfn id="parent" data-dfn-for="Window">[[\\internal slot]]</dfn>
<dfn id="broken" data-dfn-for="">[[\\broken]]</dfn>
</p>
<section data-dfn-for="">
Expand All @@ -174,24 +179,37 @@ describe("Core — Definitions", () => {
</p>
</section>
<p>
{{Test/[[slot]]}}
{{Window/[[slot]]}}
{{Test/[[internal slot]]}}
{{Test/[[I am_a method]](I, really, ...am)}}
{{Window/[[internal slot]]}}
</p>
</section>
`;
it("sets the data-dfn-type an idl attribute", async () => {

it("sets the data-dfn-type as an attribute", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
const dfn = doc.getElementById("simple");
expect(dfn.textContent).toBe("[[slot]]");
const dfn = doc.getElementById("attribute");
expect(dfn.textContent.trim()).toBe("[[internal slot]]");
expect(dfn.dataset.dfnType).toBe("attribute");
expect(dfn.dataset.idl).toBe("");
});

it("sets the data-dfn-type as a method, when it's a method", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
const dfn = doc.getElementById("method");
expect(dfn.textContent.trim()).toBe(
"[[I am_a method]](I, really, ...am)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are spaces acceptable in internal slot names? Any real world examples?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are no rules, which is another reason why I'm not very comfortable with labeling them as attributes; there are real-world examples of spaces in internal slots, like [[unique id]] (term.startsWith('[[') && term.match(/ .+\]/) lists them all on https://respec.org/xref/filter/)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I should've checked given I wrote the filter tool 😆)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though, at least no internalSlot-method with spaces.

);
expect(dfn.dataset.dfnType).toBe("method");
expect(dfn.dataset.idl).toBe("");
});

it("when data-dfn-for is missing, it use the closes data-dfn-for as parent", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
const dfn = doc.getElementById("simple");
const dfn = doc.getElementById("attribute");
expect(dfn.dataset.dfnFor).toBe("Test");
const dfnWithParent = doc.getElementById("parent");
expect(dfnWithParent.dataset.dfnFor).toBe("Window");
Expand Down