diff --git a/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs b/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs new file mode 100644 index 00000000000..0bc92e69f47 --- /dev/null +++ b/packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs @@ -0,0 +1,308 @@ +import { set } from '@ember/object'; +import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers'; +import { fn } from '@ember/helper'; +import { helperCapabilities, setHelperManager } from '@glimmer/manager'; +import { Component } from '../utils/helpers'; + +moduleFor( + 'function calls on normal functions retain JS "this" semantics', + class extends RenderingTestCase { + ['@test this.method()'](assert) { + let instance; + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + assert.step('captured:demo'); + } + + foo() { + assert.step(`match:${instance && this === instance}`); + } + + + } + + this.render(``, { Demo }); + + assert.verifySteps(['captured:demo', 'match:true']) + } + + ['@test this.obj.method()'](assert) { + let innerInstance; + + class Inner { + constructor() { + innerInstance = this; + assert.step('captured:inner'); + } + + method() { + assert.step(`match:${innerInstance && this === innerInstance}`); + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + assert.verifySteps(['captured:inner', 'match:true']) + } + + ['@test this.obj.method() when this.obj is entirely replaced'](assert) { + let instance; + let seenThis; + + class Inner { + method() { + seenThis = this; + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + let first = instance.obj; + assert.strictEqual(seenThis, first); + + let second = new Inner(); + runTask(() => set(instance, 'obj', second)); + + assert.strictEqual(seenThis, second); + } + + ['@test passing function references loses the "this"'](assert) { + let instance; + let seenThis; + + class Child extends Component { + + } + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo() { + assert.step(`match:${this === instance}`); + } + + + } + + this.render(``, { Demo }); + + assert.verifySteps(['match:false']); + } + + ['@test aliasing through let does not confuse o.method() using "o" for "this"'](assert) { + let innerInstance; + let seenThis; + let receivedArg; + + class Inner { + constructor() { + innerInstance = this; + } + + method(arg) { + assert.step(`match:${innerInstance && this === innerInstance}`); + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + assert.verifySteps(['match:true']); + } + + ['@test methods called on an iterated item, use the item as the "this"'](assert) { + let seenPairs = []; + + class Item { + constructor(name) { + this.name = name; + } + + greet() { + assert.step(`greet:${this.name}`); + } + } + + let items = [new Item('alice'), new Item('bob'), new Item('carol')]; + + class Demo extends Component { + constructor(...args) { + super(...args); + this.items = items; + } + + + } + + this.render(``, { Demo }); + + assert.verifySteps(['greet:alice', 'greet:bob', 'greet:carol']); + } + + ['@test already-bound functions are unaffected'](assert) { + let instance; + let seenThis; + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo = () => { + seenThis = this; + }; + + + } + + this.render(``, { Demo }); + + assert.strictEqual(seenThis, instance); + } + + ['@test (fn) on methods still behaves appropriately'](assert) { + let instance; + let seenThis; + + class Demo extends Component { + constructor(...args) { + super(...args); + instance = this; + } + + foo = () => { + seenThis = this; + }; + + + } + + this.render(``, { Demo }); + + assert.strictEqual(seenThis, instance); + } + + ['@test a function with a custom helper manager read off a path keeps its manager'](assert) { + let sawDefinition; + + class MyHelperManager { + capabilities = helperCapabilities('3.23', { hasValue: true }); + + createHelper(definition, args) { + return { definition, args }; + } + + getValue({ definition }) { + sawDefinition = definition; + return 'CUSTOM_MANAGER_RAN'; + } + + getDebugName() { + return 'my-custom-helper'; + } + } + + function customHelper() { + return 'PLAIN_FN_RAN'; + } + setHelperManager(() => new MyHelperManager(), customHelper); + + class Inner { + customHelper = customHelper; + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + + this.assertText('CUSTOM_MANAGER_RAN'); + assert.strictEqual( + sawDefinition, + customHelper, + // i.e.: a.foo !== a.foo.bind(a) + 'the custom manager received the original function, not a `.bind()` wrapper' + ); + } + + ['@test using "fn" un unbound functions is not allowed'](assert) { + assert.expect(4); + + class Inner { + method() { + // If we didn't throw the error, this could be allowed + assert.step('attempted'); + assert.throws(() => { + assert.step('error'); + String(this)}, 'not bound to a valid') + } + } + + class Demo extends Component { + constructor(...args) { + super(...args); + this.obj = new Inner(); + } + + + } + + this.render(``, { Demo }); + assert.verifySteps(['attempted', 'error']) + } + } +); diff --git a/packages/@glimmer/interfaces/lib/runtime/arguments.d.ts b/packages/@glimmer/interfaces/lib/runtime/arguments.d.ts index 92a1c045a85..10e66eb9598 100644 --- a/packages/@glimmer/interfaces/lib/runtime/arguments.d.ts +++ b/packages/@glimmer/interfaces/lib/runtime/arguments.d.ts @@ -16,6 +16,12 @@ export interface VMArguments { export interface CapturedArguments { positional: CapturedPositionalArguments; named: CapturedNamedArguments; + /** + * The reference the helper value was read from a path off of, if any (e.g. the + * `this.obj` in `{{(this.obj.method)}}`). Resolved lazily as `Arguments.receiver` + * so that helpers which never read it do not entangle this reference. + */ + receiver?: Reference; [CAPTURED_ARGS]: true; } @@ -59,6 +65,12 @@ export interface CapturedNamedArguments extends Record { export interface Arguments { positional: readonly unknown[]; named: Record; + /** + * The object the helper value was read from, used as `this` when the helper is a + * plain function (matching JS `obj.method()` semantics). Read lazily, so helpers + * that ignore it do not entangle the underlying reference. + */ + receiver?: unknown; } export interface ArgumentsDebug { diff --git a/packages/@glimmer/manager/lib/internal/defaults.ts b/packages/@glimmer/manager/lib/internal/defaults.ts index 3745f9d20e0..6b70fb25e3c 100644 --- a/packages/@glimmer/manager/lib/internal/defaults.ts +++ b/packages/@glimmer/manager/lib/internal/defaults.ts @@ -1,15 +1,7 @@ -import type { - CapturedArguments as Arguments, - HelperCapabilities, - HelperManagerWithValue, -} from '@glimmer/interfaces'; +import type { Arguments, HelperCapabilities, HelperManagerWithValue } from '@glimmer/interfaces'; import { buildCapabilities } from '../util/capabilities'; -type FnArgs = - | [...Args['positional'], Args['named']] - | [...Args['positional']]; - // eslint-disable-next-line @typescript-eslint/no-explicit-any type AnyFunction = (...args: any[]) => unknown; @@ -30,13 +22,16 @@ export class FunctionHelperManager implements HelperManagerWithValue { } getValue({ fn, args }: State): unknown { + // A plain function read off a path is invoked with the object it was read from + // as `this` (provided lazily via `args.receiver`), matching the JavaScript + // semantics of `obj.method()`. `this` is applied at the call itself, never by + // producing a `.bind()`ed copy, so the function keeps its identity everywhere it + // is passed around as a reference. if (Object.keys(args.named).length > 0) { - let argsForFn: FnArgs = [...args.positional, args.named]; - - return fn(...argsForFn); + return fn.apply(args.receiver, [...args.positional, args.named]); } - return fn(...args.positional); + return fn.apply(args.receiver, [...args.positional]); } getDebugName(fn: AnyFunction): string { diff --git a/packages/@glimmer/manager/lib/util/args-proxy.ts b/packages/@glimmer/manager/lib/util/args-proxy.ts index 88ec478855c..18b24857093 100644 --- a/packages/@glimmer/manager/lib/util/args-proxy.ts +++ b/packages/@glimmer/manager/lib/util/args-proxy.ts @@ -140,7 +140,7 @@ export const argsProxyFor = ( capturedArgs: CapturedArguments, type: 'component' | 'helper' | 'modifier' ): Arguments => { - const { named, positional } = capturedArgs; + const { named, positional, receiver } = capturedArgs; let getNamedTag = (_obj: object, key: string) => tagForNamedArg(named, key); let getPositionalTag = (_obj: object, key: string) => tagForPositionalArg(positional, key); @@ -184,5 +184,10 @@ export const argsProxyFor = ( // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment named: namedProxy, positional: positionalProxy, + // Read lazily: only helpers that actually use `this` (e.g. the default + // function helper manager) entangle the receiver reference. + get receiver() { + return receiver === undefined ? undefined : valueForRef(receiver); + }, }; }; diff --git a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/stdlib.ts b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/stdlib.ts index 4f4b29746ea..f11b6cf3997 100644 --- a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/stdlib.ts +++ b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/stdlib.ts @@ -63,7 +63,9 @@ export function StdAppend( }); when(ContentType.Helper, () => { - CallDynamic(op, null, null, () => { + // generic append has no syntactic receiver: a bare `{{value}}` that resolves + // to a function is invoked unbound (use `{{(value)}}` to bind `this`). + CallDynamic(op, null, null, null, () => { op(VM_INVOKE_STATIC_OP, nonDynamicAppend); }); }); diff --git a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/vm.ts b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/vm.ts index f2ef561eda3..777d0183e99 100644 --- a/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/vm.ts +++ b/packages/@glimmer/opcode-compiler/lib/opcode-builder/helpers/vm.ts @@ -16,6 +16,7 @@ import { } from '@glimmer/constants/lib/syscall-ops'; import { VM_POP_FRAME_OP, VM_PUSH_FRAME_OP } from '@glimmer/constants/lib/vm-ops'; import { $fp, $v0 } from '@glimmer/vm/lib/registers'; +import { opcodes as SexpOpcodes } from '@glimmer/wire-format/lib/opcodes'; import type { PushExpressionOp, PushStatementOp } from '../../syntax/compilers'; @@ -83,15 +84,50 @@ export function Call( * @param positional An optional list of expressions to compile * @param named An optional list of named arguments (name + expression) to compile */ +/** + * The syntactic receiver of a call, i.e. the object a member-call's `this` should be: + * `this.obj.method` → `this.obj`. Returns `null` when the callee has no syntactic + * receiver (a bare variable/argument like `@cb`), so that passing a function as an + * argument and then invoking it is unbound — matching JS `const f = obj.m; f()`. + * + * This is deliberately a *syntactic* question about the call site, independent of how + * the callee reference happens to have been derived. + */ +export function receiverExpressionFor( + expression: WireFormat.Expression +): Nullable { + if (!Array.isArray(expression)) return null; + + let [opcode, symbol, path] = expression as [number, number, (readonly string[])?]; + + if (opcode !== SexpOpcodes.GetSymbol && opcode !== SexpOpcodes.GetLexicalSymbol) { + return null; + } + + if (!Array.isArray(path) || path.length === 0) { + return null; + } + + return [opcode, symbol, path.slice(0, -1)] as unknown as WireFormat.Expression; +} + export function CallDynamic( op: PushExpressionOp, positional: WireFormat.Core.Params, named: WireFormat.Core.Hash, + receiver: Nullable, append?: () => void ): void { op(VM_PUSH_FRAME_OP); SimpleArgs(op, positional, named, false); op(VM_DUP_OP, $fp, 1); + // Push the syntactic receiver so the callee is invoked with it as `this` + // (`undefined` when there is none, e.g. `(@cb)`). + if (receiver) { + expr(op, receiver); + } else { + PushPrimitiveReference(op, undefined); + } op(VM_DYNAMIC_HELPER_OP); if (append) { op(VM_FETCH_OP, $v0); diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts b/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts index 1815ea34494..fa3b898281e 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts @@ -23,7 +23,13 @@ import type { PushExpressionOp } from './compilers'; import { expr } from '../opcode-builder/helpers/expr'; import { isGetFreeHelper } from '../opcode-builder/helpers/resolution'; import { SimpleArgs } from '../opcode-builder/helpers/shared'; -import { Call, CallDynamic, Curry, PushPrimitiveReference } from '../opcode-builder/helpers/vm'; +import { + Call, + CallDynamic, + Curry, + PushPrimitiveReference, + receiverExpressionFor, +} from '../opcode-builder/helpers/vm'; import { HighLevelResolutionOpcodes } from '../opcode-builder/opcodes'; import { Compilers } from './compilers'; @@ -44,7 +50,7 @@ EXPRESSIONS.add(SexpOpcodes.Call, (op, [, expression, positional, named]) => { }); } else { expr(op, expression); - CallDynamic(op, positional, named); + CallDynamic(op, positional, named, receiverExpressionFor(expression)); } }); diff --git a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts index cfafe8dd329..211c061b645 100644 --- a/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts +++ b/packages/@glimmer/opcode-compiler/lib/syntax/statements.ts @@ -69,6 +69,7 @@ import { CallDynamic, DynamicScope, PushPrimitiveReference, + receiverExpressionFor, } from '../opcode-builder/helpers/vm'; import { HighLevelBuilderOpcodes, HighLevelResolutionOpcodes } from '../opcode-builder/opcodes'; import { debugSymbolsOperand, labelOperand, stdlibOperand } from '../opcode-builder/operands'; @@ -233,7 +234,7 @@ STATEMENTS.add(SexpOpcodes.Append, (op, [, value]) => { }); when(ContentType.Helper, () => { - CallDynamic(op, positional, named, () => { + CallDynamic(op, positional, named, receiverExpressionFor(expression), () => { op(VM_INVOKE_STATIC_OP, stdlibOperand('cautious-non-dynamic-append')); }); }); diff --git a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts index 0e99c73c52f..737254687c9 100644 --- a/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts +++ b/packages/@glimmer/runtime/lib/compiled/opcodes/expressions.ts @@ -94,9 +94,15 @@ APPEND_OPCODES.add(VM_CURRY_OP, (vm, { op1: type, op2: _isStrict }) => { APPEND_OPCODES.add(VM_DYNAMIC_HELPER_OP, (vm) => { let stack = vm.stack; + // The compiler pushes the syntactic receiver for member-calls (`this.obj` in + // `(this.obj.method)`), or `undefined` when the call has no syntactic receiver + // (`(@cb)`). A plain function helper is invoked with it as `this`. + let receiver = check(stack.pop(), CheckReference); let ref = check(stack.pop(), CheckReference); let args = check(stack.pop(), CheckArguments).capture(); + args.receiver = receiver; + let helperRef: Initializable; let initialOwner = vm.getOwner(); diff --git a/packages/@glimmer/runtime/lib/helpers/invoke.ts b/packages/@glimmer/runtime/lib/helpers/invoke.ts index e82e44e28c3..5e4008b71a3 100644 --- a/packages/@glimmer/runtime/lib/helpers/invoke.ts +++ b/packages/@glimmer/runtime/lib/helpers/invoke.ts @@ -18,6 +18,7 @@ function getArgs(proxy: SimpleArgsProxy): Partial { class SimpleArgsProxy { argsCache?: Cache>; + readonly receiver: object; constructor( context: object, @@ -25,6 +26,8 @@ class SimpleArgsProxy { ) { let argsCache = createCache(() => computeArgs(context)); + this.receiver = context; + if (DEBUG) { // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- @fixme ARGS_CACHES!.set(this, argsCache);