Repository: ember-cli/eslint-plugin-ember (js)

Issue:
New Rule: `no-controller-access-in-routes`
Accessing a controller by any means other than `controllerFor` (eg `get(this, 'controller')`) doesn't guarantee it to be eagerly loaded.

We're proposing adding this rule to require the use of `controllerFor` when getting a controller, preventing it from being `undefined`.

Pull request description:
Add new rule `no-controller-access-in-routes`
Fixes #896 

Adding a new rule `no-controller-access-in-routes` to prevent accessing the controller in a route outside of `setupController`/`resetController`. These hooks receive the controller as an argument; accessing the controller from outside of these hooks is discouraged.

Also, accessing the controller outside of these hooks without using `controllerFor` might return undefined as the controller is not guaranteed to be eagerly loaded. The rule includes an `allowControllerFor` option (default: `false`) that allows using it to access the controller in the route outside of `setupController`/`resetController`.


Patch:
diff --git a/README.md b/README.md
index 61012818..f60eb132 100644
--- a/README.md
+++ b/README.md
@@ -159,6 +159,7 @@ Rules are grouped by category to help you understand their purpose. Each rule ha
 |    | Rule ID | Description |
 |:---|:--------|:------------|
 | :white_check_mark: | [no-capital-letters-in-routes](./docs/rules/no-capital-letters-in-routes.md) | disallow routes with uppercased letters in router.js |
+|  | [no-controller-access-in-routes](./docs/rules/no-controller-access-in-routes.md) | disallow routes from accessing the controller outside of setupController/resetController |
 | :white_check_mark: | [no-private-routing-service](./docs/rules/no-private-routing-service.md) | disallow injecting the private routing service |
 |  | [no-unnecessary-index-route](./docs/rules/no-unnecessary-index-route.md) | disallow unnecessary `index` route definition |
 | :white_check_mark::wrench: | [no-unnecessary-route-path-option](./docs/rules/no-unnecessary-route-path-option.md) | disallow unnecessary usage of the route `path` option |
diff --git a/docs/rules/no-controller-access-in-routes.md b/docs/rules/no-controller-access-in-routes.md
new file mode 100644
index 00000000..b883c701
--- /dev/null
+++ b/docs/rules/no-controller-access-in-routes.md
@@ -0,0 +1,66 @@
+# no-controller-access-in-routes
+
+Accessing the controller in a route outside of `setupController`/`resetController` hooks (where it is passed as an argument) is discouraged.
+
+If access is required regardless, `controllerFor` must be used to assert the controller isn't undefined as it is not guaranteed to be eagerly loaded (for optimization purposes).
+
+## Rule Details
+
+This rule disallows routes from accessing the controller outside of `setupController`/`resetController`.
+
+## Examples
+
+Examples of **incorrect** code for this rule:
+
+```js
+import Route from '@ember/routing/route';
+import { action } from '@ember/object';
+
+export default class MyRoute extends Route {
+  @action
+  myAction() {
+    const controller = this.controller;
+  }
+}
+```
+
+```js
+import Route from '@ember/routing/route';
+import { action } from '@ember/object';
+
+export default class MyRoute extends Route {
+  @action
+  myAction() {
+    const controller = this.controllerFor('my');
+  }
+}
+```
+
+Examples of **correct** code for this rule:
+
+```js
+import Route from '@ember/routing/route';
+
+export default class MyRoute extends Route {
+  setupController(controller, ...args) {
+    super.setupController(controller, ...args);
+    const foo = controller.foo;
+  }
+}
+```
+
+```js
+import Route from '@ember/routing/route';
+
+export default class MyRoute extends Route {
+  resetController(controller, ...args) {
+    super.resetController(controller, ...args);
+    const foo = controller.foo;
+  }
+}
+```
+
+## Configuration
+
+* object -- containing the following properties:
+  * boolean -- `allowControllerFor` -- wheter the rule should allow or disallow routes from accessing the controller outside of setupController/resetController via `controllerFor` (default: `false`)
diff --git a/lib/index.js b/lib/index.js
index 2502aab2..95a38c87 100644
--- a/lib/index.js
+++ b/lib/index.js
@@ -22,6 +22,7 @@ module.exports = {
     'no-classic-components': require('./rules/no-classic-components'),
     'no-component-lifecycle-hooks': require('./rules/no-component-lifecycle-hooks'),
     'no-computed-properties-in-native-classes': require('./rules/no-computed-properties-in-native-classes'),
+    'no-controller-access-in-routes': require('./rules/no-controller-access-in-routes'),
     'no-controllers': require('./rules/no-controllers'),
     'no-deeply-nested-dependent-keys-with-each': require('./rules/no-deeply-nested-dependent-keys-with-each'),
     'no-duplicate-dependent-keys': require('./rules/no-duplicate-dependent-keys'),
diff --git a/lib/rules/no-controller-access-in-routes.js b/lib/rules/no-controller-access-in-routes.js
new file mode 100644
index 00000000..839ef556
--- /dev/null
+++ b/lib/rules/no-controller-access-in-routes.js
@@ -0,0 +1,169 @@
+'use strict';
+
+const ember = require('../utils/ember');
+const types = require('../utils/types');
+const { getImportIdentifier } = require('../utils/import');
+
+const ERROR_MESSAGE =
+  'Do not access controller in route outside of setupController/resetController';
+
+//------------------------------------------------------------------------------
+// Routing - No controller access in routes
+//------------------------------------------------------------------------------
+
+module.exports = {
+  meta: {
+    type: 'problem',
+    docs: {
+      description:
+        'disallow routes from accessing the controller outside of setupController/resetController',
+      category: 'Routes',
+      recommended: false,
+      url:
+        'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-controller-access-in-routes.md',
+    },
+    fixable: null,
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          allowControllerFor: {
+            type: 'boolean',
+            default: false,
+          },
+        },
+        additionalProperties: false,
+      },
+    ],
+  },
+
+  ERROR_MESSAGE,
+
+  create(context) {
+    const allowControllerFor = context.options[0] && context.options[0].allowControllerFor;
+    let importedGetName;
+    let importedGetPropertiesName;
+    let currentRouteNode = null;
+
+    return {
+      ImportDeclaration(node) {
+        if (node.source.value === '@ember/object') {
+          importedGetName = importedGetName || getImportIdentifier(node, '@ember/object', 'get');
+          importedGetPropertiesName =
+            importedGetPropertiesName ||
+            getImportIdentifier(node, '@ember/object', 'getProperties');
+        }
+      },
+
+      ClassDeclaration(node) {
+        if (ember.isEmberRoute(context, node)) {
+          currentRouteNode = node;
+        }
+      },
+
+      'ClassDeclaration:exit'(node) {
+        if (currentRouteNode === node) {
+          currentRouteNode = null;
+        }
+      },
+
+      MemberExpression(node) {
+        if (!currentRouteNode) {
+          return;
+        }
+
+        if (
+          types.isThisExpression(node.object) &&
+          types.isIdentifier(node.property) &&
+          node.property.name === 'controller'
+        ) {
+          // Example: this.controller;
+          context.report(node, ERROR_MESSAGE);
+        }
+      },
+
+      // eslint-disable-next-line complexity
+      CallExpression(node) {
+        if (ember.isEmberRoute(context, node)) {
+          currentRouteNode = node;
+        }
+
+        if (!currentRouteNode) {
+          return;
+        }
+
+        if (
+          types.isMemberExpression(node.callee) &&
+          types.isThisExpression(node.callee.object) &&
+          types.isIdentifier(node.callee.property) &&
+          !allowControllerFor &&
+          node.callee.property.name === 'controllerFor'
+        ) {
+          // Example this.controllerFor(...);
+          context.report(node, ERROR_MESSAGE);
+        }
+
+        if (
+          types.isMemberExpression(node.callee) &&
+          types.isThisExpression(node.callee.object) &&
+          types.isIdentifier(node.callee.property) &&
+          node.callee.property.name === 'get' &&
+          node.arguments.length === 1 &&
+          types.isStringLiteral(node.arguments[0]) &&
+          node.arguments[0].value === 'controller'
+        ) {
+          // Example: this.get('controller');
+          context.report(node, ERROR_MESSAGE);
+        }
+
+        if (
+          types.isIdentifier(node.callee) &&
+          node.callee.name === importedGetName &&
+          node.arguments.length === 2 &&
+          types.isThisExpression(node.arguments[0]) &&
+          types.isStringLiteral(node.arguments[1]) &&
+          node.arguments[1].value === 'controller'
+        ) {
+          // Example: get(this, 'controller');
+          context.report(node, ERROR_MESSAGE);
+        }
+
+        if (
+          types.isMemberExpression(node.callee) &&
+          types.isThisExpression(node.callee.object) &&
+          types.isIdentifier(node.callee.property) &&
+          node.callee.property.name === 'getProperties' &&
+          getPropertiesArgumentsIncludeController(node.arguments)
+        ) {
+          // Example: this.getProperties('controller');
+          context.report(node, ERROR_MESSAGE);
+        }
+
+        if (
+          types.isIdentifier(node.callee) &&
+          node.callee.name === importedGetPropertiesName &&
+          types.isThisExpression(node.arguments[0]) &&
+          getPropertiesArgumentsIncludeController(node.arguments.slice(1))
+        ) {
+          // Example: getProperties(this, 'controller');
+          context.report(node, ERROR_MESSAGE);
+        }
+      },
+
+      'CallExpression:exit'(node) {
+        if (currentRouteNode === node) {
+          currentRouteNode = null;
+        }
+      },
+    };
+  },
+};
+
+function getPropertiesArgumentsIncludeController(args) {
+  if (args.length === 1 && types.isArrayExpression(args[0])) {
+    return getPropertiesArgumentsIncludeController(args[0].elements);
+  }
+  return args.find(
+    (argument) => types.isStringLiteral(argument) && argument.value === 'controller'
+  );
+}


Test patch:
diff --git a/tests/lib/rules/no-controller-access-in-routes.js b/tests/lib/rules/no-controller-access-in-routes.js
new file mode 100644
index 00000000..a0a92321
--- /dev/null
+++ b/tests/lib/rules/no-controller-access-in-routes.js
@@ -0,0 +1,281 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/no-controller-access-in-routes');
+const RuleTester = require('eslint').RuleTester;
+
+const { ERROR_MESSAGE } = rule;
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester({
+  parser: require.resolve('babel-eslint'),
+  parserOptions: {
+    ecmaVersion: 6,
+    sourceType: 'module',
+  },
+});
+
+ruleTester.run('no-controller-access-in-routes', rule, {
+  valid: [
+    `
+      import Route from '@ember/routing/route';
+      export default class MyRoute extends Route {
+        setupController(controller, ...args) {
+          super.setupController(controller, ...args);
+          const foo = controller.foo;
+        }
+      }
+    `,
+    `
+      import Route from '@ember/routing/route';
+      export default Route.extend({
+        setupController(controller, ...args) {
+          this._super(controller, ...args);
+          const foo = controller.foo;
+        }
+      });
+    `,
+    `
+      import Route from '@ember/routing/route';
+      export default class MyRoute extends Route {
+        resetController(controller, ...args) {
+          super.resetController(controller, ...args);
+          const foo = controller.foo;
+        }
+      }
+    `,
+    `
+      import Route from '@ember/routing/route';
+      export default Route.extend({
+        resetController(controller, ...args) {
+          this._super(controller, ...args);
+          const foo = controller.foo;
+        }
+      });
+    `,
+
+    `
+      import Component from '@ember/component';
+      import { action, get } from '@ember/object';
+      export default class MyComponent extends Component {
+        @action
+        myAction() {
+          const controller = this.controller;
+        }
+      }
+    `,
+    `
+      import Route from '@ember/routing/route';
+      export default class MyRoute extends Route {}
+      this.controller;
+      this.controllerFor('my');
+    `,
+    `
+      import Route from '@ember/routing/route';
+      export default Route.extend({});
+      this.controller;
+      this.controllerFor('my');
+    `,
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = this.controllerFor('my');
+          }
+        }
+      `,
+      options: [{ allowControllerFor: true }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        export default Route.extend({
+          actions: {
+            myAction() {
+              const controller = this.controllerFor('my');
+            },
+          },
+        });
+      `,
+      options: [{ allowControllerFor: true }],
+    },
+  ],
+  invalid: [
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = this.controller;
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        export default Route.extend({
+          actions: {
+            myAction() {
+              const controller = this.controller;
+            },
+          },
+        });
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'MemberExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = this.controllerFor('my');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = this.controllerFor('my');
+          }
+        }
+      `,
+      options: [{ allowControllerFor: false }],
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = this.get('controller');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action, get } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = get(this, 'controller');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action, get as g } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const controller = g(this, 'controller');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const { foo, controller } = this.getProperties('foo', 'controller');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const { foo, controller } = this.getProperties(['foo', 'controller']);
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action, getProperties } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const { foo, controller } = getProperties(this, 'foo', 'controller');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action, getProperties } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const { foo, controller } = getProperties(this, ['foo', 'controller']);
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+    {
+      code: `
+        import Route from '@ember/routing/route';
+        import { action, getProperties as gp } from '@ember/object';
+        export default class MyRoute extends Route {
+          @action
+          myAction() {
+            const { controller } = gp(this, 'controller');
+          }
+        }
+      `,
+      output: null,
+      errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
+    },
+  ],
+});