Skip to content

Commit

Permalink
Fix: improve detection of static arguments of context.report() in sev…
Browse files Browse the repository at this point in the history
…eral rules (eslint-community#129)

Several of our rules check the arguments of `context.report()`. This PR makes some improvements:

1. When inferring the arguments passed to `context.report()` in `utils.getReportInfo()`, take into account the static value of the second argument when available.
2. Update the rules using `utils.getReportInfo()` to also consider the static value or variable declaration of the message argument when necessary.

I noticed these issues because in a number of my plugins, we have stored rule error messages in variables like `const MESSAGE = 'do A instead of B';`, and this prevented many of our rules from operating correctly.

Example of how this improvement enables the `no-deprecated-report-api` rule to correctly autofix this code:

```js
const MESSAGE = 'do A instead of B';
module.exports = {
    create(context) {
		// Before autofix
    	context.report(theNode, MESSAGE);

		// After autofix
		context.report({node: theNode, message: MESSAGE});
	}
};
```
  • Loading branch information
bmish authored Jun 16, 2021
1 parent b4320c6 commit 6d5be9f
Show file tree
Hide file tree
Showing 13 changed files with 298 additions and 31 deletions.
2 changes: 1 addition & 1 deletion lib/rules/no-deprecated-report-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ module.exports = {
fix (fixer) {
const openingParen = sourceCode.getTokenBefore(node.arguments[0]);
const closingParen = sourceCode.getLastToken(node);
const reportInfo = utils.getReportInfo(node.arguments);
const reportInfo = utils.getReportInfo(node.arguments, context);

if (!reportInfo) {
return null;
Expand Down
17 changes: 11 additions & 6 deletions lib/rules/no-missing-placeholders.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

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

// ------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -40,21 +41,25 @@ module.exports = {
contextIdentifiers.has(node.callee.object) &&
node.callee.property.type === 'Identifier' && node.callee.property.name === 'report'
) {
const reportInfo = utils.getReportInfo(node.arguments);
const reportInfo = utils.getReportInfo(node.arguments, context);
if (!reportInfo || !reportInfo.message) {
return;
}

const messageStaticValue = getStaticValue(reportInfo.message, context.getScope());
if (
reportInfo &&
reportInfo.message &&
reportInfo.message.type === 'Literal' &&
typeof reportInfo.message.value === 'string' &&
(
(reportInfo.message.type === 'Literal' && typeof reportInfo.message.value === 'string') ||
(messageStaticValue && typeof messageStaticValue.value === 'string')
) &&
(!reportInfo.data || reportInfo.data.type === 'ObjectExpression')
) {
// Same regex as the one ESLint uses
// https://github.com/eslint/eslint/blob/e5446449d93668ccbdb79d78cc69f165ce4fde07/lib/eslint.js#L990
const PLACEHOLDER_MATCHER = /\{\{\s*([^{}]+?)\s*\}\}/g;
let match;

while ((match = PLACEHOLDER_MATCHER.exec(reportInfo.message.value))) { // eslint-disable-line no-extra-parens
while ((match = PLACEHOLDER_MATCHER.exec(reportInfo.message.value || messageStaticValue.value))) { // eslint-disable-line no-extra-parens
const matchingProperty = reportInfo.data &&
reportInfo.data.properties.find(prop => utils.getKeyName(prop) === match[1]);

Expand Down
20 changes: 13 additions & 7 deletions lib/rules/no-unused-placeholders.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

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

// ------------------------------------------------------------------------------
// Rule Definition
Expand Down Expand Up @@ -40,16 +41,21 @@ module.exports = {
contextIdentifiers.has(node.callee.object) &&
node.callee.property.type === 'Identifier' && node.callee.property.name === 'report'
) {
const reportInfo = utils.getReportInfo(node.arguments);
const reportInfo = utils.getReportInfo(node.arguments, context);
if (!reportInfo || !reportInfo.message) {
return;
}

const messageStaticValue = getStaticValue(reportInfo.message, context.getScope());
if (
reportInfo &&
reportInfo.message &&
reportInfo.message.type === 'Literal' &&
typeof reportInfo.message.value === 'string' &&
reportInfo.data && reportInfo.data.type === 'ObjectExpression'
(
(reportInfo.message.type === 'Literal' && typeof reportInfo.message.value === 'string') ||
(messageStaticValue && typeof messageStaticValue.value === 'string')
) &&
reportInfo.data &&
reportInfo.data.type === 'ObjectExpression'
) {
const message = reportInfo.message.value;
const message = reportInfo.message.value || messageStaticValue.value;
// https://github.com/eslint/eslint/blob/2874d75ed8decf363006db25aac2d5f8991bd969/lib/linter.js#L986
const PLACEHOLDER_MATCHER = /\{\{\s*([^{}]+?)\s*\}\}/g;
const placeholdersInMessage = new Set();
Expand Down
42 changes: 36 additions & 6 deletions lib/rules/prefer-placeholders.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

const utils = require('../utils');
const { findVariable } = require('eslint-utils');

// ------------------------------------------------------------------------------
// Rule Definition
Expand All @@ -26,6 +27,9 @@ module.exports = {
create (context) {
let contextIdentifiers;

const sourceCode = context.getSourceCode();
const { scopeManager } = sourceCode;

// ----------------------------------------------------------------------
// Public
// ----------------------------------------------------------------------
Expand All @@ -40,16 +44,42 @@ module.exports = {
contextIdentifiers.has(node.callee.object) &&
node.callee.property.type === 'Identifier' && node.callee.property.name === 'report'
) {
const reportInfo = utils.getReportInfo(node.arguments);
const reportInfo = utils.getReportInfo(node.arguments, context);

if (!reportInfo || !reportInfo.message) {
return;
}

let messageNode = reportInfo.message;

if (messageNode.type === 'Identifier') {
// See if we can find the variable declaration.

const variable = findVariable(
scopeManager.acquire(messageNode) || scopeManager.globalScope,
messageNode
);

if (
!variable ||
!variable.defs ||
!variable.defs[0] ||
!variable.defs[0].node ||
variable.defs[0].node.type !== 'VariableDeclarator' ||
!variable.defs[0].node.init
) {
return;
}

messageNode = variable.defs[0].node.init;
}

if (
reportInfo && reportInfo.message && (
(reportInfo.message.type === 'TemplateLiteral' && reportInfo.message.expressions.length) ||
(reportInfo.message.type === 'BinaryExpression' && reportInfo.message.operator === '+')
)
(messageNode.type === 'TemplateLiteral' && messageNode.expressions.length) ||
(messageNode.type === 'BinaryExpression' && messageNode.operator === '+')
) {
context.report({
node: reportInfo.message,
node: messageNode,
message: 'Use report message placeholders instead of string concatenation.',
});
}
Expand Down
2 changes: 1 addition & 1 deletion lib/rules/prefer-replace-text.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ module.exports = {
parent.parent.parent.type === 'CallExpression' &&
contextIdentifiers.has(parent.parent.parent.callee.object) &&
parent.parent.parent.callee.property.name === 'report' &&
utils.getReportInfo(parent.parent.parent.arguments).fix === node;
utils.getReportInfo(parent.parent.parent.arguments, context).fix === node;

funcInfo = {
upper: funcInfo,
Expand Down
7 changes: 5 additions & 2 deletions lib/rules/report-message-format.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

'use strict';

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

// ------------------------------------------------------------------------------
Expand Down Expand Up @@ -35,9 +36,11 @@ module.exports = {
* @returns {void}
*/
function processMessageNode (message) {
const staticValue = getStaticValue(message, context.getScope());
if (
(message.type === 'Literal' && typeof message.value === 'string' && !pattern.test(message.value)) ||
(message.type === 'TemplateLiteral' && message.quasis.length === 1 && !pattern.test(message.quasis[0].value.cooked))
(message.type === 'TemplateLiteral' && message.quasis.length === 1 && !pattern.test(message.quasis[0].value.cooked)) ||
(staticValue && !pattern.test(staticValue.value))
) {
context.report({
node: message,
Expand Down Expand Up @@ -75,7 +78,7 @@ module.exports = {
contextIdentifiers.has(node.callee.object) &&
node.callee.property.type === 'Identifier' && node.callee.property.name === 'report'
) {
const reportInfo = utils.getReportInfo(node.arguments);
const reportInfo = utils.getReportInfo(node.arguments, context);
const message = reportInfo && reportInfo.message;

if (!message) {
Expand Down
11 changes: 8 additions & 3 deletions lib/utils.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
'use strict';

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

/**
* Determines whether a node is a 'normal' (i.e. non-async, non-generator) function expression.
* @param {ASTNode} node The node in question
Expand Down Expand Up @@ -260,8 +262,9 @@ module.exports = {
/**
* Gets information on a report, given the arguments passed to context.report().
* @param {ASTNode[]} reportArgs The arguments passed to context.report()
* @param {Context} context
*/
getReportInfo (reportArgs) {
getReportInfo (reportArgs, context) {
// If there is exactly one argument, the API expects an object.
// Otherwise, if the second argument is a string, the arguments are interpreted as
// ['node', 'message', 'data', 'fix'].
Expand All @@ -287,15 +290,17 @@ module.exports = {

let keys;

const secondArgStaticValue = getStaticValue(reportArgs[1], context.getScope());
if (
(reportArgs[1].type === 'Literal' && typeof reportArgs[1].value === 'string') ||
(secondArgStaticValue && typeof secondArgStaticValue.value === 'string') ||
reportArgs[1].type === 'TemplateLiteral'
) {
keys = ['node', 'message', 'data', 'fix'];
} else if (
reportArgs[1].type === 'ObjectExpression' ||
reportArgs[1].type === 'ArrayExpression' ||
(reportArgs[1].type === 'Literal' && typeof reportArgs[1].value !== 'string')
(reportArgs[1].type === 'Literal' && typeof reportArgs[1].value !== 'string') ||
(secondArgStaticValue && ['object', 'number'].includes(typeof secondArgStaticValue.value))
) {
keys = ['node', 'loc', 'message', 'data', 'fix'];
} else {
Expand Down
94 changes: 94 additions & 0 deletions tests/lib/rules/no-deprecated-report-api.js
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,24 @@ ruleTester.run('no-deprecated-report-api', rule, {
}
};
`,
// With object as variable.
`
const OBJ = {node, message};
module.exports = {
create(context) {
context.report(OBJ);
}
};
`,
// With object as variable but cannot determine its value statically.
`
const OBJ = getObj();
module.exports = {
create(context) {
context.report(OBJ);
}
};
`,
],

invalid: [
Expand Down Expand Up @@ -161,6 +179,39 @@ ruleTester.run('no-deprecated-report-api', rule, {
`,
errors: [ERROR],
},
{
// With message string in variable.
code: `
const MESSAGE = 'foo';
module.exports = {
create(context) {
context.report(theNode, MESSAGE);
}
};
`,
output: `
const MESSAGE = 'foo';
module.exports = {
create(context) {
context.report({node: theNode, message: MESSAGE});
}
};
`,
errors: [ERROR],
},
{
// With message in variable but no autofix since we can't statically determine its type.
code: `
const MESSAGE = getMessage();
module.exports = {
create(context) {
context.report(theNode, MESSAGE);
}
};
`,
output: null,
errors: [ERROR],
},
{
code: `
module.exports = {
Expand Down Expand Up @@ -198,6 +249,49 @@ ruleTester.run('no-deprecated-report-api', rule, {
`,
errors: [ERROR],
},
{
// Location in variable as number.
code: `
const LOC = 5;
module.exports.create = context => {
context.report(theNode, LOC, foo, bar);
};
`,
output: `
const LOC = 5;
module.exports.create = context => {
context.report({node: theNode, loc: LOC, message: foo, data: bar});
};
`,
errors: [ERROR],
},
{
// Location in variable as object.
code: `
const LOC = { line: 1, column: 2 };
module.exports.create = context => {
context.report(theNode, LOC, foo, bar);
};
`,
output: `
const LOC = { line: 1, column: 2 };
module.exports.create = context => {
context.report({node: theNode, loc: LOC, message: foo, data: bar});
};
`,
errors: [ERROR],
},
{
// Location in variable but no autofix since we can't statically determine its type.
code: `
const LOC = getLoc();
module.exports.create = context => {
context.report(theNode, LOC, foo, bar);
};
`,
output: null,
errors: [ERROR],
},
{
code: `
module.exports = {
Expand Down
28 changes: 26 additions & 2 deletions tests/lib/rules/no-missing-placeholders.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ const RuleTester = require('eslint').RuleTester;
* @param {string} missingKey The placeholder that is missing
* @returns {object} An expected error
*/
function error (missingKey) {
return { type: 'Literal', message: `The placeholder {{${missingKey}}} does not exist.` };
function error (missingKey, type = 'Literal') {
return { type, message: `The placeholder {{${missingKey}}} does not exist.` };
}

// ------------------------------------------------------------------------------
Expand Down Expand Up @@ -114,6 +114,20 @@ ruleTester.run('no-missing-placeholders', rule, {
}
};
`,
// Message in variable.
`
const MESSAGE = 'foo {{bar}}';
module.exports = context => {
context.report(node, MESSAGE, { bar: 'baz' });
};
`,
// Message in variable but cannot statically determine its type.
`
const MESSAGE = getMessage();
module.exports = context => {
context.report(node, MESSAGE, { baz: 'qux' });
};
`,
],

invalid: [
Expand Down Expand Up @@ -166,6 +180,16 @@ ruleTester.run('no-missing-placeholders', rule, {
`,
errors: [error('bar')],
},
{
// Message in variable.
code: `
const MESSAGE = 'foo {{bar}}';
module.exports = context => {
context.report(node, MESSAGE, { baz: 'qux' });
};
`,
errors: [error('bar', 'Identifier')],
},
{
code: `
module.exports = context => {
Expand Down
Loading

0 comments on commit 6d5be9f

Please sign in to comment.