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

Breaking: Reduce false positives by only detecting function-style rules when the function returns an object #211

Merged

Conversation

bmish
Copy link
Member

@bmish bmish commented Oct 13, 2021

Fixes #210.

When detecting the deprecated, function-style rule, we can be more strict and only detect default exports with a function that returns an object.

module.exports = function(context) {
    return {
        CallExpression(node) {}
    };
};

This will help reduce false positives which could previously happen with detecting default exports of arbitrary functions.

I implemented this for both CJS (pre-v4) and ESM (v4+) rules. We might as well be more strict for all function-style rules, given that they are deprecated and rare anyway (only about 6% of rules are function-style according to an analysis I performed).

https://eslint.org/docs/developer-guide/working-with-rules-deprecated

Part of v4 release (#120).

@bmish bmish force-pushed the esm-func-return-object-rule-detect branch from ea7383e to d09645e Compare October 13, 2021 16:06
@bmish bmish marked this pull request as draft October 13, 2021 16:18
@bmish bmish force-pushed the esm-func-return-object-rule-detect branch from d09645e to 542d83c Compare October 13, 2021 16:49
@bmish bmish marked this pull request as ready for review October 13, 2021 16:50
@bmish bmish changed the title Fix: Reduce false positives by only detecting function-style rules when function returns an object Breaking: Reduce false positives by only detecting function-style rules when function returns an object Oct 13, 2021
@bmish bmish changed the title Breaking: Reduce false positives by only detecting function-style rules when function returns an object Breaking: Reduce false positives by only detecting function-style rules when the function returns an object Oct 13, 2021
@bmish bmish mentioned this pull request Oct 13, 2021
20 tasks
@aladdin-add aladdin-add self-requested a review October 14, 2021 02:53
});
return foundMatch;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module.exports = function(){
  function foo() {return {};}
  // no return here.
};

does it consider code like this? (Just a question, I don't think it's worthy to introduce the code path analysis to eliminate these false positives).

off topic: we better encourage to use overrides (2e86892), rather than to guess whether it's an eslint rule.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I realize that this current return statement detection isn't perfect, and I was planning to follow-up later as a bug fix to ensure that we only detect return statements directly inside the current function. The current implementation in this PR still lays the groundwork for future improvements and gets us most of the way there in reducing false positives. It's also nice to get the bigger change in now along with the major release and then make smaller bug fixes later.
  2. estraverse has millions of weekly downloads and is a dependency of eslint-plugin-react too. I find the traversal functionality it provides to be very powerful and useful. But I'm open to investigating alternatives if this dependency is the concern.
  3. I agree that consumers of this plugin should consider using overrides. However:
    1. There will always be some amount of guessing involved in detecting rules. I have seen many plugins that contain util functions/files in their rules/ directory. So it's still valuable that we can making our rule detection as smart as possible.
    2. Most consumers of this plugin will just enable the recommended config for their entire codebase. That's just the way people are used to using ESLint plugins. Most people won't bother with overrides (although it's a great option we provide and suggest). So it's nice if we can make the recommended config as smart as possible even when it applies to non-rule files.
  4. Given that only about 6% of rules across the ecosystem are function-style according to an analysis I performed, I believe it's a good time to become stricter in how we detect them to avoid false positives. There will always be some heuristics involved in detecting rules and I think we can improve them over-time when we discover opportunities to do so.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have seen many plugins that contain util functions/files in their rules/ directory

excludedFiles can be helpful for these cases.

just like the above example, it's also likely to be an eslint rule, but the dev forgot to return the object. So, ignoring these cases is not always reasonable. I would highly suggest users to use overrides (as also disscussed in #81 (comment)).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the rule doesn't return an object, then it's likely there's little or no code for our rules to actually check (most of the code we would be checking would be inside the returned object).

@aladdin-add aladdin-add merged commit 1bd45d9 into eslint-community:master Oct 14, 2021
@aladdin-add
Copy link
Contributor

I'm having a bad network, will try to do a prerelease tomorrow. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stricter ESM function-style rule detection
2 participants