Skip to content

Commit

Permalink
Breaking: Reduce false positives by only detecting function-style rul…
Browse files Browse the repository at this point in the history
…es when function returns an object
  • Loading branch information
bmish committed Oct 13, 2021
1 parent f8a642a commit 542d83c
Show file tree
Hide file tree
Showing 12 changed files with 133 additions and 49 deletions.
27 changes: 23 additions & 4 deletions lib/utils.js
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;
}

/**
* 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
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
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
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
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
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
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
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
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
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
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

0 comments on commit 542d83c

Please sign in to comment.