fix: false positive with Set/Map-initialized class properties in no-a…

…rray-prototype-extensions rule (#1543)
ember-cli · Jul 26, 2022 · ae70c0a · ae70c0a
1 parent 95e2571
commit ae70c0a
Show file tree

Hide file tree

Showing 7 changed files with 248 additions and 34 deletions.
diff --git a/lib/rules/no-array-prototype-extensions.js b/lib/rules/no-array-prototype-extensions.js
@@ -1,6 +1,8 @@
 'use strict';
 
 const { getName, getNodeOrNodeFromVariable } = require('../utils/utils');
+const { isClassPropertyOrPropertyDefinition } = require('../utils/types');
+const Stack = require('../utils/stack');
 
 const ERROR_MESSAGE = "Don't use Ember's array prototype extensions";
 
@@ -121,6 +123,9 @@ module.exports = {
     const sourceCode = context.getSourceCode();
     const { scopeManager } = sourceCode;
 
+    // Track some information about the current class we're inside.
+    const classStack = new Stack();
+
     return {
       /**
        * Cover cases when `EXTENSION_METHODS` is getting called.
@@ -162,6 +167,25 @@ module.exports = {
           return;
         }
 
+        if (
+          node.callee.type === 'MemberExpression' &&
+          node.callee.object.type === 'MemberExpression' &&
+          node.callee.object.object.type === 'ThisExpression' &&
+          node.callee.object.property.type === 'Identifier' &&
+          classStack.peek() &&
+          classStack.peek().classPropertiesToIgnore.has(node.callee.object.property.name)
+        ) {
+          // Ignore when we can tell the class property was initialized to an instance of a non-array class.
+          // Example:
+          /*
+          class MyClass {
+            foo = new Set();
+            myFunc() { this.foo.clear() }
+          }
+          */
+          return;
+        }
+
         if (EXTENSION_METHODS.has(node.callee.property.name)) {
           context.report({ node, messageId: 'main' });
         }
@@ -210,6 +234,30 @@ module.exports = {
           context.report({ node, messageId: 'main' });
         }
       },
+
+      ClassDeclaration(node) {
+        // Keep track of class properties to ignore because we know they were initialized to an instance of a non-array class.
+        const classPropertiesToIgnore = new Set(
+          node.body.body
+            .filter(
+              (n) =>
+                isClassPropertyOrPropertyDefinition(n) &&
+                n.key.type === 'Identifier' &&
+                n.value &&
+                n.value.type === 'NewExpression' &&
+                n.value.callee.type === 'Identifier' &&
+                KNOWN_NON_ARRAY_CLASSES.has(n.value.callee.name)
+            )
+            .map((n) => n.key.name)
+        );
+
+        classStack.push({ node, classPropertiesToIgnore });
+      },
+
+      'ClassDeclaration:exit'() {
+        // Leaving the current class.
+        classStack.pop();
+      },
     };
   },
 };
diff --git a/lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js b/lib/rules/no-assignment-of-untracked-properties-used-in-tracking-contexts.js
@@ -10,6 +10,7 @@ const {
   findComputedPropertyDependentKeys,
   keyExistsAsPrefixInList,
 } = require('../utils/computed-property-dependent-keys');
+const Stack = require('../utils/stack');
 
 function createMacrosByPathMap(macroConfigurations) {
   const macrosByPath = new Map();
@@ -65,24 +66,6 @@ function findTrackedProperties(nodeClassDeclaration, trackedImportName) {
   );
 }
 
-class Stack {
-  constructor() {
-    this.stack = new Array();
-  }
-  pop() {
-    return this.stack.pop();
-  }
-  push(item) {
-    this.stack.push(item);
-  }
-  peek() {
-    return this.stack.length > 0 ? this.stack[this.stack.length - 1] : undefined;
-  }
-  size() {
-    return this.stack.length;
-  }
-}
-
 /** @type {import('eslint').Rule.RuleModule} */
 module.exports = {
   meta: {

diff --git a/lib/rules/no-unused-services.js b/lib/rules/no-unused-services.js
@@ -8,21 +8,7 @@ const {
   getMacros,
   MACROS_TO_TRACKED_ARGUMENT_COUNT,
 } = require('../utils/computed-property-macros');
-
-class Stack {
-  constructor() {
-    this.stack = new Array();
-  }
-  pop() {
-    return this.stack.pop();
-  }
-  push(item) {
-    this.stack.push(item);
-  }
-  peek() {
-    return this.stack.length > 0 ? this.stack[this.stack.length - 1] : undefined;
-  }
-}
+const Stack = require('../utils/stack');
 
 /** @type {import('eslint').Rule.RuleModule} */
 module.exports = {

diff --git a/lib/utils/stack.js b/lib/utils/stack.js
@@ -0,0 +1,17 @@
+module.exports = class Stack {
+  constructor() {
+    this.stack = new Array();
+  }
+  pop() {
+    return this.stack.pop();
+  }
+  push(item) {
+    this.stack.push(item);
+  }
+  peek() {
+    return this.stack.length > 0 ? this.stack[this.stack.length - 1] : undefined;
+  }
+  size() {
+    return this.stack.length;
+  }
+};
diff --git a/package.json b/package.json
@@ -75,6 +75,7 @@
     "@babel/eslint-parser": "^7.15.8",
     "@babel/plugin-proposal-class-properties": "^7.13.0",
     "@babel/plugin-proposal-decorators": "^7.15.8",
+    "@typescript-eslint/parser": "^5.31.0",
     "eslint": "^8.20.0",
     "eslint-config-prettier": "^8.5.0",
     "eslint-plugin-eslint-comments": "^3.2.0",
@@ -95,7 +96,8 @@
     "prettier": "^2.1.1",
     "release-it": "^15.1.3",
     "release-it-lerna-changelog": "^5.0.0",
-    "sort-package-json": "^1.52.0"
+    "sort-package-json": "^1.52.0",
+    "typescript": "^4.7.4"
   },
   "peerDependencies": {
     "eslint": ">= 7"

diff --git a/tests/lib/rules/no-array-prototype-extensions.js b/tests/lib/rules/no-array-prototype-extensions.js
@@ -88,6 +88,30 @@ ruleTester.run('no-array-prototype-extensions', rule, {
     'const foo = new TrackedSet(); foo.clear();',
     'const foo = new TrackedWeakMap(); foo.clear();',
     'const foo = new TrackedWeakSet(); foo.clear();',
+
+    // Class property definition with non-array class.
+    `class MyClass {
+      foo = new Set();
+      myFunc() {
+        this.foo.clear();
+      }
+    }`,
+
+    {
+      // Class property definition with non-array class (TypeScript).
+      code: `
+      class MyClass {
+        foo: Set<UploadFile> = new Set();
+        myFunc() {
+          this.foo.clear();
+        }
+      }
+      `,
+      parser: require.resolve('@typescript-eslint/parser'),
+    },
+
+    // TODO: handle non-Identifier property names:
+    'foo["clear"]();',
   ],
   invalid: [
     {
@@ -100,6 +124,11 @@ ruleTester.run('no-array-prototype-extensions', rule, {
       output: null,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
+    {
+      code: 'something.else.filterBy()',
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
     {
       code: 'something?.firstObject?.foo',
       output: null,
@@ -326,5 +355,105 @@ ruleTester.run('no-array-prototype-extensions', rule, {
       output: null,
       errors: [{ messageId: 'main', type: 'CallExpression' }],
     },
+    {
+      // Variable.
+      code: 'const foo = new Array(); foo.clear();',
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Class property with array value.
+      code: `
+      class MyClass {
+        foo = new Array();
+        myFunc() {
+          this.foo.clear();
+        }
+      }`,
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Class property with array value (TypeScript).
+      code: `
+      class MyClass {
+        foo: Array<UploadFile> = new Array();
+        myFunc() {
+          this.foo.clear();
+        }
+      }
+      `,
+      output: null,
+      parser: require.resolve('@typescript-eslint/parser'),
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Class property with no value.
+      code: `
+      class MyClass {
+        foo;
+        myFunc() {
+          this.foo.clear();
+        }
+      }`,
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Class property but not declared anywhere.
+      code: `
+      class MyClass {
+        myFunc() {
+          this.foo.clear();
+        }
+      }`,
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Two classes (should not ignore first class property).
+      code: `
+      class MyClass1 {
+        foo = new Set();
+      }
+      class MyClass2 {
+        myFunc() {
+          this.foo.clear();
+        }
+      }`,
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Nested classes (should not ignore outer class property).
+      code: `
+      class MyClass1 {
+        foo = new Set();
+        myFunc() {
+          class MyClass2 {
+            myFunc() {
+              this.foo.clear();
+            }
+          }
+        }
+      }
+      `,
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
+    {
+      // Nested classes (should remember first class properties).
+      code: `
+      class MyClass1 {
+        foo = new Array();
+        myFunc() {
+          class MyClass2 {}
+          this.foo.clear();
+        }
+      }
+      `,
+      output: null,
+      errors: [{ messageId: 'main', type: 'CallExpression' }],
+    },
   ],
 });