Skip to content

Improve detection of static arguments of context.report() in several rules #129

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
Jun 16, 2021
Merged

Improve detection of static arguments of context.report() in several rules #129

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
2 changes: 1 addition & 1 deletion lib/rules/no-deprecated-report-api.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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