-
Notifications
You must be signed in to change notification settings - Fork 471
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
Add tests for Function.prototype.caller and arguments properties #4355
base: main
Are you sure you want to change the base?
Conversation
LGTM but i'll defer to someone more recently familiar with this part of the spec :-) |
d89d02c
to
1c42cfc
Compare
const otherCallerDesc = Object.getOwnPropertyDescriptor(otherFunctionProto, "caller"); | ||
|
||
assert.sameValue(mainCallerDesc.get, otherCallerDesc.get, "caller getter should be same across realms"); | ||
assert.sameValue(mainCallerDesc.set, otherCallerDesc.set, "caller setter should be same across realms"); |
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 incorrect: %ThrowTypeError%
is defined once per realm, cf. https://tc39.es/ecma262/#sec-%throwtypeerror%.
|
||
assert.sameValue(typeof argumentsDesc.get, "function"); | ||
assert.sameValue(typeof argumentsDesc.set, "function"); | ||
assert.sameValue(argumentsDesc.get, argumentsDesc.set, "arguments getter/setter should be same function"); |
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.
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.
Oh, nice! Then this PR can be largely refactored to modernize those existing tests.
test/built-ins/Function/prototype/restricted-property-arguments.js → test/built-ins/Function/prototype/arguments/prop-desc.js
// Copyright (C) 2024 Justin Dorfman. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: sec-addrestrictedfunctionproperties
description: >
Function.prototype.arguments
info: |
2. Let _thrower_ be _realm_.[[Intrinsics]].[[%ThrowTypeError%]].
3. Perform ! DefinePropertyOrThrow(_F_, *"caller"*, PropertyDescriptor { [[Get]]: _thrower_, [[Set]]: _thrower_, [[Enumerable]]: *false*, [[Configurable]]: *true* }).
4. Perform ! DefinePropertyOrThrow(_F_, *"arguments"*, PropertyDescriptor { [[Get]]: _thrower_, [[Set]]: _thrower_, [[Enumerable]]: *false*, [[Configurable]]: *true* }).
includes: [propertyHelper.js, wellKnownIntrinsicObjects.js]
---*/
verifyProperty(Function.prototype, "arguments", {
enumerable: false,
configurable: true
});
var descriptor = Object.getOwnPropertyDescriptor(Function.prototype, "caller");
assert.sameValue(typeof descriptor.get, "function", "Function.prototype.arguments has a getter");
assert.sameValue(typeof descriptor.set, "function", "Function.prototype.arguments has a setter");
assert.sameValue(descriptor.get, descriptor.set, "Function.prototype.arguments getter and setter are identical");
var throwTypeError;
WellKnownIntrinsicObjects.forEach(function(record) {
if (record.name === "%ThrowTypeError%") {
throwTypeError = record.value;
}
});
if (throwTypeError) {
assert.sameValue(descriptor.set, throwTypeError, "Function.prototype.arguments getter is %ThrowTypeError%");
}
assert.throws(TypeError, function() {
return Function.prototype.arguments;
});
assert.throws(TypeError, function() {
Function.prototype.arguments = {};
});
test/built-ins/Function/prototype/restricted-property-caller.js → test/built-ins/Function/prototype/caller/prop-desc.js
(analogous, replacing arguments
with caller
)
By the way @anba, how are you finding the existing coverage? It's something that I struggle with, especially in the case of test subdirectories that heavily use old patterns.
return nonStrictFunc.caller; | ||
} | ||
|
||
function strictFunc() { |
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.
Missing "use strict"
to make strictFunc
actually a strict-mode function.
---*/ | ||
|
||
function normalFunc() {} | ||
function strictFunc() { } |
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.
Functions are implicitly in strict-mode when defined in modules.
} | ||
|
||
assert(!Object.hasOwnProperty.call(nonStrictFunc, "caller"), "non-strict function should not have own caller property"); | ||
assert(!Object.hasOwnProperty.call(strictFunc, "caller"), "strict function should not have own caller property"); |
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.
The strict-mode case is already covered by:
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 we should modernize that as well, introducing a helper like verifyNoRestrictedFunctionProperties
and using it at e.g.
- test/language/expressions/{arrow-function,async-arrow-function,async-function,async-generator,class,function,generators}/{restricted-properties,restricted-properties-module}.js (covering expressions like
var f = function(){};
and generalizations, in script and module code) - test/language/statements/{async-function,async-generator,class,function,generators}/restricted-properties.js (covering statements like
function f(){}
and generalizations, in script and module code) - test/language/statements/class/definition/{methods-restricted-properties,methods-restricted-properties-module}.js (covering methods like
var m = new (class { m(){} })().m
, in script and module code)- likewise somewhere for generalizations—class static methods, object literal methods, and syntactic
get
/set
accessors
- likewise somewhere for generalizations—class static methods, object literal methods, and syntactic
- test/built-ins/{AsyncFunction,AsyncGeneratorFunction,Function,GeneratorFunction}/instance-restricted-properties.js (each covering both
$Function()
and$Function('"use strict"')
, or alternatively split into a pair of files) - test/built-ins/Function/prototype/bind/instance-restricted-properties.js
and removing all other restricted-properties files.
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 @jdorfman! Let's tweak this a bit.
/*--- | ||
description: Function.prototype caller and arguments properties are accessor properties with ThrowTypeError | ||
esid: sec-function.prototype.caller | ||
info: | | ||
Function instances do not inherit the "caller" and "arguments" accessors | ||
from Function.prototype. The accessors exist only on Function.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.
sec-function.prototype.caller
does not appear to be a valid anchor: https://tc39.es/ecma262/#sec-function.prototype.caller
See also CONTRIBUTING.md: Test Case Style.
|
||
assert.sameValue(typeof argumentsDesc.get, "function"); | ||
assert.sameValue(typeof argumentsDesc.set, "function"); | ||
assert.sameValue(argumentsDesc.get, argumentsDesc.set, "arguments getter/setter should be same function"); |
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.
Oh, nice! Then this PR can be largely refactored to modernize those existing tests.
test/built-ins/Function/prototype/restricted-property-arguments.js → test/built-ins/Function/prototype/arguments/prop-desc.js
// Copyright (C) 2024 Justin Dorfman. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: sec-addrestrictedfunctionproperties
description: >
Function.prototype.arguments
info: |
2. Let _thrower_ be _realm_.[[Intrinsics]].[[%ThrowTypeError%]].
3. Perform ! DefinePropertyOrThrow(_F_, *"caller"*, PropertyDescriptor { [[Get]]: _thrower_, [[Set]]: _thrower_, [[Enumerable]]: *false*, [[Configurable]]: *true* }).
4. Perform ! DefinePropertyOrThrow(_F_, *"arguments"*, PropertyDescriptor { [[Get]]: _thrower_, [[Set]]: _thrower_, [[Enumerable]]: *false*, [[Configurable]]: *true* }).
includes: [propertyHelper.js, wellKnownIntrinsicObjects.js]
---*/
verifyProperty(Function.prototype, "arguments", {
enumerable: false,
configurable: true
});
var descriptor = Object.getOwnPropertyDescriptor(Function.prototype, "caller");
assert.sameValue(typeof descriptor.get, "function", "Function.prototype.arguments has a getter");
assert.sameValue(typeof descriptor.set, "function", "Function.prototype.arguments has a setter");
assert.sameValue(descriptor.get, descriptor.set, "Function.prototype.arguments getter and setter are identical");
var throwTypeError;
WellKnownIntrinsicObjects.forEach(function(record) {
if (record.name === "%ThrowTypeError%") {
throwTypeError = record.value;
}
});
if (throwTypeError) {
assert.sameValue(descriptor.set, throwTypeError, "Function.prototype.arguments getter is %ThrowTypeError%");
}
assert.throws(TypeError, function() {
return Function.prototype.arguments;
});
assert.throws(TypeError, function() {
Function.prototype.arguments = {};
});
test/built-ins/Function/prototype/restricted-property-caller.js → test/built-ins/Function/prototype/caller/prop-desc.js
(analogous, replacing arguments
with caller
)
By the way @anba, how are you finding the existing coverage? It's something that I struggle with, especially in the case of test subdirectories that heavily use old patterns.
} | ||
|
||
assert(!Object.hasOwnProperty.call(nonStrictFunc, "caller"), "non-strict function should not have own caller property"); | ||
assert(!Object.hasOwnProperty.call(strictFunc, "caller"), "strict function should not have own caller property"); |
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 we should modernize that as well, introducing a helper like verifyNoRestrictedFunctionProperties
and using it at e.g.
- test/language/expressions/{arrow-function,async-arrow-function,async-function,async-generator,class,function,generators}/{restricted-properties,restricted-properties-module}.js (covering expressions like
var f = function(){};
and generalizations, in script and module code) - test/language/statements/{async-function,async-generator,class,function,generators}/restricted-properties.js (covering statements like
function f(){}
and generalizations, in script and module code) - test/language/statements/class/definition/{methods-restricted-properties,methods-restricted-properties-module}.js (covering methods like
var m = new (class { m(){} })().m
, in script and module code)- likewise somewhere for generalizations—class static methods, object literal methods, and syntactic
get
/set
accessors
- likewise somewhere for generalizations—class static methods, object literal methods, and syntactic
- test/built-ins/{AsyncFunction,AsyncGeneratorFunction,Function,GeneratorFunction}/instance-restricted-properties.js (each covering both
$Function()
and$Function('"use strict"')
, or alternatively split into a pair of files) - test/built-ins/Function/prototype/bind/instance-restricted-properties.js
and removing all other restricted-properties files.
For issue #4340 (Possible missing tests for legacy function properties)