Skip to content

Fix: Prevent false positives in rules-of-hooks for regular classesFix: Prevent false positives in rules-of-hooks for regular classes #35265

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.

Sign up for GitHub

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

Open
hukshh wants to merge 1 commit into
base: main
Choose a base branch
from
+104 −41
Open

Fix: Prevent false positives in rules-of-hooks for regular classesFix: Prevent false positives in rules-of-hooks for regular classes #35265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
74 changes: 54 additions & 20 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,20 @@ const allTests = {
}
`,
},
{
code: normalizeIndent`
// Valid because this is not a React component class.
// Hooks can be called in regular class methods.
class Store {
use() {
return React.useState(4);
}
useHook() {
return useState(0);
}
}
`,
},
{
code: normalizeIndent`
// Valid -- this is a regression test.
Expand Down Expand Up @@ -1537,6 +1551,26 @@ const allTests = {
`,
errors: [classError('React.useState')],
},
{
code: normalizeIndent`
class MyComponent extends React.Component {
componentDidMount() {
useState(0);
}
}
`,
errors: [classError('useState')],
},
{
code: normalizeIndent`
class MyComponent extends Component {
useCustomHook() {
return useHook();
}
}
`,
errors: [classError('useHook')],
},
{
code: normalizeIndent`
(class {useHook = () => { useState(); }});
Expand Down Expand Up @@ -1788,7 +1822,7 @@ const allTests = {
})} />;
}
`,
errors: [{...useEffectEventError(null, false), line: 4}],
errors: [{ ...useEffectEventError(null, false), line: 4 }],
},
{
syntax: 'flow',
Expand All @@ -1801,7 +1835,7 @@ const allTests = {
})} />;
}
`,
errors: [{...useEffectEventError(null, false), line: 5}],
errors: [{ ...useEffectEventError(null, false), line: 5 }],
},
{
code: normalizeIndent`
Expand Down Expand Up @@ -1834,8 +1868,8 @@ const allTests = {
}
`,
errors: [
{...useEffectEventError('onClick', false), line: 7},
{...useEffectEventError('onClick', true), line: 15},
{ ...useEffectEventError('onClick', false), line: 7 },
{ ...useEffectEventError('onClick', true), line: 15 },
],
},
{
Expand Down Expand Up @@ -1871,8 +1905,8 @@ const allTests = {
}
`,
errors: [
{...useEffectEventError('onClick', false), line: 8},
{...useEffectEventError('onClick', true), line: 16},
{ ...useEffectEventError('onClick', false), line: 8 },
{ ...useEffectEventError('onClick', true), line: 16 },
],
},
{
Expand All @@ -1897,7 +1931,7 @@ const allTests = {
return <Bar onClick={foo} />
}
`,
errors: [{...useEffectEventError('onClick', false), line: 7}],
errors: [{ ...useEffectEventError('onClick', false), line: 7 }],
},
{
syntax: 'flow',
Expand All @@ -1912,7 +1946,7 @@ const allTests = {
return <Bar onClick={foo} />
}
`,
errors: [{...useEffectEventError('onClick', false), line: 8}],
errors: [{ ...useEffectEventError('onClick', false), line: 8 }],
},
{
code: normalizeIndent`
Expand Down Expand Up @@ -1972,15 +2006,15 @@ const allTests = {
// Explicitly test error messages here for various cases
errors: [
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'Effects and Effect Events in the same component.',
'Effects and Effect Events in the same component.',
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'Effects and Effect Events in the same component.',
'Effects and Effect Events in the same component.',
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
],
},
{
Expand Down Expand Up @@ -2009,15 +2043,15 @@ const allTests = {
// Explicitly test error messages here for various cases
errors: [
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'Effects and Effect Events in the same component.',
'Effects and Effect Events in the same component.',
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
'Effects and Effect Events in the same component.',
'Effects and Effect Events in the same component.',
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
`\`onClick\` is a function created with React Hook "useEffectEvent", and can only be called from ` +
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
`Effects and Effect Events in the same component. ` +
`It cannot be assigned to a variable or passed down.`,
],
},
],
Expand Down
71 changes: 50 additions & 21 deletions packages/eslint-plugin-react-hooks/src/rules/RulesOfHooks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
/* eslint-disable no-for-of-loops/no-for-of-loops */

import type {Rule, Scope} from 'eslint';
import type { Rule, Scope } from 'eslint';
import type {
CallExpression,
CatchClause,
Expand All @@ -20,7 +20,7 @@ import type {

// @ts-expect-error untyped module
import CodePathAnalyzer from '../code-path-analysis/code-path-analyzer';
import {getAdditionalEffectHooksFromSettings} from '../shared/Utils';
import { getAdditionalEffectHooksFromSettings } from '../shared/Utils';

/**
* Catch all identifiers that begin with "use" followed by an uppercase Latin
Expand Down Expand Up @@ -191,6 +191,34 @@ function isUseIdentifier(node: Node): boolean {
return isReactFunction(node, 'use');
}

function isReactComponentClass(node: Node): boolean {
// Walk up the tree to find the class declaration
let current: Node | undefined = node;
while (current) {
if (current.type === 'ClassDeclaration' || current.type === 'ClassExpression') {
const classNode = current as any;
if (classNode.superClass) {
const superClass = classNode.superClass;
// Check for: extends Component, extends React.Component, extends PureComponent, extends React.PureComponent
if (superClass.type === 'Identifier') {
return superClass.name === 'Component' || superClass.name === 'PureComponent';
} else if (superClass.type === 'MemberExpression') {
return (
superClass.object.type === 'Identifier' &&
superClass.object.name === 'React' &&
superClass.property.type === 'Identifier' &&
(superClass.property.name === 'Component' || superClass.property.name === 'PureComponent')
);
}
}
return false;
}
current = current.parent;
}
return false;
}


const rule = {
meta: {
type: 'problem',
Expand Down Expand Up @@ -255,22 +283,22 @@ const rule = {
const getSourceCode =
typeof context.getSourceCode === 'function'
? () => {
return context.getSourceCode();
}
return context.getSourceCode();
}
: () => {
return context.sourceCode;
};
return context.sourceCode;
};
/**
* SourceCode#getScope that also works down to ESLint 3.0.0
*/
const getScope =
typeof context.getScope === 'function'
? (): Scope.Scope => {
return context.getScope();
}
return context.getScope();
}
: (node: Node): Scope.Scope => {
return getSourceCode().getScope(node);
};
return getSourceCode().getScope(node);
};

function hasFlowSuppression(node: Node, suppression: string) {
const sourceCode = getSourceCode();
Expand Down Expand Up @@ -339,7 +367,7 @@ const rule = {
segment: Rule.CodePathSegment,
pathHistory?: Set<string>,
): bigint {
const {cache} = countPathsFromStart;
const { cache } = countPathsFromStart;
let paths = cache.get(segment.id);
const pathList = new Set<string>(pathHistory);

Expand Down Expand Up @@ -413,7 +441,7 @@ const rule = {
segment: Rule.CodePathSegment,
pathHistory?: Set<string>,
): bigint {
const {cache} = countPathsToEnd;
const { cache } = countPathsToEnd;
let paths = cache.get(segment.id);
const pathList = new Set(pathHistory);

Expand Down Expand Up @@ -479,7 +507,7 @@ const rule = {
function shortestPathLengthToStart(
segment: Rule.CodePathSegment,
): number {
const {cache} = shortestPathLengthToStart;
const { cache } = shortestPathLengthToStart;
let length = cache.get(segment.id);

// If `length` is null then we found a cycle! Return infinity since
Expand Down Expand Up @@ -533,7 +561,7 @@ const rule = {
isInsideComponentOrHook(codePathNode);
const isDirectlyInsideComponentOrHook = codePathFunctionName
? isComponentName(codePathFunctionName) ||
isHook(codePathFunctionName)
isHook(codePathFunctionName)
: isForwardRefCallback(codePathNode) || isMemoCallback(codePathNode);

// Compute the earliest finalizer level using information from the
Expand Down Expand Up @@ -679,17 +707,18 @@ const rule = {
'same order in every component render.' +
(possiblyHasEarlyReturn
? ' Did you accidentally call a React Hook after an' +
' early return?'
' early return?'
: '');
context.report({node: hook, message});
context.report({ node: hook, message });
}
} else if (
codePathNode.parent != null &&
(codePathNode.parent.type === 'MethodDefinition' ||
// @ts-expect-error `ClassProperty` was removed from typescript-estree in https://github.com/typescript-eslint/typescript-eslint/pull/3806
codePathNode.parent.type === 'ClassProperty' ||
codePathNode.parent.type === 'PropertyDefinition') &&
codePathNode.parent.value === codePathNode
codePathNode.parent.value === codePathNode &&
isReactComponentClass(codePathNode.parent)
) {
// Custom message for hooks inside a class
const message =
Expand All @@ -698,7 +727,7 @@ const rule = {
)}" cannot be called ` +
'in a class component. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
context.report({ node: hook, message });
} else if (codePathFunctionName) {
// Custom message if we found an invalid function name.
const message =
Expand All @@ -708,7 +737,7 @@ const rule = {
'React Hook function.' +
' React component names must start with an uppercase letter.' +
' React Hook names must start with the word "use".';
context.report({node: hook, message});
context.report({ node: hook, message });
} else if (codePathNode.type === 'Program') {
// These are dangerous if you have inline requires enabled.
const message =
Expand All @@ -717,7 +746,7 @@ const rule = {
)}" cannot be called ` +
'at the top level. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
context.report({ node: hook, message });
} else {
// Assume in all other cases the user called a hook in some
// random function callback. This should usually be true for
Expand All @@ -732,7 +761,7 @@ const rule = {
)}" cannot be called ` +
'inside a callback. React Hooks must be called in a ' +
'React function component or a custom React Hook function.';
context.report({node: hook, message});
context.report({ node: hook, message });
}
}
}
Expand Down