Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 4 additions & 5 deletions lib/rules/jsx-max-depth.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')) {
Expand All @@ -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));

Expand Down Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions lib/rules/jsx-no-leaked-render.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions lib/rules/jsx-sort-default-props.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 2 additions & 4 deletions lib/rules/no-danger-with-children.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
}
Expand Down
3 changes: 1 addition & 2 deletions lib/rules/react-in-jsx-scope.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', {
Expand Down
3 changes: 1 addition & 2 deletions lib/rules/sort-default-props.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
3 changes: 1 addition & 2 deletions lib/rules/style-prop-object.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
9 changes: 1 addition & 8 deletions lib/util/Components.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
3 changes: 1 addition & 2 deletions lib/util/isDestructuredFromPragmaImport.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
3 changes: 1 addition & 2 deletions lib/util/propTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
40 changes: 21 additions & 19 deletions lib/util/variable.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

'use strict';

const toReversed = require('array.prototype.toreversed');
const getScope = require('./eslint').getScope;

/**
Expand All @@ -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;
}

/**
Expand All @@ -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;
Expand Down Expand Up @@ -93,6 +95,6 @@ module.exports = {
findVariable,
findVariableByName,
getVariable,
variablesInScope,
getVariableFromContext,
getLatestVariableDefinition,
};
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down