Auto-bind prototype methods resolved via template path reads

packages/@ember/-internals/glimmer/tests/integration/helpers/fn-test.js

@@ -1,5 +1,4 @@
 import { set } from '@ember/object';
-import { DEBUG } from '@glimmer/env';
 import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers';
 import { template } from '@ember/template-compiler/runtime';
 import { Component } from '../../utils/helpers';
@@ -129,19 +128,17 @@ moduleFor(
       assert.equal(this.stashedFn(), 'arg1: foo, arg2: bar');
     }
 
-    '@test there is no `this` context within the callback'(assert) {
-      if (DEBUG) {
-        assert.expect(0);
-        return;
-      }
+    '@test this context is preserved within the callback'(assert) {
+      let seenThis;
 
       this.render(`{{stash stashedFn=(fn this.myFunc this.arg1)}}`, {
         myFunc() {
-          assert.strictEqual(this, null, 'this is bound to null in production builds');
+          seenThis = this;
         },
       });
 
       this.stashedFn();
+      assert.ok(seenThis !== null && seenThis !== undefined, 'this is bound to the context object');
     }
 
     '@test can use `this` if bound prior to passing to fn'(assert) {

packages/@ember/-internals/glimmer/tests/integration/this-binding-test.gjs

@@ -0,0 +1,95 @@
+import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers';
+import { on } from '@ember/modifier';
+import { precompileTemplate } from '@ember/template-compilation';
+import { setComponentTemplate } from '@glimmer/manager';
+
+import { Component } from '../utils/helpers';
+
+moduleFor(
+  'Path expression this-binding for class methods',
+  class extends RenderingTestCase {
+    ['@test this.foo maintains this binding'](assert) {
+      let instance;
+      let seenThis;
+
+      let DemoComponent = class extends Component {
+        init() {
+          super.init(...arguments);
+          instance = this;
+        }
+
+        foo() {
+          seenThis = this;
+        }
+      };
+
+      this.owner.register(
+        'component:demo-el',
+        setComponentTemplate(
+          precompileTemplate(
+            '<button type="button" {{on "click" this.foo}}>click me</button>',
+            { strictMode: true, scope: () => ({ on }) }
+          ),
+          DemoComponent
+        )
+      );
+
+      this.render('{{demo-el}}');
+
+      assert.ok(instance, 'component instance was captured');
+
+      runTask(() => this.$('button').click());
+
+      assert.strictEqual(
+        seenThis,
+        instance,
+        '`this` inside the class method should be the component instance'
+      );
+    }
+
+    ['@test this.obj.method maintains this binding through property chain'](assert) {
+      let innerInstance;
+      let seenThis;
+
+      class Inner {
+        constructor() {
+          innerInstance = this;
+        }
+
+        method() {
+          seenThis = this;
+        }
+      }
+
+      let DemoComponent = class extends Component {
+        init() {
+          super.init(...arguments);
+          this.obj = new Inner();
+        }
+      };
+
+      this.owner.register(
+        'component:demo-el',
+        setComponentTemplate(
+          precompileTemplate(
+            '<button type="button" {{on "click" this.obj.method}}>click me</button>',
+            { strictMode: true, scope: () => ({ on }) }
+          ),
+          DemoComponent
+        )
+      );
+
+      this.render('{{demo-el}}');
+
+      assert.ok(innerInstance, 'inner instance was captured');
+
+      runTask(() => this.$('button').click());
+
+      assert.strictEqual(
+        seenThis,
+        innerInstance,
+        '`this` inside the nested method should be the inner object instance'
+      );
+    }
+  }
+);

packages/@glimmer-workspace/integration-tests/test/debug-render-tree-test.ts

@@ -519,7 +519,10 @@ class DebugRenderTreeTest extends RenderTest {
       {
         type: 'modifier',
         name: 'on',
-        args: { positional: ['click', didInsert], named: {} },
+        args: (args: any) =>
+          args.positional[0] === 'click' &&
+          typeof args.positional[1] === 'function' &&
+          Object.keys(args.named).length === 0,
         instance: null,
         bounds: this.nodeBounds(this.element.firstChild),
         children: [],
@@ -558,7 +561,10 @@ class DebugRenderTreeTest extends RenderTest {
       {
         type: 'modifier',
         name: 'on',
-        args: { positional: ['click', didInsert], named: {} },
+        args: (args: any) =>
+          args.positional[0] === 'click' &&
+          typeof args.positional[1] === 'function' &&
+          Object.keys(args.named).length === 0,
         instance: null,
         bounds: this.nodeBounds(this.element.firstChild),
         children: [],
@@ -590,7 +596,10 @@ class DebugRenderTreeTest extends RenderTest {
       {
         type: 'modifier',
         name: 'on',
-        args: { positional: ['click', didInsert], named: { passive: true } },
+        args: (args: any) =>
+          args.positional[0] === 'click' &&
+          typeof args.positional[1] === 'function' &&
+          args.named.passive === true,
         instance: null,
         bounds: this.nodeBounds(this.element.firstChild!.lastChild),
         children: [],
@@ -605,7 +614,10 @@ class DebugRenderTreeTest extends RenderTest {
       {
         type: 'modifier',
         name: 'on',
-        args: { positional: ['click', didInsert], named: {} },
+        args: (args: any) =>
+          args.positional[0] === 'click' &&
+          typeof args.positional[1] === 'function' &&
+          Object.keys(args.named).length === 0,
         instance: null,
         bounds: this.nodeBounds(this.element.firstChild),
         children: [],

packages/@glimmer-workspace/integration-tests/test/helpers/fn-test.ts

@@ -159,10 +159,8 @@ class FnTest extends RenderTest {
     }, /You must pass a function as the `fn` helper's first argument, you passed null. While rendering:\n{2}this.myFunc/u);
   }
 
-  @test({ skip: !DEBUG })
-  'asserts if the provided function accesses `this` without being bound prior to passing to fn'(
-    assert: Assert
-  ) {
+  @test
+  'this.myFunc passed to fn preserves this context'(assert: Assert) {
     this.render(`<Stash @stashedFn={{fn this.myFunc this.arg1}}/>`, {
       myFunc(arg1: string) {
         return `arg1: ${arg1}, arg2: ${this['arg2']}`;
@@ -172,24 +170,29 @@ class FnTest extends RenderTest {
       arg2: 'bar',
     });
 
-    assert.throws(() => {
-      this.stashedFn?.();
-    }, /You accessed `this.arg2` from a function passed to the `fn` helper, but the function itself was not bound to a valid `this` context. Consider updating to use a bound function./u);
+    assert.strictEqual(
+      this.stashedFn?.(),
+      'arg1: foo, arg2: bar',
+      'this.myFunc preserves this binding through fn'
+    );
   }
 
-  @test({ skip: !DEBUG })
-  'there is no `this` context within the callback'(assert: Assert) {
+  @test
+  'this context is available within the callback'(assert: Assert) {
+    let seenThis: unknown;
+
     this.render(`<Stash @stashedFn={{fn this.myFunc this.arg1}}/>`, {
       myFunc() {
         assert.step('calling stashed function');
-        // eslint-disable-next-line @typescript-eslint/no-base-to-string
-        assert.throws(() => String(this), /not bound to a valid `this` context/u);
-        // assert.strictEqual(this, null, 'this is bound to null in production builds');
+        seenThis = this;
       },
+
+      arg1: 'foo',
     });
 
     this.stashedFn?.();
     assert.verifySteps(['calling stashed function']);
+    assert.ok(seenThis !== null && seenThis !== undefined, 'this is bound to the context');
   }
 
   @test

packages/@glimmer-workspace/integration-tests/test/modifiers/on-test.ts

@@ -366,21 +366,21 @@ if (hasDom) {
       }, /You must pass a function as the second argument to the `on` modifier; you passed null. While rendering:\n{2}this.foo/u);
     }
 
-    @test({ skip: !DEBUG })
-    'asserts if the provided callback accesses `this` without being bound prior to passing to on'(
-      assert: Assert
-    ) {
+    @test
+    'this.myFunc passed to on preserves this context'(assert: Assert) {
+      let seenThis: unknown;
+
       this.render(`<button {{on 'click' this.myFunc}}>Click Me</button>`, {
         myFunc(this: any) {
-          assert.throws(() => {
-            consume(this.arg1);
-          }, /You accessed `this.arg1` from a function passed to the `on` modifier, but the function itself was not bound to a valid `this` context. Consider updating to use a bound function/u);
+          seenThis = this;
         },
 
         arg1: 'foo',
       });
 
       this.findButton().click();
+      assert.ok(seenThis !== null && seenThis !== undefined, 'this is preserved');
+      assert.strictEqual((seenThis as any).arg1, 'foo', 'this is the context object');
     }
 
     @test({ skip: !DEBUG })

packages/@glimmer/constants/lib/syscall-ops.ts

@@ -40,6 +40,7 @@ import type {
   VmGetComponentTagName,
   VmGetDynamicVar,
   VmGetProperty,
+  VmGetPropertyBound,
   VmGetVariable,
   VmHasBlock,
   VmHasBlockParams,
@@ -187,7 +188,8 @@ export const VM_IF_INLINE_OP = 109 satisfies VmIfInline;
 export const VM_NOT_OP = 110 satisfies VmNot;
 export const VM_GET_DYNAMIC_VAR_OP = 111 satisfies VmGetDynamicVar;
 export const VM_LOG_OP = 112 satisfies VmLog;
-export const VM_SYSCALL_SIZE = 113 satisfies VmSize;
+export const VM_GET_PROPERTY_BOUND_OP = 113 satisfies VmGetPropertyBound;
+export const VM_SYSCALL_SIZE = 114 satisfies VmSize;
 
 export function isOp(value: number): value is VmOp {
   return value >= 16;

packages/@glimmer/debug/lib/opcode-metadata.ts

@@ -41,6 +41,7 @@ import {
   VM_GET_COMPONENT_LAYOUT_OP,
   VM_GET_COMPONENT_SELF_OP,
   VM_GET_COMPONENT_TAG_NAME_OP,
+  VM_GET_PROPERTY_BOUND_OP,
   VM_GET_PROPERTY_OP,
   VM_GET_VARIABLE_OP,
   VM_HAS_BLOCK_OP,
@@ -220,6 +221,13 @@ if (LOCAL_DEBUG) {
     ops: ['property:const/str'],
   };
 
+  METADATA[VM_GET_PROPERTY_BOUND_OP] = {
+    name: 'GetPropertyBound',
+    mnemonic: 'getpropbound',
+    stackChange: 0,
+    ops: ['property:const/str'],
+  };
+
   METADATA[VM_GET_BLOCK_OP] = {
     name: 'GetBlock',
     mnemonic: 'blockload',

packages/@glimmer/interfaces/lib/vm-opcodes.d.ts

@@ -111,7 +111,8 @@ export type VmIfInline = 109;
 export type VmNot = 110;
 export type VmGetDynamicVar = 111;
 export type VmLog = 112;
-export type VmSize = 113;
+export type VmGetPropertyBound = 113;
+export type VmSize = 114;
 
 export type VmOp =
   | VmHelper
@@ -206,6 +207,7 @@ export type VmOp =
   | VmIfInline
   | VmNot
   | VmGetDynamicVar
-  | VmLog;
+  | VmLog
+  | VmGetPropertyBound;
 
 export type SomeVmOp = VmOp | VmMachineOp;

packages/@glimmer/opcode-compiler/lib/syntax/expressions.ts

@@ -5,6 +5,7 @@ import {
   VM_CONSTANT_REFERENCE_OP,
   VM_FETCH_OP,
   VM_GET_DYNAMIC_VAR_OP,
+  VM_GET_PROPERTY_BOUND_OP,
   VM_GET_PROPERTY_OP,
   VM_GET_VARIABLE_OP,
   VM_HAS_BLOCK_OP,
@@ -55,7 +56,17 @@ EXPRESSIONS.add(SexpOpcodes.Curry, (op, [, expr, type, positional, named]) => {
 
 EXPRESSIONS.add(SexpOpcodes.GetSymbol, (op, [, sym, path]) => {
   op(VM_GET_VARIABLE_OP, sym);
-  withPath(op, path);
+
+  if (sym === 0 && path !== undefined && path.length > 0) {
+    // For `this.*` paths, emit GetPropertyBound for the last segment so that
+    // class methods preserve their `this` binding when passed as callbacks.
+    for (let i = 0; i < path.length - 1; i++) {
+      op(VM_GET_PROPERTY_OP, path[i]);
+    }
+    op(VM_GET_PROPERTY_BOUND_OP, path[path.length - 1]);
+  } else {
+    withPath(op, path);
+  }
 });
 
 EXPRESSIONS.add(SexpOpcodes.GetLexicalSymbol, (op, [, sym, path]) => {

packages/@glimmer/reference/index.ts

@@ -9,6 +9,7 @@ export {
 } from './lib/iterable';
 export {
   childRefFor,
+  type ChildRefTransform,
   childRefFromParts,
   createComputeRef,
   createConstRef,