Repository: ember-cli/eslint-plugin-ember
Pull request #1195 (2021-05-12 18:27:05)

Fix false positives with service/controller/observer detection in some rules
Fixes https://github.com/ember-cli/eslint-plugin-ember/issues/1190

When looking into the issue above, I noticed that `isPropOfType` is checking for MemberExpressions in the property, but it does not care about what the member expression is, as long as the property name is the specified type. This leads to various false positives, including:

- `ember.isInjectedServiceProp` returns true for `xyz: foo.service()` 
- `ember.isInjectedControllerProp` returns true for `application: foo.controller()`
- `ember.isObserverProp` returns true for `xyz: foo.observer()`

It seemed like the reason `isPropOfType` was implemented in this way was to account for usages like `Ember.inject.service`, `Ember.inject.controller`, and `Ember.observer`. To fix this, we now instead pass the name that `Ember` is imported under and have explicit MemberExpression checks for these patterns.


Patch:
diff --git a/lib/utils/ember.js b/lib/utils/ember.js
index 584343c9..896e20d3 100644
--- a/lib/utils/ember.js
+++ b/lib/utils/ember.js
@@ -115,12 +115,7 @@ function isEmberModule(callee, element, module) {
 function isPropOfType(node, type) {
   if (types.isProperty(node) && types.isCallExpression(node.value)) {
     const calleeNode = node.value.callee;
-    return (
-      (types.isIdentifier(calleeNode) && calleeNode.name === type) ||
-      (types.isMemberExpression(calleeNode) &&
-        types.isIdentifier(calleeNode.property) &&
-        calleeNode.property.name === type)
-    );
+    return types.isIdentifier(calleeNode) && calleeNode.name === type;
   } else if ((types.isClassProperty(node) || types.isMethodDefinition(node)) && node.decorators) {
     return node.decorators.some((decorator) => {
       const expression = decorator.expression;
@@ -336,12 +331,50 @@ function isInjectedServiceProp(node, importedEmberName, importedInjectName) {
   );
 }
 
-function isInjectedControllerProp(node) {
-  return isPropOfType(node, 'controller');
+/**
+ * Checks if a node is a controller injection. Looks for:
+ * * controller()
+ * * Ember.inject.controller()
+ * @param {node} node
+ * @param {string} importedEmberName name that `Ember` is imported under
+ * @returns
+ */
+function isInjectedControllerProp(node, importedEmberName) {
+  return (
+    isPropOfType(node, 'controller') ||
+    (types.isProperty(node) &&
+      types.isCallExpression(node.value) &&
+      types.isMemberExpression(node.value.callee) &&
+      types.isMemberExpression(node.value.callee.object) &&
+      types.isIdentifier(node.value.callee.object.object) &&
+      node.value.callee.object.object.name === importedEmberName &&
+      types.isIdentifier(node.value.callee.object.property) &&
+      node.value.callee.object.property.name === 'inject' &&
+      types.isIdentifier(node.value.callee.property) &&
+      node.value.callee.property.name === 'controller')
+  );
 }
 
-function isObserverProp(node) {
-  return isPropOfType(node, 'observer');
+/**
+ * Checks if a node is an observer prop. Looks for:
+ * * observer()
+ * * Ember.observer()
+ * @param {node} node
+ * @param {string} importedEmberName name that `Ember` is imported under
+ * @returns
+ */
+function isObserverProp(node, importedEmberName) {
+  return (
+    isPropOfType(node, 'observer') ||
+    (types.isProperty(node) &&
+      types.isCallExpression(node.value) &&
+      types.isMemberExpression(node.value.callee) &&
+      types.isIdentifier(node.value.callee.object) &&
+      node.value.callee.object.name === importedEmberName &&
+      types.isIdentifier(node.value.callee.property) &&
+      types.isIdentifier(node.value.callee.property) &&
+      node.value.callee.property.name === 'observer')
+  );
 }
 
 const allowedMemberExpNames = new Set(['volatile', 'readOnly', 'property', 'meta']);
@@ -585,26 +618,26 @@ function getEmberImportAliasName(importDeclaration) {
   return importDeclaration.specifiers[0].local.name;
 }
 
-function isSingleLineFn(property) {
+function isSingleLineFn(property, importedEmberName) {
   return (
     (types.isMethodDefinition(property) && utils.getSize(property) === 1) ||
     (property.value &&
       types.isCallExpression(property.value) &&
       utils.getSize(property.value) === 1 &&
-      !isObserverProp(property) &&
-      (isComputedProp(property.value, 'Ember', 'computed', { includeSuffix: true }) ||
+      !isObserverProp(property, importedEmberName) &&
+      (isComputedProp(property.value, importedEmberName, 'computed', { includeSuffix: true }) ||
         !types.isCallWithFunctionExpression(property.value)))
   );
 }
 
-function isMultiLineFn(property) {
+function isMultiLineFn(property, importedEmberName) {
   return (
     (types.isMethodDefinition(property) && utils.getSize(property) > 1) ||
     (property.value &&
       types.isCallExpression(property.value) &&
       utils.getSize(property.value) > 1 &&
-      !isObserverProp(property) &&
-      (isComputedProp(property.value, 'Ember', 'computed', { includeSuffix: true }) ||
+      !isObserverProp(property, importedEmberName) &&
+      (isComputedProp(property.value, importedEmberName, 'computed', { includeSuffix: true }) ||
         !types.isCallWithFunctionExpression(property.value)))
   );
 }
diff --git a/lib/utils/property-order.js b/lib/utils/property-order.js
index f4b8c216..22e0db2a 100644
--- a/lib/utils/property-order.js
+++ b/lib/utils/property-order.js
@@ -57,7 +57,7 @@ function determinePropertyType(node, parentType, ORDER, importedEmberName, impor
     return 'service';
   }
 
-  if (ember.isInjectedControllerProp(node)) {
+  if (ember.isInjectedControllerProp(node, importedEmberName)) {
     return 'controller';
   }
 
@@ -98,7 +98,7 @@ function determinePropertyType(node, parentType, ORDER, importedEmberName, impor
     }
   }
 
-  if (ember.isObserverProp(node)) {
+  if (ember.isObserverProp(node, importedEmberName)) {
     return 'observer';
   }
 
@@ -106,11 +106,11 @@ function determinePropertyType(node, parentType, ORDER, importedEmberName, impor
     return 'actions';
   }
 
-  if (ember.isSingleLineFn(node)) {
+  if (ember.isSingleLineFn(node, importedEmberName)) {
     return 'single-line-function';
   }
 
-  if (ember.isMultiLineFn(node)) {
+  if (ember.isMultiLineFn(node, importedEmberName)) {
     return 'multi-line-function';
   }
 


Test patch:
diff --git a/tests/lib/utils/ember-test.js b/tests/lib/utils/ember-test.js
index d2b25f72..d0cd6583 100644
--- a/tests/lib/utils/ember-test.js
+++ b/tests/lib/utils/ember-test.js
@@ -852,6 +852,30 @@ describe('isInjectedServiceProp', () => {
       expect(emberUtils.isInjectedServiceProp(node, undefined, importName)).toBeTruthy();
     });
 
+    it("should check that it's not an injected service prop with foo.service", () => {
+      const context = new FauxContext(`
+        import {inject as service} from '@ember/service';
+        export default Controller.extend({
+          currentUser: foo.service()
+        });
+      `);
+      const importName = context.ast.body[0].specifiers[0].local.name;
+      const node = context.ast.body[1].declaration.arguments[0].properties[0];
+      expect(emberUtils.isInjectedServiceProp(node, undefined, importName)).toBeFalsy();
+    });
+
+    it("should check that it's not an injected service prop with foo.service.inject", () => {
+      const context = new FauxContext(`
+        import {inject as service} from '@ember/service';
+        export default Controller.extend({
+          currentUser: foo.service.inject()
+        });
+      `);
+      const importName = context.ast.body[0].specifiers[0].local.name;
+      const node = context.ast.body[1].declaration.arguments[0].properties[0];
+      expect(emberUtils.isInjectedServiceProp(node, undefined, importName)).toBeFalsy();
+    });
+
     it("should check that it's not an injected service prop without the renamed import", () => {
       const context = new FauxContext(`
         export default Controller.extend({
@@ -920,16 +944,6 @@ describe('isInjectedServiceProp', () => {
       expect(emberUtils.isInjectedServiceProp(node, importName, undefined)).toBeFalsy();
     });
 
-    it("should check that it's not an injected service prop with foo.service.inject", () => {
-      const context = new FauxContext(`
-        export default Controller.extend({
-          currentUser: foo.service.inject()
-        });
-      `);
-      const node = context.ast.body[0].declaration.arguments[0].properties[0];
-      expect(emberUtils.isInjectedServiceProp(node)).toBeFalsy();
-    });
-
     it("should check that it's not an injected service prop", () => {
       const context = new FauxContext(`
         export default Controller.extend({
@@ -1035,13 +1049,35 @@ describe('isInjectedControllerProp', () => {
     });
 
     it("should check if it's an injected controller prop with full import", () => {
+      const context = new FauxContext(`
+        import Ember from 'ember';
+        export default Controller.extend({
+          application: Ember.inject.controller(),
+        });
+      `);
+      const importName = context.ast.body[0].specifiers[0].local.name;
+      const node = context.ast.body[1].declaration.arguments[0].properties[0];
+      expect(emberUtils.isInjectedControllerProp(node, importName)).toBeTruthy();
+    });
+
+    it("should check if it's not an injected controller prop without full import", () => {
       const context = new FauxContext(`
         export default Controller.extend({
           application: Ember.inject.controller(),
         });
       `);
       const node = context.ast.body[0].declaration.arguments[0].properties[0];
-      expect(emberUtils.isInjectedControllerProp(node)).toBeTruthy();
+      expect(emberUtils.isInjectedControllerProp(node)).toBeFalsy();
+    });
+
+    it("should check if it's not an injected controller prop with foo.controller", () => {
+      const context = new FauxContext(`
+        export default Controller.extend({
+          application: foo.controller(),
+        });
+      `);
+      const node = context.ast.body[0].declaration.arguments[0].properties[0];
+      expect(emberUtils.isInjectedControllerProp(node)).toBeFalsy();
     });
   });
 
@@ -1194,13 +1230,25 @@ describe('isObserverProp', () => {
     });
 
     it("should check if it's an observer prop with full import", () => {
+      const context = new FauxContext(`
+        import Ember from 'ember';
+        export default Controller.extend({
+          someObserver: Ember.observer(),
+        });
+      `);
+      const importName = context.ast.body[0].specifiers[0].local.name;
+      const node = context.ast.body[1].declaration.arguments[0].properties[0];
+      expect(emberUtils.isObserverProp(node, importName)).toBeTruthy();
+    });
+
+    it("should check that it's not an observer prop without full import", () => {
       const context = new FauxContext(`
         export default Controller.extend({
           someObserver: Ember.observer(),
         });
       `);
       const node = context.ast.body[0].declaration.arguments[0].properties[0];
-      expect(emberUtils.isObserverProp(node)).toBeTruthy();
+      expect(emberUtils.isObserverProp(node)).toBeFalsy();
     });
 
     it("should check if it's an observer prop with multi-line observer", () => {
diff --git a/tests/lib/utils/property-order-test.js b/tests/lib/utils/property-order-test.js
index 0ee0bfd1..f53b6850 100644
--- a/tests/lib/utils/property-order-test.js
+++ b/tests/lib/utils/property-order-test.js
@@ -62,6 +62,20 @@ describe('determinePropertyType', () => {
       ).toStrictEqual('service');
     });
 
+    it('should determine controller-type props with full import', () => {
+      const context = new FauxContext(
+        `import Ember from 'ember';
+        export default Controller.extend({
+          application: Ember.inject.controller(),
+        });`
+      );
+      const importEmberName = context.ast.body[0].specifiers[0].local.name;
+      const node = context.ast.body[1].declaration.arguments[0].properties[0];
+      expect(
+        propertyOrder.determinePropertyType(node, 'controller', [], importEmberName)
+      ).toStrictEqual('controller');
+    });
+
     it('should determine controller-type props', () => {
       const context = new FauxContext(
         `export default Controller.extend({
@@ -136,6 +150,20 @@ describe('determinePropertyType', () => {
       expect(propertyOrder.determinePropertyType(node, 'model')).toStrictEqual('relationship');
     });
 
+    it('should determine observer-type props with full import', () => {
+      const context = new FauxContext(
+        `import Ember from 'ember';
+        export default Controller.extend({
+          someObvs: Ember.observer(),
+        });`
+      );
+      const importEmberName = context.ast.body[0].specifiers[0].local.name;
+      const node = context.ast.body[1].declaration.arguments[0].properties[0];
+      expect(
+        propertyOrder.determinePropertyType(node, 'controller', [], importEmberName)
+      ).toStrictEqual('observer');
+    });
+
     it('should determine observer-type props', () => {
       const context = new FauxContext(
         `export default Controller.extend({