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

Issue:
Make `require-super-in-init` more generic
As originally suggested in #82 IMHO it does make sense to require calling `this._super()` in all life-cycle hooks. If you don't call it that should be a deliberate decision and can be expressed by disabling the rule for that specific hook using an ESLint comment.

Pull request:
Add `checkInitOnly` (default true) and `checkNativeClasses` (default false) options to `require-super-in-init` rule
In the next major release, we can rename this rule to `require-super-in-lifecycle-hooks`, and change the defaults for these options.

Fixes #228.

Patch:
diff --git a/README.md b/README.md
index 6e1ee140..0f952f82 100644
--- a/README.md
+++ b/README.md
@@ -124,7 +124,7 @@ Rules are grouped by category to help you understand their purpose. Each rule ha
 | :white_check_mark::wrench: | [no-get-with-default](./docs/rules/no-get-with-default.md) | disallow usage of the Ember's `getWithDefault` function |
 | :white_check_mark::wrench: | [no-get](./docs/rules/no-get.md) | require using ES5 getters instead of Ember's `get` / `getProperties` functions |
 |  | [no-proxies](./docs/rules/no-proxies.md) | disallow using array or object proxies |
-| :white_check_mark::wrench: | [require-super-in-init](./docs/rules/require-super-in-init.md) | require `this._super` to be called in `init` hooks |
+| :white_check_mark::wrench: | [require-super-in-init](./docs/rules/require-super-in-init.md) | require super to be called in lifecycle hooks |
 | :wrench: | [use-ember-get-and-set](./docs/rules/use-ember-get-and-set.md) | enforce usage of `Ember.get` and `Ember.set` |
 
 ### Ember Octane
diff --git a/docs/rules/require-super-in-init.md b/docs/rules/require-super-in-init.md
index f600122c..20bc704e 100644
--- a/docs/rules/require-super-in-init.md
+++ b/docs/rules/require-super-in-init.md
@@ -4,9 +4,11 @@
 
 :wrench: The `--fix` option on the [command line](https://eslint.org/docs/user-guide/command-line-interface#fixing-problems) can automatically fix some of the problems reported by this rule.
 
-Call `_super` in init lifecycle hooks.
+Call super in lifecycle hooks.
 
-When overriding the `init` lifecycle hook inside Ember Components, Controllers, Routes or Mixins, it is necessary to include a call to `_super`.
+When overriding lifecycle hooks inside Ember Components, Controllers, Routes, Mixins, or Services, it is necessary to include a call to super.
+
+By default, this rule only applies to the `init` lifecycle hook and classic classes (see rule options to configure this).
 
 ## Examples
 
@@ -22,6 +24,28 @@ export default Component.extend({
 });
 ```
 
+```javascript
+// With: checkInitOnly = false
+import Component from '@ember/component';
+
+export default Component.extend({
+  didInsertElement() {
+    // ...
+  }
+});
+```
+
+```javascript
+// With: checkNativeClasses = true
+import Component from '@ember/component';
+
+class Foo extends Component {
+  init() {
+    // ...
+  }
+}
+```
+
 Examples of **correct** code for this rule:
 
 ```javascript
@@ -34,3 +58,43 @@ export default Component.extend({
   }
 });
 ```
+
+```javascript
+import Component from '@ember/component';
+
+export default Component.extend({
+  didInsertElement(...args) {
+    this._super(...args);
+    // ...
+  }
+});
+```
+
+```javascript
+import Component from '@ember/component';
+
+class Foo extends Component {
+  init(...args) {
+    super.init(...args);
+    // ...
+  }
+}
+```
+
+```javascript
+import Component from '@ember/component';
+
+class Foo extends Component {
+  didInsertElement(...args) {
+    super.didInsertElement(...args);
+    // ...
+  }
+}
+```
+
+## Configuration
+
+This rule takes an optional object containing:
+
+* `boolean` -- `checkInitOnly` -- whether the rule should only check the `init` lifecycle hook and not other lifecycle hooks (default `true`, TODO: change default to `false` and rename rule to `require-super-in-lifecycle-hooks` in next major release)
+* `boolean` -- `checkNativeClasses` -- whether the rule should check lifecycle hooks in native classes (in addition to classic classes) (default `false`, TODO: change default to `true` in next major release)
diff --git a/lib/rules/require-super-in-init.js b/lib/rules/require-super-in-init.js
index 96140d52..75601a06 100644
--- a/lib/rules/require-super-in-init.js
+++ b/lib/rules/require-super-in-init.js
@@ -1,140 +1,234 @@
 'use strict';
 
 const ember = require('../utils/ember');
-const utils = require('../utils/utils');
 const types = require('../utils/types');
+const Traverser = require('../utils/traverser');
+
+function hasMatchingNode(node, matcher) {
+  let foundMatch = false;
+  new Traverser().traverse(node, {
+    enter(child) {
+      if (!foundMatch && matcher(child)) {
+        foundMatch = true;
+      }
+    },
+  });
+  return foundMatch;
+}
 
 /**
- * Locates nodes with either an ExpressionStatement or ReturnStatement
- * given name.
- * @param {Node[]} nodeBody Array of nodes.
- * @returns {Node[]} Array of nodes with given names.
+ * Checks for this._super() call.
+ * @param {node} node
+ * @returns {Boolean}
  */
-function findStmtNodes(nodeBody) {
-  const nodes = [];
-  const fnExpressions = utils.findNodes(nodeBody, 'ExpressionStatement');
-  const returnStatement = utils.findNodes(nodeBody, 'ReturnStatement');
-
-  if (fnExpressions.length !== 0) {
-    fnExpressions.forEach((item) => {
-      nodes.push(item);
-    });
-  }
-
-  if (returnStatement.length !== 0) {
-    returnStatement.forEach((item) => {
-      nodes.push(item);
-    });
-  }
-
-  return nodes;
+function isClassicSuper(node) {
+  return (
+    types.isCallExpression(node) &&
+    types.isMemberExpression(node.callee) &&
+    types.isThisExpression(node.callee.object) &&
+    types.isIdentifier(node.callee.property) &&
+    node.callee.property.name === '_super'
+  );
 }
 
 /**
- * Checks whether a node has the '_super' property.
- * @param {Node[]} nodes An array of nodes.
+ * Checks for a call like super.init() or super.didInsertElement().
+ * @param {node} node
+ * @param {string} hook - name of hook
  * @returns {Boolean}
  */
-function checkForSuper(nodes) {
-  if (nodes.length === 0) {
-    return false;
-  }
-
-  return nodes.some((n) => {
-    if (types.isCallExpression(n.expression)) {
-      const fnCallee = n.expression.callee;
-      return (
-        types.isMemberExpression(fnCallee) &&
-        types.isThisExpression(fnCallee.object) &&
-        types.isIdentifier(fnCallee.property) &&
-        fnCallee.property.name === '_super'
-      );
-    } else if (types.isReturnStatement(n)) {
-      if (!n.argument || !types.isCallExpression(n.argument)) {
-        return false;
-      }
-
-      const fnCallee = n.argument.callee;
-      return fnCallee.property.name === '_super';
-    }
-
-    return false;
-  });
+function isNativeSuper(node, hook) {
+  return (
+    types.isCallExpression(node) &&
+    types.isMemberExpression(node.callee) &&
+    node.callee.object.type === 'Super' &&
+    types.isIdentifier(node.callee.property) &&
+    node.callee.property.name === hook
+  );
 }
 
 //----------------------------------------------
-// General rule - Call _super in init lifecycle hooks
+// General rule - Call super in lifecycle hooks
 //----------------------------------------------
 
+const ERROR_MESSAGE = 'Call super in lifecycle hooks';
+
 module.exports = {
+  ERROR_MESSAGE,
+
   meta: {
     type: 'problem',
     docs: {
-      description: 'require `this._super` to be called in `init` hooks',
+      description: 'require super to be called in lifecycle hooks',
       category: 'Ember Object',
       recommended: true,
       url:
         'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/require-super-in-init.md',
     },
     fixable: 'code',
-    schema: [],
+    schema: [
+      {
+        type: 'object',
+        properties: {
+          checkInitOnly: {
+            type: 'boolean',
+            default: true,
+          },
+          checkNativeClasses: {
+            type: 'boolean',
+            default: false,
+          },
+        },
+        additionalProperties: false,
+      },
+    ],
   },
 
   create(context) {
-    const message = 'Call this._super(...arguments) in init hook';
+    const checkInitOnly = !context.options[0] || context.options[0].checkInitOnly;
+    const checkNativeClasses = context.options[0] && context.options[0].checkNativeClasses;
 
-    const report = function (node) {
+    function report(node, isNativeClass, lifecycleHookName) {
       context.report({
         node,
-        message,
+        message: ERROR_MESSAGE,
         fix(fixer) {
+          const replacement = isNativeClass
+            ? `super.${lifecycleHookName}(...arguments);`
+            : 'this._super(...arguments);';
           if (node.value.body.body.length > 0) {
             // Function has at least one statement in it so just insert before that.
-            return fixer.insertTextBefore(node.value.body.body[0], 'this._super(...arguments);\n');
+            return fixer.insertTextBefore(node.value.body.body[0], `${replacement}\n`);
           } else {
             // Function is empty so insert after curly brace.
             const sourceCode = context.getSourceCode();
             const startOfBlockStatement = sourceCode.getFirstToken(node.value.body);
-            return fixer.insertTextAfter(startOfBlockStatement, 'this._super(...arguments);');
+            return fixer.insertTextAfter(startOfBlockStatement, replacement);
           }
         },
       });
-    };
+    }
+
+    function isLifecycleHook(node) {
+      return (
+        types.isIdentifier(node.key) &&
+        types.isFunctionExpression(node.value) &&
+        ((checkInitOnly && node.key.name === 'init') ||
+          (!checkInitOnly && (node.key.name === 'init' || ember.isComponentLifecycleHook(node))))
+      );
+    }
+
+    function checkAndReport(
+      node,
+      isInEmberComponent,
+      isInEmberController,
+      isInEmberRoute,
+      isInEmberMixin,
+      isInEmberService,
+      isNativeClass
+    ) {
+      const hookName = node.key.name;
+
+      if (
+        hookName === 'init' &&
+        !isInEmberComponent &&
+        !isInEmberController &&
+        !isInEmberRoute &&
+        !isInEmberMixin &&
+        !isInEmberService
+      ) {
+        // Checking `init` hook but not inside any Ember class.
+        return;
+      } else if (hookName !== 'init' && !isInEmberComponent && !isInEmberMixin) {
+        // Checking a component lifecycle hook but not inside a component/mixin which could have them.
+        return;
+      }
+
+      const body = isNativeSuper ? node.value.body : node.body;
+      const hasSuper = hasMatchingNode(body, (bodyChild) =>
+        isNativeClass ? isNativeSuper(bodyChild, hookName) : isClassicSuper(bodyChild)
+      );
+      if (!hasSuper) {
+        report(node, isNativeClass, hookName);
+      }
+    }
+
+    let currentEmberComponent = null;
+    let currentEmberController = null;
+    let currentEmberRoute = null;
+    let currentEmberMixin = null;
+    let currentEmberService = null;
 
     return {
-      Property(node) {
-        if (
-          !types.isIdentifier(node.key) ||
-          node.key.name !== 'init' ||
-          !types.isFunctionExpression(node.value)
-        ) {
-          // Not init function.
+      ClassDeclaration(node) {
+        if (ember.isEmberComponent(context, node)) {
+          currentEmberComponent = node;
+        } else if (ember.isEmberController(context, node)) {
+          currentEmberController = node;
+        } else if (ember.isEmberRoute(context, node)) {
+          currentEmberRoute = node;
+        } else if (ember.isEmberMixin(context, node)) {
+          currentEmberMixin = node;
+        } else if (ember.isEmberService(context, node)) {
+          currentEmberService = node;
+        }
+      },
+
+      'ClassDeclaration:exit'(node) {
+        if (currentEmberComponent === node) {
+          currentEmberComponent = null;
+        } else if (currentEmberController === node) {
+          currentEmberController = null;
+        } else if (currentEmberRoute === node) {
+          currentEmberRoute = null;
+        } else if (currentEmberMixin === node) {
+          currentEmberMixin = null;
+        } else if (currentEmberService === node) {
+          currentEmberService = null;
+        }
+      },
+
+      MethodDefinition(node) {
+        if (!checkNativeClasses) {
+          // Option off.
+          return;
+        }
+
+        if (!isLifecycleHook(node)) {
           return;
         }
 
+        checkAndReport(
+          node,
+          currentEmberComponent,
+          currentEmberController,
+          currentEmberRoute,
+          currentEmberMixin,
+          currentEmberService,
+          true
+        );
+      },
+
+      Property(node) {
         const parentParent = node.parent.parent;
-        if (!types.isCallExpression(node.parent.parent)) {
+        if (!types.isCallExpression(parentParent)) {
           // Not inside potential Ember class.
           return;
         }
 
-        if (
-          !ember.isEmberComponent(context, parentParent) &&
-          !ember.isEmberController(context, parentParent) &&
-          !ember.isEmberRoute(context, parentParent) &&
-          !ember.isEmberMixin(context, parentParent) &&
-          !ember.isEmberService(context, parentParent)
-        ) {
-          // Not inside an Ember class.
+        if (!isLifecycleHook(node)) {
           return;
         }
 
-        const initPropertyBody = node.value.body.body;
-        const nodes = findStmtNodes(initPropertyBody);
-        const hasSuper = checkForSuper(nodes);
-        if (!hasSuper) {
-          report(node);
-        }
+        checkAndReport(
+          node,
+          ember.isEmberComponent(context, parentParent),
+          ember.isEmberController(context, parentParent),
+          ember.isEmberRoute(context, parentParent),
+          ember.isEmberMixin(context, parentParent),
+          ember.isEmberService(context, parentParent),
+          false
+        );
       },
     };
   },


Test patch:
diff --git a/docs/rules/no-test-module-for.md b/docs/rules/no-test-module-for.md
index 3aa2e85f..922710fb 100644
--- a/docs/rules/no-test-module-for.md
+++ b/docs/rules/no-test-module-for.md
@@ -11,16 +11,12 @@ Use `module` instead of `moduleFor`.
 Examples of **incorrect** code for this rule:
 
 ```js
-import { moduleFor } from 'ember-qunit';
-
 moduleFor('Test Name');
 ```
 
 Examples of **correct** code for this rule:
 
 ```js
-import { module } from 'qunit';
-
 module('Test Name', function (hooks) {
   // ...
 });
diff --git a/tests/lib/rules/require-super-in-init.js b/tests/lib/rules/require-super-in-init.js
index 483893a9..2d2b11c0 100644
--- a/tests/lib/rules/require-super-in-init.js
+++ b/tests/lib/rules/require-super-in-init.js
@@ -5,16 +5,17 @@
 const rule = require('../../../lib/rules/require-super-in-init');
 const RuleTester = require('eslint').RuleTester;
 
+const { ERROR_MESSAGE: message } = rule;
+
 // ------------------------------------------------------------------------------
 // Tests
 // ------------------------------------------------------------------------------
 
 const eslintTester = new RuleTester({
   parserOptions: { ecmaVersion: 6, sourceType: 'module' },
+  parser: require.resolve('babel-eslint'),
 });
 
-const message = 'Call this._super(...arguments) in init hook';
-
 eslintTester.run('require-super-in-init', rule, {
   valid: [
     `export default Component.extend({
@@ -22,6 +23,19 @@ eslintTester.run('require-super-in-init', rule, {
           return this._super(...arguments);
         }
       });`,
+    `export default Component.extend({
+        didInsertElement() {
+          return this._super(...arguments);
+        }
+      });`,
+    {
+      code: `export default Component.extend({
+        didInsertElement() {
+          return this._super(...arguments);
+        }
+      });`,
+      options: [{ checkInitOnly: true }],
+    },
     `export default Route.extend({
         init() {
           return this._super(...arguments);
@@ -129,6 +143,93 @@ eslintTester.run('require-super-in-init', rule, {
     `export default Service({
         init
       });`,
+
+    // Non-init hooks should not be checked when checkInitOnly = true (default)
+    'export default Component({ didInsertElement() {} });',
+    {
+      code: 'export default Component({ didInsertElement() {} });',
+      options: [{ checkInitOnly: true }],
+    },
+    {
+      code:
+        "import Component from '@ember/component'; class Foo extends Component { didInsertElement() {} }",
+      options: [{ checkNativeClasses: true }],
+    },
+    {
+      code:
+        "import Component from '@ember/component'; class Foo extends Component { didInsertElement() {} }",
+      options: [{ checkNativeClasses: true, checkInitOnly: true }],
+    },
+
+    // Native classes should not be checked when checkNativeClasses = false (default)
+    `import Component from '@ember/component';
+     class Foo extends Component { init() { } }`,
+    {
+      code: `import Component from '@ember/component';
+     class Foo extends Component { init() { } }`,
+      options: [{ checkNativeClasses: false }],
+    },
+
+    // checkNativeClasses = true
+    {
+      code: `import Service from '@ember/service';
+     class Foo extends Service { init() { super.init(...arguments); }}`,
+      options: [{ checkNativeClasses: true }],
+    },
+    {
+      code: `import Component from '@ember/component';
+     class Foo extends Component { didInsertElement() { super.didInsertElement(...arguments); }}`,
+      options: [{ checkNativeClasses: true, checkInitOnly: false }],
+    },
+    {
+      code: `import Service from '@ember/service';
+     class Foo extends Service { didInsertElement() { }}`,
+      options: [{ checkNativeClasses: true, checkInitOnly: false }],
+    },
+
+    // checkInitOnly = false
+    {
+      code: 'export default Component({ didInsertElement() { this._super();} });',
+      options: [{ checkInitOnly: false }],
+    },
+
+    // Not inside Ember class:
+    {
+      code: 'export default Foo({ init() { } });',
+      options: [{ checkInitOnly: false }],
+    },
+    {
+      code: 'class Foo { init() { } }',
+      options: [{ checkNativeClasses: true }],
+    },
+    'export default Foo({ didInsertElement() { } });',
+    {
+      code: 'class Foo { didInsertElement() { } }',
+      options: [{ checkNativeClasses: true, checkInitOnly: false }],
+    },
+    'const foo = { init() { } }',
+
+    // init hook should be checked in all Ember classes:
+    {
+      code:
+        "import Controller from '@ember/controller'; class Foo extends Controller { init() { super.init(...arguments); }}",
+      options: [{ checkNativeClasses: true }],
+    },
+    {
+      code:
+        "import Route from '@ember/routing/route'; class Foo extends Route { init() { super.init(...arguments); }}",
+      options: [{ checkNativeClasses: true }],
+    },
+    {
+      code:
+        "import Service from '@ember/service'; class Foo extends Service { init() { super.init(...arguments); }}",
+      options: [{ checkNativeClasses: true }],
+    },
+    {
+      code:
+        "import Mixin from '@ember/object/mixin'; class Foo extends Mixin { init() { super.init(...arguments); }}",
+      options: [{ checkNativeClasses: true }],
+    },
   ],
   invalid: [
     {
@@ -140,6 +241,26 @@ eslintTester.run('require-super-in-init', rule, {
       });`,
       errors: [{ message, line: 2 }],
     },
+    {
+      code: `export default Component.extend({
+        didInsertElement() {},
+      });`,
+      output: `export default Component.extend({
+        didInsertElement() {this._super(...arguments);},
+      });`,
+      options: [{ checkInitOnly: false }],
+      errors: [{ message, line: 2 }],
+    },
+    {
+      code: `export default Component.extend({
+        init() {},
+      });`,
+      output: `export default Component.extend({
+        init() {this._super(...arguments);},
+      });`,
+      options: [{ checkInitOnly: false }],
+      errors: [{ message, line: 2 }],
+    },
     {
       code: `export default Component.extend({
         init() {
@@ -624,5 +745,43 @@ someRandomIdentifier;
       });`,
       errors: [{ message, line: 2 }],
     },
+
+    // Native classes:
+    {
+      code: `import Service from '@ember/service';
+      class Foo extends Service {
+        init() {}
+      }`,
+      output: `import Service from '@ember/service';
+      class Foo extends Service {
+        init() {super.init(...arguments);}
+      }`,
+      options: [{ checkNativeClasses: true }],
+      errors: [{ message, line: 3 }],
+    },
+    {
+      code: `import Component from '@ember/component';
+      class Foo extends Component {
+        didInsertElement() {}
+      }`,
+      output: `import Component from '@ember/component';
+      class Foo extends Component {
+        didInsertElement() {super.didInsertElement(...arguments);}
+      }`,
+      options: [{ checkNativeClasses: true, checkInitOnly: false }],
+      errors: [{ message, line: 3 }],
+    },
+    {
+      code: `import Mixin from '@ember/object/mixin';
+      class Foo extends Mixin {
+        didInsertElement() {}
+      }`,
+      output: `import Mixin from '@ember/object/mixin';
+      class Foo extends Mixin {
+        didInsertElement() {super.didInsertElement(...arguments);}
+      }`,
+      options: [{ checkNativeClasses: true, checkInitOnly: false }],
+      errors: [{ message, line: 3 }],
+    },
   ],
 });