Strictness, `caller` and `arguments` spec compliancy

[leotm]

Bug Description

When repairing Hermes intrinsics via Secure EcmaScript to reach spec compliancy

• endojs/endo#2655

we detect non-standard/deprecated .caller and .arguments properties on several intrinsics

so have an expedient temp fix (proposed) to account for these

but would love to understand what Hermes is trying to do and the path towards spec-compliancy cc @tmikov @erights

some more details

• f22a18f Initial commit
• 72cc1fc Remove use of ctx for ThrowTypeError, encode message in throwTypeError cc @avp
• #165 corner case, fix unlikely, not found legit use cases
• #374 functionality chosen not to support
• #414 Function.caller support Hermes workaround, WIP
• [static_h] 1bc9202 Fix strictness of SH closures cc @neildhar
• [static_h] #1117 cc @fbmal7
• [static_h] 7156840 Change error message for restricted properties
• [static_h] 4bb59fa Update restricted properties test
• [static_h] 2b40f7c Merge identical shermes and hermes tests
• endojs/endo#2655 (comment)

https://github.com/facebook/hermes/blob/main/doc/Features.md#miscellaneous-incompatibilities

arguments changes in non-strict mode will not sync with named parameters

ref: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Strict_mode#no_syncing_between_parameters_and_arguments_indices

hermes

hermes/lib/VM/JSLib/GlobalObject.cpp

Lines 246 to 258 in 95b2b52

	CallResult<HermesValue>
	throwTypeError(void *ctx, Runtime &runtime, NativeArgs) {
	static const char *TypeErrorMessage[] = {
	"Restricted in strict mode",
	"Dynamic requires are not allowed after static resolution",
	};
	uint64_t kind = (uint64_t)ctx;
	assert(
	kind < (uint64_t)TypeErrorKind::NumKinds &&
	"[[ThrowTypeError]] wrong error kind passed as context");
	return runtime.raiseTypeError(TypeErrorMessage[kind]);
	}

hermes/test/hermes/function-non-strict.js

Lines 18 to 33 in 95b2b52

	print('function properties');
	// CHECK-LABEL: function properties
	print(typeof strict, typeof nonStrict);
	// CHECK-NEXT: function function
	print(nonStrict.caller, nonStrict.arguments);
	// CHECK-NEXT: undefined undefined
	try { print(strict.caller); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted in strict mode
	try { print(strict.arguments); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted in strict mode
	var bound = nonStrict.bind(42);
	try { print(bound.caller); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted in strict mode
	try { print(bound.arguments); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted in strict mode

static_h

hermes/lib/VM/JSLib/GlobalObject.cpp

Lines 247 to 259 in 696b483

	CallResult<HermesValue>
	throwTypeError(void *ctx, Runtime &runtime, NativeArgs) {
	static const char *TypeErrorMessage[] = {
	"Restricted property cannot be accessed",
	"Dynamic requires are not allowed after static resolution",
	};
	uint64_t kind = (uint64_t)ctx;
	assert(
	kind < (uint64_t)TypeErrorKind::NumKinds &&
	"[[ThrowTypeError]] wrong error kind passed as context");
	return runtime.raiseTypeError(TypeErrorMessage[kind]);
	}

hermes/test/hermes/function-non-strict.js

Lines 19 to 41 in 696b483

	print('function properties');
	// CHECK-LABEL: function properties
	print(typeof strict, typeof nonStrict);
	// CHECK-NEXT: function function
	try { print(nonStrict.caller); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	try { print(nonStrict.arguments); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	try { print(strict.caller); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	try { print(strict.arguments); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	var bound = nonStrict.bind(42);
	try { print(bound.caller); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	try { print(bound.arguments); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	try { print(Function.prototype.caller); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed
	try { print(Function.prototype.arguments); } catch(e) { print('caught', e.name, e.message); }
	// CHECK-NEXT: caught TypeError Restricted property cannot be accessed

[tmikov]

Hi, I am not exactly sure what you are asking.

If there are specific bugs, please file issues for them (or submit PRs). We will address them, except the ones that we have explicitly said we do not support, though nothing is set in stone. For example, we have a PR from Snapchat adding support for with.

Admittedly, some issues are not high priority, like allowing assignment to arguments in sloppy mode, or ones that provide no functionality like .caller and .arguments (although I am not sure what the issue is there).

BTW, the latest most compliant implementation is in the static_h branch.

but would love to understand what Hermes is trying to do and the path towards spec-compliancy

Spec compliance is important to us, but due to internal constraints and priorities, we have taken a pragmatic approach. We separate compliance into different axis:

1. Lack of functionality from modern versions of the spec (like for await ... of).
2. Lack of library features from modern versions of the spec.
3. Lack of support of esoteric sloppy mode features (e.g. syncing between named args and arguments[]).
4. Incorrect implementation likely to cause bugs.
5. Incorrect implementation less likely to cause a bug (like modifying a function expression name inside the expression).
6. Early reporting of errors that ought to be reported at runtime.

Clearly, among these 4) is by far the most important and we try to always fix these urgently. The rest of the list has to compete with other priorities. We are not a self-governing organization, we are employees of a corporation...

We have recently made great strides with 1, 6 and somewhat 5. We added native support for block scoping, TDZ, classes, and more is coming. We have improved sloppy mode identifier resolution in corner cases and, again, more is coming (including #1547). For 6) we have added a flag to move some compiler time errors to runtime.

[gibson042]

I think endojs/endo#2655 and tc39/test262#4340 provide good summaries:

w.r.t. function caller and arguments properties…

• JSC and SM follow spec by defining them only on Function.prototype (but violate it by having distinct values for the 4 respective getter/setter functions).
• Hermes violates the spec by omitting them from Function.prototype, and additionally by defining them on strict function(){…} instances (non-configurable accessor properties with %ThrowTypeError% getter/setter functions).

In short, caller and arguments accessor properties should be defined on Function.prototype and not directly on function instances, and those properties should be configurable so that user code can remove them.

[avp]

For caller and arguments in strict mode: I'm pretty sure our implementation was referencing the 5.1 spec, we never realized that this changed in ES6, and it hasn't really mattered because most of our code is strict mode and nobody uses these.

The ES5.1 spec on function creation (step 19 b,c) says that "caller" and "arguments" in strict mode are defined on the function itself and are non-configurable.

EDIT: As noted above, this is fixed for strict mode in Static Hermes (static_h branch).

[tmikov]

@gibson042 As @avp mentioned, this has already been fixed in the Static Hermes branch. That is the branch being developed and supported.

But I am curious - why do .caller and arguments actually matter? Since they do nothing but throw, what is the use case? Is this a theoretical "doesn't 100% follow the spec" thing?

[gibson042]

The biggest problem is their non-configurability... Endo tries to ensure uniformity, and not being able to delete these properties interferes with that.

[tmikov]

Fixed in bb90b33.

I am applying the label fixed-in-sh, meaning that this has been fixed in Static Hermes but is "Wontfix" in Hermes. We need this label because technically SH has not been released yet, but at the same time we don't want to keep issues like this open.

[gibson042]

@tmikov Thanks!

[leotm]

thanks ^^ tested the change our side here