-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Implement a merge() method to apply partials and mixins #584
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
export { parse } from "./lib/webidl2.js"; | ||
export { write } from "./lib/writer.js"; | ||
export { merge } from "./lib/merge.js"; | ||
export { validate } from "./lib/validator.js"; | ||
export { WebIDLParseError } from "./lib/tokeniser.js"; |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,140 @@ | ||||
import { ExtendedAttributes } from "./productions/extended-attributes.js"; | ||||
import { Tokeniser } from "./tokeniser.js"; | ||||
|
||||
// Remove this once all of our support targets expose `.flat()` by default | ||||
function flatten(array) { | ||||
if (array.flat) { | ||||
return array.flat(); | ||||
} | ||||
return [].concat(...array); | ||||
} | ||||
|
||||
// https://heycam.github.io/webidl/#own-exposure-set | ||||
function getOwnExposureSet(node) { | ||||
const exposedAttr = node.extAttrs.find((a) => a.name === "Exposed"); | ||||
if (!exposedAttr) { | ||||
return null; | ||||
} | ||||
const exposure = new Set(); | ||||
const { type, value } = exposedAttr.rhs; | ||||
if (type === "identifier") { | ||||
exposure.add(value); | ||||
} else if (type === "identifier-list") { | ||||
for (const ident of value) { | ||||
exposure.add(ident.value); | ||||
} | ||||
} | ||||
return exposure; | ||||
} | ||||
|
||||
/** | ||||
* @param {Set?} a a Set or null | ||||
* @param {Set?} b a Set or null | ||||
* @return {Set?} a new intersected set, one of the original sets, or null | ||||
*/ | ||||
function intersectNullable(a, b) { | ||||
if (a && b) { | ||||
const intersection = new Set(); | ||||
for (const v of a.values()) { | ||||
if (b.has(v)) { | ||||
intersection.add(v); | ||||
} | ||||
} | ||||
return intersection; | ||||
} | ||||
return a || b; | ||||
} | ||||
|
||||
/** | ||||
* @param {Set?} a a Set or null | ||||
* @param {Set?} b a Set or null | ||||
* @return true if a and b have the same values, or both are null | ||||
*/ | ||||
function equalsNullable(a, b) { | ||||
if (a && b) { | ||||
if (a.size !== b.size) { | ||||
return false; | ||||
} | ||||
for (const v of a.values()) { | ||||
if (!b.has(v)) { | ||||
return false; | ||||
} | ||||
} | ||||
} | ||||
return a === b; | ||||
} | ||||
|
||||
/** | ||||
* @param {Container} target definition to copy members to | ||||
* @param {Container} source definition to copy members from | ||||
*/ | ||||
function copyMembers(target, source) { | ||||
const targetExposure = getOwnExposureSet(target); | ||||
const parentExposure = intersectNullable( | ||||
targetExposure, | ||||
getOwnExposureSet(source) | ||||
); | ||||
// TODO: extended attributes | ||||
for (const orig of source.members) { | ||||
const origExposure = getOwnExposureSet(orig); | ||||
const copyExposure = intersectNullable(origExposure, parentExposure); | ||||
|
||||
// Make a copy of the member with the same prototype and own properties. | ||||
const copy = Object.create( | ||||
Object.getPrototypeOf(orig), | ||||
Object.getOwnPropertyDescriptors(orig) | ||||
); | ||||
|
||||
if (!equalsNullable(targetExposure, copyExposure)) { | ||||
let value = Array.from(copyExposure.values()).join(","); | ||||
if (copyExposure.size !== 1) { | ||||
value = `(${value})`; | ||||
} | ||||
copy.extAttrs = ExtendedAttributes.parse( | ||||
new Tokeniser(` [Exposed=${value}] `) | ||||
); | ||||
} | ||||
|
||||
target.members.push(copy); | ||||
} | ||||
} | ||||
|
||||
/** | ||||
* @param {*[]} ast AST or array of ASTs | ||||
* @return {*[]} | ||||
*/ | ||||
export function merge(ast) { | ||||
const dfns = new Map(); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably good to reuse this function: Line 24 in 49c514f
|
||||
const partials = []; | ||||
const includes = []; | ||||
|
||||
for (const dfn of flatten(ast)) { | ||||
if (dfn.partial) { | ||||
partials.push(dfn); | ||||
} else if (dfn.type === "includes") { | ||||
includes.push(dfn); | ||||
} else if (dfn.name) { | ||||
dfns.set(dfn.name, dfn); | ||||
} else { | ||||
throw new Error(`definition with no name`); | ||||
} | ||||
} | ||||
|
||||
// merge partials (including partial mixins) | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea here is to first merge partials and then merge mixins, performing basically the same steps each time. But this will have the downside of copying members and merging extended attributes twice in the case of a partial interface mixin. (The mixin step isn't here yet.) Performance isn't much of a concern, but I wonder if this is overgeneralized, in particular it becomes hard to judge whether https://heycam.github.io/webidl/#dfn-exposure-set is followed. The alternative would be to first collect all includes statements and then to copy members directly from their source to their final destination. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Not sure I follow this part, in what situation would it be problematic? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean if the implementation isn't similar to the algorithm in the spec, it's harder to know if it get the same result in all cases. I think the two-step approach would be equivalent, but I'm not certain. The rules about whether the extended attributes of the member have to be a subset of the container's or if the intersection is used differ at least, so a unified implementation would allow for more invalid stuff. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, well, I'd say we can go this way and fix it if it turns out to be problematic. |
||||
for (const partial of partials) { | ||||
const target = dfns.get(partial.name); | ||||
if (!target) { | ||||
throw new Error( | ||||
`original definition of partial ${partial.type} ${partial.name} not found` | ||||
); | ||||
} | ||||
if (partial.type !== target.type) { | ||||
throw new Error( | ||||
`partial ${partial.type} ${partial.name} inherits from ${target.type} ${target.name} (wrong type)` | ||||
); | ||||
} | ||||
copyMembers(target, partial); | ||||
} | ||||
|
||||
return Array.from(dfns.values()); | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
import expect from "expect"; | ||
import { parse, write, merge } from "webidl2"; | ||
|
||
// Collapse sequences of whitespace to a single space. | ||
function collapse(s) { | ||
return s.trim().replace(/\s+/g, " "); | ||
} | ||
|
||
expect.extend({ | ||
toMergeAs(received, expected) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, didn't know There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Me neither, I found out after I had repeated myself a lot and wanted a helper function :) |
||
received = collapse(received); | ||
expected = collapse(expected); | ||
const ast = parse(received); | ||
const merged = merge(ast); | ||
const actual = collapse(write(merged)); | ||
if (actual === expected) { | ||
return { | ||
message: () => | ||
`expected ${JSON.stringify( | ||
received | ||
)} to not merge as ${JSON.stringify(expected)} but it did`, | ||
pass: true, | ||
}; | ||
} else { | ||
return { | ||
message: () => | ||
`expected ${JSON.stringify(received)} to merge as ${JSON.stringify( | ||
expected | ||
)} but got ${JSON.stringify(actual)}`, | ||
pass: false, | ||
}; | ||
} | ||
}, | ||
}); | ||
|
||
describe("merge()", () => { | ||
it("empty array", () => { | ||
const result = merge([]); | ||
expect(result).toHaveLength(0); | ||
}); | ||
|
||
it("partial dictionary", () => { | ||
expect(` | ||
dictionary D { }; | ||
partial dictionary D { boolean extra = true; }; | ||
`).toMergeAs(` | ||
dictionary D { boolean extra = true; }; | ||
`); | ||
}); | ||
|
||
it("partial interface", () => { | ||
expect(` | ||
interface I { }; | ||
partial interface I { attribute boolean extra; }; | ||
`).toMergeAs(` | ||
interface I { attribute boolean extra; }; | ||
`); | ||
}); | ||
|
||
it("partial interface with [Exposed]", () => { | ||
expect(` | ||
[Exposed=(Window,Worker)] interface I { }; | ||
[Exposed=Worker] partial interface I { | ||
attribute boolean extra; | ||
}; | ||
`).toMergeAs(` | ||
[Exposed=(Window,Worker)] interface I { | ||
[Exposed=Worker] attribute boolean extra; | ||
}; | ||
`); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@saschanaz I found it quite hard to get this right, and I didn't. The code here ultimately needs to copy any extended attributes from partial/mixin to its members, and to merge
[Exposed]
as part of that. That might end up adding, removing or modifying the original list of extended attributes. It seems like the only way to do this will be to carefully modify the original tokens, serialize it, and thenExtendedAttributes.parse
the string. Are there helpers that will make this easier, or should I go about it differently?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's something requested by #490, and no good alternative exists. I think this is okay for now until I actually implement it.