-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Breaking: Reduce false positives by only detecting function-style rules when the function returns an object #211
Conversation
ea7383e
to
d09645e
Compare
…es when function returns an object
d09645e
to
542d83c
Compare
}); | ||
return foundMatch; | ||
} | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 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.
- 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.
- I agree that consumers of this plugin should consider using overrides. However:
- 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. - 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 therecommended
config as smart as possible even when it applies to non-rule files.
- There will always be some amount of guessing involved in detecting rules. I have seen many plugins that contain util functions/files in their
- 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.
There was a problem hiding this comment.
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)).
There was a problem hiding this comment.
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).
I'm having a bad network, will try to do a prerelease tomorrow. :) |
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.
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).