Repository: ember-template-lint/ember-template-lint
Pull request #392 (2018-04-15 04:14:22)

Add no-implicit-this rule
This rule disallows paths with an implicit `this` as described in https://github.com/emberjs/rfcs/pull/308.

For example, `{{foo}}` and `{{my-component foo.bar}}` are disallowed.

This lint suggests three possible alternatives:

1. If the root of the path is passed in as an argument to the component then the path should be prefixed with `@`
e.g. `onClick` → `@onClick`. This suggestion is only possbile when using Ember 3.1+.

2. If the root of the path is a property on `this` then prefix the path with `this.`
e.g. `isExpanded` → `this.isExpanded`.

3. If the path is in fact a component or helper invocation that has no arguments, you must add this component to the `allow` whitelist in the rule's configuration.

Unfortunately, without something like module unification it is near impossible to know for certain whether something like `{{foo-bar}}` is a component or helper invocation, but I believe that an app will generally not have many components like this so it's ok to require them to be whitelisted.

rwjblue: @mmun - I just pushed some changes, mostly just cleanup and rebasing stuff.  I think this rule is good to go once we get some docs. Thoughts on `no-ambiguous-paths` vs `strict-paths`?


rwjblue: OK, I have updated the rule name to `no-implicit-this-paths` and added documentation.

Anything else needed here?
rwjblue: Chatted with @mmun about this, and I think we both prefer `no-implicit-this`.  I'll update accordingly...

Patch:
diff --git a/docs/rule/no-implicit-this-paths.md b/docs/rule/no-implicit-this-paths.md
new file mode 100644
index 00000000..418dc67e
--- /dev/null
+++ b/docs/rule/no-implicit-this-paths.md
@@ -0,0 +1,98 @@
+## no-implicit-this
+
+This rule aides in the migration path for [emberjs/rfcs#308](https://github.com/emberjs/rfcs/pull/308).
+
+### Motivation
+
+Currently, the way to access properties on a components class is `{{greeting}}`
+from a template. This works because the component class is one of the objects
+we resolve against during the evaluation of the expression.
+
+The first problem with this approach is that the `{{greeting}}` syntax is
+ambiguous, as it could be referring to a local variable (block param), a helper
+with no arguments, a closed over component, or a property on the component
+class.
+
+#### Exemplar
+
+Consider the following example where the ambiguity can cause issues:
+
+You have a component class that looks like the following component and
+template:
+
+```js
+import Component from '@ember/component';
+import computed from '@ember/computed';
+
+export default Component.extend({
+  formatName: computed('firstName', 'lastName', function() {
+    return `${this.firstName} ${this.lastName}`;
+  });
+});
+```
+
+```hbs
+<h1>Hello {{formatName}}!</h1>
+```
+
+Given `{ firstName: 'Chad', lastName: 'Hietala' }`, Ember will render the
+following:
+
+```html
+<h1>Hello Chad Hietala!</h1>
+```
+
+Now some time goes on and someone adds a `formatName` helper at
+`app/helpers/fortmatName.js` that looks like the following:
+
+```js
+export default function formatName([firstName, lastName]) {
+  return `${firstName} ${lastName}`;
+}
+```
+
+Due to the fact that helpers take precedence over property lookups, our
+`{{formatName}}` now resolves to a helper. When the helper runs it doesn't have
+any arguments so our template now renders the following:
+
+```html
+<h1>Hello !</h1>
+```
+
+This can be a refactoring hazard and can often lead to confusion for readers of
+the template. Upon encountering `{{greeting}}` in a component's template, the
+reader has to check all of these places: first, you need to scan the
+surrounding lines for block params with that name; next, you check in the
+helpers folder to see if there is a helper with that name (it could also be
+coming from an addon!); finally, you check the component's JavaScript class to
+look for a (computed) property.
+
+Like
+[RFC#0276](https://github.com/emberjs/rfcs/blob/master/text/0276-named-args.md)
+made argument usage explicit through the `@` prefix, the `this` prefix will
+resolve the ambiguity and greatly improve clarity, especially in big projects
+with a lot of files (and uses a lot of addons).
+
+As an aside, the ambiguity that causes confusion for human readers is also a
+problem for the compiler. While it is not the main goal of this proposal,
+resolving this ambiguity also helps the rendering system. Currently, the
+"runtime" template compiler has to perform a helper lookup for every
+`{{greeting}}` in each template. It will be able to skip this resolution
+process and perform other optimizations (such as reusing the internal
+[reference](https://github.com/glimmerjs/glimmer-vm/blob/master/guides/04-references.md)
+object and caches) with this addition.
+
+Furthermore, by enforcing the `this` prefix, tooling like the [Ember Language
+Server](https://github.com/emberwatch/ember-language-server) does not need to
+know about fallback resolution rules. This makes common features like ["Go To
+Definition"](https://code.visualstudio.com/docs/editor/editingevolved#_go-to-definition)
+much easier to implement since we have semantics that mean "property on class".
+
+### Configuration
+
+ The following values are valid configuration:
+
+  * boolean - `true` to enable / `false` to disable
+  * object -- An object with the following keys:
+    * `allow` -- An array of component / helper names for that may be called
+      without arguments
diff --git a/docs/rules.md b/docs/rules.md
index e6b30474..44242a09 100644
--- a/docs/rules.md
+++ b/docs/rules.md
@@ -29,6 +29,7 @@
 * [template-length](rule/template-length.md)
 * [no-triple-curlies](rule/no-triple-curlies.md)
 * [no-unused-block-params](rule/no-unused-block-params.md)
+* [no-implicit-this](rule/no-implicit-this.md)
 
 ## Deprecations
 
diff --git a/lib/rules/index.js b/lib/rules/index.js
index 4e0265c8..23a1cda6 100644
--- a/lib/rules/index.js
+++ b/lib/rules/index.js
@@ -37,4 +37,5 @@ module.exports = {
   'no-partial': require('./lint-no-partial'),
   'invocable-blacklist': require('./lint-invocable-blacklist'),
   'no-attrs-in-components': require('./lint-no-attrs-in-components'),
+  'no-implicit-this': require('./lint-no-implicit-this'),
 };
diff --git a/lib/rules/internal/scope.js b/lib/rules/internal/scope.js
index 8863fb3b..decb0cfa 100644
--- a/lib/rules/internal/scope.js
+++ b/lib/rules/internal/scope.js
@@ -3,11 +3,18 @@
 function getLocalName(node) {
 
   switch (node.type) {
+
   case 'ElementNode':
     // unfortunately the ElementNode stores `tag` as a string
     // if that changes in glimmer-vm this will need to be updated
     return node.tag.split('.')[0];
 
+  case 'SubExpression':
+  case 'MustacheStatement':
+  case 'BlockStatement':
+    return node.path.parts[0];
+
+  case 'PathExpression':
   default:
     return node.parts[0];
   }
diff --git a/lib/rules/lint-invocable-blacklist.js b/lib/rules/lint-invocable-blacklist.js
index a048152d..cd27fb10 100644
--- a/lib/rules/lint-invocable-blacklist.js
+++ b/lib/rules/lint-invocable-blacklist.js
@@ -71,18 +71,16 @@ module.exports = class InvocableBlacklist extends Rule {
   }
 
   _checkNode(node, name) {
-    if (node.type === 'ElementNode') {
-      if (this._scope.isLocal(node)) { return; }
+    if (this._scope.isLocal(node)) {
+      return;
+    }
 
+    if (node.type === 'ElementNode') {
       if (dasherize(node.tag) === name || node.tag === name) {
         this._logNode(node, `<${node.tag} />`);
       }
     } else {
-      if (this._scope.isLocal(node.path)) { return; }
-
-      if ((node.path.original === name ||
-        checkForComponentHelper(node, name)) &&
-        !this._scope.isLocal(node.path)) {
+      if (node.path.original === name || checkForComponentHelper(node, name)) {
         this._logNode(node, `{{${name}}}`);
       }
     }
diff --git a/lib/rules/lint-no-implicit-this.js b/lib/rules/lint-no-implicit-this.js
new file mode 100644
index 00000000..9807abc5
--- /dev/null
+++ b/lib/rules/lint-no-implicit-this.js
@@ -0,0 +1,129 @@
+'use strict';
+
+const Rule = require('./base');
+const createErrorMessage = require('../helpers/create-error-message');
+const Scope = require('./internal/scope');
+
+function message(original) {
+  return `Ambiguous path '${original}' is not allowed. ` +
+   `Use '@${original}' if it is a named argument ` +
+   `or 'this.${original}' if it is a property on 'this'. ` +
+   'If it is a helper or component that has no arguments ' +
+   'you must manually add it to the \'no-implicit-this\' rule configuration, e.g. ' +
+   `'no-implicit-this': { allow: ['${original}'] }.`;
+}
+
+function isString(value) {
+  return typeof value === 'string';
+}
+
+// Allow Ember's builtin argless syntaxes
+const ARGLESS_BUILTINS = [
+  'debugger',
+  'input',
+  'outlet',
+  'textarea',
+  'yield',
+];
+
+module.exports = class StrictPaths extends Rule {
+  parseConfig(config) {
+    if (config === false || config === undefined) {
+      return false;
+    }
+
+    switch (typeof config) {
+    case 'undefined':
+      return false;
+
+    case 'boolean':
+      if (config) {
+        return {
+          allow: ARGLESS_BUILTINS
+        };
+      } else {
+        return false;
+      }
+
+    case 'object':
+      if (Array.isArray(config.allow) && config.allow.every(isString)) {
+        return {
+          allow: ARGLESS_BUILTINS.concat(config.allow)
+        };
+      }
+      break;
+    }
+
+    let errorMessage = createErrorMessage(this.ruleName, [
+      '  * boolean - `true` to enable / `false` to disable',
+      '  * object -- An object with the following keys:',
+      '    * `allow` -- An array of component / helper names for that may be called without arguments',
+    ], config);
+
+    throw new Error(errorMessage);
+  }
+
+  // The way this visitor works is a bit sketchy. We need to lint the PathExpressions
+  // in the callee position differently those in an argument position.
+  //
+  // Unfortunately, the current visitor API doesn't give us a good way to differentiate
+  // these two cases. Instead, we rely on the fact that the _first_ PathExpression that
+  // we enter after entering a MustacheStatement/BlockStatement/... will be the callee
+  // and we track this using a flag called `nextPathIsCallee`.
+  visitor() {
+    let scope = new Scope();
+
+    let nextPathIsCallee = false;
+
+    return {
+      PathExpression(path) {
+        if (nextPathIsCallee) {
+          // All paths are valid callees so there's nothing to check.
+        } else {
+          let valid = path.data ||
+                      path.this ||
+                      scope.isLocal(path) ||
+                      this.config.allow.indexOf(path.original) > -1;
+
+
+          if (!valid) {
+            this.log({
+              message: message(path.original),
+              line: path.loc && path.loc.start.line,
+              column: path.loc && path.loc.start.column,
+              source: this.sourceForNode(path)
+            });
+          }
+        }
+
+        nextPathIsCallee = false;
+      },
+
+      SubExpression() {
+        nextPathIsCallee = true;
+      },
+
+      ElementModifierStatement() {
+        nextPathIsCallee = true;
+      },
+
+      MustacheStatement(node) {
+        let isCall = node.params.length > 0 || node.hash.pairs.length > 0;
+
+        nextPathIsCallee = isCall;
+      },
+
+      BlockStatement: {
+        enter(node) {
+          nextPathIsCallee = true;
+          scope.pushFrame(node.program.blockParams);
+        },
+        exit(node) {
+          scope.popFrame(node);
+        }
+      }
+    };
+  }
+};
+
+module.exports.message = message;


Test patch:
diff --git a/test/unit/rules/lint-no-implicit-this-test.js b/test/unit/rules/lint-no-implicit-this-test.js
new file mode 100644
index 00000000..6854c48c
--- /dev/null
+++ b/test/unit/rules/lint-no-implicit-this-test.js
@@ -0,0 +1,106 @@
+'use strict';
+
+const generateRuleTests = require('../../helpers/rule-test-harness');
+const message = require('../../../lib/rules/lint-no-implicit-this').message;
+
+let statements = [
+  path => `{{${path}}}`,
+  path => `{{${path} argument=true}}`,
+  path => `{{#${path}}}{{/${path}}}`,
+  path => `{{helper argument=${path}}}`,
+  path => `{{#helper argument=${path}}}{{/helper}}`,
+  path => `{{echo (helper ${path})}}`,
+  path => `<div {{helper ${path}}}></div>`,
+];
+
+let good = [
+  '{{debugger}}',
+  '{{input}}',
+  '{{outlet}}',
+  '{{textarea}}',
+  '{{yield}}',
+  {
+    config: { allow: ['book-details'] },
+    template: '{{book-details}}'
+  }
+];
+
+statements.forEach(statement => {
+  good.push(`${statement('@book')}`);
+  good.push(`${statement('@book.author')}`);
+  good.push(`${statement('this.book')}`);
+  good.push(`${statement('this.book.author')}`);
+  good.push(`{{#books as |book|}}${statement('book')}{{/books}}`);
+  good.push(`{{#books as |book|}}${statement('book.author')}{{/books}}`);
+});
+
+generateRuleTests({
+  name: 'no-implicit-this',
+
+  config: true,
+
+  good,
+
+  bad: [
+    {
+      template: '{{book}}',
+      result: {
+        message: message('book'),
+        moduleId: 'layout.hbs',
+        source: 'book',
+        line: 1,
+        column: 2
+      }
+    },
+    {
+      template: '{{book-details}}',
+      result: {
+        message: message('book-details'),
+        moduleId: 'layout.hbs',
+        source: 'book-details',
+        line: 1,
+        column: 2
+      }
+    },
+    {
+      template: '{{book.author}}',
+      result: {
+        message: message('book.author'),
+        moduleId: 'layout.hbs',
+        source: 'book.author',
+        line: 1,
+        column: 2
+      }
+    },
+    {
+      template: '{{book.author argument=true}}',
+      result: {
+        message: message('book.author'),
+        moduleId: 'layout.hbs',
+        source: 'book.author',
+        line: 1,
+        column: 2
+      }
+    },
+    {
+      template: '{{helper book}}',
+      result: {
+        message: message('book'),
+        moduleId: 'layout.hbs',
+        source: 'book',
+        line: 1,
+        column: 9
+      }
+    },
+    {
+      template: '{{#helper book}}{{/helper}}',
+      result: {
+        message: message('book'),
+        moduleId: 'layout.hbs',
+        source: 'book',
+        line: 1,
+        column: 10
+      }
+    },
+  ]
+});