Enable plain methods to work on classes when called from templates

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,23 @@ 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)}}`, {
+      let context = {
         myFunc() {
-          assert.strictEqual(this, null, 'this is bound to null in production builds');
+          seenThis = this;
         },
-      });
+      };
+
+      this.render(`{{stash stashedFn=(fn this.myFunc this.arg1)}}`, context);
 
       this.stashedFn();
+      assert.strictEqual(
+        seenThis,
+        this.context,
+        'this is bound to the object that myFunc is defined on'
+      );
     }
 
     '@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,160 @@
+import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers';
+import { fn } from '@ember/helper';
+import { on } from '@ember/modifier';
+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;
+
+      class Demo extends Component {
+        constructor(...args) {
+          super(...args);
+          instance = this;
+        }
+
+        foo() {
+          seenThis = this;
+        }
+
+        <template><button type="button" {{on "click" this.foo}}>click me</button></template>
+      }
+
+      this.render(`<this.Demo />`, { Demo });
+
+      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;
+        }
+      }
+
+      class Demo extends Component {
+        constructor(...args) {
+          super(...args);
+          this.obj = new Inner();
+        }
+
+        <template><button type="button" {{on "click" this.obj.method}}>click me</button></template>
+      }
+
+      this.render(`<this.Demo />`, { Demo });
+
+      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'
+      );
+    }
+    ['@test #let block param preserves this binding on method access'](assert) {
+      let innerInstance;
+      let seenThis;
+      let receivedArg;
+
+      class Inner {
+        constructor() {
+          innerInstance = this;
+        }
+
+        method(arg) {
+          seenThis = this;
+          receivedArg = arg;
+        }
+      }
+
+      class Demo extends Component {
+        constructor(...args) {
+          super(...args);
+          this.obj = new Inner();
+        }
+
+        <template>
+          {{#let this.obj as |o|}}
+            <button type="button" {{on "click" (fn o.method "did it")}}>click me</button>
+          {{/let}}
+        </template>
+      }
+
+      this.render(`<this.Demo />`, { Demo });
+
+      assert.ok(innerInstance, 'inner instance was captured');
+
+      runTask(() => this.$('button').click());
+
+      assert.strictEqual(
+        seenThis,
+        innerInstance,
+        '`this` inside o.method should be the inner object instance'
+      );
+      assert.strictEqual(receivedArg, 'did it', 'argument from fn is passed through');
+    }
+
+    ['@test each over array of objects preserves this binding on item methods'](assert) {
+      let seenPairs = [];
+
+      class Item {
+        constructor(name) {
+          this.name = name;
+        }
+
+        greet() {
+          seenPairs.push({ self: this, name: this.name });
+        }
+      }
+
+      let items = [new Item('alice'), new Item('bob'), new Item('carol')];
+
+      class Demo extends Component {
+        constructor(...args) {
+          super(...args);
+          this.items = items;
+        }
+
+        <template>
+          {{#each this.items as |item|}}
+            <button type="button" class={{item.name}} {{on "click" item.greet}}>{{item.name}}</button>
+          {{/each}}
+        </template>
+      }
+
+      this.render(`<this.Demo />`, { Demo });
+
+      runTask(() => this.$('button.alice').click());
+      runTask(() => this.$('button.bob').click());
+      runTask(() => this.$('button.carol').click());
+
+      assert.strictEqual(seenPairs.length, 3, 'all three handlers fired');
+      assert.strictEqual(seenPairs[0].self, items[0], 'alice: this is the Item instance');
+      assert.strictEqual(seenPairs[0].name, 'alice', 'alice: this.name is correct');
+      assert.strictEqual(seenPairs[1].self, items[1], 'bob: this is the Item instance');
+      assert.strictEqual(seenPairs[1].name, 'bob', 'bob: this.name is correct');
+      assert.strictEqual(seenPairs[2].self, items[2], 'carol: this is the Item instance');
+      assert.strictEqual(seenPairs[2].name, 'carol', 'carol: this.name is correct');
+    }
+  }
+);

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,36 @@ 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) {
-    this.render(`<Stash @stashedFn={{fn this.myFunc this.arg1}}/>`, {
+  @test
+  'this context is available within the callback'(assert: Assert) {
+    let seenThis: unknown;
+
+    let context = {
       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.render(`<Stash @stashedFn={{fn this.myFunc this.arg1}}/>`, context);
 
     this.stashedFn?.();
     assert.verifySteps(['calling stashed function']);
+    assert.ok(seenThis !== null && seenThis !== undefined, 'this is not null/undefined');
+    assert.strictEqual(
+      (seenThis as Record<string, unknown>)['arg1'],
+      'foo',
+      'this is bound to the object myFunc is defined on'
+    );
   }
 
   @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,
@@ -84,9 +85,13 @@ EXPRESSIONS.add(SexpOpcodes.GetFreeAsHelperHead, (op, expr) => {
 function withPath(op: PushExpressionOp, path?: string[]) {
   if (path === undefined || path.length === 0) return;
 
-  for (let i = 0; i < path.length; i++) {
+  for (let i = 0; i < path.length - 1; i++) {
     op(VM_GET_PROPERTY_OP, path[i]);
   }
+
+  // The last segment uses GetPropertyBound to tag the ref with its parent,
+  // so that consumers (on, fn) can bind `this` at invocation time.
+  op(VM_GET_PROPERTY_BOUND_OP, path[path.length - 1]);
 }
 
 EXPRESSIONS.add(SexpOpcodes.Undefined, (op) => PushPrimitiveReference(op, undefined));

packages/@glimmer/reference/index.ts

@@ -9,6 +9,8 @@ export {
 } from './lib/iterable';
 export {
   childRefFor,
+  getBindingParentRef,
+  setBindingParentRef,
   childRefFromParts,
   createComputeRef,
   createConstRef,

packages/@glimmer/reference/lib/reference.ts

@@ -248,6 +248,19 @@ export function childRefFor(_parentRef: Reference, path: string): Reference {
   return child;
 }
 
+// WeakMap used to tag a reference created by VM_GET_PROPERTY_BOUND_OP with its
+// parent reference. Consumers like `on` and `fn` check this to bind `this`
+// at invocation time, while `valueForRef` returns the original value unchanged.
+const BOUND_THIS_REFS: WeakMap<Reference, Reference> = new WeakMap();
+
+export function setBindingParentRef(ref: Reference, parentRef: Reference): void {
+  BOUND_THIS_REFS.set(ref, parentRef);
+}
+
+export function getBindingParentRef(ref: Reference): Reference | undefined {
+  return BOUND_THIS_REFS.get(ref);
+}
+
 export function childRefFromParts(root: Reference, parts: string[]): Reference {
   let reference = root;