[ESLint] Treat functions that don't capture anything as static (#14996)

* Treat functions that don't capture anything as static

* Fix comment
This commit is contained in:
Dan Abramov
2019-03-05 21:07:37 +00:00
committed by GitHub
parent fd557d453d
commit fa5d4ee43b
2 changed files with 674 additions and 78 deletions
@@ -652,6 +652,206 @@ const tests = {
}
`,
},
{
code: `
function MyComponent(props) {
function handleNext1() {
console.log('hello');
}
const handleNext2 = () => {
console.log('hello');
};
let handleNext3 = function() {
console.log('hello');
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, [handleNext1]);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, [handleNext2]);
useMemo(() => {
return Store.subscribe(handleNext3);
}, [handleNext3]);
}
`,
},
{
// Declaring handleNext is optional because
// it doesn't use anything in the function scope.
code: `
function MyComponent(props) {
function handleNext1() {
console.log('hello');
}
const handleNext2 = () => {
console.log('hello');
};
let handleNext3 = function() {
console.log('hello');
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, []);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, []);
useMemo(() => {
return Store.subscribe(handleNext3);
}, []);
}
`,
},
{
// Declaring handleNext is optional because
// it doesn't use anything in the function scope.
code: `
function MyComponent(props) {
function handleNext() {
console.log('hello');
}
useEffect(() => {
return Store.subscribe(handleNext);
}, []);
useLayoutEffect(() => {
return Store.subscribe(handleNext);
}, []);
useMemo(() => {
return Store.subscribe(handleNext);
}, []);
}
`,
},
{
// Declaring handleNext is optional because
// everything they use is fully static.
code: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
function handleNext1(value) {
let value2 = value * 100;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(foo(value));
console.log('hello');
};
let handleNext3 = function(value) {
console.log(value);
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, []);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, []);
useMemo(() => {
return Store.subscribe(handleNext3);
}, []);
}
`,
},
{
code: `
function useInterval(callback, delay) {
const savedCallback = useRef();
useEffect(() => {
savedCallback.current = callback;
});
useEffect(() => {
function tick() {
savedCallback.current();
}
if (delay !== null) {
let id = setInterval(tick, delay);
return () => clearInterval(id);
}
}, [delay]);
}
`,
},
{
code: `
function Counter() {
const [count, setCount] = useState(0);
useEffect(() => {
let id = setInterval(() => {
setCount(c => c + 1);
}, 1000);
return () => clearInterval(id);
}, []);
return <h1>{count}</h1>;
}
`,
},
{
code: `
function Counter() {
const [count, setCount] = useState(0);
function tick() {
setCount(c => c + 1);
}
useEffect(() => {
let id = setInterval(() => {
tick();
}, 1000);
return () => clearInterval(id);
}, []);
return <h1>{count}</h1>;
}
`,
},
{
code: `
function Counter() {
const [count, dispatch] = useReducer((state, action) => {
if (action === 'inc') {
return state + 1;
}
}, 0);
useEffect(() => {
let id = setInterval(() => {
dispatch('inc');
}, 1000);
return () => clearInterval(id);
}, []);
return <h1>{count}</h1>;
}
`,
},
{
code: `
function Counter() {
const [count, dispatch] = useReducer((state, action) => {
if (action === 'inc') {
return state + 1;
}
}, 0);
const tick = () => {
dispatch('inc');
};
useEffect(() => {
let id = setInterval(tick, 1000);
return () => clearInterval(id);
}, []);
return <h1>{count}</h1>;
}
`,
},
],
invalid: [
{
@@ -2858,6 +3058,313 @@ const tests = {
"because their mutation doesn't re-render the component.",
],
},
{
// Every almost-static function is tainted by a dynamic value.
code: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
let taint = props.foo;
function handleNext1(value) {
let value2 = value * taint;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(taint(value));
console.log('hello');
};
let handleNext3 = function(value) {
setTimeout(() => console.log(taint));
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, []);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, []);
useMemo(() => {
return Store.subscribe(handleNext3);
}, []);
}
`,
output: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
let taint = props.foo;
function handleNext1(value) {
let value2 = value * taint;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(taint(value));
console.log('hello');
};
let handleNext3 = function(value) {
setTimeout(() => console.log(taint));
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, [handleNext1]);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, [handleNext2]);
useMemo(() => {
return Store.subscribe(handleNext3);
}, [handleNext3]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'handleNext1'. " +
'Either include it or remove the dependency array.',
"React Hook useLayoutEffect has a missing dependency: 'handleNext2'. " +
'Either include it or remove the dependency array.',
"React Hook useMemo has a missing dependency: 'handleNext3'. " +
'Either include it or remove the dependency array.',
],
},
{
// Regression test
code: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
let taint = props.foo;
// Shouldn't affect anything
function handleChange() {}
function handleNext1(value) {
let value2 = value * taint;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(taint(value));
console.log('hello');
};
let handleNext3 = function(value) {
console.log(taint);
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, []);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, []);
useMemo(() => {
return Store.subscribe(handleNext3);
}, []);
}
`,
output: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
let taint = props.foo;
// Shouldn't affect anything
function handleChange() {}
function handleNext1(value) {
let value2 = value * taint;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(taint(value));
console.log('hello');
};
let handleNext3 = function(value) {
console.log(taint);
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, [handleNext1]);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, [handleNext2]);
useMemo(() => {
return Store.subscribe(handleNext3);
}, [handleNext3]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'handleNext1'. " +
'Either include it or remove the dependency array.',
"React Hook useLayoutEffect has a missing dependency: 'handleNext2'. " +
'Either include it or remove the dependency array.',
"React Hook useMemo has a missing dependency: 'handleNext3'. " +
'Either include it or remove the dependency array.',
],
},
{
// Regression test
code: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
let taint = props.foo;
// Shouldn't affect anything
const handleChange = () => {};
function handleNext1(value) {
let value2 = value * taint;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(taint(value));
console.log('hello');
};
let handleNext3 = function(value) {
console.log(taint);
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, []);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, []);
useMemo(() => {
return Store.subscribe(handleNext3);
}, []);
}
`,
output: `
function MyComponent(props) {
let [, setState] = useState();
let [, dispatch] = React.useReducer();
let taint = props.foo;
// Shouldn't affect anything
const handleChange = () => {};
function handleNext1(value) {
let value2 = value * taint;
setState(value2);
console.log('hello');
}
const handleNext2 = (value) => {
setState(taint(value));
console.log('hello');
};
let handleNext3 = function(value) {
console.log(taint);
dispatch({ type: 'x', value });
};
useEffect(() => {
return Store.subscribe(handleNext1);
}, [handleNext1]);
useLayoutEffect(() => {
return Store.subscribe(handleNext2);
}, [handleNext2]);
useMemo(() => {
return Store.subscribe(handleNext3);
}, [handleNext3]);
}
`,
errors: [
"React Hook useEffect has a missing dependency: 'handleNext1'. " +
'Either include it or remove the dependency array.',
"React Hook useLayoutEffect has a missing dependency: 'handleNext2'. " +
'Either include it or remove the dependency array.',
"React Hook useMemo has a missing dependency: 'handleNext3'. " +
'Either include it or remove the dependency array.',
],
},
{
code: `
function Counter() {
let [count, setCount] = useState(0);
useEffect(() => {
let id = setInterval(() => {
setCount(count + 1);
}, 1000);
return () => clearInterval(id);
}, []);
return <h1>{count}</h1>;
}
`,
output: `
function Counter() {
let [count, setCount] = useState(0);
useEffect(() => {
let id = setInterval(() => {
setCount(count + 1);
}, 1000);
return () => clearInterval(id);
}, [count]);
return <h1>{count}</h1>;
}
`,
// TODO: ideally this should suggest useState updater form
// since this code doesn't actually work.
errors: [
"React Hook useEffect has a missing dependency: 'count'. " +
'Either include it or remove the dependency array.',
],
},
{
code: `
function Counter() {
const [count, setCount] = useState(0);
function tick() {
setCount(count + 1);
}
useEffect(() => {
let id = setInterval(() => {
tick();
}, 1000);
return () => clearInterval(id);
}, []);
return <h1>{count}</h1>;
}
`,
output: `
function Counter() {
const [count, setCount] = useState(0);
function tick() {
setCount(count + 1);
}
useEffect(() => {
let id = setInterval(() => {
tick();
}, 1000);
return () => clearInterval(id);
}, [tick]);
return <h1>{count}</h1>;
}
`,
// TODO: ideally this should suggest useState updater form
// since this code doesn't actually work. The autofix could
// at least avoid suggesting 'tick' since it's obviously
// always different, and thus useless.
errors: [
"React Hook useEffect has a missing dependency: 'tick'. " +
'Either include it or remove the dependency array.',
],
},
],
};
@@ -34,6 +34,22 @@ export default {
: undefined;
const options = {additionalHooks};
// Should be shared between visitors.
let staticKnownValueCache = new WeakMap();
let functionWithoutCapturedValueCache = new WeakMap();
function memoizeWithWeakMap(fn, map) {
return function(arg) {
if (map.has(arg)) {
// to verify cache hits:
// console.log(arg.name)
return map.get(arg);
}
const result = fn(arg);
map.set(arg, result);
return result;
};
}
return {
FunctionExpression: visitFunctionExpression,
ArrowFunctionExpression: visitFunctionExpression,
@@ -105,6 +121,153 @@ export default {
componentScope = currentScope;
}
// Next we'll define a few helpers that helps us
// tell if some values don't have to be declared as deps.
// Some are known to be static based on Hook calls.
// const [state, setState] = useState() / React.useState()
// ^^^ true for this reference
// const [state, dispatch] = useReducer() / React.useReducer()
// ^^^ true for this reference
// const ref = useRef()
// ^^^ true for this reference
// False for everything else.
function isStaticKnownHookValue(resolved) {
if (!Array.isArray(resolved.defs)) {
return false;
}
const def = resolved.defs[0];
if (def == null) {
return false;
}
// Look for `let stuff = ...`
if (def.node.type !== 'VariableDeclarator') {
return false;
}
const init = def.node.init;
if (init == null) {
return false;
}
// Detect primitive constants
// const foo = 42
const declaration = def.node.parent;
if (
declaration.kind === 'const' &&
init.type === 'Literal' &&
(typeof init.value === 'string' ||
typeof init.value === 'number' ||
init.value === null)
) {
// Definitely static
return true;
}
// Detect known Hook calls
// const [_, setState] = useState()
if (init.type !== 'CallExpression') {
return false;
}
let callee = init.callee;
// Step into `= React.something` initializer.
if (
callee.type === 'MemberExpression' &&
callee.object.name === 'React' &&
callee.property != null &&
!callee.computed
) {
callee = callee.property;
}
if (callee.type !== 'Identifier') {
return false;
}
const id = def.node.id;
if (callee.name === 'useRef' && id.type === 'Identifier') {
// useRef() return value is static.
return true;
} else if (callee.name === 'useState' || callee.name === 'useReducer') {
// Only consider second value in initializing tuple static.
if (
id.type === 'ArrayPattern' &&
id.elements.length === 2 &&
Array.isArray(resolved.identifiers) &&
// Is second tuple value the same reference we're checking?
id.elements[1] === resolved.identifiers[0]
) {
return true;
}
}
// By default assume it's dynamic.
return false;
}
// Some are just functions that don't reference anything dynamic.
function isFunctionWithoutCapturedValues(resolved) {
if (!Array.isArray(resolved.defs)) {
return false;
}
const def = resolved.defs[0];
if (def == null) {
return false;
}
if (def.node == null || def.node.id == null) {
return false;
}
// Search the direct component subscopes for
// top-level function definitions matching this reference.
const fnNode = def.node;
let childScopes = componentScope.childScopes;
let fnScope = null;
let i;
for (i = 0; i < childScopes.length; i++) {
let childScope = childScopes[i];
let childScopeBlock = childScope.block;
if (
// function handleChange() {}
(fnNode.type === 'FunctionDeclaration' &&
childScopeBlock === fnNode) ||
// const handleChange = () => {}
// const handleChange = function() {}
(fnNode.type === 'VariableDeclarator' &&
childScopeBlock.parent === fnNode)
) {
// Found it!
fnScope = childScope;
break;
}
}
if (fnScope == null) {
return false;
}
// Does this function capture any values
// that are in pure scopes (aka render)?
for (i = 0; i < fnScope.through.length; i++) {
const ref = fnScope.through[i];
if (ref.resolved == null) {
continue;
}
if (
pureScopes.has(ref.resolved.scope) &&
// Static values are fine though,
// although we won't check functions deeper.
!memoizedIsStaticKnownHookValue(ref.resolved)
) {
return false;
}
}
// If we got here, this function doesn't capture anything
// from render--or everything it captures is known static.
return true;
}
// Remember such values. Avoid re-running extra checks on them.
const memoizedIsStaticKnownHookValue = memoizeWithWeakMap(
isStaticKnownHookValue,
staticKnownValueCache,
);
const memoizedIsFunctionWithoutCapturedValues = memoizeWithWeakMap(
isFunctionWithoutCapturedValues,
functionWithoutCapturedValueCache,
);
// These are usually mistaken. Collect them.
const currentRefsInEffectCleanup = new Map();
@@ -172,7 +335,10 @@ export default {
// Add the dependency to a map so we can make sure it is referenced
// again in our dependencies array. Remember whether it's static.
if (!dependencies.has(dependency)) {
const isStatic = isDefinitelyStaticDependency(reference);
const resolved = reference.resolved;
const isStatic =
memoizedIsStaticKnownHookValue(resolved) ||
memoizedIsFunctionWithoutCapturedValues(resolved);
dependencies.set(dependency, {
isStatic,
reference,
@@ -778,83 +944,6 @@ function getReactiveHookCallbackIndex(calleeNode, options) {
}
}
// const [state, setState] = useState() / React.useState()
// ^^^ true for this reference
// const [state, dispatch] = useReducer() / React.useReducer()
// ^^^ true for this reference
// const ref = useRef()
// ^^^ true for this reference
// False for everything else.
function isDefinitelyStaticDependency(reference) {
// This function is written defensively because I'm not sure about corner cases.
// TODO: we can strengthen this if we're sure about the types.
const resolved = reference.resolved;
if (resolved == null || !Array.isArray(resolved.defs)) {
return false;
}
const def = resolved.defs[0];
if (def == null) {
return false;
}
// Look for `let stuff = ...`
if (def.node.type !== 'VariableDeclarator') {
return false;
}
const init = def.node.init;
if (init == null) {
return false;
}
// Detect primitive constants
// const foo = 42
const declaration = def.node.parent;
if (
declaration.kind === 'const' &&
init.type === 'Literal' &&
(typeof init.value === 'string' ||
typeof init.value === 'number' ||
init.value === null)
) {
// Definitely static
return true;
}
// Detect known Hook calls
// const [_, setState] = useState()
if (init.type !== 'CallExpression') {
return false;
}
let callee = init.callee;
// Step into `= React.something` initializer.
if (
callee.type === 'MemberExpression' &&
callee.object.name === 'React' &&
callee.property != null &&
!callee.computed
) {
callee = callee.property;
}
if (callee.type !== 'Identifier') {
return false;
}
const id = def.node.id;
if (callee.name === 'useRef' && id.type === 'Identifier') {
// useRef() return value is static.
return true;
} else if (callee.name === 'useState' || callee.name === 'useReducer') {
// Only consider second value in initializing tuple static.
if (
id.type === 'ArrayPattern' &&
id.elements.length === 2 &&
Array.isArray(reference.resolved.identifiers) &&
// Is second tuple value the same reference we're checking?
id.elements[1] === reference.resolved.identifiers[0]
) {
return true;
}
}
// By default assume it's dynamic.
return false;
}
/**
* ESLint won't assign node.parent to references from context.getScope()
*