-
Notifications
You must be signed in to change notification settings - Fork 479
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
Some improvements to well-known intrinsic objects harness file #4392
base: main
Are you sure you want to change the base?
Conversation
As far as I know, %AsyncFromSyncIteratorPrototype% and %ForInIteratorPrototype% are not (and not intended to be) accessible to ECMAScript user code, so they are impossible to test directly. Add a clarifying comment, and make the source expression consistent ('' vs. 'undefined').
As far as I can tell these are wrong, giving %GeneratorFunction.prototype% and %AsyncGeneratorFunction.prototype% instead. These new expressions are how MDN claims you can get the intrinsics.
Looks like this list hasn't been updated in a while. Add %AsyncGeneratorPrototype%, %GeneratorPrototype%, %Iterator%, %IteratorHelperPrototype%, and %WrapForValidIteratorPrototype%. %IteratorPrototype% is no longer a well-known intrinsic; I guess it was removed because ever since iterator helpers it's accessible as %Iterator.prototype%. %Iterator% is available as the global property Iterator, but include a fallback for implementations that haven't yet implemented iterator helpers.
wellKnownIntrinsicObjects.js now exposes a getWellKnownIntrinsicObject() function which returns the object corresponding to a key like %Array%. If the object is not provided by the implementation, or not accessible, it throws a Test262Error. This is so that tests depending on that intrinsic object can easily fail.
The objects it provides are also available in another harness file, wellKnownIntrinsicObjects.js. There's no point in duplicating that in the harness. Rewrite each test that used hidden-constructors.js to use getWellKnownIntrinsicObject instead.
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.
Looks like a nice improvement!
@@ -140,8 +150,12 @@ const WellKnownIntrinsicObjects = [ | |||
source: 'isNaN', | |||
}, | |||
{ | |||
name: '%IteratorPrototype%', |
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.
this intrinsic name will still be in the spec, so we should probably still have it in this list?
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.
It is no longer there as far as I can tell. It seems to have been replaced by %Iterator.prototype% which means you can get it in this case with getWellKnownIntrinsic('%Iterator%').prototype
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.
hmm, that change was my PR, and the intention was to leave intact any pre-existing intrinsics, so that might be a spec bug I’ll have to fix.
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.
I think your PR left %IteratorPrototype% intact, but it was subsequently replaced by tc39/ecma262#3395 (Iterator Helpers stage 4).
I did check if it was used by any existing tests (it isn't, except for Function/prototype/toString/built-in-function-object.js, which walks all of the properties of all well-known intrinsic objects and therefore will visit %Iterator.prototype% anyway via %Iterator%.)
So I think it's OK to just say that the entries here should match the entries in the table https://tc39.es/ecma262/#table-well-known-intrinsic-objects. Sound good?
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.
Thanks
We have a
harness/wellKnownIntrinsicObjects.js
helper to aid with testing well-known intrinsic objects. There were some issues with it:harness/hidden-constructors.js
file that duplicated some of the functionalityHere's an attempt at fixing this up a bit.
It doesn't convert any existing tests to use
getWellKnownIntrinsicObject
other than the ones that previously usedharness/hidden-constructors.js
which is now removed. If we like this API, I'll go on and update more existing tests to use it.