Skip to content

Commit

Permalink
Fix instances of function declaration after return (#19733)
Browse files Browse the repository at this point in the history 
* Add ESLint plugin to check for any function declare after return
* Refactor code to move function declarations before return and fix failing lint
  • Loading branch information
@bhumijgupta
bhumijgupta authored Sep 1, 2020
1 parent b7d18c4 commit 53e622c
Show file tree
Hide file tree
Showing 6 changed files with 170 additions and 158 deletions.
11 changes: 10 additions & 1 deletion .eslintrc.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,13 @@ module.exports = {
// Stop ESLint from looking for a configuration file in parent folders
root: true,

plugins: ['jest', 'no-for-of-loops', 'react', 'react-internal'],
plugins: [
'jest',
'no-for-of-loops',
'no-function-declare-after-return',
'react',
'react-internal',
],

parser: 'babel-eslint',
parserOptions: {
Expand Down Expand Up @@ -91,6 +97,9 @@ module.exports = {
// You can disable this rule for code that isn't shipped (e.g. build scripts and tests).
'no-for-of-loops/no-for-of-loops': ERROR,

// Prevent function declarations after return statements
'no-function-declare-after-return/no-function-declare-after-return': ERROR,

// CUSTOM RULES
// the second argument of warning/invariant should be a literal string
'react-internal/no-primitive-constructors': ERROR,
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,7 @@
"eslint-plugin-flowtype": "^2.25.0",
"eslint-plugin-jest": "^22.15.0",
"eslint-plugin-no-for-of-loops": "^1.0.0",
"eslint-plugin-no-function-declare-after-return": "^1.0.0",
"eslint-plugin-react": "^6.7.1",
"eslint-plugin-react-internal": "link:./scripts/eslint-rules",
"fbjs-scripts": "1.2.0",
Expand Down
295 changes: 147 additions & 148 deletions packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,154 +86,6 @@ export default {
return result;
};
}

return {
CallExpression: visitCallExpression,
};

function visitCallExpression(node) {
const callbackIndex = getReactiveHookCallbackIndex(node.callee, options);
if (callbackIndex === -1) {
// Not a React Hook call that needs deps.
return;
}
const callback = node.arguments[callbackIndex];
const reactiveHook = node.callee;
const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name;
const declaredDependenciesNode = node.arguments[callbackIndex + 1];
const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName);

// Check the declared dependencies for this reactive hook. If there is no
// second argument then the reactive callback will re-run on every render.
// So no need to check for dependency inclusion.
if (!declaredDependenciesNode && !isEffect) {
// These are only used for optimization.
if (
reactiveHookName === 'useMemo' ||
reactiveHookName === 'useCallback'
) {
// TODO: Can this have a suggestion?
reportProblem({
node: reactiveHook,
message:
`React Hook ${reactiveHookName} does nothing when called with ` +
`only one argument. Did you forget to pass an array of ` +
`dependencies?`,
});
}
return;
}

switch (callback.type) {
case 'FunctionExpression':
case 'ArrowFunctionExpression':
visitFunctionWithDependencies(
callback,
declaredDependenciesNode,
reactiveHook,
reactiveHookName,
isEffect,
);
return; // Handled
case 'Identifier':
if (!declaredDependenciesNode) {
// No deps, no problems.
return; // Handled
}
// The function passed as a callback is not written inline.
// But perhaps it's in the dependencies array?
if (
declaredDependenciesNode.elements &&
declaredDependenciesNode.elements.some(
el => el && el.type === 'Identifier' && el.name === callback.name,
)
) {
// If it's already in the list of deps, we don't care because
// this is valid regardless.
return; // Handled
}
// We'll do our best effort to find it, complain otherwise.
const variable = context.getScope().set.get(callback.name);
if (variable == null || variable.defs == null) {
// If it's not in scope, we don't care.
return; // Handled
}
// The function passed as a callback is not written inline.
// But it's defined somewhere in the render scope.
// We'll do our best effort to find and check it, complain otherwise.
const def = variable.defs[0];
if (!def || !def.node) {
break; // Unhandled
}
if (def.type !== 'Variable' && def.type !== 'FunctionName') {
// Parameter or an unusual pattern. Bail out.
break; // Unhandled
}
switch (def.node.type) {
case 'FunctionDeclaration':
// useEffect(() => { ... }, []);
visitFunctionWithDependencies(
def.node,
declaredDependenciesNode,
reactiveHook,
reactiveHookName,
isEffect,
);
return; // Handled
case 'VariableDeclarator':
const init = def.node.init;
if (!init) {
break; // Unhandled
}
switch (init.type) {
// const effectBody = () => {...};
// useEffect(effectBody, []);
case 'ArrowFunctionExpression':
case 'FunctionExpression':
// We can inspect this function as if it were inline.
visitFunctionWithDependencies(
init,
declaredDependenciesNode,
reactiveHook,
reactiveHookName,
isEffect,
);
return; // Handled
}
break; // Unhandled
}
break; // Unhandled
default:
// useEffect(generateEffectBody(), []);
reportProblem({
node: reactiveHook,
message:
`React Hook ${reactiveHookName} received a function whose dependencies ` +
`are unknown. Pass an inline function instead.`,
});
return; // Handled
}

// Something unusual. Fall back to suggesting to add the body itself as a dep.
reportProblem({
node: reactiveHook,
message:
`React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` +
`Either include it or remove the dependency array.`,
suggest: [
{
desc: `Update the dependencies array to be: [${callback.name}]`,
fix(fixer) {
return fixer.replaceText(
declaredDependenciesNode,
`[${callback.name}]`,
);
},
},
],
});
}

/**
* Visitor for both function expressions and arrow function expressions.
*/
Expand Down Expand Up @@ -1251,6 +1103,153 @@ export default {
],
});
}

function visitCallExpression(node) {
const callbackIndex = getReactiveHookCallbackIndex(node.callee, options);
if (callbackIndex === -1) {
// Not a React Hook call that needs deps.
return;
}
const callback = node.arguments[callbackIndex];
const reactiveHook = node.callee;
const reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name;
const declaredDependenciesNode = node.arguments[callbackIndex + 1];
const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName);

// Check the declared dependencies for this reactive hook. If there is no
// second argument then the reactive callback will re-run on every render.
// So no need to check for dependency inclusion.
if (!declaredDependenciesNode && !isEffect) {
// These are only used for optimization.
if (
reactiveHookName === 'useMemo' ||
reactiveHookName === 'useCallback'
) {
// TODO: Can this have a suggestion?
reportProblem({
node: reactiveHook,
message:
`React Hook ${reactiveHookName} does nothing when called with ` +
`only one argument. Did you forget to pass an array of ` +
`dependencies?`,
});
}
return;
}

switch (callback.type) {
case 'FunctionExpression':
case 'ArrowFunctionExpression':
visitFunctionWithDependencies(
callback,
declaredDependenciesNode,
reactiveHook,
reactiveHookName,
isEffect,
);
return; // Handled
case 'Identifier':
if (!declaredDependenciesNode) {
// No deps, no problems.
return; // Handled
}
// The function passed as a callback is not written inline.
// But perhaps it's in the dependencies array?
if (
declaredDependenciesNode.elements &&
declaredDependenciesNode.elements.some(
el => el && el.type === 'Identifier' && el.name === callback.name,
)
) {
// If it's already in the list of deps, we don't care because
// this is valid regardless.
return; // Handled
}
// We'll do our best effort to find it, complain otherwise.
const variable = context.getScope().set.get(callback.name);
if (variable == null || variable.defs == null) {
// If it's not in scope, we don't care.
return; // Handled
}
// The function passed as a callback is not written inline.
// But it's defined somewhere in the render scope.
// We'll do our best effort to find and check it, complain otherwise.
const def = variable.defs[0];
if (!def || !def.node) {
break; // Unhandled
}
if (def.type !== 'Variable' && def.type !== 'FunctionName') {
// Parameter or an unusual pattern. Bail out.
break; // Unhandled
}
switch (def.node.type) {
case 'FunctionDeclaration':
// useEffect(() => { ... }, []);
visitFunctionWithDependencies(
def.node,
declaredDependenciesNode,
reactiveHook,
reactiveHookName,
isEffect,
);
return; // Handled
case 'VariableDeclarator':
const init = def.node.init;
if (!init) {
break; // Unhandled
}
switch (init.type) {
// const effectBody = () => {...};
// useEffect(effectBody, []);
case 'ArrowFunctionExpression':
case 'FunctionExpression':
// We can inspect this function as if it were inline.
visitFunctionWithDependencies(
init,
declaredDependenciesNode,
reactiveHook,
reactiveHookName,
isEffect,
);
return; // Handled
}
break; // Unhandled
}
break; // Unhandled
default:
// useEffect(generateEffectBody(), []);
reportProblem({
node: reactiveHook,
message:
`React Hook ${reactiveHookName} received a function whose dependencies ` +
`are unknown. Pass an inline function instead.`,
});
return; // Handled
}

// Something unusual. Fall back to suggesting to add the body itself as a dep.
reportProblem({
node: reactiveHook,
message:
`React Hook ${reactiveHookName} has a missing dependency: '${callback.name}'. ` +
`Either include it or remove the dependency array.`,
suggest: [
{
desc: `Update the dependencies array to be: [${callback.name}]`,
fix(fixer) {
return fixer.replaceText(
declaredDependenciesNode,
`[${callback.name}]`,
);
},
},
],
});
}

return {
CallExpression: visitCallExpression,
};
},
};

Expand Down
3 changes: 1 addition & 2 deletions ...ages/react-native-renderer/src/__mocks__/react-native/Libraries/ReactPrivate/UIManager.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,6 @@ function removeChild(parent, child) {

const RCTUIManager = {
__dumpHierarchyForJestTestsOnly: function() {
return roots.map(tag => dumpSubtree(tag, 0)).join('\n');

function dumpSubtree(tag, indent) {
const info = views.get(tag);
let out = '';
Expand All @@ -73,6 +71,7 @@ const RCTUIManager = {
}
return out;
}
return roots.map(tag => dumpSubtree(tag, 0)).join('\n');
},
clearJSResponder: jest.fn(),
createView: jest.fn(function createView(reactTag, viewName, rootTag, props) {
Expand Down
13 changes: 6 additions & 7 deletions packages/react-native-renderer/src/legacy-events/SyntheticEvent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,13 +259,6 @@ addEventPoolingTo(SyntheticEvent);
* @return {object} defineProperty object
*/
function getPooledWarningPropertyDefinition(propName, getVal) {
const isFunction = typeof getVal === 'function';
return {
configurable: true,
set: set,
get: get,
};

function set(val) {
const action = isFunction ? 'setting the method' : 'setting the property';
warn(action, 'This is effectively a no-op');
Expand Down Expand Up @@ -296,6 +289,12 @@ function getPooledWarningPropertyDefinition(propName, getVal) {
);
}
}
const isFunction = typeof getVal === 'function';
return {
configurable: true,
set: set,
get: get,
};
}

function createOrGetPooledEvent(
Expand Down
5 changes: 5 additions & 0 deletions yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5407,6 +5407,11 @@ eslint-plugin-no-for-of-loops@^1.0.0:
resolved "https://registry.yarnpkg.com/eslint-plugin-no-for-of-loops/-/eslint-plugin-no-for-of-loops-1.0.1.tgz#2ee3732d50c642b8d1bfacedbfe3b28f86222369"
integrity sha512-uCotzBHt2W+HbLw2srRmqDJHOPbJGzeVLstKh8YyxS3ppduq2P50qdpJfHKoD+UGbnqA/zhy8NRgPH6p0y8bnA==

eslint-plugin-no-function-declare-after-return@^1.0.0:
version "1.0.0"
resolved "https://registry.yarnpkg.com/eslint-plugin-no-function-declare-after-return/-/eslint-plugin-no-function-declare-after-return-1.0.0.tgz#ddf01d71d27b37ced61ccb295c6ac0708d87b209"
integrity sha512-/w6tuSK4kYSwHSlPtf36u/rQOG8YVPgl8a0bh4rV/ElZNaUGYOquY/hp4jaCnEmPta0aTpAkFEmSi6ZTd7wCEQ==

eslint-plugin-no-unsanitized@3.1.2:
version "3.1.2"
resolved "https://registry.yarnpkg.com/eslint-plugin-no-unsanitized/-/eslint-plugin-no-unsanitized-3.1.2.tgz#a54724e0b81d43279bb1f8f5e1d82c97da78c858"
Expand Down

0 comments on commit 53e622c

Please sign in to comment.