[BUGFIX] plain method calling from templates (no explicit binding required -- follows JS semantics) - on still requires binding

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}`);
+        }
+
+        <template>{{(this.foo)}}</template>
+      }
+
+      this.render(`<this.Demo />`, { 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();
+        }
+
+        <template>{{ (this.obj.method) }}</template>
+      }
+
+      this.render(`<this.Demo />`, { 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();
+        }
+
+        <template>{{ (this.obj.method) }}</template>
+      }
+
+      this.render(`<this.Demo />`, { 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 {
+        <template>{{ (@cb) }}</template>
+      }
+
+      class Demo extends Component {
+        constructor(...args) {
+          super(...args);
+          instance = this;
+        }
+
+        foo() {
+          assert.step(`match:${this === instance}`);
+        }
+
+        <template><Child @cb={{this.foo}} /></template>
+      }
+
+      this.render(`<this.Demo />`, { 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();
+        }
+
+        <template>
+          {{#let this.obj as |o|}}
+            {{(o.method "did it")}}
+          {{/let}}
+        </template>
+      }
+
+      this.render(`<this.Demo />`, { 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;
+        }
+
+        <template>
+          <ul>
+            {{#each this.items as |item|}}
+              <li>{{(item.greet)}}</li>
+            {{/each}}
+          </ul>
+        </template>
+      }
+
+      this.render(`<this.Demo />`, { 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;
+        };
+
+        <template>{{this.foo}}</template>
+      }
+
+      this.render(`<this.Demo />`, { 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;
+        };
+
+        <template>
+          {{#let (fn this.foo) as |fned|}}
+            {{ (fned) }}
+          {{/let}}
+        </template>
+      }
+
+      this.render(`<this.Demo />`, { 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();
+        }
+
+        <template>{{(this.obj.customHelper)}}</template>
+      }
+
+      this.render(`<this.Demo />`, { 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();
+        }
+
+        <template>{{#let (fn this.obj.method) as |f|}}{{(f)}}{{/let}}</template>
+      }
+
+      this.render(`<this.Demo />`, { Demo });
+      assert.verifySteps(['attempted', 'error'])
+    }
+  }
+);

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<string, Reference> {
 export interface Arguments {
   positional: readonly unknown[];
   named: Record<string, unknown>;
+  /**
+   * 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 {

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 extends Arguments = Arguments> =
-  | [...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<State> {
   }
 
   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 {

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