Type conversion overhaul #33

TimothyGu · 2017-02-06T06:16:59Z

Implementing union type conversion is next, followed by overload resolution algorithm (if need be).

domenic

Only major issue is the dictionary conversion is more subtle than done here.

This is overall really awesome; thanks for working on it. Would you be able to show some sample outputs? Eventually I guess we should get better about writing tests for webidl2js, but for now just being able to spot-check the output would be helpful.

Are there particular features you're planning on adding to jsdom using these?

domenic · 2017-02-06T19:09:42Z

lib/constructs/dictionary.js

@@ -1,6 +1,6 @@
 "use strict";

-const conversions = require("webidl-conversions");
+const Types = require("../types.js");


Since this is not a class, it should not be capitalized. Also, in this project it seems like we're not including .js in the requires.

I'm mirroring how require('../parameters') is usually called Parameters, even though Parameters is not a class. See e.g. constructs/interface.js. Will fix the .js.

Oh, I see. I guess good to stay consistent, even though that's a weird style choice from @Sebmaster :)

domenic · 2017-02-06T19:11:43Z

lib/types.js

+
+const utils = require("./utils");
+
+module.exports.generateTypeConversion = function (name, idlType, argAttrs, customTypes, opts) {


This function is very long. I think it would benefit from calling out to a few non-exported factored-out functions for the two cases of sequence and IDL type. What do you think?

Agreed, will split.

domenic · 2017-02-06T19:12:19Z

lib/types.js

+};
+
+module.exports.canHandleType = function (idlType, customTypes) {
+  return idlType.generic === "sequence" || conversions[idlType.idlType] || customTypes.has(idlType.idlType);


idlType.idlType in conversions seems a bit nicer since it guarantees the return value will always be a boolean.

domenic · 2017-02-06T19:12:44Z

lib/types.js

+  if (typeof ${name} !== "object") {
+    throw new TypeError("The value provided is not an object");
+  } else {
+    const result = new Map;


Nit: parens after constructor

domenic · 2017-02-06T19:14:25Z

lib/types.js

+    throw new TypeError("The value provided is not an object");
+  } else {
+    const result = new Map;
+    const keys = Object.keys(${name});


Per spec this should be Object.getOwnPropertyKeys, and then you should use getOwnPropertyDescriptor, and check enumerability, and so on. This is important as it impacts how many side effects occur before an exception happens in exceptional cases.

domenic · 2017-02-06T19:16:39Z

lib/transformer.js

        yield fs.writeFile(path.join(outputDir, this._dictionaries[key].name + ".js"), "");
+      } else {


I guess while here it would be good to change this to else if (this._typedefs[key]) as that seems a bit less likely to blow up in the future.

TBH this entire thing should be changed to use Map anyway...

TimothyGu · 2017-02-06T19:55:37Z

Only major issue is the dictionary conversion is more subtle than done here.

Any specific cases you have in mind?

This is overall really awesome; thanks for working on it. Would you be able to show some sample outputs? Eventually I guess we should get better about writing tests for webidl2js, but for now just being able to spot-check the output would be helpful.

Yeah of course. I already tested this against JSDOM to make sure it still works.

Are there particular features you're planning on adding to jsdom using these?

Haven't decided yet, though I want to eventually implement the Fetch API and spec. Might also implement URLSearchParams (which uses records and union types) for whatwg-url as a proof of concept.

domenic · 2017-02-06T20:01:30Z

Any specific cases you have in mind?

Oops, I meant records, not dictionaries, as per the comments you already saw.

Haven't decided yet, though I want to eventually implement the Fetch API and spec. Might also implement URLSearchParams (which uses records and union types) for whatwg-url as a proof of concept.

That would be awesome! +1 to URLSearchParams as a first step; as seen in jsdom/whatwg-url#37 people seem to definitely want this.

TimothyGu · 2017-02-07T02:34:45Z

@domenic, okay, updated. I made some example I/Os in https://github.com/TimothyGu/webidl2js/tree/34c9ae6c1356d7a6ec0a898a774a2479414acc26/test. cases contain the IDL source, while output has all the generated JS files.

domenic · 2017-02-08T00:41:45Z

OK, awesome! Are you planning to commit those in this PR?

Reviewing the output:

It looks like record args go through the unwrapping. That's not ideal; we should be operating on the wrapper, not the impl. Same for sequence. This may require fixing Unroll argument conversion loop (for non-... args) #5 :-/.
Indentation seems off in general, but I think that's better solved by a postprocessing step. Filed Reformat the output somehow #35.
The type checks inside the sequence conversion are unnecessary, I believe. for-of will give the same errors if you pass it a non-object/non-iterable, and technically it's better to not access the [Symbol.iterator] property twice.

TimothyGu · 2017-02-08T06:06:35Z

I just pushed a patch that fixes arg unwrapping (and hence #5) but it's fairly big I'll need more time to make sure things like variadic arguments still work, so DO NOT MERGE. I've tested it against jsdom though and other than a few small fixes here and there in jsdom, its tests all pass.

Another thing is that I changed my mind regarding typedef. They should not generate interface files, but rather be inlined and flattened when they are used elsewhere. This is necessary because union type handling requires fully flattened member types. I have not fixed this yet, however.

Do you want me to include the test cases? And if so, do you want to just put them in test/ or write some actual tests with them?

Updated outputs are in https://github.com/TimothyGu/webidl2js/tree/354eb7acaa5c2911c895cd858da1f5c8df68fc64/test (compare with before.

Here's what's changed (so far):

If the argument is of an interface type, check if the argument is indeed of that type and throw an error otherwise
- Use this.requires more extensively to prevent duplicated require()'s
- Pass interfaces to individual classes alongside customTypes
- Use an opts object containing both in various places
Instead of unwrapping as soon as an argument is seen, first store the arguments in an array then process them one-by-one
- Then, unwrap only if needed for interface types or for currently unhandled types (notably, union type)
Add support for Promise and FrozenArray types
- Not necessarily needed in this PR, but to prevent trying to unwrap them when it is not needed
Handle nullability solely in Types.generateTypeConversion() (instead of in both Parameters and Types), and handle optionality before nullability per spec
- Remove Types.canHandleType()
Fix default arguments processing
- None of the default arguments in jsdom use Infinity or NaN, which would have immediately exposed the bug
- Default argument as null was previously masked by incorrect order of optionality/nullability handling
Use Types.generateTypeConversion() instead of Parameters.generateVarConversion() in Attributes
- Attributes setters have nothing to do with parameter optionality, which is now the only thing Parameters.generateVarConversion() handles
- Unexport Parameters.generateVarConversion()
Fix faulty logic in Overloads.proveSimiliarity()

stevenvachon · 2017-02-10T00:10:56Z

+1 for URLSearchParams

domenic · 2017-02-11T23:11:39Z

Do you want me to include the test cases? And if so, do you want to just put them in test/ or write some actual tests with them?

I'm not sure what our best strategy is here personally. What do you think? In my opinion it's useful to know when things change, so maybe trying to use baseline-tester would do the trick?

We could at some point try to write semantic tests, e.g. actually pass in various inputs to a generated file, and check how they get converted to a sequence by the time the test impl sees them. That sounds like a lot of work though :-/. Maybe we can add them as we spot particular bugs we want to guard against regressions on.

Those fixes are all really wonderful <3. Looking forward to being able to merge this.

Updated outputs are in https://github.com/TimothyGu/webidl2js/tree/354eb7acaa5c2911c895cd858da1f5c8df68fc64/test

This still seems to do a loop to copy arguments into args, right? I had envisioned the loop being fully unrolled, so that you process each arguments member in turn, with the effect of the block processing arguments[i] being that args[i] is set.

domenic · 2017-03-02T17:38:06Z

@TimothyGu thoughts on moving this forward? I'd love to merge it :)

TimothyGu · 2017-03-14T14:48:40Z

I'm having some problems with overloaded rest arguments. I haven't had a lot of time to work on this recently, but I'll try to get back at it over this weekend.

stevenvachon · 2017-05-01T16:26:07Z

@TimothyGu hi 😄

TimothyGu · 2017-05-02T00:37:47Z

Guess what… the HD that contained my WIP crashed a few weeks ago, so the commits I didn't push to GitHub are now gone. I'll see what I can do about it, but I'll have to change the implementation for typedef anyway so I might just consider breaking up this PR into smaller chunks…

stevenvachon · 2017-05-02T01:11:41Z

💥 🖥 💥

TimothyGu · 2017-05-02T10:38:08Z

Rebased. I removed the experimental (i.e. buggy) fix for #5 as well as incorrect typedef and union support. Those are bigger projects that will need to be tackled at a later time.

After those changes, this PR now only contains:

Support for record, sequence, Promise, FrozenArray types
Support for interface types as first-class types
- E.g. now calling Node.prototype.appendChild with a string will throw an error from the wrapper
Only unwrap parameters when necessary
- Arguments are still copied to a new array, however

Better Chrome-like error context

> window.eval('document.createTreeWalker(document.body).currentNode = 2;')
TypeError: Failed to set the 'currentNode' property on 'TreeWalker': The provided value is not of type 'Node'.
    at convert (jsdom/lib/jsdom/living/generated/Node.js:490:11)
    at TreeWalker.set [as currentNode] (jsdom/lib/jsdom/living/generated/TreeWalker.js:90:9)
    at eval (eval at <anonymous> (repl:1:8), <anonymous>:1:54)
    at eval (<anonymous>)
    at repl:1:8

Simplify attribute generation due to conflation of parameters and types
Remove outdated special dictionary handling for Date and RegExp
Subtle fixes for default parameters and overload similarity algorithm
Internal refactoring

The different handling for sequences and parameter unwrapping means that this PR is an incompatible set of changes. In JSDOM, jsdom/jsdom@master...TimothyGu:f9bbb2b is necessary to keep the tests green. Optionally, certain explicit error handling in implementation files can also be removed (TimothyGu/jsdom@2ab95dd) since the logic is now a part of the generated files.

domenic · 2017-05-06T13:40:47Z

Awesome! I guess I'm most able to review this by trying to figure out its impact on jsdom; apart from that, each commit looks reasonable. You say that all the tests are green with that diff? That's awesome and a little surprising :D.

-  const event = ErrorEvent.createImpl(["error", {
+  const event = ErrorEvent.create(["error", {

What are these changes about? In general our philosphy was that impl code should only know about or deal with impls, if possible.

-//  void append(USVString name, USVString value); // handle overloads manually; https://github.com/jsdom/webidl2js/issues/29
+  void append(USVString name, USVString value);
   void append(USVString name, Blob value, optional USVString filename);

What does uncommenting these help with? It still seems like we're handling overloads manually.

-    if (!HTMLElement.isImpl(value)) {
-      throw new TypeError("Argument must be a HTMLElement");
-    }
-    if (value._namespaceURI !== "http://www.w3.org/1999/xhtml" ||
+    if (!HTMLElement.isImpl(value) ||
+        value._namespaceURI !== "http://www.w3.org/1999/xhtml" ||

Why are we still testing HTMLElement.isImpl?

PR has been overhauled

TimothyGu · 2017-05-06T16:24:10Z

You say that all the tests are green with that diff?

Yep 😄

-  const event = ErrorEvent.createImpl(["error", {
+  const event = ErrorEvent.create(["error", {
What are these changes about? In general our philosphy was that impl code should only know about or deal with impls, if possible.

This is indeed pretty tricky, so I'm going to try to explain it the best I can. The function this piece of diff is located in, reportAnError(), is always called with target being an instance of Window. The Window interface in JSDOM is implemented by hand, but extends a generated class EventTarget. Now, after this PR the generated code enforces strict check for parameter types, which means that only ErrorEvent wrappers are accepted in the EventTarget.prototype.dispatchEvent() wrapper, which is what the event above is passed to, not the underlying impl function.

Now this does mean that instead of passing a wrapper function around in the guts, we can specifically unwrap window, like so:

diff --git a/lib/jsdom/living/helpers/runtime-script-errors.js b/lib/jsdom/living/helpers/runtime-script-errors.js
index 5f334e87..71f91477 100644
--- a/lib/jsdom/living/helpers/runtime-script-errors.js
+++ b/lib/jsdom/living/helpers/runtime-script-errors.js
@@ -1,5 +1,6 @@
 "use strict";
 const util = require("util");
+const idlUtils = require("../generated/utils");
 const ErrorEvent = require("../generated/ErrorEvent");
 
 const errorReportingMode = Symbol("error reporting mode");
@@ -15,7 +16,7 @@ function reportAnError(line, col, target, errorObject, message, location) {
   target[errorReportingMode] = true;
 
   // TODO Events: use constructor directly, once they are no longer tied to a window.
-  const event = ErrorEvent.create(["error", {
+  const event = ErrorEvent.createImpl(["error", {
     bubbles: false,
     cancelable: true,
     message,
@@ -51,7 +52,7 @@ module.exports = function reportException(window, error, filenameHint) {
   const lineNumber = pieces && parseInt(pieces[3]) || 0;
   const columnNumber = pieces && parseInt(pieces[4]) || 0;
 
-  const handled = reportAnError(lineNumber, columnNumber, window, error, error.message, fileName);
+  const handled = reportAnError(lineNumber, columnNumber, idlUtils.tryImplForWrapper(window), error, error.message, fileName);
 
   if (!handled) {
     const errorString = shouldBeDisplayedAsError(error) ? `[${error.name}: ${error.message}]` : util.inspect(error);

I can do that instead if that's what you prefer.

I'll explain the other changes later today.

TimothyGu · 2017-05-06T16:33:11Z

-//  void append(USVString name, USVString value); // handle overloads manually; https://github.com/jsdom/webidl2js/issues/29
+  void append(USVString name, USVString value);
   void append(USVString name, Blob value, optional USVString filename);

What does uncommenting these help with? It still seems like we're handling overloads manually.

Yep we are (for now), but if the first line is not commented webidl2js will think Blob is the only possible type for the second parameter, and therefore throw an error if it is not a Blob.

-    if (!HTMLElement.isImpl(value)) {
-      throw new TypeError("Argument must be a HTMLElement");
-    }
-    if (value._namespaceURI !== "http://www.w3.org/1999/xhtml" ||
+    if (!HTMLElement.isImpl(value) ||
+        value._namespaceURI !== "http://www.w3.org/1999/xhtml" ||

Why are we still testing HTMLElement.isImpl?

The old code is actually not spec-compliant, since by spec value can legally be either null or a HTMLElement, and in case of null a "HierarchyRequestError" DOMException should be thrown instead of TypeError. Now, non-null and non-HTMLElement values are filtered in the IDL layer, so there is no need for the continued existence of throw new TypeError here. But we still need to handle the case where value is null.

domenic · 2017-05-06T19:40:01Z

Now this does mean that instead of passing a wrapper function around in the guts, we can specifically unwrap window, like so:

Thanks for the explanation. Yeah, I think passing the "Window impl" (actually an EventTarget impl, if I understand correctly) would be the way to go here. That also has the benefit that we don't get borked when some on-page script overwrites EventTarget.prototype.dispatchEvent.

It might be worth adding a comment to reportAnError that it takes an EventTarget impl, to help things be clearer. (It's time like these one can appreciate the benefits of TypeScript...)

Yep we are (for now), but if the first line is not commented webidl2js will think Blob is the only possible type for the second parameter, and therefore throw an error if it is not a Blob.

Hmm. Looking at the generated output though, I don't see it working great...

URLSearchParams.prototype.append = function append(name, value) {
  if (!this || !module.exports.is(this)) {
    throw new TypeError("Illegal invocation");
  }
  if (arguments.length < 2) {
    throw new TypeError("Failed to execute 'append' on 'URLSearchParams': 2 arguments required, but only " + arguments.length + " present.");
  }

  const args = [];
  for (let i = 0; i < arguments.length && i < 2; ++i) {
    args[i] = arguments[i];
  }
  args[0] = conversions["USVString"](args[0], { context: "Failed to execute 'append' on 'URLSearchParams': parameter 1" });
  args[1] = conversions["USVString"](args[1], { context: "Failed to execute 'append' on 'URLSearchParams': parameter 2" });
  return this[impl].append(...args);
};

Should we instead be creating our own IDL, like

void append(USVString name, any value, optional USVString filename);

? In any case this warrants commenting in the IDL file.

I guess this also explains the impl -> wrapper change

   append(name, value, filename) {
     // Handling this manually for now: https://github.com/jsdom/webidl2js/issues/29
-    if (!Blob.isImpl(value)) {
+    if (Blob.is(value)) {
+      value = Blob.convert(value);
+    } else {
       value = conversions.USVString(value);
     }

You removed all the tryWrapperToImpl stuff, so now we need to assume unknown inputs are wrappers. But again it seems like we should be using any...

Now, non-null and non-HTMLElement values are filtered in the IDL layer, so there is no need for the continued existence of throw new TypeError here. But we still need to handle the case where value is null.

Can we just test for null then?

Overall it sounds like the webidl2js side of all this is pretty solid, so probably not many changes needed in this PR, but I thought it'd be good to be sure before merging.

TimothyGu · 2017-05-06T22:35:01Z

It might be worth adding a comment to reportAnError that it takes an EventTarget impl, to help things be clearer.

Agreed, done.

Hmm. Looking at the generated output though, I don't see it working great...

What you were initially referring to was actually FormData, not URLSearchParams. Compare:

--- orig.js	2017-05-06 14:33:21.436963019 -0700
+++ after-pr.js	2017-05-06 14:33:16.084943368 -0700
@@ -8,11 +8,12 @@
 
   const args = [];
   for (let i = 0; i < arguments.length && i < 3; ++i) {
-    args[i] = utils.tryImplForWrapper(arguments[i]);
-  }
-  args[0] = conversions["USVString"](args[0]);
-  if (args[2] !== undefined) {
-  args[2] = conversions["USVString"](args[2]);
+    args[i] = arguments[i];
   }
+  args[0] = conversions["USVString"](args[0], { context: "Failed to execute 'append' on 'FormData': parameter 1" });
+  args[1] = convertBlob(args[1], { context: "Failed to execute 'append' on 'FormData': parameter 2" });
+if (args[2] !== undefined) {
+  args[2] = conversions["USVString"](args[2], { context: "Failed to execute 'append' on 'FormData': parameter 3" });
+}
   return this[impl].append(...args);
 };

The important thing here is that convertBlob() gets the impl of the provided value is a Blob, and throws a TypeError otherwise. We don't want it to throw here, and that's why I deliberately uncommented the other overload to make sure webidl2js doesn't try to call convertBlob().

Should we instead be creating our own IDL, like
void append(USVString name, any value, optional USVString filename);
?

Or yeah, this works too. Changed.

In any case this warrants commenting in the IDL file.

Again, agreed.

I guess this also explains the impl -> wrapper change

Yep, though I added back tryImplForWrapper and support for impl in the impl method.

Can we just test for null then?

Sure.

I've created a PR at jsdom with the jsdom changes to ease merging: jsdom/jsdom#1841

Throw a TypeError when the object that is supposed to implement an interface does not.

Also eliminate outdated dictionary error for Date and RegExp.

See whatwg/webidl#305

domenic · 2017-05-14T19:36:45Z

All right, let's give this a shot...

TimothyGu force-pushed the master branch from a40aded to 7a16d62 Compare February 6, 2017 06:20

domenic previously requested changes Feb 6, 2017

View reviewed changes

TimothyGu force-pushed the master branch from 8e75c09 to 409cb77 Compare February 7, 2017 02:03

domenic mentioned this pull request Feb 8, 2017

Reformat the output somehow #35

Closed

TimothyGu force-pushed the master branch from ea5f0e7 to cbfca48 Compare February 8, 2017 05:38

TimothyGu force-pushed the master branch 2 times, most recently from 013432e to 12b95ee Compare May 2, 2017 09:05

This was referenced May 5, 2017

How to get/set query string parameters? jsdom/whatwg-url#37

Closed

WIP: add support for URLSearchParams jsdom/whatwg-url#87

Merged

TimothyGu mentioned this pull request May 6, 2017

Upgrade to latest webidl2js features jsdom/jsdom#1841

Merged

TimothyGu force-pushed the master branch from 0b23e56 to 22f22a4 Compare May 6, 2017 22:58

TimothyGu force-pushed the master branch from 22f22a4 to ac504f6 Compare May 6, 2017 23:18

TimothyGu changed the title ~~Add support for sequence, record, and typedef~~ Add conversions for sequence, record, interface, Promise, and FrozenArray types May 6, 2017

TimothyGu changed the title ~~Add conversions for sequence, record, interface, Promise, and FrozenArray types~~ Type conversion overhaul May 6, 2017

TimothyGu added 8 commits May 8, 2017 11:11

Add support for record, sequence, Promise, FrozenArray types

a0cbdf0

Various cleanups

806a98d

Condense transformer-global data into a Context class

8d25d1e

Treat interfaces as first-class types

03c53a3

Throw a TypeError when the object that is supposed to implement an interface does not.

Only unwrap arguments when necessary

3b1cdf8

Add error context

820df70

Also eliminate outdated dictionary error for Date and RegExp.

Better promise support

2e9c569

Add and update test snapshots

b691935

TimothyGu force-pushed the master branch from ac504f6 to b691935 Compare May 8, 2017 18:35

Fixup sequence, record, dictionary Type() Object check

7a07506

See whatwg/webidl#305

TimothyGu force-pushed the master branch from 5b63dad to 7a07506 Compare May 8, 2017 21:30

domenic merged commit f93bbd0 into jsdom:master May 14, 2017

This was referenced May 14, 2017

Include symbol properties in record processing #41

Merged

Add brand checks (for known types only I guess) to tryImplForWrapper #17

Closed

TimothyGu mentioned this pull request Aug 5, 2017

Unroll argument conversion loop (for non-... args) #5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Type conversion overhaul #33

Type conversion overhaul #33

TimothyGu commented Feb 6, 2017

domenic left a comment

domenic Feb 6, 2017

TimothyGu Feb 6, 2017

domenic Feb 6, 2017

domenic Feb 6, 2017

TimothyGu Feb 6, 2017

domenic Feb 6, 2017

domenic Feb 6, 2017

domenic Feb 6, 2017 •

edited

domenic Feb 6, 2017

TimothyGu Feb 6, 2017

TimothyGu commented Feb 6, 2017 •

edited

domenic commented Feb 6, 2017

TimothyGu commented Feb 7, 2017

domenic commented Feb 8, 2017

TimothyGu commented Feb 8, 2017

stevenvachon commented Feb 10, 2017 •

edited

domenic commented Feb 11, 2017 •

edited

domenic commented Mar 2, 2017

TimothyGu commented Mar 14, 2017

stevenvachon commented May 1, 2017

TimothyGu commented May 2, 2017

stevenvachon commented May 2, 2017

TimothyGu commented May 2, 2017 •

edited

domenic commented May 6, 2017

TimothyGu commented May 6, 2017

TimothyGu commented May 6, 2017 •

edited

domenic commented May 6, 2017

TimothyGu commented May 6, 2017

domenic commented May 14, 2017


		const utils = require("./utils");

		module.exports.generateTypeConversion = function (name, idlType, argAttrs, customTypes, opts) {

		yield fs.writeFile(path.join(outputDir, this._dictionaries[key].name + ".js"), "");
		} else {

Type conversion overhaul #33

Type conversion overhaul #33

Conversation

TimothyGu commented Feb 6, 2017

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic Feb 6, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Feb 6, 2017 • edited

domenic commented Feb 6, 2017

TimothyGu commented Feb 7, 2017

domenic commented Feb 8, 2017

TimothyGu commented Feb 8, 2017

stevenvachon commented Feb 10, 2017 • edited

domenic commented Feb 11, 2017 • edited

domenic commented Mar 2, 2017

TimothyGu commented Mar 14, 2017

stevenvachon commented May 1, 2017

TimothyGu commented May 2, 2017

stevenvachon commented May 2, 2017

TimothyGu commented May 2, 2017 • edited

domenic commented May 6, 2017

TimothyGu commented May 6, 2017

TimothyGu commented May 6, 2017 • edited

domenic commented May 6, 2017

TimothyGu commented May 6, 2017

domenic commented May 14, 2017

domenic Feb 6, 2017 •

edited

TimothyGu commented Feb 6, 2017 •

edited

stevenvachon commented Feb 10, 2017 •

edited

domenic commented Feb 11, 2017 •

edited

TimothyGu commented May 2, 2017 •

edited

TimothyGu commented May 6, 2017 •

edited