Repository: ember-cli/eslint-plugin-ember
Pull request #1165 (2021-04-21 21:03:28)

Improve robustness of classic class body detection in several rules using `getModuleProperties` util
Several rules use the `getModuleProperties` to get the properties in a classic class body. There were a few problems with this:
* Did not handle when the object was passed in as a variable: `const body = {someProp: []}; export default Component.extend(body);`
* Did not handle when there were multiple objects passed in: `export default Component.extend({ ... }, { ... });`


Patch:
diff --git a/lib/rules/alias-model-in-controller.js b/lib/rules/alias-model-in-controller.js
index 345e7a44..451a05a4 100644
--- a/lib/rules/alias-model-in-controller.js
+++ b/lib/rules/alias-model-in-controller.js
@@ -28,13 +28,16 @@ module.exports = {
       context.report(node, message);
     };
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       CallExpression(node) {
         if (!ember.isEmberController(context, node)) {
           return;
         }
 
-        const properties = ember.getModuleProperties(node);
+        const properties = ember.getModuleProperties(node, scopeManager);
         let aliasPresent = false;
 
         for (const property of properties) {
diff --git a/lib/rules/avoid-leaking-state-in-ember-objects.js b/lib/rules/avoid-leaking-state-in-ember-objects.js
index 39bd7200..80e53c10 100644
--- a/lib/rules/avoid-leaking-state-in-ember-objects.js
+++ b/lib/rules/avoid-leaking-state-in-ember-objects.js
@@ -80,6 +80,9 @@ module.exports = {
       context.report(node, message);
     };
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       CallExpression(node) {
         if (
@@ -92,7 +95,7 @@ module.exports = {
           return;
         }
 
-        const properties = ember.getModuleProperties(node);
+        const properties = ember.getModuleProperties(node, scopeManager);
 
         for (const property of properties.filter(
           (property) => property.key && !ignoredProperties.includes(property.key.name)
diff --git a/lib/rules/avoid-using-needs-in-controllers.js b/lib/rules/avoid-using-needs-in-controllers.js
index 850474bd..a7c07b31 100644
--- a/lib/rules/avoid-using-needs-in-controllers.js
+++ b/lib/rules/avoid-using-needs-in-controllers.js
@@ -27,6 +27,9 @@ module.exports = {
       context.report(node, message);
     };
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       CallExpression(node) {
         const isReopenNode = ember.isReopenObject(node) || ember.isReopenClassObject(node);
@@ -35,7 +38,7 @@ module.exports = {
           return;
         }
 
-        const properties = ember.getModuleProperties(node);
+        const properties = ember.getModuleProperties(node, scopeManager);
 
         for (const property of properties) {
           if (
diff --git a/lib/rules/no-actions-hash.js b/lib/rules/no-actions-hash.js
index c38d6056..564519fe 100644
--- a/lib/rules/no-actions-hash.js
+++ b/lib/rules/no-actions-hash.js
@@ -19,6 +19,9 @@ module.exports = {
   },
 
   create: (context) => {
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     function reportActionsProp(properties) {
       const actionsProp = properties.find((property) => ember.isActionsProp(property));
       if (actionsProp) {
@@ -34,7 +37,7 @@ module.exports = {
       },
       CallExpression(node) {
         if (inClassWhichCanContainActions(context, node)) {
-          reportActionsProp(ember.getModuleProperties(node));
+          reportActionsProp(ember.getModuleProperties(node, scopeManager));
         }
       },
     };
diff --git a/lib/rules/no-empty-attrs.js b/lib/rules/no-empty-attrs.js
index 11b2362e..275f5b1e 100644
--- a/lib/rules/no-empty-attrs.js
+++ b/lib/rules/no-empty-attrs.js
@@ -28,13 +28,16 @@ module.exports = {
       context.report(node, message);
     };
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       CallExpression(node) {
         if (!ember.isDSModel(node, filePath)) {
           return;
         }
 
-        const allProperties = ember.getModuleProperties(node);
+        const allProperties = ember.getModuleProperties(node, scopeManager);
         const isDSAttr = allProperties.filter((property) =>
           ember.isModule(property.value, 'attr', 'DS')
         );
diff --git a/lib/rules/no-get.js b/lib/rules/no-get.js
index 043beaee..4f7dc4e9 100644
--- a/lib/rules/no-get.js
+++ b/lib/rules/no-get.js
@@ -140,6 +140,9 @@ module.exports = {
 
     const filename = context.getFilename();
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     // Skip mirage directory
     if (emberUtils.isMirageConfig(filename)) {
       return {};
@@ -177,7 +180,7 @@ module.exports = {
         if (emberUtils.isEmberProxy(context, node)) {
           currentProxyObject = node; // Keep track of being inside a proxy object.
         }
-        if (emberUtils.isEmberObjectImplementingUnknownProperty(node)) {
+        if (emberUtils.isEmberObjectImplementingUnknownProperty(node, scopeManager)) {
           currentClassWithUnknownPropertyMethod = node; // Keep track of being inside an object implementing `unknownProperty`.
         }
       },
@@ -191,7 +194,7 @@ module.exports = {
         if (emberUtils.isEmberProxy(context, node)) {
           currentProxyObject = node; // Keep track of being inside a proxy object.
         }
-        if (emberUtils.isEmberObjectImplementingUnknownProperty(node)) {
+        if (emberUtils.isEmberObjectImplementingUnknownProperty(node, scopeManager)) {
           currentClassWithUnknownPropertyMethod = node;
         }
         if (currentProxyObject || currentClassWithUnknownPropertyMethod) {
diff --git a/lib/rules/no-on-calls-in-components.js b/lib/rules/no-on-calls-in-components.js
index 8f7712e5..b7bba242 100644
--- a/lib/rules/no-on-calls-in-components.js
+++ b/lib/rules/no-on-calls-in-components.js
@@ -62,13 +62,18 @@ module.exports = {
       context.report(node, message);
     };
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       CallExpression(node) {
         if (!ember.isEmberComponent(context, node)) {
           return;
         }
 
-        const propertiesWithOnCalls = ember.getModuleProperties(node).filter(isOnCall);
+        const propertiesWithOnCalls = ember
+          .getModuleProperties(node, scopeManager)
+          .filter(isOnCall);
 
         if (propertiesWithOnCalls.length > 0) {
           for (const property of propertiesWithOnCalls) {
diff --git a/lib/rules/order-in-components.js b/lib/rules/order-in-components.js
index 34570a53..5d4898bb 100644
--- a/lib/rules/order-in-components.js
+++ b/lib/rules/order-in-components.js
@@ -89,6 +89,9 @@ module.exports = {
     let importedInjectName;
     let importedEmberName;
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       ImportDeclaration(node) {
         if (node.source.value === 'ember') {
@@ -110,7 +113,8 @@ module.exports = {
           'component',
           order,
           importedEmberName,
-          importedInjectName
+          importedInjectName,
+          scopeManager
         );
       },
     };
diff --git a/lib/rules/order-in-controllers.js b/lib/rules/order-in-controllers.js
index 07b42268..9d800daa 100644
--- a/lib/rules/order-in-controllers.js
+++ b/lib/rules/order-in-controllers.js
@@ -61,6 +61,9 @@ module.exports = {
     let importedInjectName;
     let importedEmberName;
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       ImportDeclaration(node) {
         if (node.source.value === 'ember') {
@@ -82,7 +85,8 @@ module.exports = {
           'controller',
           order,
           importedEmberName,
-          importedInjectName
+          importedInjectName,
+          scopeManager
         );
       },
     };
diff --git a/lib/rules/order-in-models.js b/lib/rules/order-in-models.js
index 3b0788a8..64bf4090 100644
--- a/lib/rules/order-in-models.js
+++ b/lib/rules/order-in-models.js
@@ -56,6 +56,9 @@ module.exports = {
     let importedInjectName;
     let importedEmberName;
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       ImportDeclaration(node) {
         if (node.source.value === 'ember') {
@@ -77,7 +80,8 @@ module.exports = {
           'model',
           order,
           importedEmberName,
-          importedInjectName
+          importedInjectName,
+          scopeManager
         );
       },
     };
diff --git a/lib/rules/order-in-routes.js b/lib/rules/order-in-routes.js
index be2c1299..71d7f741 100644
--- a/lib/rules/order-in-routes.js
+++ b/lib/rules/order-in-routes.js
@@ -84,6 +84,9 @@ module.exports = {
     let importedInjectName;
     let importedEmberName;
 
+    const sourceCode = context.getSourceCode();
+    const { scopeManager } = sourceCode;
+
     return {
       ImportDeclaration(node) {
         if (node.source.value === 'ember') {
@@ -104,7 +107,8 @@ module.exports = {
           'route',
           order,
           importedEmberName,
-          importedInjectName
+          importedInjectName,
+          scopeManager
         );
       },
     };
diff --git a/lib/utils/ember.js b/lib/utils/ember.js
index 5e851c44..8f140ac0 100644
--- a/lib/utils/ember.js
+++ b/lib/utils/ember.js
@@ -6,6 +6,8 @@ const importUtils = require('./import');
 const types = require('./types');
 const assert = require('assert');
 const decoratorUtils = require('../utils/decorators');
+const { getNodeOrNodeFromVariable } = require('../utils/utils');
+const { flatMap } = require('../utils/javascript');
 
 module.exports = {
   isDSModel,
@@ -548,9 +550,13 @@ function isControllerDefaultProp(property) {
   );
 }
 
-function getModuleProperties(module) {
-  const firstObjectExpressionNode = utils.findNodes(module.arguments, 'ObjectExpression')[0];
-  return firstObjectExpressionNode ? firstObjectExpressionNode.properties : [];
+function getModuleProperties(moduleNode, scopeManager) {
+  return flatMap(moduleNode.arguments, (arg) => {
+    const resultingNode = getNodeOrNodeFromVariable(arg, scopeManager);
+    return resultingNode && resultingNode.type === 'ObjectExpression'
+      ? resultingNode.properties
+      : [];
+  });
 }
 
 /**
@@ -675,13 +681,13 @@ function unwrapBraceExpressions(dependentKeys) {
   return unwrappedExpressions.reduce((acc, cur) => acc.concat(cur), []);
 }
 
-function isEmberObjectImplementingUnknownProperty(node) {
+function isEmberObjectImplementingUnknownProperty(node, scopeManager) {
   if (types.isCallExpression(node)) {
     if (!isExtendObject(node) && !isReopenObject(node)) {
       return false;
     }
     // Classic class.
-    const properties = getModuleProperties(node);
+    const properties = getModuleProperties(node, scopeManager);
     return properties.some(
       (property) => types.isIdentifier(property.key) && property.key.name === 'unknownProperty'
     );
diff --git a/lib/utils/property-order.js b/lib/utils/property-order.js
index 64e25a19..da633938 100644
--- a/lib/utils/property-order.js
+++ b/lib/utils/property-order.js
@@ -194,7 +194,8 @@ function reportUnorderedProperties(
   parentType,
   ORDER,
   importedEmberName,
-  importedInjectName
+  importedInjectName,
+  scopeManager
 ) {
   let maxOrder = -1;
   const firstPropertyOfType = {};
@@ -202,7 +203,7 @@ function reportUnorderedProperties(
 
   const properties = types.isClassDeclaration(node)
     ? node.body.body
-    : ember.getModuleProperties(node);
+    : ember.getModuleProperties(node, scopeManager);
 
   for (const property of properties) {
     const type = determinePropertyType(


Test patch:
diff --git a/tests/lib/rules/alias-model-in-controller.js b/tests/lib/rules/alias-model-in-controller.js
index c139632b..615ecc2c 100644
--- a/tests/lib/rules/alias-model-in-controller.js
+++ b/tests/lib/rules/alias-model-in-controller.js
@@ -27,6 +27,7 @@ eslintTester.run('alias-model-in-controller', rule, {
     'export default Ember.Controller.extend({nail: computed.reads("model")});',
     'export default Ember.Controller.extend({nail: Ember.computed.reads("model")});',
     'export default Ember.Controller.extend(TestMixin, {nail: Ember.computed.alias("model")});',
+    'const body = {nail: Ember.computed.alias("model")}; export default Ember.Controller.extend(TestMixin, body);', // With object variable.
     'export default Ember.Controller.extend(TestMixin, TestMixin2, {nail: Ember.computed.alias("model")});',
     {
       filename: 'example-app/controllers/path/to/some-feature.js',
diff --git a/tests/lib/rules/avoid-leaking-state-in-ember-objects.js b/tests/lib/rules/avoid-leaking-state-in-ember-objects.js
index 71d7ce2f..e755b7e0 100644
--- a/tests/lib/rules/avoid-leaking-state-in-ember-objects.js
+++ b/tests/lib/rules/avoid-leaking-state-in-ember-objects.js
@@ -83,6 +83,17 @@ eslintTester.run('avoid-leaking-state-in-ember-objects', rule, {
         },
       ],
     },
+    {
+      // With object variable.
+      code: 'const body = {someProp: []}; export default Foo.extend(body);',
+      output: null,
+      errors: [
+        {
+          message:
+            'Only string, number, symbol, boolean, null, undefined, and function are allowed as default properties',
+        },
+      ],
+    },
     {
       code: 'export default Foo.extend({someProp: new Ember.A()});',
       output: null,
diff --git a/tests/lib/rules/avoid-using-needs-in-controllers.js b/tests/lib/rules/avoid-using-needs-in-controllers.js
index b212eeab..038b26e2 100644
--- a/tests/lib/rules/avoid-using-needs-in-controllers.js
+++ b/tests/lib/rules/avoid-using-needs-in-controllers.js
@@ -38,6 +38,17 @@ eslintTester.run('avoid-using-needs-in-controllers', rule, {
         },
       ],
     },
+    {
+      // With object variable.
+      code: 'const body = { needs: [] }; export default Controller.extend(body);',
+      output: null,
+      errors: [
+        {
+          message:
+            '`needs` API has been deprecated, `Ember.inject.controller` should be used instead',
+        },
+      ],
+    },
     {
       code: 'Controller.reopenClass({ needs: [] });',
       output: null,
diff --git a/tests/lib/rules/no-actions-hash.js b/tests/lib/rules/no-actions-hash.js
index 008acaed..550bfc6f 100644
--- a/tests/lib/rules/no-actions-hash.js
+++ b/tests/lib/rules/no-actions-hash.js
@@ -82,6 +82,12 @@ ruleTester.run('no-actions-hash', rule, {
       output: null,
       errors: [{ type: 'Property', message: ERROR_MESSAGE }],
     },
+    {
+      // With object variable.
+      code: 'const body = { actions: { } }; export default Component.extend(body); ',
+      output: null,
+      errors: [{ type: 'Property', message: ERROR_MESSAGE }],
+    },
     {
       code: `
         import Component from '@ember/component';
diff --git a/tests/lib/rules/no-empty-attrs.js b/tests/lib/rules/no-empty-attrs.js
index 19d7e851..c308d28c 100644
--- a/tests/lib/rules/no-empty-attrs.js
+++ b/tests/lib/rules/no-empty-attrs.js
@@ -42,6 +42,12 @@ eslintTester.run('no-empty-attrs', rule, {
       output: null,
       errors: [{ message, line: 2 }],
     },
+    {
+      // With object variable.
+      code: 'const body = {name: attr()}; export default Model.extend(body);',
+      output: null,
+      errors: [{ message, line: 1 }],
+    },
     {
       code: `export default Model.extend({
         name: attr("string"),
diff --git a/tests/lib/rules/no-on-calls-in-components.js b/tests/lib/rules/no-on-calls-in-components.js
index e94dc18a..33ec5ff8 100644
--- a/tests/lib/rules/no-on-calls-in-components.js
+++ b/tests/lib/rules/no-on-calls-in-components.js
@@ -51,6 +51,13 @@ eslintTester.run('no-on-calls-in-components', rule, {
       output: null,
       errors: [{ message, line: 2 }],
     },
+    {
+      // With object variable.
+      code:
+        'const body = { test: on("didInsertElement", function () {}) }; export default Component.extend(body);',
+      output: null,
+      errors: [{ message, line: 1 }],
+    },
     {
       code: `export default Component.extend({
         test: on("init", observer("someProperty", function () {
diff --git a/tests/lib/utils/ember-test.js b/tests/lib/utils/ember-test.js
index efd69121..a389049e 100644
--- a/tests/lib/utils/ember-test.js
+++ b/tests/lib/utils/ember-test.js
@@ -1293,16 +1293,61 @@ describe('isActionsProp', () => {
 });
 
 describe('getModuleProperties', () => {
-  const module = parse(`
+  it("returns module's properties", () => {
+    const code = `
     Ember.Component.extend(SomeMixin, {
       asd: 'qwe',
       actions: {},
       someMethod() {}
+    })`;
+    const parsed = babelEslint.parseForESLint(code);
+    const moduleNode = parsed.ast.body[0].expression;
+    const properties = emberUtils.getModuleProperties(moduleNode, parsed.scopeManager);
+    expect(properties).toHaveLength(3);
+  });
+
+  it("returns module's properties when object is not last argument", () => {
+    const code = `
+    Ember.Component.extend({
+      asd: 'qwe',
+      actions: {},
+      someMethod() {}
+    }, SomeMixin)
+  `;
+    const parsed = babelEslint.parseForESLint(code);
+    const moduleNode = parsed.ast.body[0].expression;
+    const properties = emberUtils.getModuleProperties(moduleNode, parsed.scopeManager);
+    expect(properties).toHaveLength(3);
+  });
+
+  it("returns module's properties when there are multiple objects", () => {
+    const code = `
+    Ember.Component.extend({
+      asd: 'qwe',
+      actions: {},
+      someMethod() {}
+    }, {
+      asd: 'abc'
     })
-  `);
+  `;
+    const parsed = babelEslint.parseForESLint(code);
+    const moduleNode = parsed.ast.body[0].expression;
+    const properties = emberUtils.getModuleProperties(moduleNode, parsed.scopeManager);
+    expect(properties).toHaveLength(4);
+  });
 
-  it("returns module's properties", () => {
-    const properties = emberUtils.getModuleProperties(module);
+  it("returns module's properties when object from a variable", () => {
+    const code = `
+    const body = {
+      asd: 'qwe',
+      actions: {},
+      someMethod() {}
+    };
+    Ember.Component.extend(body)
+  `;
+    const parsed = babelEslint.parseForESLint(code);
+    const moduleNode = parsed.ast.body[1].expression;
+    const properties = emberUtils.getModuleProperties(moduleNode, parsed.scopeManager);
     expect(properties).toHaveLength(3);
   });
 });
@@ -1556,6 +1601,16 @@ describe('isEmberObjectImplementingUnknownProperty', () => {
     expect(emberUtils.isEmberObjectImplementingUnknownProperty(node)).toBeTruthy();
   });
 
+  it('should be true for a classic class EmberObject with `unknownProperty` in an object variable', () => {
+    const parsed = babelEslint.parseForESLint(
+      'const body = { unknownProperty() {} }; EmberObject.extend(body);'
+    );
+    const node = parsed.ast.body[1].expression;
+    expect(
+      emberUtils.isEmberObjectImplementingUnknownProperty(node, parsed.scopeManager)
+    ).toBeTruthy();
+  });
+
   it('should be false for a native class EmberObject without `unknownProperty`', () => {
     const node = babelEslint.parse('class MyClass extends EmberObject { somethingElse() {} }')
       .body[0];