Add tests for Function.prototype.caller and arguments properties #4355

jdorfman · 2024-12-17T22:15:25Z

For issue #4340 (Possible missing tests for legacy function properties)

ljharb · 2024-12-17T22:20:07Z

LGTM but i'll defer to someone more recently familiar with this part of the spec :-)

anba · 2024-12-18T12:22:17Z

test/built-ins/Function/prototype/caller-arguments/cross-realm-behavior.js

+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");


This is incorrect: %ThrowTypeError% is defined once per realm, cf. https://tc39.es/ecma262/#sec-%throwtypeerror%.

See also https://github.com/tc39/test262/blob/main/test/built-ins/ThrowTypeError/unique-per-realm-function-proto.js.

anba · 2024-12-18T12:22:18Z

test/built-ins/Function/prototype/caller-arguments/accessor-properties.js

+
+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");


This is already covered by the existing tests in:

https://github.com/tc39/test262/blob/main/test/built-ins/Function/prototype/restricted-property-arguments.js

https://github.com/tc39/test262/blob/main/test/built-ins/Function/prototype/restricted-property-caller.js

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.

@gibson042 I added the refactored tests. For the other files I'm not sure what to do. Should I still address the issues or will they be removed?

anba · 2024-12-18T12:23:30Z

test/built-ins/Function/prototype/caller-arguments/strict-vs-nonstrict.js

+    return nonStrictFunc.caller;
+}
+
+function strictFunc() {


Missing "use strict" to make strictFunc actually a strict-mode function.

anba · 2024-12-18T12:26:12Z

test/built-ins/Function/prototype/caller-arguments/module-context.js

+---*/
+
+function normalFunc() {}
+function strictFunc() { }


Functions are implicitly in strict-mode when defined in modules.

anba · 2024-12-18T12:29:04Z

test/built-ins/Function/prototype/caller-arguments/strict-vs-nonstrict.js

+}
+
+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");


The strict-mode case is already covered by:

https://github.com/tc39/test262/blob/main/test/built-ins/Function/StrictFunction_restricted-properties.js

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

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.

gibson042

Thanks @jdorfman! Let's tweak this a bit.

gibson042 · 2024-12-18T17:50:36Z

test/built-ins/Function/prototype/caller-arguments/accessor-properties.js

+/*---
+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.
+---*/


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.

gibson042 · 2024-12-18T20:38:00Z

test/built-ins/Function/prototype/caller-arguments/accessor-properties.js

+
+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");


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.

gibson042 · 2024-12-18T20:43:20Z

test/built-ins/Function/prototype/caller-arguments/strict-vs-nonstrict.js

+}
+
+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");


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

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.

jdorfman · 2024-12-19T22:20:56Z

Thanks @gibson042 @anba @ljharb I will dig into these

jdorfman requested a review from a team as a code owner December 17, 2024 22:15

ljharb requested a review from gibson042 December 17, 2024 22:19

Add tests for Function.prototype.caller and arguments properties

1c42cfc

ljharb force-pushed the jd/issue-4340 branch from d89d02c to 1c42cfc Compare December 17, 2024 22:20

anba suggested changes Dec 18, 2024

View reviewed changes

gibson042 requested changes Dec 18, 2024

View reviewed changes

Add tests for Function.prototype.caller and arguments properties

9875dd4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for Function.prototype.caller and arguments properties #4355

Add tests for Function.prototype.caller and arguments properties #4355

jdorfman commented Dec 17, 2024

ljharb commented Dec 17, 2024

anba Dec 18, 2024

anba Dec 18, 2024

gibson042 Dec 18, 2024

jdorfman Dec 20, 2024

anba Dec 18, 2024

anba Dec 18, 2024

anba Dec 18, 2024

gibson042 Dec 18, 2024

gibson042 left a comment

gibson042 Dec 18, 2024

gibson042 Dec 18, 2024

gibson042 Dec 18, 2024

jdorfman commented Dec 19, 2024

Add tests for Function.prototype.caller and arguments properties #4355

Are you sure you want to change the base?

Add tests for Function.prototype.caller and arguments properties #4355

Conversation

jdorfman commented Dec 17, 2024

ljharb commented Dec 17, 2024

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

gibson042 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

jdorfman commented Dec 19, 2024