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 19 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
62 changes: 57 additions & 5 deletions src/core/dfn.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,16 @@
// Module core/dfn
// - Finds all <dfn> elements and populates definitionMap to identify them.

import { getDfnTitles, norm } from "./utils.js";
import {
codedJoinOr,
docLink,
getDfnTitles,
norm,
showError,
toMDCode,
} 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 +20,10 @@ export function run() {
const titles = getDfnTitles(dfn);
registerDefinition(dfn, titles);

// Treat Internal Slots as IDL.
if (!dfn.dataset.dfnType && /^\[\[\w+\]\]$/.test(titles[0])) {
dfn.dataset.dfnType = "idl";
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
Expand All @@ -26,9 +35,52 @@ export function run() {
}

// Only add `lt`s that are different from the text content
if (titles.length === 1 && titles[0] === norm(dfn.textContent)) {
if (titles.length === 1 && linkingText === norm(dfn.textContent)) {
return;
}
dfn.dataset.lt = titles.join("|");
});
}
/**
*
* @param {string} title
* @param {HTMLElement} dfn
*/
function processAsInternalSlot(title, dfn) {
if (!dfn.dataset.hasOwnProperty("idl")) {
dfn.dataset.idl = "";
}

// Automatically use the closest data-dfn-for as the parent.
/** @type HTMLElement */
const parent = dfn.closest("[data-dfn-for]");
if (dfn !== parent && parent?.dataset.dfnFor) {
dfn.dataset.dfnFor = parent.dataset.dfnFor;
}

// 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 = 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.
const derivedType = title.endsWith(")") ? "method" : "attribute";
if (!dfn.dataset.dfnType) {
dfn.dataset.dfnType = derivedType;
return;
}

// Perform validation on the dfn's type.
const allowedSlotTypes = ["attribute", "method"];
const { dfnType } = dfn.dataset;
if (!allowedSlotTypes.includes(dfnType) || derivedType !== dfnType) {
const msg = docLink`Invalid ${"[data-dfn-type]"} attribute on internal slot.`;
const prettyTypes = codedJoinOr(allowedSlotTypes, {
quotes: true,
});
const hint = `The only allowed types are: ${prettyTypes}. The slot "${title}" seems to be a "${toMDCode(derivedType)}"?`;
showError(msg, name, { hint, elements: [dfn] });
}
}
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
71 changes: 71 additions & 0 deletions tests/spec/core/dfn-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,4 +153,75 @@ describe("Core — Definitions", () => {
expect(el.hash).toBe("#dfn-first");
}
});

describe("internal slot definitions", () => {
const body = `
<section data-dfn-for="Test" data-cite="HTML">
<h2>Internal slots</h2>
<pre class="idl">
[Exposed=Window]
interface Foo{};
</pre>
<p>
<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="">
<h2>.</h2>
<p>
<dfn id="broken-parent">[[\\broken]]</dfn>
</p>
</section>
<p>
{{Test/[[internal slot]]}}
{{Test/[[I am_a method]](I, really, ...am)}}
{{Window/[[internal slot]]}}
</p>
</section>
`;

it("sets the data-dfn-type as an attribute", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
Comment on lines +190 to +191
Copy link
Member

Choose a reason for hiding this comment

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

This is repeated (with same args) in each test here. Can probably move it into a beforeAll?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's specific to this particular describe() group, so beforeAll is probably not ideal... wish they had a "before you run these tests".

Copy link
Member

Choose a reason for hiding this comment

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

I think beforeAll is scoped to desribe() blocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, will try. The docs said they beforeAll was global.

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("attribute");
expect(dfn.dataset.dfnFor).toBe("Test");
const dfnWithParent = doc.getElementById("parent");
expect(dfnWithParent.dataset.dfnFor).toBe("Window");
});

it("errors if the internal slot is not for something", async () => {
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);
const dfnErrors = doc.respec.errors.filter(
({ plugin }) => plugin === "core/dfn"
);
expect(dfnErrors).toHaveSize(2);
});
});
});
53 changes: 52 additions & 1 deletion tests/spec/core/link-to-dfn-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ describe("Core — Link to definitions", () => {
const bodyText = `
<section>
<h2>Test section</h2>
<p><dfn>[[\\test]]</dfn><a id="testAnchor">[[\\test]]</a>
<p>
<dfn data-dfn-for="Window">[[\\test]]</dfn>
<a data-link-for="Window" id="testAnchor">[[\\test]]</a>
</section>`;
const ops = makeStandardOps(null, bodyText);
const doc = await makeRSDoc(ops);
Expand Down Expand Up @@ -139,10 +141,12 @@ describe("Core — Link to definitions", () => {
const body = `
<section id="test">
<dfn data-dfn-for="MyEvent">[[\\aSlot]]</dfn>
<dfn data-dfn-for="MyEvent">[[\\an internal slot]](foo, bar)</dfn>
<dfn>MyEvent</dfn>
<p id="link-slots">
<span>{{ MyEvent/[[aSlot]] }}</span>
<span>{{ MyEvent.[[aSlot]] }}</span>
<span>{{ MyEvent/[[an internal slot]](foo, bar) }}</span>
</p>
</section>
`;
Expand All @@ -156,6 +160,53 @@ describe("Core — Link to definitions", () => {
expect(links[1].hash).toBe(href);
});

it("supports internal slot being a method with no args", async () => {
const body = `
<section>
<dfn id="method" data-dfn-for="MyEvent">
[[\\an internal_slot]]()
</dfn>
<dfn>MyEvent</dfn>
<p id="link-slots">
<span>{{ MyEvent/[[an internal_slot]]() }}</span>
</p>
</section>
`;
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);

const dfn = doc.querySelector("#method");
const links = doc.querySelectorAll("#link-slots a");
expect(links[0].hash).toBe(`#${dfn.id}`);
const vars = document.querySelectorAll("#link-slots var");
expect(vars).toHaveSize(0);
});

it("supports internal slot being a method with arguments", async () => {
const body = `
<section>
<dfn id="method" data-dfn-for="MyEvent">
[[\\an internal_slot]](foo, bar)
</dfn>
<dfn>MyEvent</dfn>
<p id="link-slots">
<span>{{ MyEvent/[[an internal_slot]](foo, bar) }}</span>
</p>
</section>
`;
const ops = makeStandardOps(null, body);
const doc = await makeRSDoc(ops);

const dfn = doc.querySelector("#method");
const links = doc.querySelectorAll("#link-slots a");
expect(links[0].hash).toBe(`#${dfn.id}`);
const vars = doc.querySelectorAll("#link-slots var");
expect(vars).toHaveSize(2);
const [foo, bar] = vars;
expect(foo.textContent).toBe("foo");
expect(bar.textContent).toBe("bar");
});

it("has empty data-dfn-for on top level things", async () => {
const bodyText = `
<section data-dfn-for="HyperStar" data-link-for="HyperStar">
Expand Down
5 changes: 4 additions & 1 deletion tests/spec/core/simple.html
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ <h2>Some title</h2>
Some content
</p>
<p><dfn id="custom_id(¡™£¢∞§¶•ªº)">custom id</dfn></p>
<p><dfn>[[\escapedSlot]]</dfn> <a>[[\escapedSlot]]</a></p>
</section>
<section data-dfn-for="Test">
<h2><dfn>Test</dfn> interface</h2>
Expand All @@ -42,5 +41,9 @@ <h2><dfn>Test</dfn> interface</h2>
</pre>
<section>
<h3><dfn>foo()</dfn> method</h3>
<p>
<dfn>[[\escapedSlot]]</dfn>
{{Test/[[escapedSlot]]}}
</p>
</section>
</section>
14 changes: 12 additions & 2 deletions tests/spec/core/xref-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -756,9 +756,12 @@ describe("Core — xref", () => {
it("links internalSlots", async () => {
const body = `
<section>
<p><dfn>[[\\type]]</dfn></p>
<p id="link1">{{ [[type]] }}</p>
<p><dfn data-dfn-for="Window">[[\\type]]</dfn></p>
<p id="link1">{{ Window/[[type]] }}</p>
<p id="link2">{{ Credential.[[type]] }}</p>
<p id="link3">{{
PasswordCredential/[[CollectFromCredentialStore]](origin, options, sameOriginWithAncestors)
}}</p>
</section>
`;
const config = { xref: true, localBiblio };
Expand All @@ -780,6 +783,13 @@ describe("Core — xref", () => {
);
expect(link2a.firstElementChild.localName).toBe("code");
expect(link2b.firstElementChild.localName).toBe("code");

// [[CollectFromCredentialStore]](origin, options, sameOriginWithAncestors)
const link3 = doc.querySelector("#link3 a");
expect(link3.firstElementChild.localName).toBe("code");
expect(link3.href).toBe(
"https://www.w3.org/TR/credential-management-1/#dom-passwordcredential-collectfromcredentialstore-slot"
);
});

it("links enum and enum-values", async () => {
Expand Down