[Refactor] variableUtil: Avoid creating a single flat variable scope for each lookup

DanielRosenwasser · ljharb · commit 4d2fd861d2f9 · 2024-07-15T10:46:00.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,10 @@ This change log adheres to standards from [Keep a CHANGELOG](https://keepachange
 * [`forbid-component-props`]: add `propNamePattern` to allow / disallow prop name patterns ([#3774][] @akulsr0)
 * [`jsx-handler-names`]: support ignoring component names ([#3772][] @akulsr0)
 
+### Changed
+* [Refactor] `variableUtil`: Avoid creating a single flat variable scope for each lookup ([#3782][] @DanielRosenwasser)
+
+[#3782]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3782
 [#3774]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3774
 [#3772]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3772
 [#3759]: https://github.com/jsx-eslint/eslint-plugin-react/pull/3759
diff --git a/lib/rules/jsx-max-depth.js b/lib/rules/jsx-max-depth.js
@@ -88,7 +88,7 @@ module.exports = {
       });
     }
 
-    function findJSXElementOrFragment(variables, name, previousReferences) {
+    function findJSXElementOrFragment(startNode, name, previousReferences) {
       function find(refs, prevRefs) {
         for (let i = refs.length - 1; i >= 0; i--) {
           if (has(refs[i], 'writeExpr')) {
@@ -97,14 +97,14 @@ module.exports = {
             return (jsxUtil.isJSX(writeExpr)
               && writeExpr)
               || ((writeExpr && writeExpr.type === 'Identifier')
-              && findJSXElementOrFragment(variables, writeExpr.name, prevRefs));
+              && findJSXElementOrFragment(startNode, writeExpr.name, prevRefs));
           }
         }
 
         return null;
       }
 
-      const variable = variableUtil.getVariable(variables, name);
+      const variable = variableUtil.getVariableFromContext(context, startNode, name);
       if (variable && variable.references) {
         const containDuplicates = previousReferences.some((ref) => includes(variable.references, ref));
 
@@ -150,8 +150,7 @@ module.exports = {
           return;
         }
 
-        const variables = variableUtil.variablesInScope(context, node);
-        const element = findJSXElementOrFragment(variables, node.expression.name, []);
+        const element = findJSXElementOrFragment(node, node.expression.name, []);
 
         if (element) {
           const baseDepth = getDepth(node);
diff --git a/lib/rules/jsx-no-leaked-render.js b/lib/rules/jsx-no-leaked-render.js
@@ -161,8 +161,7 @@ module.exports = {
           if (isCoerceValidLeftSide || getIsCoerceValidNestedLogicalExpression(leftSide)) {
             return;
           }
-          const variables = variableUtil.variablesInScope(context, node);
-          const leftSideVar = variableUtil.getVariable(variables, leftSide.name);
+          const leftSideVar = variableUtil.getVariableFromContext(context, node, leftSide.name);
           if (leftSideVar) {
             const leftSideValue = leftSideVar.defs
               && leftSideVar.defs.length
diff --git a/lib/rules/jsx-sort-default-props.js b/lib/rules/jsx-sort-default-props.js
@@ -97,8 +97,7 @@ module.exports = {
      */
     function findVariableByName(node, name) {
       const variable = variableUtil
-        .variablesInScope(context, node)
-        .find((item) => item.name === name);
+        .getVariableFromContext(context, node, name);
 
       if (!variable || !variable.defs[0] || !variable.defs[0].node) {
         return null;
diff --git a/lib/rules/no-danger-with-children.js b/lib/rules/no-danger-with-children.js
@@ -32,8 +32,7 @@ module.exports = {
   },
   create(context) {
     function findSpreadVariable(node, name) {
-      return variableUtil.variablesInScope(context, node)
-        .find((item) => item.name === name);
+      return variableUtil.getVariableFromContext(context, node, name);
     }
     /**
      * Takes a ObjectExpression and returns the value of the prop if it has it
@@ -128,8 +127,7 @@ module.exports = {
           let props = node.arguments[1];
 
           if (props.type === 'Identifier') {
-            const variable = variableUtil.variablesInScope(context, node)
-              .find((item) => item.name === props.name);
+            const variable = variableUtil.getVariableFromContext(context, node, props.name);
             if (variable && variable.defs.length && variable.defs[0].node.init) {
               props = variable.defs[0].node.init;
             }
diff --git a/lib/rules/react-in-jsx-scope.js b/lib/rules/react-in-jsx-scope.js
@@ -37,8 +37,7 @@ module.exports = {
     const pragma = pragmaUtil.getFromContext(context);
 
     function checkIfReactIsInScope(node) {
-      const variables = variableUtil.variablesInScope(context, node);
-      if (variableUtil.findVariable(variables, pragma)) {
+      if (variableUtil.getVariableFromContext(context, node, pragma)) {
         return;
       }
       report(context, messages.notInScope, 'notInScope', {
diff --git a/lib/rules/sort-default-props.js b/lib/rules/sort-default-props.js
@@ -91,8 +91,7 @@ module.exports = {
      * @returns {ASTNode|null} Return null if the variable could not be found, ASTNode otherwise.
      */
     function findVariableByName(node, name) {
-      const variable = variableUtil.variablesInScope(context, node)
-        .find((item) => item.name === name);
+      const variable = variableUtil.getVariableFromContext(context, node, name);
 
       if (!variable || !variable.defs[0] || !variable.defs[0].node) {
         return null;
diff --git a/lib/rules/style-prop-object.js b/lib/rules/style-prop-object.js
@@ -61,8 +61,7 @@ module.exports = {
      * @param {object} node A Identifier node
      */
     function checkIdentifiers(node) {
-      const variable = variableUtil.variablesInScope(context, node)
-        .find((item) => item.name === node.name);
+      const variable = variableUtil.getVariableFromContext(context, node, node.name);
 
       if (!variable || !variable.defs[0] || !variable.defs[0].node.init) {
         return;
diff --git a/lib/util/Components.js b/lib/util/Components.js
@@ -669,14 +669,7 @@ function componentRule(rule, context) {
       if (!variableName) {
         return null;
       }
-      let variableInScope;
-      const variables = variableUtil.variablesInScope(context, node);
-      for (i = 0, j = variables.length; i < j; i++) {
-        if (variables[i].name === variableName) {
-          variableInScope = variables[i];
-          break;
-        }
-      }
+      const variableInScope = variableUtil.getVariableFromContext(context, node, variableName);
       if (!variableInScope) {
         return null;
       }
diff --git a/lib/util/isDestructuredFromPragmaImport.js b/lib/util/isDestructuredFromPragmaImport.js
@@ -13,8 +13,7 @@ const variableUtil = require('./variable');
  */
 module.exports = function isDestructuredFromPragmaImport(context, node, variable) {
   const pragma = pragmaUtil.getFromContext(context);
-  const variables = variableUtil.variablesInScope(context, node);
-  const variableInScope = variableUtil.getVariable(variables, variable);
+  const variableInScope = variableUtil.getVariableFromContext(context, node, variable);
   if (variableInScope) {
     const latestDef = variableUtil.getLatestVariableDefinition(variableInScope);
     if (latestDef) {
diff --git a/lib/util/propTypes.js b/lib/util/propTypes.js
@@ -965,8 +965,7 @@ module.exports = function propTypesInstructions(context, components, utils) {
         break;
       }
       case 'Identifier': {
-        const firstMatchingVariable = variableUtil.variablesInScope(context, node)
-          .find((variableInScope) => variableInScope.name === propTypes.name);
+        const firstMatchingVariable = variableUtil.getVariableFromContext(context, node, propTypes.name);
         if (firstMatchingVariable) {
           const defInScope = firstMatchingVariable.defs[firstMatchingVariable.defs.length - 1];
           markPropTypesAsDeclared(node, defInScope.node && defInScope.node.init, rootNode);
diff --git a/lib/util/variable.js b/lib/util/variable.js
@@ -5,7 +5,6 @@
 
 'use strict';
 
-const toReversed = require('array.prototype.toreversed');
 const getScope = require('./eslint').getScope;
 
 /**
@@ -29,30 +28,33 @@ function getVariable(variables, name) {
 }
 
 /**
- * List all variable in a given scope
- *
- * Contain a patch for babel-eslint to avoid https://github.com/babel/babel-eslint/issues/21
+ * Searches for a variable in the given scope.
  *
  * @param {Object} context The current rule context.
  * @param {ASTNode} node The node to start looking from.
- * @returns {Array} The variables list
+ * @param {string} name The name of the variable to search.
+ * @returns {Object | undefined} Variable if the variable was found, undefined if not.
  */
-function variablesInScope(context, node) {
+function getVariableFromContext(context, node, name) {
   let scope = getScope(context, node);
-  let variables = scope.variables;
 
-  while (scope.type !== 'global') {
-    scope = scope.upper;
-    variables = scope.variables.concat(variables);
-  }
-  if (scope.childScopes.length) {
-    variables = scope.childScopes[0].variables.concat(variables);
-    if (scope.childScopes[0].childScopes.length) {
-      variables = scope.childScopes[0].childScopes[0].variables.concat(variables);
+  while (scope) {
+    let variable = getVariable(scope.variables, name);
+
+    if (!variable && scope.childScopes.length) {
+      variable = getVariable(scope.childScopes[0].variables, name);
+
+      if (!variable && scope.childScopes[0].childScopes.length) {
+        variable = getVariable(scope.childScopes[0].childScopes[0].variables, name);
+      }
     }
-  }
 
-  return toReversed(variables);
+    if (variable) {
+      return variable;
+    }
+    scope = scope.upper;
+  }
+  return undefined;
 }
 
 /**
@@ -63,7 +65,7 @@ function variablesInScope(context, node) {
  * @returns {ASTNode|null} Return null if the variable could not be found, ASTNode otherwise.
  */
 function findVariableByName(context, node, name) {
-  const variable = getVariable(variablesInScope(context, node), name);
+  const variable = getVariableFromContext(context, node, name);
 
   if (!variable || !variable.defs[0] || !variable.defs[0].node) {
     return null;
@@ -93,6 +95,6 @@ module.exports = {
   findVariable,
   findVariableByName,
   getVariable,
-  variablesInScope,
+  getVariableFromContext,
   getLatestVariableDefinition,
 };
diff --git a/package.json b/package.json
@@ -28,7 +28,6 @@
     "array-includes": "^3.1.8",
     "array.prototype.findlast": "^1.2.5",
     "array.prototype.flatmap": "^1.3.2",
-    "array.prototype.toreversed": "^1.1.2",
     "array.prototype.tosorted": "^1.1.4",
     "doctrine": "^2.1.0",
     "es-iterator-helpers": "^1.0.19",

Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,7 @@ module.exports = {`
`88`	`88`	`});`
`89`	`89`	`}`
`90`	`90`
`91`		`- function findJSXElementOrFragment(variables, name, previousReferences) {`
	`91`	`+ function findJSXElementOrFragment(startNode, name, previousReferences) {`
`92`	`92`	`function find(refs, prevRefs) {`
`93`	`93`	`for (let i = refs.length - 1; i >= 0; i--) {`
`94`	`94`	`if (has(refs[i], 'writeExpr')) {`
`@@ -97,14 +97,14 @@ module.exports = {`
`97`	`97`	`return (jsxUtil.isJSX(writeExpr)`
`98`	`98`	`&& writeExpr)`
`99`	`99`	`\|\| ((writeExpr && writeExpr.type === 'Identifier')`
`100`		`- && findJSXElementOrFragment(variables, writeExpr.name, prevRefs));`
	`100`	`+ && findJSXElementOrFragment(startNode, writeExpr.name, prevRefs));`
`101`	`101`	`}`
`102`	`102`	`}`
`103`	`103`
`104`	`104`	`return null;`
`105`	`105`	`}`
`106`	`106`
`107`		`- const variable = variableUtil.getVariable(variables, name);`
	`107`	`+ const variable = variableUtil.getVariableFromContext(context, startNode, name);`
`108`	`108`	`if (variable && variable.references) {`
`109`	`109`	`const containDuplicates = previousReferences.some((ref) => includes(variable.references, ref));`
`110`	`110`
`@@ -150,8 +150,7 @@ module.exports = {`
`150`	`150`	`return;`
`151`	`151`	`}`
`152`	`152`
`153`		`- const variables = variableUtil.variablesInScope(context, node);`
`154`		`- const element = findJSXElementOrFragment(variables, node.expression.name, []);`
	`153`	`+ const element = findJSXElementOrFragment(node, node.expression.name, []);`
`155`	`154`
`156`	`155`	`if (element) {`
`157`	`156`	`const baseDepth = getDepth(node);`
Original file line number	Diff line number	Diff line change
`@@ -37,8 +37,7 @@ module.exports = {`
`37`	`37`	`const pragma = pragmaUtil.getFromContext(context);`
`38`	`38`
`39`	`39`	`function checkIfReactIsInScope(node) {`
`40`		`- const variables = variableUtil.variablesInScope(context, node);`
`41`		`- if (variableUtil.findVariable(variables, pragma)) {`
	`40`	`+ if (variableUtil.getVariableFromContext(context, node, pragma)) {`
`42`	`41`	`return;`
`43`	`42`	`}`
`44`	`43`	`report(context, messages.notInScope, 'notInScope', {`