Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 14, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 23 additions & 4 deletions lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
'use strict';

const { getStaticValue } = require('eslint-utils');
const estraverse = require('estraverse');

/**
* Determines whether a node is a 'normal' (i.e. non-async, non-generator) function expression.
Expand Down Expand Up @@ -102,6 +103,24 @@ function collectInterestingProperties (properties, interestingKeys) {
}, {});
}

/**
* Check if there is a return statement that returns an object somewhere inside the given node.
* @param {Node} node
* @returns {boolean}
*/
function hasObjectReturn (node) {
let foundMatch = false;
estraverse.traverse(node, {
enter (child) {
if (child.type === 'ReturnStatement' && child.argument && child.argument.type === 'ObjectExpression') {
foundMatch = true;
}
},
fallback: 'iteration', // Don't crash on unexpected node types.
});
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

@aladdin-add aladdin-add Oct 14, 2021 •
edited
Loading

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).

/**
* Helper for `getRuleInfo`. Handles ESM and TypeScript rules.
*/
Expand All @@ -114,8 +133,8 @@ function getRuleExportsESM (ast) {
if (node.type === 'ObjectExpression') {
// Check `export default { create() {}, meta: {} }`
return collectInterestingProperties(node.properties, INTERESTING_RULE_KEYS);
} else if (isNormalFunctionExpression(node)) {
// Check `export default function() {}`
} else if (isNormalFunctionExpression(node) && hasObjectReturn(node)) {
// Check `export default function() { return { ... }; }`
return { create: node, meta: null, isNewStyle: false };
} else if (
node.type === 'CallExpression' &&
Expand Down Expand Up @@ -156,8 +175,8 @@ function getRuleExportsCJS (ast) {
node.left.property.type === 'Identifier' && node.left.property.name === 'exports'
) {
exportsVarOverridden = true;
if (isNormalFunctionExpression(node.right)) {
// Check `module.exports = function () {}`
if (isNormalFunctionExpression(node.right) && hasObjectReturn(node.right)) {
// Check `module.exports = function () { return {}; }`

exportsIsFunction = true;
return { create: node.right, meta: null, isNewStyle: false };
Expand Down
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@
},
"homepage": "https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin#readme",
"dependencies": {
"eslint-utils": "^3.0.0"
"eslint-utils": "^3.0.0",
"estraverse": "^5.2.0"
},
"nyc": {
"branches": 98,
Expand All @@ -51,7 +52,6 @@
"eslint-plugin-unicorn": "^37.0.0",
"eslint-scope": "^6.0.0",
"espree": "^9.0.0",
"estraverse": "^5.0.0",
"lodash": "^4.17.2",
"markdownlint-cli": "^0.28.1",
"mocha": "^9.1.2",
Expand Down
6 changes: 3 additions & 3 deletions tests/lib/rules/no-deprecated-context-methods.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ ruleTester.run('no-deprecated-context-methods', rule, {
`
module.exports = context => {
const sourceCode = context.getSourceCode();

sourceCode.getFirstToken();
return {};
}
`,
],
Expand Down Expand Up @@ -70,12 +70,12 @@ ruleTester.run('no-deprecated-context-methods', rule, {
{
code: `
module.exports = myRuleContext => {
myRuleContext.getFirstToken;
myRuleContext.getFirstToken; return {};
}
`,
output: `
module.exports = myRuleContext => {
myRuleContext.getSourceCode().getFirstToken;
myRuleContext.getSourceCode().getFirstToken; return {};
}
`,
errors: [
Expand Down
4 changes: 2 additions & 2 deletions tests/lib/rules/no-deprecated-report-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,12 @@ ruleTester.run('no-deprecated-report-api', rule, {
`,
`
module.exports = function(context) {
context.report({node, message: "Foo"});
context.report({node, message: "Foo"}); return {};
};
`,
`
module.exports = (context) => {
context.report({node, message: "Foo"});
context.report({node, message: "Foo"}); return {};
};
`,
`
Expand Down
14 changes: 7 additions & 7 deletions tests/lib/rules/no-missing-placeholders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,12 @@ ruleTester.run('no-missing-placeholders', rule, {
`,
`
module.exports = context => {
context.report(node, 'foo {{bar}}', { bar: 'baz' });
context.report(node, 'foo {{bar}}', { bar: 'baz' }); return {};
};
`,
`
module.exports = context => {
context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { bar: 'baz' });
context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { bar: 'baz' }); return {};
};
`,
`
Expand Down Expand Up @@ -118,14 +118,14 @@ ruleTester.run('no-missing-placeholders', rule, {
`
const MESSAGE = 'foo {{bar}}';
module.exports = context => {
context.report(node, MESSAGE, { bar: 'baz' });
context.report(node, MESSAGE, { bar: 'baz' }); return {};
};
`,
// Message in variable but cannot statically determine its type.
`
const MESSAGE = getMessage();
module.exports = context => {
context.report(node, MESSAGE, { baz: 'qux' });
context.report(node, MESSAGE, { baz: 'qux' }); return {};
};
`,
// Suggestion with placeholder
Expand Down Expand Up @@ -193,7 +193,7 @@ ruleTester.run('no-missing-placeholders', rule, {
{
code: `
module.exports = context => {
context.report(node, 'foo {{bar}}', { baz: 'qux' });
context.report(node, 'foo {{bar}}', { baz: 'qux' }); return {};
};
`,
errors: [error('bar')],
Expand All @@ -203,15 +203,15 @@ ruleTester.run('no-missing-placeholders', rule, {
code: `
const MESSAGE = 'foo {{bar}}';
module.exports = context => {
context.report(node, MESSAGE, { baz: 'qux' });
context.report(node, MESSAGE, { baz: 'qux' }); return {};
};
`,
errors: [error('bar', 'Identifier')],
},
{
code: `
module.exports = context => {
context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { baz: 'baz' });
context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { baz: 'baz' }); return {};
};
`,
errors: [error('bar')],
Expand Down
8 changes: 4 additions & 4 deletions tests/lib/rules/no-unused-placeholders.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,26 +85,26 @@ ruleTester.run('no-unused-placeholders', rule, {
`,
`
module.exports = context => {
context.report(node, 'foo {{bar}}', { bar: 'baz' });
context.report(node, 'foo {{bar}}', { bar: 'baz' }); return {};
};
`,
// With message as variable.
`
const MESSAGE = 'foo {{bar}}';
module.exports = context => {
context.report(node, MESSAGE, { bar: 'baz' });
context.report(node, MESSAGE, { bar: 'baz' }); return {};
};
`,
// With message as variable but cannot statically determine its type.
`
const MESSAGE = getMessage();
module.exports = context => {
context.report(node, MESSAGE, { bar: 'baz' });
context.report(node, MESSAGE, { bar: 'baz' }); return {};
};
`,
`
module.exports = context => {
context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { bar: 'baz' });
context.report(node, { line: 1, column: 3 }, 'foo {{bar}}', { bar: 'baz' }); return {};
};
`,
// Suggestion
Expand Down
8 changes: 4 additions & 4 deletions tests/lib/rules/prefer-object-rule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@ ruleTester.run('prefer-object-rule', rule, {
errors: [{ messageId: 'preferObject', line: 2, column: 24 }],
},
{
code: 'export default function create() {};',
output: 'export default {create() {}};',
code: 'export default function create() { return {}; };',
output: 'export default {create() { return {}; }};',
parserOptions: { sourceType: 'module' },
errors: [{ messageId: 'preferObject', line: 1, column: 16 }],
},
{
code: 'export default () => {};',
output: 'export default {create: () => {}};',
code: 'export default () => { return {}; };',
output: 'export default {create: () => { return {}; }};',
parserOptions: { sourceType: 'module' },
errors: [{ messageId: 'preferObject', line: 1, column: 16 }],
},
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/rules/report-message-format.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ ruleTester.run('report-message-format', rule, {

valid: [
// with no configuration, everything is allowed
'module.exports = context => context.report(node, "foo");',
'module.exports = context => { context.report(node, "foo"); return {}; }',
{
code: `
module.exports = {
Expand Down
6 changes: 3 additions & 3 deletions tests/lib/rules/require-meta-docs-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ tester.run('require-meta-docs-url', rule, {
invalid: [
{
code: `
module.exports = function() {}
module.exports = function() { return {}; }
`,
output: null,
errors: [{ messageId: 'missing', type: 'FunctionExpression' }],
Expand Down Expand Up @@ -310,7 +310,7 @@ tester.run('require-meta-docs-url', rule, {
// -------------------------------------------------------------------------
{
code: `
module.exports = function() {}
module.exports = function() { return {}; }
`,
output: null,
options: [{
Expand Down Expand Up @@ -492,7 +492,7 @@ tester.run('require-meta-docs-url', rule, {
{
filename: 'test.js',
code: `
module.exports = function() {}
module.exports = function() { return {}; }
`,
output: null,
options: [{
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/rules/require-meta-fixable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ ruleTester.run('require-meta-fixable', rule, {
create(context) {}
};
`,
'module.exports = context => {};',
'module.exports = context => { return {}; };',
`
module.exports = {
meta: { fixable: 'code' },
Expand Down
2 changes: 1 addition & 1 deletion tests/lib/rules/require-meta-has-suggestions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ const RuleTester = require('eslint').RuleTester;
const ruleTester = new RuleTester({ parserOptions: { ecmaVersion: 6 } });
ruleTester.run('require-meta-has-suggestions', rule, {
valid: [
'module.exports = context => {};',
'module.exports = context => { return {}; };',
// No suggestions reported, no violations reported, no meta object.
`
module.exports = {
Expand Down
Loading