diff --git a/.eslintrc.js b/.eslintrc.js index 0c14d962..223fd7ff 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -8,6 +8,7 @@ module.exports = { }, extends: [ 'not-an-aardvark/node', + 'plugin:eslint-comments/recommended', 'plugin:node/recommended', 'plugin:prettier/recommended', 'plugin:unicorn/recommended', @@ -23,6 +24,9 @@ module.exports = { ], 'require-jsdoc': 'error', + 'eslint-comments/no-unused-disable': 'error', + 'eslint-comments/require-description': 'error', + 'unicorn/consistent-function-scoping': 'off', 'unicorn/no-array-callback-reference': 'off', 'unicorn/no-array-for-each': 'off', @@ -65,6 +69,8 @@ module.exports = { 'no-unused-vars': 'off', strict: 'off', + 'eslint-comments/require-description': 'off', + 'unicorn/filename-case': 'off', }, }, diff --git a/CHANGELOG.md b/CHANGELOG.md index 33e7027a..db800bed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ +### [5.0.4](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/compare/v5.0.3...v5.0.4) (2022-08-15) + + +### Bug Fixes + +* false positives with violation reporting helper function in `no-unused-message-ids` rule ([#290](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/issues/290)) ([1c30165](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/commit/1c301653a1bd120b121029c83d2d0914615cf9a5)) + ### [5.0.3](https://github.com/not-an-aardvark/eslint-plugin-eslint-plugin/compare/v5.0.2...v5.0.3) (2022-08-12) diff --git a/lib/index.js b/lib/index.js index a6892c22..e0a81cee 100644 --- a/lib/index.js +++ b/lib/index.js @@ -43,7 +43,7 @@ const allRules = Object.fromEntries( module.exports.rules = allRules; -// eslint-disable-next-line unicorn/prefer-object-from-entries +// eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now module.exports.configs = Object.keys(configFilters).reduce( (configs, configName) => { return Object.assign(configs, { diff --git a/lib/rules/no-unused-message-ids.js b/lib/rules/no-unused-message-ids.js index 8aa4bfb0..01fc262c 100644 --- a/lib/rules/no-unused-message-ids.js +++ b/lib/rules/no-unused-message-ids.js @@ -120,7 +120,8 @@ module.exports = { if ( values.length === 0 || - values.some((val) => val.type !== 'Literal') + values.some((val) => val.type !== 'Literal') || + utils.isVariableFromParameter(node.value, scopeManager) ) { // When a dynamic messageId is used and we can't detect its value, disable the rule to avoid false positives. hasSeenUnknownMessageId = true; diff --git a/lib/rules/require-meta-docs-url.js b/lib/rules/require-meta-docs-url.js index 49324675..a1955030 100644 --- a/lib/rules/require-meta-docs-url.js +++ b/lib/rules/require-meta-docs-url.js @@ -114,7 +114,7 @@ module.exports = { messageId: !urlPropNode ? 'missing' - : // eslint-disable-next-line unicorn/no-nested-ternary + : // eslint-disable-next-line unicorn/no-nested-ternary -- this is fine for now !expectedUrl ? 'wrongType' : /* otherwise */ 'mismatch', diff --git a/lib/utils.js b/lib/utils.js index d2bc6505..3129903d 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -41,7 +41,7 @@ const INTERESTING_RULE_KEYS = new Set(['create', 'meta']); * @returns Object */ function collectInterestingProperties(properties, interestingKeys) { - // eslint-disable-next-line unicorn/prefer-object-from-entries + // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now return properties.reduce((parsedProps, prop) => { const keyValue = module.exports.getKeyName(prop); if (interestingKeys.has(keyValue)) { @@ -128,7 +128,7 @@ function getRuleExportsESM(ast, scopeManager) { ].includes(statement.type) ) .map((statement) => statement.declaration || statement.expression) - // eslint-disable-next-line unicorn/prefer-object-from-entries + // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now .reduce((currentExports, node) => { if (node.type === 'ObjectExpression') { // Check `export default { create() {}, meta: {} }` @@ -184,7 +184,7 @@ function getRuleExportsCJS(ast, scopeManager) { .map((statement) => statement.expression) .filter((expression) => expression.type === 'AssignmentExpression') .filter((expression) => expression.left.type === 'MemberExpression') - // eslint-disable-next-line unicorn/prefer-object-from-entries + // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now .reduce((currentExports, node) => { if ( node.left.object.type === 'Identifier' && @@ -600,7 +600,7 @@ module.exports = { if (reportArgs.length === 1) { if (reportArgs[0].type === 'ObjectExpression') { - // eslint-disable-next-line unicorn/prefer-object-from-entries + // eslint-disable-next-line unicorn/prefer-object-from-entries -- this is fine for now return reportArgs[0].properties.reduce((reportInfo, property) => { const propName = module.exports.getKeyName(property); @@ -888,4 +888,19 @@ module.exports = { return []; }); }, + + /** + * Check whether a variable's definition is from a function parameter. + * @param {Node} node - the Identifier node for the variable. + * @param {ScopeManager} scopeManager + * @returns {boolean} whether the variable comes from a function parameter + */ + isVariableFromParameter(node, scopeManager) { + const variable = findVariable( + scopeManager.acquire(node) || scopeManager.globalScope, + node + ); + + return variable?.defs[0]?.type === 'Parameter'; + }, }; diff --git a/package.json b/package.json index cf38d742..fb7672fe 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "eslint-plugin-eslint-plugin", - "version": "5.0.3", + "version": "5.0.4", "description": "An ESLint plugin for linting ESLint plugins", "author": "Teddy Katz", "main": "./lib/index.js", @@ -54,6 +54,7 @@ "eslint": "^8.7.0", "eslint-config-not-an-aardvark": "^2.1.0", "eslint-config-prettier": "^8.3.0", + "eslint-plugin-eslint-comments": "^3.2.0", "eslint-plugin-eslint-plugin": "file:./", "eslint-plugin-markdown": "^2.0.1", "eslint-plugin-node": "^11.1.0", diff --git a/tests/lib/rules/no-missing-message-ids.js b/tests/lib/rules/no-missing-message-ids.js index 22a465dd..02cd8a05 100644 --- a/tests/lib/rules/no-missing-message-ids.js +++ b/tests/lib/rules/no-missing-message-ids.js @@ -196,6 +196,48 @@ ruleTester.run('no-missing-message-ids', rule, { } }; `, + // Helper function with messageId parameter, outside rule. + ` + function report(node, messageId) { + context.report({node, messageId}); + } + module.exports = { + meta: { messages: { foo: 'hello' } }, + create(context) { + report(node, 'foo'); + } + }; + `, + // Helper function with messageId parameter, inside rule, with parameter reassignment. + ` + module.exports = { + meta: { messages: { foo: 'hello', bar: 'world' } }, + create(context) { + function report(node, messageId) { + if (foo) { + messageId = 'bar'; + } + context.report({node, messageId}); + } + report(node, 'foo'); + } + }; + `, + // Helper function with messageId parameter, inside rule, with missing messageId. + // TODO: this should be an invalid test case because a non-existent `messageId` is used. + // Eventually, we should be able to detect what values are passed to this function for its `messageId` parameter. + ` + module.exports = { + meta: { messages: { foo: 'hello' } }, + create(context) { + function report(node, messageId) { + context.report({node, messageId}); + } + report(node, 'foo'); + report(node, 'bar'); + } + }; + `, ], invalid: [ @@ -287,5 +329,29 @@ ruleTester.run('no-missing-message-ids', rule, { }, ], }, + { + // Helper function with messageId parameter, inside rule, with missing messageId due to parameter reassignment. + code: ` + module.exports = { + meta: { messages: { foo: 'hello' } }, + create(context) { + function report(node, messageId) { + if (foo) { + messageId = 'bar'; + } + context.report({node, messageId}); + } + report(node, 'foo'); + } + }; + `, + errors: [ + { + messageId: 'missingMessage', + data: { messageId: 'bar' }, + type: 'Literal', + }, + ], + }, ], }); diff --git a/tests/lib/rules/no-unused-message-ids.js b/tests/lib/rules/no-unused-message-ids.js index d35cadfe..52e3f80c 100644 --- a/tests/lib/rules/no-unused-message-ids.js +++ b/tests/lib/rules/no-unused-message-ids.js @@ -227,6 +227,50 @@ ruleTester.run('no-unused-message-ids', rule, { create(context) {} }; `, + // Helper function messageId parameter, outside rule. + ` + function reportFoo(node, messageId) { + context.report({ node, messageId }); + } + module.exports = { + meta: { messages: { foo: 'hello', bar: 'world', baz: 'planet' } }, + create(context) { + reportFoo(node, 'foo'); + reportFoo(node, 'bar'); + reportFoo(node, 'baz'); + } + }; + `, + // Helper function with messageId parameter, inside rule, parameter reassignment. + ` + module.exports = { + meta: { messages: { foo: 'hello', bar: 'world', baz: 'planet' } }, + create(context) { + function reportFoo(node, messageId) { + if (foo) { + messageId = 'baz'; + } + context.report({ node, messageId }); + } + reportFoo(node, 'foo'); + reportFoo(node, 'bar'); + } + }; + `, + // Helper function with messageId parameter, outside rule, with an unused messageId. + // TODO: this should be an invalid test case because a messageId is unused. + // Eventually, we should be able to detect what values are passed to this function for its messageId parameter. + ` + function reportFoo(node, messageId) { + context.report({ node, messageId }); + } + module.exports = { + meta: { messages: { foo: 'hello', bar: 'world' } }, + create(context) { + reportFoo(node, 'foo'); + } + }; + `, ], invalid: [ @@ -363,7 +407,7 @@ ruleTester.run('no-unused-message-ids', rule, { context.report({ node, messageId }); } module.exports = { - meta: { messages: { foo: 'hello world' } }, + meta: { messages: { foo: 'hello world', bar: 'baz' } }, create(context) { reportFoo(node); } diff --git a/tests/lib/utils.js b/tests/lib/utils.js index 66e1b02d..ebebacca 100644 --- a/tests/lib/utils.js +++ b/tests/lib/utils.js @@ -1646,4 +1646,39 @@ describe('utils', () => { ); }); }); + + describe('isVariableFromParameter', function () { + it('returns true for function parameter', () => { + const code = + 'function myFunc(x) { if (foo) { x = "abc"; } console.log(x) }; myFunc("def");'; + const ast = espree.parse(code, { + ecmaVersion: 9, + range: true, + }); + + const scopeManager = eslintScope.analyze(ast); + assert.ok( + utils.isVariableFromParameter( + ast.body[0].body.body[1].expression.arguments[0], + scopeManager + ) + ); + }); + + it('returns false for const variable', () => { + const code = 'const x = "abc"; console.log(x);'; + const ast = espree.parse(code, { + ecmaVersion: 9, + range: true, + }); + + const scopeManager = eslintScope.analyze(ast); + assert.notOk( + utils.isVariableFromParameter( + ast.body[1].expression.arguments[0], + scopeManager + ) + ); + }); + }); });