-
Notifications
You must be signed in to change notification settings - Fork 460
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
Compatibility with Hardened JavaScript - start of built-ins #4234
base: main
Are you sure you want to change the base?
Conversation
To be clear, some of this won't be fixable - a fully locked-down JS realm is not simply 262-compliant. |
You should probably add some documentation in tree that explains when to use |
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.
With the same caveat here as in #4088 (review) about not formally approving work that I already consulted on, I love this! And I agree with @Ms2ger about wanting to document use of the new helper, which I guess would be a new RFC.
harness/propertyHelper.js
Outdated
@@ -107,7 +108,7 @@ function verifyProperty(obj, name, desc, options) { | |||
} | |||
} | |||
|
|||
assert(!failures.length, failures.join('; ')); | |||
assert(!failures.length, join.call(failures, '; ')); |
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 is also going to fail when the test for Function.prototype.call
is updated to use verifyProperty
, and likewise for some other functions and methods used in these helpers. I'd leave verifyProperty
alone in this PR (since it isn't updating the test for Array.prototype.join
), and a followup can add starting code like
// Capture primordial functions and receiver-uncurried primordial methods that
// are used in verification but might be destroyed *by* that process itself.
var __isArray = Array.isArray;
var __defineProperty = Object.defineProperty;
var __join = Function.prototype.call.bind(Array.prototype.join);
var __push = Function.prototype.call.bind(Array.prototype.push);
var __hasOwnProperty = Function.prototype.call.bind(Object.prototype.hasOwnProperty);
var __propertyIsEnumerable = Function.prototype.call.bind(Object.prototype.propertyIsEnumerable);
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.
Such changes turned out to be independent, so we don't even need to wait for followup: #4235
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.
Understood. I considered caching join
at the top-level like propertyHelper.js/isWritable does for Array.isArray
but followed the model of propertyHelper.js/isConfigurable
instead. Agreed that this is better addressed separately from this PR.
harness/propertyHelper.js
Outdated
@@ -268,3 +269,25 @@ function verifyNotConfigurable(obj, name) { | |||
throw new Test262Error("Expected obj[" + String(name) + "] NOT to be configurable, but was."); | |||
} | |||
} | |||
|
|||
function verifyBuiltinProperty(obj, name, desc, options) { |
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 name is potentially misleading, e.g. the IteratorResult object returned from (function*(){})().next()
and the { proxy, revoke }
object returned from Proxy.revocable({}, {})
(and both of its values, for that matter) are also built-in objects but their properties are not expected to be non-writable or non-configurable even in locked-down environments.
function verifyBuiltinProperty(obj, name, desc, options) { | |
function verifyPrimordialProperty(obj, name, desc, options) { |
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 makes sense.
… at runtime by verification Ref tc39#4234 (comment)
Thanks for the suggestion, @Ms2ger. I've added documentation to the source since I didn't find a better place. FWIW – the suggestion by @gibson042 to change the name to |
harness/propertyHelper.js
Outdated
/** | ||
* Use this function to verify the properties of a primordial object. | ||
* verifyPrimordialProperty accounts for the expected differences in the writable and configurable attributes when running under Hardened JavaScript's lockdown. | ||
* For non-priordial objects, use verifyProperty. |
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.
* For non-priordial objects, use verifyProperty. | |
* For non-primordial objects, use verifyProperty. |
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.
Well... your recommended name change will give me more practice typing "primordial."
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.
Sorry for the unrelated comment, but I have to say as a keen observer of development in this repository... As a non-native speaker I have to google (merriam-websterize) these things 😄 This is very interesting direction especially for runtimes allowing interop access (Jint is a .NET interpreter that allows injecting .NET services and objects into runtime - there are sandboxing needs for big players wanting to support user-supplied scripting).
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 making the distinction between verifyProperty
and verifyPrimordialProperty
everywhere is good, and I'm especially happy that you are motivated to go through and update all those old tests that didn't use verifyProperty
yet.
I'd prefer one thing to be different in implementation: I'd rather not add $262.isLockedDown
and other custom code specifically for Hardened JS. Primoridals being locked-down is not compliant with ECMA-262, and in the past our position has been to not add customizations specifically for intentional non-compliance (a non-Hardened JS example is implementations that prefer not to ship a with
statement.)
INTERPRETING.md
says this:
In addition, consumers may choose to override any of the functions defined by test harness files as they see fit.
This seems like a case where Hardened JS should do exactly that, with verifyPrimordialProperty
. test262's upstream implementation of it can simply be var verifyPrimordialProperty = verifyProperty;
or similar, and the special case can go into Hardened JS's test262 harness. That wouldn't be possible if we didn't make the distinction between verifyProperty
and verifyPrimordialProperty
upstream, so I'm fine with doing that, I'd just rather not have lockdown-specific code in the upstream harness.
… at runtime by verification Ref #4234 (comment)
@ptomato – thanks for the feedback.
Great. That's the main point of this PR. It will make test262 more consistent and, potentially, even more thorough.
Yes and no. Locking-down primordials is a proposal for ECMA-262. The proposal deliberately creates incompatibilities with strict mode, just like strict mode deliberately created incompatibilities with sloppy mode. A benefit of this work on test262 is that it allows us to characterize those incompatibilities much more precisely because test262 is so thorough.
Sure, until the relevant Hardened JavaScript proposals make their way to Stage 2.7, let's keep the Hardened JavaScript specific harness changes outside the test262 main branch. |
@phoddie right, but test262 doesn't include proposals until they're at stage 2.7 - so we need to make sure that the tests aren't testing for something that isn't currently allowed. |
harness/propertyHelper.js
Outdated
|
||
/** | ||
* Use this function to verify the properties of a primordial object. | ||
* verifyPrimordialProperty accounts for the expected differences in the writable and configurable attributes when running under Hardened JavaScript's lockdown. |
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.
for example, there are no compliant differences allowed in these attributes. writability and configurability is strictly defined by the spec.
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.
@ljharb – Please read the full thread, specifically:
Sure, until the relevant Hardened JavaScript proposals make their way to Stage 2.7, let's keep the Hardened JavaScript specific harness changes outside the test262 main branch.
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.
ah ok, well then that'll be great once implemented :-)
I believe all of the changes discussed have been applied. In addition, there is an update to one of the linter checks. One question: the repository contains two copies of propertyHelper.js -- one for the harness and one for the linter. I have applied my changes to both. The recent update to propertyHelper.js in the harness ( |
I am mystified by the presence of this copy. I wonder if it's because the location of the harness folder is hardcoded relative to the location of the tests? |
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 seems fine to me. Will hold on a little in case someone else has comments
This PR is a continuation of the effort to improve test262 compatibility with Hardened JavaScript after lockdown. The initial PR focused on language tests. This one starts the work on built-in tests.
The approach is to modify the tests as little as possible to allow them to pass before and after lockdown. None of the changes so far require the tests themselves to have knowledge of Hardened JavaScript.
The Writable and Configurable attributes of built-ins are a significant source of failures under lockdown. Lockdown increases the integrity level of built-ins to
FROZEN
, causing over 1500 tests that verify the attributes of built-ins to fail.There are three ways that the tests verify the attributes of built-ins:
verifyProperty
helper function inpropertyHelper.js
. This is the preferred method, according to the test262 sources. Some test that do not useverifyProperty
are marked for update.verifyConfigurable
,verifyWritable
,verifyEnumerable
,verifyNotConfigurable
,verifyNotWritable
, andverifyMotEnumerable
helper functions. These are marked as deprecated in the test262 sources. They perform more-or-less the same checks asverifyProperty
as they use the same helper functions,isWritable
andisConfigurable
.The approach taken is to add the new
verifyBuiltinProperty
function topropertyHelper.js
. This function calls through toverifyProperty
after setting thewritable
andconfigurable
options, when present, tofalse
when running under lockdown. This accounts for the expected raised integrity level of built-ins. A new function is necessary becauseverifyProperty
is used with objects created at runtime, not only built-ins.With the new
verifyBuiltinProperty
function in place, the tests are modified as follows:verifyProperty
that operate on a built-in are replaced with calls toverifyBuiltinProperty
.verifyConfigurable
, etc.) are replaced with calls toverifyProperty
orverifyBuiltinProperty
as appropriate.verifyBuiltinProperty
.The benefits of these changes include:
verifyConfigurable
, etc.) is significantly reduced (perhaps eliminated...) from test262verifyBuiltinProperty
, a function that expects a built-in object.Having an explicit check for built-in properties may open the door to more extensive and consistent tests of built-in properties. For example, test262 is inconsistent in checking:
length
andname
properties of built-in functions matches the order required byCreateBuiltinFunction
abstract operationThe
$262.isLockedDown()
function is introduced to determine if lockdown has completed. It is only invoked byverifyBuiltinProperty
at this time and only ifisLockedDown
is present on$262
.There was a small bug in
verifyProperty
. When checking that the configurable attribute isfalse
,verifyProperty
is destructive - it deletes the property. The implementation ofverifyProperty
usesArray.prototype.join
. Once the test forArray.prototype.join
was updated to useverifyProperty
the test harness failed. The solution is to cacheArray.prototype.join
in a local variable and invoke it using.call
. This approach mirrors howisConfigurable()
inpropertyHelper.js
cachesObject.prototype.hasOwnProperty
.