Add script to generate standard property tests #4535
Conversation
`flags: [generated]` is also used by RegExp test generator and will be used for the property test generator. Ignoring generated files from other generators ensures the "clean" command won't remove those files.
Generated by running: ``` python tools/property-test-generator/main.py $ECMA262/spec.html --include=Number* ```
Generated by running: ``` python tools/property-test-generator/main.py --global=$ECMA262/spec.html --include=Number* $ECMA402/spec/ ```
Generated by running: ``` python tools/property-test-generator/main.py --global=$ECMA262/spec.html --features=TypedArray,uint8array-base64 $PROPOSALS/proposal-arraybuffer-base64/spec.html ``` And then removed previous tests which are now covered by the automatically generated tests.
There was a problem hiding this comment.
I had held off on helpers like this because of dependency upon Reflect and especially Proxy, but if we're ready to raise that bar then I think this is great—although I do have concerns about the maintenance burden of the assumptions and parsing aspects of property-test-generator, which seem underdocumented. Maybe some of it should be pulled out of test262 for broader use and distinct maintenance?
I also love seeing removal of superseded files like test/built-ins/Number/isFinite/length.js, test/built-ins/Number/isFinite/name.js, test/built-ins/Number/isFinite/not-a-constructor.js, etc.
| // Built-in function objects that are not constructors do not have a | ||
| // "prototype" property unless otherwise specified in the description of a | ||
| // particular function. | ||
| // | ||
| // NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two | ||
| // own properties: "name" and "length". | ||
| assert.sameValue(__Reflect_ownKeys(fun).length, 2); | ||
|
|
||
| // Unless otherwise specified, the "name" property of a built-in function | ||
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | ||
| // [[Configurable]]: true }. | ||
| verifyPrimordialProperty(fun, "name", { | ||
| value: name, | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| // Unless otherwise specified, the "length" property of a built-in function | ||
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | ||
| // [[Configurable]]: true }. | ||
| verifyPrimordialProperty(fun, "length", { | ||
| value: length, | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
Assertion ordering suggestion:
| // Built-in function objects that are not constructors do not have a | |
| // "prototype" property unless otherwise specified in the description of a | |
| // particular function. | |
| // | |
| // NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two | |
| // own properties: "name" and "length". | |
| assert.sameValue(__Reflect_ownKeys(fun).length, 2); | |
| // Unless otherwise specified, the "name" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "name", { | |
| value: name, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Unless otherwise specified, the "length" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "length", { | |
| value: length, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Unless otherwise specified, the "name" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "name", { | |
| value: name, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Unless otherwise specified, the "length" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "length", { | |
| value: length, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Built-in function objects that are not constructors do not have a | |
| // "prototype" property unless otherwise specified in the description of a | |
| // particular function. | |
| // | |
| // NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two | |
| // own properties: "name" and "length". | |
| assert.sameValue(__Reflect_ownKeys(fun).length, 2); |
There was a problem hiding this comment.
We could also follow the __push(failures, msg); approach of harness/propertyHelper.js.
There was a problem hiding this comment.
Reflect.ownKeys is intentionally called first, because verifyPrimordialProperty deletes the properties. And adding {restore: true} just to move Reflect.ownKeys after verifyPrimordialProperty didn't seem useful to me.
There was a problem hiding this comment.
So alternatively:
// NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two
// own properties: "name" and "length".
- assert.sameValue(__Reflect_ownKeys(fun).length, 2);
+ assert.compareArray(__Reflect_ownKeys(fun).sort(), ["length", "name"],
+ "own property keys");
// Unless otherwise specified, the "name" property of a built-in functionGitHub PR suggestion
| // Built-in function objects that are not constructors do not have a | |
| // "prototype" property unless otherwise specified in the description of a | |
| // particular function. | |
| // | |
| // NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two | |
| // own properties: "name" and "length". | |
| assert.sameValue(__Reflect_ownKeys(fun).length, 2); | |
| // Unless otherwise specified, the "name" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "name", { | |
| value: name, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Unless otherwise specified, the "length" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "length", { | |
| value: length, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Built-in function objects that are not constructors do not have a | |
| // "prototype" property unless otherwise specified in the description of a | |
| // particular function. | |
| // | |
| // NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two | |
| // own properties: "name" and "length". | |
| assert.compareArray(__Reflect_ownKeys(fun).sort(), ["length", "name"], | |
| "own property keys"); | |
| // Unless otherwise specified, the "name" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "name", { | |
| value: name, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); | |
| // Unless otherwise specified, the "length" property of a built-in function | |
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | |
| // [[Configurable]]: true }. | |
| verifyPrimordialProperty(fun, "length", { | |
| value: length, | |
| writable: false, | |
| enumerable: false, | |
| configurable: true, | |
| }); |
There was a problem hiding this comment.
Calling assert.compareArray requires including "harness/compareArray.js" in each test file including "harness/builtin.js". This kind of dependency is a bit annoying, so I think I'd prefer this alternative:
var ownKeys = __Reflect_ownKeys(fun);
assert.sameValue(ownKeys.length, 2);
assert(
(ownKeys[0] === "length" && ownKeys[1] === "name") ||
(ownKeys[0] === "name" && ownKeys[1] === "length")
);Strictly speaking (ownKeys[0] === "name" && ownKeys[1] === "length") isn't allowed, because all built-in functions are created through CreateBuiltinFunction, which first defines "length" and then "name". But not all implementations get this right:
- https://bugs.webkit.org/show_bug.cgi?id=295485 (recently fixed)
- https://bugzilla.mozilla.org/show_bug.cgi?id=1629803 (still open)
There was a problem hiding this comment.
That makes sense to me, but I'm specifically trying to ensure that assertion-failure output is useful. So:
// NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two
// own properties: "name" and "length".
var ownKeys = __Reflect_ownKeys(fun);
if (
ownKeys.length !== 2 ||
(ownKeys[0] !== "name" && ownKeys[1] !== "name") ||
(ownKeys[0] !== "length" && ownKeys[1] !== "length")
) {
var actualKeysStr = "[" + ownKeys.map(String).join(", ") + "]";
var msg = "Actual own keys " + actualKeysStr + " should have been [length, name]";
assert(false, msg);
}
// Unless otherwise specified, the "name" property of a built-in function
There was a problem hiding this comment.
I agree with the goal, but the resulting code is pretty dense. Are we worried about the impact of doing the formatting if the test passes? Otherwise it could be something like
var actualKeysStr = "[" + ownKeys.map(String).join(", ") + "]";
var msg = "Actual own keys " + actualKeysStr + " should have been [length, name]";
var ownKeys = __Reflect_ownKeys(fun);
assert.sameValue(ownKeys.length, 2, msg);
assert(ownKeys[0] === "name" || ownKeys[1] === "name", msg);
assert(ownKeys[0] === "length" || ownKeys[1] === "length", msg);but note that the formatting code is not robust against changes in the environment either (.map() in particular)
| assert.throws(TypeError, function() { | ||
| // Test with `new` expression in addition to Reflect.construct. |
There was a problem hiding this comment.
| assert.throws(TypeError, function() { | |
| // Test with `new` expression in addition to Reflect.construct. | |
| // Test with `new` expression in addition to Reflect.construct. | |
| assert.throws(TypeError, function() { |
| // Unless specified otherwise, a built-in object that is callable as a | ||
| // function is a built-in function object with the characteristics described | ||
| // in 10.3. | ||
| assert.sameValue(typeof fun, "function"); |
There was a problem hiding this comment.
I wonder if the common assertions between verifyBuiltinFunction and verifyBuiltinConstructor should be extracted.
/**
* Verify `fun` is a normal built-in function object or constructor
* function object with the default properties as defined in
* <https://tc39.es/ecma262/#sec-ecmascript-standard-built-in-objects>.
*/
function verifyBuiltinCallable(fun, name, length, prototype) {
// Unless specified otherwise, a built-in object that is callable as a
// function is a built-in function object with the characteristics described
// in 10.3.
assert.sameValue(typeof fun, "function");
// Unless otherwise specified every built-in function and every built-in
// constructor has the Function prototype object, which is the initial value
// of the expression Function.prototype (20.2.3), as the value of its
// [[Prototype]] internal slot.
if (prototype === undefined) {
prototype = __Function.prototype;
}
assert.sameValue(__Reflect_getPrototypeOf(fun), prototype);
// Unless specified otherwise, the [[Extensible]] internal slot of a built-in
// object initially has the value true.
assert.sameValue(__Reflect_isExtensible(fun), true);
// Unless otherwise specified, the "name" property of a built-in function
// object has the attributes { [[Writable]]: false, [[Enumerable]]: false,
// [[Configurable]]: true }.
verifyPrimordialProperty(fun, "name", {
value: name,
writable: false,
enumerable: false,
configurable: true,
});
// Unless otherwise specified, the "length" property of a built-in function
// object has the attributes { [[Writable]]: false, [[Enumerable]]: false,
// [[Configurable]]: true }.
verifyPrimordialProperty(fun, "length", {
value: length,
writable: false,
enumerable: false,
configurable: true,
});
}
/**
* Verify `fun` is a normal built-in function object with the default
* properties as defined in
* <https://tc39.es/ecma262/#sec-ecmascript-standard-built-in-objects>.
*/
function verifyBuiltinFunction(fun, name, length) {
verifyBuiltinCallable(fun, name, length);
// Built-in function objects that are not constructors do not have a
// "prototype" property unless otherwise specified in the description of a
// particular function.
// |verifyBuiltinFunction| is more strict, allowing only "name" and "length".
assert.sameValue(__Reflect_ownKeys(fun).length, 2);
// Built-in function objects that are not identified as constructors do not
// implement the [[Construct]] internal method unless otherwise specified in
// the description of a particular function.
assert.throws(TypeError, function() {
// Reflect.construct throws a TypeError if `fun` is not a constructor. Use
// the Proxy constructor because it ignores `NewTarget`.
//
// Two alternatives which also ensure no additional operations are called:
//
// 1. Create a Proxy for `fun` with a "construct" trap. Then call `new` on
// the newly created Proxy object.
// ```
// var p = new Proxy(fun, {construct(){ return {}; }});
// assert.throws(TypeError, function() { new p; });
// ```
//
// 2. Use a derived class object.
// ```
// class C extends null { constructor() { return {}; } };
// assert.throws(TypeError, function() {
// __Reflect_construct(C, [], fun);
// });
// ```
__Reflect_construct(__Proxy, [{}, {}], fun);
});
// Test with `new` expression in addition to Reflect.construct.
assert.throws(TypeError, function() {
new fun();
});
}
/**
* Verify `fun` is a normal built-in constructor function object with the
* default properties as defined in
* <https://tc39.es/ecma262/#sec-ecmascript-standard-built-in-objects>.
*/
function verifyBuiltinConstructor(fun, name, length, prototype) {
verifyBuiltinCallable(fun, name, length, prototype);
// Built-in function objects that are not identified as constructors do not
// implement the [[Construct]] internal method unless otherwise specified in
// the description of a particular function.
// Reflect.construct throws a TypeError if `fun` is not a constructor.
//
// See verifyBuiltinFunction for why Proxy is used here.
assert.throws(Test262Error, function() {
__Reflect_construct(__Proxy, [{}, {}], fun);
// Throw the expected error.
throw new Test262Error();
});
}| - `--dry-run`: Don't write any output files. | ||
| - `--include`: Comma separated list of globs to filter which built-ins are included. | ||
| - `--exclude`: Comma separated list of globs to filter which built-ins are excluded. | ||
| - `--features`: Comma separated list of features from "features.txt". If not specified features are guessed based on files in the same directory. |
There was a problem hiding this comment.
| - `--features`: Comma separated list of features from "features.txt". If not specified features are guessed based on files in the same directory. | |
| - `--features`: Comma separated list of features from "features.txt". If not specified, features are guessed based on files in the same directory. |
|
|
||
| - Generate property tests for the `Number` constructor, but not the `Number` prototype object: | ||
| ```sh | ||
| $ python tools/property-test-generator/main.py --include=Number* --exclude=Number.prototype* $ECMA262/spec.html |
There was a problem hiding this comment.
For better shell and environment portability:
| $ python tools/property-test-generator/main.py --include=Number* --exclude=Number.prototype* $ECMA262/spec.html | |
| $ python tools/property-test-generator/main.py --include='Number*' --exclude='Number.prototype*' $ECMA262/spec.html |
| - Generate property tests for all built-ins defined in "$ECMA402/spec": | ||
| ```sh | ||
| python tools/property-test-generator/main.py --global=$ECMA262/spec.html $ECMA402/spec/ | ||
| ``` | ||
|
|
||
| The `--global` argument is necessary to correctly pick up global definitions from ECMA-262. |
There was a problem hiding this comment.
Internal consistency:
| - Generate property tests for all built-ins defined in "$ECMA402/spec": | |
| ```sh | |
| python tools/property-test-generator/main.py --global=$ECMA262/spec.html $ECMA402/spec/ | |
| ``` | |
| The `--global` argument is necessary to correctly pick up global definitions from ECMA-262. | |
| - Generate property tests for all built-ins defined in "$ECMA402/spec": | |
| ```sh | |
| python tools/property-test-generator/main.py --globals=$ECMA262/spec.html $ECMA402/spec/ | |
| ``` | |
| The `--globals` argument is necessary to correctly pick up global definitions from ECMA-262. |
I don't actually know if there are any test262 consumers which pull the latest test262 commits, but don't support
Hmm, I understand your proposal, but I'm not sure it's useful right now to invest time making the parser more reusable for others, if there aren't yet any other projects which could benefit from it. Should this ever happen, I'm open to move the parser into a separate project, so it's easier to coordinate and plan how to extend it to match the project needs of others. I don't think we need to spend much time on maintenance, because the specification seems to be in a fairly stable shape w.r.t. how properties and functions are defined. When trying the parser on past ECMA-262 editions, I didn't observe any issues which can't be easily fixed through small code updates:
|
|
I didn't review it as closely as Richard. The first few commits seem fine to go ahead with, maybe in a separate PR so they're not blocked while resolving the rest of the comments. I have to say that I'm somewhat skeptical of running regular expressions on the ECMA-262 prose and generating tests from that directly. Personally, I'd prefer a config file with all the entries for which to generate property tests. The config file could still be generated/updated with a fancy script that reads ECMA-262 if you like, but at least that way there'd be a human in the loop and a clear overview of what changed whenever we update it. |
| var __Function = Function; | ||
| var __Proxy = Proxy; | ||
| var __Reflect_construct = Reflect.construct; | ||
| var __Reflect_getPrototypeOf = Reflect.getPrototypeOf; | ||
| var __Reflect_isExtensible = Reflect.isExtensible; | ||
| var __Reflect_ownKeys = Reflect.ownKeys; |
There was a problem hiding this comment.
Should we use an IIFE rather than this naming convention?
| // Built-in function objects that are not constructors do not have a | ||
| // "prototype" property unless otherwise specified in the description of a | ||
| // particular function. | ||
| // | ||
| // NOTE: |verifyBuiltinFunction| is more strict and allows only exactly two | ||
| // own properties: "name" and "length". | ||
| assert.sameValue(__Reflect_ownKeys(fun).length, 2); | ||
|
|
||
| // Unless otherwise specified, the "name" property of a built-in function | ||
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | ||
| // [[Configurable]]: true }. | ||
| verifyPrimordialProperty(fun, "name", { | ||
| value: name, | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }); | ||
|
|
||
| // Unless otherwise specified, the "length" property of a built-in function | ||
| // object has the attributes { [[Writable]]: false, [[Enumerable]]: false, | ||
| // [[Configurable]]: true }. | ||
| verifyPrimordialProperty(fun, "length", { | ||
| value: length, | ||
| writable: false, | ||
| enumerable: false, | ||
| configurable: true, | ||
| }); | ||
|
|
There was a problem hiding this comment.
I agree with the goal, but the resulting code is pretty dense. Are we worried about the impact of doing the formatting if the test passes? Otherwise it could be something like
var actualKeysStr = "[" + ownKeys.map(String).join(", ") + "]";
var msg = "Actual own keys " + actualKeysStr + " should have been [length, name]";
var ownKeys = __Reflect_ownKeys(fun);
assert.sameValue(ownKeys.length, 2, msg);
assert(ownKeys[0] === "name" || ownKeys[1] === "name", msg);
assert(ownKeys[0] === "length" || ownKeys[1] === "length", msg);but note that the formatting code is not robust against changes in the environment either (.map() in particular)
| # "abstract operation" in type or | ||
| # "sdo" in type or | ||
| # "concrete method" in type or | ||
| # "implementation-defined" in type or | ||
| # "host-defined" in type or | ||
| # "internal method" in type or | ||
| # "numeric method" in type |
There was a problem hiding this comment.
Please remove (or explain why it's here)
| # Read the spec html file. | ||
| parser = SpecParser() | ||
| parser.feed(text) | ||
| parser.close() | ||
|
|
||
| # Assert all tags are properly closed. | ||
| assert parser.current_element == parser.root_element |
There was a problem hiding this comment.
Please provide a helper function for this near the implementation of SpecParser
| return | ||
|
|
||
| if os.path.exists(out_path) and not os.path.isdir(out_path): | ||
| logger.error(f"Can't overwrite existent non-directoy file '{cfg.out}'!") |
There was a problem hiding this comment.
| logger.error(f"Can't overwrite existent non-directoy file '{cfg.out}'!") | |
| logger.error(f"Can't overwrite existing non-directory file '{cfg.out}'!") |
|
|
||
| @staticmethod | ||
| def ignorable_tag(tag): | ||
| """Tuple of tags which are ignored.""" |
There was a problem hiding this comment.
"Returns whether the tag should be ignored"? The tuple is an implementation detail.
| description: Property test for Number.MAX_SAFE_INTEGER | ||
| info: | | ||
| Number.MAX_SAFE_INTEGER | ||
| - The value of `Number.MAX_SAFE_INTEGER` is 9007199254740991𝔽 (𝔽(253 - 1)). |
There was a problem hiding this comment.
Hm, that's quite a confusing way to render 2**53.
There was a problem hiding this comment.
Hmm, fixing this will require rewriting <sup> tags to some other representation.
This the source code from spec.html:
<p>The value of `Number.MAX_SAFE_INTEGER` is *9007199254740991*<sub>𝔽</sub> (𝔽(2<sup>53</sup> - 1)).</p>
There was a problem hiding this comment.
| - The value of `Number.MAX_SAFE_INTEGER` is 9007199254740991𝔽 (𝔽(253 - 1)). | |
| - The value of `Number.MAX_SAFE_INTEGER` is 9007199254740991𝔽 (𝔽(2⁵³ - 1)). |
There was a problem hiding this comment.
Hmm, fixing this will require rewriting
<sup>tags to some other representation.
We're already doing this as of tc39/ecmarkup#517 . For example, the text at https://tc39.es/ecma262/multipage/numbers-and-dates.html#sec-number.max_safe_integer now copies as
| - The value of `Number.MAX_SAFE_INTEGER` is 9007199254740991𝔽 (𝔽(253 - 1)). | |
| - The value of Number.MAX_SAFE_INTEGER is 9007199254740991𝔽 (𝔽(2**53 - 1)). |
0166442
verifyBuiltinFunctionandverifyBuiltinConstructor. Both functions are used in the generated tests.ReflectandProxyto identify constructors without invoking any extra MOP methods. The existingisConstructorharness function performs an extra [[Get]] access from [[Construct]] →OrdinaryCreateFromConstructor→GetPrototypeFromConstructor→Get. The new approach avoids this extra [[Get]].9ad1b2b
c5351e4
cfe8c2a
Numberbuilt-in.b012d8e
Numberbuilt-in.933d746
3b224c4
8b5cdbc