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

Fix: Unused recursive function expressions should be flagged (fixes #… #11032

Merged
merged 1 commit into from Nov 9, 2018

Conversation

@sergei-startsev
Copy link
Contributor

commented Oct 30, 2018

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[x] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

#10982

What changes did you make? (Give an overview)

Added getFunctionDefinitions to get a list of function definitions for a passed variable, added checks for FunctionExpression and ArrowFunctionExpression, see corresponding unit tests.

@jsf-clabot

This comment has been minimized.

Copy link

commented Oct 30, 2018

CLA assistant check
All committers have signed the CLA.

@eslint eslint bot added the triage label Oct 30, 2018

@sergei-startsev sergei-startsev force-pushed the sergei-startsev:issue10982 branch from 03e2981 to 8096f33 Oct 30, 2018

* @returns {ASTNode[]} Function nodes.
* @private
*/
function getFunctionDefinitions(variable) {

This comment has been minimized.

Copy link
@aliaksandr-yermalayeu

aliaksandr-yermalayeu Oct 30, 2018

I would suggest 'map' usage instead of 'forEach':

const getFunctionDefinitions = variable =>
    variable.defs.map(def => {
        const { type, node } = def;
        // FunctionDeclarations
        if (type === "FunctionName") {
            return node;
        }
        // FunctionExpressions
        if (type === "Variable" && node.init &&
            (node.init.type === "FunctionExpression" || node.init.type === "ArrowFunctionExpression")) {
            return node.init;
        }
    }).filter(a => a);

This comment has been minimized.

Copy link
@sergei-startsev

sergei-startsev Oct 30, 2018

Author Contributor

I wouldn't like to use both map and filter here, but let's wait maintainers' feedback

@platinumazure platinumazure added bug rule and removed triage labels Oct 30, 2018

@platinumazure
Copy link
Member

left a comment

Hi @sergei-startsev, thanks for the PR!

I don't have strong feelings on forEach vs map/filter (although I'll say I have no problem with map then filter).

Are there any test cases that show a recursive function not triggering this rule if it's initially called from the outside? Something like: function a() { a(); } a(); If any tests like that are missing, please add them. Thanks!

@sergei-startsev sergei-startsev force-pushed the sergei-startsev:issue10982 branch from 8096f33 to 2f1bfed Oct 30, 2018

@sergei-startsev

This comment has been minimized.

Copy link
Contributor Author

commented Oct 30, 2018

@platinumazure Thanks for the review, I've added a few more tests to be sure that the rule doesn't have false positive issues.

@nzakas

nzakas approved these changes Nov 9, 2018

@btmills

btmills approved these changes Nov 9, 2018

Copy link
Member

left a comment

LGTM, thanks @sergei-startsev!

@nzakas nzakas merged commit 9436712 into eslint:master Nov 9, 2018

5 checks passed

commit-message Commit message follows guidelines
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details

@eslint eslint bot locked and limited conversation to collaborators May 9, 2019

@eslint eslint bot added the archived due to age label May 9, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants
You can’t perform that action at this time.