[BUGFIX lts] Restore {{this.attrs.foo}} deprecation

Previously (as of 3.28) `{{this.attrs.foo}}` in the template was
deprecated with a suggestion to switch to `{{@foo}}`, since that
is what we internally rewrite that syntax into.

When the deprecation was [removed in 4.0][1], we correctly made the
observation `this.attrs` is a real property on classic component
classes, and accessing it from the template is perfectly legal and
should not trigger any deprecation.

However, what slipped everyone's mind is that `this.attrs.*` gives
you a "Mutable Cell" object. That is not, inheriently problematic.
While it's not particularly useful to render these objects directly
(it stingifies into `[object Object]`), you can render the `.value`
property or perhaps pass them around as values.

What slipped everyone's mind however, is that that's not what the
previously-deprecated `{{this.attrs.foo}}` does or mean – it gets
rewritten into `{{@foo}}`, so in fact, `{{this.attrs.foo}}` is a
magic syntax that automagically unwraps the Mutable Cell for you,
which is indeed unexpected and the whole point of that originally
deprecation.

One reason why this slipped our minds and why it was so hard to
spot was that the rewrite logic is hidden deeply/inconspicuously
as a side-effect inside the `isAttrs()` function.

The original intent (and probably too "clever") of the code was
probably that: at this point, we already know we are going to
throw away and rewrite this node as `{{@foo}}`, we may as well
normalize away any differences between `{{attrs.foo}}` and
`{{this.attrs.foo}}` to make it easier to write the code for
the deprecation message.

Note that even with this generous reading, the original code is
_still_ incorrect. `node.original.slice(5)` is presumably trying
to remove `this.` from the string, and `node.parts.shift()` is
presumably trying to do the same. The latter, however, is wrong
and unnecessary. `node.parts` does not contain the `this` part
to begin with, so this ends up removing `attrs` from the array
instead.

This mistake wasn't consequential in the original deprecated
code, because, as mentioned, the code is going to replace the
`{{this.attrs.foo}}` node with a newly built `{{@foo}}` node
anyway, and the original node is only used for the purpose of
assembling the deprecation message, which doesn't do anything
with `node.parts`.

When the deprecation was removed though, this side-effect ends
up mattering. This modification is an undefined behavior – it
left `node.original` and `node.parts` disagreeing with each
other (ironically, that was because the original author tried
to keep them in-sync, just end up doing it incorrectly).

The latest version of glimmer-vm will ensure these kind of
modifications stay in-sync, but prior to that, it seems like
we end up favoring `node.parts` and so, seemingly without
anyone noticing, the code ends up sneakily transfoming
`{{this.attrs.foo}}` into `{{this.foo}}`, which was never the
intent of any version of this code, but kind of works (for
classic components at least) and went unnoticed so far.

This wasn't caught in the test, because `assertTransformed`
doesn't do what you think. It may sound like that you are
giving it the untransformed code (LHS) and assert that, after
running the AST, it matches exactly the expected result (RHS).

However, that is not at all what it does. The implementation:

```js
this.assert.deepEqual(deloc(ast(before)), deloc(ast(after)));
```

As you can see, it does exactly the same thing with the LHS
and RHS. All it is doing is confirming that either version of
the source code will ultimately normalizes into the same
result after running the AST plugins. Which is to say, all
existing tests that checks `assertTransformed("blah", "blah")`
is testing exactly nothing, because of course compiling the
identical template twice will give you the same result.

Those tests need to be corrected. Or better yet, change the
implementation of `assertTransformed` to do what you would
expect, so that we can _actually_ test no-op transforms. This
commit doesn't address that and that's left for someone else
to pick up.

This commit refactors the code for clearity and reintroduces
the original deprecation with a 6.0 removal. We probably
could have just called it a bugfix, but with the next major
on the horrizon anyway, it doesn't hurt to give this a proper
deprecation cycle in case someone still uses it.

Note that: this has nothing to do with accessing and using
`this.attrs.foo` from the JavaScript class in JS context,
which is not being deprecated here.

[1]: #19660 (comment)

(cherry picked from commit dba5b9a)

Author: chancancode

packages/@ember/-internals/glimmer/tests/integration/components/curly-components-test.js

@@ -1267,10 +1267,13 @@ moduleFor(
       }, "Using {{attrs}} to reference named arguments is not supported. {{attrs.someProp}} should be updated to {{@someProp}}. ('my-app/templates/components/non-block.hbs' @ L1:C24) ");
     }
 
+    // Perhaps change this test to `{{this.attrs.someProp.value}}` when removing the deprecation?
     ['@test non-block with properties on this.attrs']() {
-      this.registerComponent('non-block', {
-        template: 'In layout - someProp: {{this.attrs.someProp}}',
-      });
+      expectDeprecation(() => {
+        this.registerComponent('non-block', {
+          template: 'In layout - someProp: {{this.attrs.someProp}}',
+        });
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.someProp}} should be updated to {{@someProp}}./);
 
       this.render('{{non-block someProp=this.prop}}', {
         prop: 'something here',
@@ -1476,21 +1479,24 @@ moduleFor(
       );
     }
 
+    // Perhaps change this test to `{{this.attrs.foo.value}}` when removing the deprecation?
     ['@test this.attrs.foo === @foo === foo']() {
-      this.registerComponent('foo-bar', {
-        template: strip`
-        Args: {{this.attrs.value}} | {{@value}} | {{this.value}}
-        {{#each this.attrs.items as |item|}}
-          {{item}}
-        {{/each}}
-        {{#each @items as |item|}}
-          {{item}}
-        {{/each}}
-        {{#each this.items as |item|}}
-          {{item}}
-        {{/each}}
-      `,
-      });
+      expectDeprecation(() => {
+        this.registerComponent('foo-bar', {
+          template: strip`
+          Args: {{this.attrs.value}} | {{@value}} | {{this.value}}
+          {{#each this.attrs.items as |item|}}
+            {{item}}
+          {{/each}}
+          {{#each @items as |item|}}
+            {{item}}
+          {{/each}}
+          {{#each this.items as |item|}}
+            {{item}}
+          {{/each}}
+        `,
+        });
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs..+}} should be updated to {{@.+}}./);
 
       this.render('{{foo-bar value=this.model.value items=this.model.items}}', {
         model: {
@@ -1575,10 +1581,13 @@ moduleFor(
       }, "Using {{attrs}} to reference named arguments is not supported. {{attrs.someProp}} should be updated to {{@someProp}}. ('my-app/templates/components/with-block.hbs' @ L1:C24) ");
     }
 
+    // Perhaps change this test to `{{this.attrs.someProp.value}}` when removing the deprecation?
     ['@test block with properties on this.attrs']() {
-      this.registerComponent('with-block', {
-        template: 'In layout - someProp: {{this.attrs.someProp}} - {{yield}}',
-      });
+      expectDeprecation(() => {
+        this.registerComponent('with-block', {
+          template: 'In layout - someProp: {{this.attrs.someProp}} - {{yield}}',
+        });
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.someProp}} should be updated to {{@someProp}}./);
 
       this.render(
         strip`
@@ -3304,16 +3313,19 @@ moduleFor(
       }, "Using {{attrs}} to reference named arguments is not supported. {{attrs.myVar}} should be updated to {{@myVar}}. ('my-app/templates/components/foo-bar.hbs' @ L1:C10) ");
     }
 
+    // Perhaps change this test to `{{this.attrs.myVar.value}}` when removing the deprecation?
     ['@test using this.attrs for positional params']() {
       let MyComponent = Component.extend();
 
-      this.registerComponent('foo-bar', {
-        ComponentClass: MyComponent.reopenClass({
-          positionalParams: ['myVar'],
-        }),
-        template:
-          'MyVar1: {{this.attrs.myVar}} {{this.myVar}} MyVar2: {{this.myVar2}} {{this.attrs.myVar2}}',
-      });
+      expectDeprecation(() => {
+        this.registerComponent('foo-bar', {
+          ComponentClass: MyComponent.reopenClass({
+            positionalParams: ['myVar'],
+          }),
+          template:
+            'MyVar1: {{this.attrs.myVar}} {{this.myVar}} MyVar2: {{this.myVar2}} {{this.attrs.myVar2}}',
+        });
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.myVar2?}} should be updated to {{@myVar2?}}./);
 
       this.render('{{foo-bar 1 myVar2=2}}');
 

packages/ember-template-compiler/lib/plugins/assert-against-attrs.ts

@@ -1,4 +1,4 @@
-import { assert } from '@ember/debug';
+import { assert, deprecate } from '@ember/debug';
 import type { AST, ASTPlugin } from '@glimmer/syntax';
 import calculateLocationDisplay from '../system/calculate-location-display';
 import type { EmberASTPluginEnvironment } from '../types';
@@ -56,17 +56,41 @@ export default function assertAgainstAttrs(env: EmberASTPluginEnvironment): ASTP
 
       PathExpression(node: AST.PathExpression): AST.Node | void {
         if (isAttrs(node, stack[stack.length - 1]!)) {
-          let path = b.path(node.original.substring(6));
-
           assert(
-            `Using {{attrs}} to reference named arguments is not supported. {{attrs.${
-              path.original
-            }}} should be updated to {{@${path.original}}}. ${calculateLocationDisplay(
+            `Using {{attrs}} to reference named arguments is not supported. {{${
+              node.original
+            }}} should be updated to {{@${node.original.slice(6)}}}. ${calculateLocationDisplay(
+              moduleName,
+              node.loc
+            )}`
+          );
+        } else if (isThisDotAttrs(node)) {
+          // When removing this, ensure `{{this.attrs.foo}}` is left as-is, without triggering
+          // any assertions/deprecations. It's perfectly legal to reference `{{this.attrs.foo}}`
+          // in the template since it is a real property on the backing class – it will give you
+          // a `MutableCell` wrapper object, but maybe that's what you want. And in any case,
+          // there is no compelling to special case that property access.
+          deprecate(
+            `Using {{this.attrs}} to reference named arguments has been deprecated. {{${
+              node.original
+            }}} should be updated to {{@${node.original.slice(11)}}}. ${calculateLocationDisplay(
               moduleName,
               node.loc
             )}`,
-            node.this !== false
+            false,
+            {
+              id: 'attrs-arg-access',
+              url: 'https://deprecations.emberjs.com/v3.x/#toc_attrs-arg-access',
+              until: '6.0.0',
+              for: 'ember-source',
+              since: {
+                available: '3.26.0',
+                enabled: '3.26.0',
+              },
+            }
           );
+
+          return b.path(`@${node.original.slice(11)}`, node.loc);
         }
       },
     },
@@ -75,19 +99,10 @@ export default function assertAgainstAttrs(env: EmberASTPluginEnvironment): ASTP
 
 function isAttrs(node: AST.PathExpression, symbols: string[]) {
   let name = node.parts[0];
+  return node.head.type === 'VarHead' && name === 'attrs' && symbols.indexOf(name) === -1;
+}
 
-  if (name && symbols.indexOf(name) !== -1) {
-    return false;
-  }
-
-  if (name === 'attrs') {
-    if (node.this === true) {
-      node.parts.shift();
-      node.original = node.original.slice(5);
-    }
-
-    return true;
-  }
-
-  return false;
+function isThisDotAttrs(node: AST.PathExpression) {
+  let name = node.parts[0];
+  return node.head.type === 'ThisHead' && name === 'attrs';
 }

packages/ember-template-compiler/tests/plugins/assert-against-attrs-test.js

@@ -29,14 +29,26 @@ moduleFor(
       }, /Using {{attrs}} to reference named arguments is not supported. {{attrs.foo.bar}} should be updated to {{@foo.bar}}./);
     }
 
-    ['@test it does not assert against this.attrs']() {
-      this.assertTransformed(`{{this.attrs.foo}}`, `{{this.attrs.foo}}`);
-      this.assertTransformed(`{{if this.attrs.foo "foo"}}`, `{{if this.attrs.foo "foo"}}`);
-      this.assertTransformed(`{{#if this.attrs.foo}}{{/if}}`, `{{#if this.attrs.foo}}{{/if}}`);
-      this.assertTransformed(
-        `{{deeply (nested this.attrs.foo.bar)}}`,
-        `{{deeply (nested this.attrs.foo.bar)}}`
-      );
+    // When removing the deprecation, ensure `{{this.attrs.foo}}` isn't rewritten and does NOT trigger any assertions/deprecations
+    ['@test this.attrs is deprecated']() {
+      expectDeprecation(() => {
+        this.assertTransformed(`{{this.attrs.foo}}`, `{{@foo}}`);
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo}} should be updated to {{@foo}}./);
+
+      expectDeprecation(() => {
+        this.assertTransformed(`{{if this.attrs.foo "foo"}}`, `{{if @foo "foo"}}`);
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo}} should be updated to {{@foo}}./);
+
+      expectDeprecation(() => {
+        this.assertTransformed(`{{#if this.attrs.foo}}{{/if}}`, `{{#if @foo}}{{/if}}`);
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo}} should be updated to {{@foo}}./);
+
+      expectDeprecation(() => {
+        this.assertTransformed(
+          `{{deeply (nested this.attrs.foo.bar)}}`,
+          `{{deeply (nested @foo.bar)}}`
+        );
+      }, /Using {{this.attrs}} to reference named arguments has been deprecated. {{this.attrs.foo.bar}} should be updated to {{@foo.bar}}./);
     }
   }
 );