Rewrite error-message rule (#922)

sindresorhus · Dec 9, 2020 · 877bef9 · 877bef9
1 parent 311e4b4
commit 877bef9
Show file tree

Hide file tree

Showing 7 changed files with 295 additions and 139 deletions.
diff --git a/docs/rules/error-message.md b/docs/rules/error-message.md
@@ -1,6 +1,6 @@
-# Enforce passing a `message` value when throwing a built-in error
+# Enforce passing a `message` value when creating a built-in error
 
-This rule enforces a `message` value to be passed in when throwing an instance of a built-in `Error` object, which leads to more readable and debuggable code.
+This rule enforces a `message` value to be passed in when creating an instance of a built-in [`Error`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error) object, which leads to more readable and debuggable code.
 
 
 ## Fail
@@ -19,7 +19,6 @@ throw new TypeError();
 
 ```js
 const error = new Error();
-throw error;
 ```
 
 
@@ -35,5 +34,4 @@ throw new TypeError('Foo');
 
 ```js
 const error = new Error('Foo');
-throw error;
 ```
diff --git a/readme.md b/readme.md
@@ -104,7 +104,7 @@ Configure it in `package.json`.
 - [catch-error-name](docs/rules/catch-error-name.md) - Enforce a specific parameter name in catch clauses. *(fixable)*
 - [consistent-function-scoping](docs/rules/consistent-function-scoping.md) - Move function definitions to the highest possible scope.
 - [custom-error-definition](docs/rules/custom-error-definition.md) - Enforce correct `Error` subclassing. *(fixable)*
-- [error-message](docs/rules/error-message.md) - Enforce passing a `message` value when throwing a built-in error.
+- [error-message](docs/rules/error-message.md) - Enforce passing a `message` value when creating a built-in error.
 - [escape-case](docs/rules/escape-case.md) - Require escape sequences to use uppercase values. *(fixable)*
 - [expiring-todo-comments](docs/rules/expiring-todo-comments.md) - Add expiration conditions to TODO comments.
 - [explicit-length-check](docs/rules/explicit-length-check.md) - Enforce explicitly comparing the `length` property of a value. *(partly fixable)*

diff --git a/rules/error-message.js b/rules/error-message.js
@@ -1,14 +1,17 @@
 'use strict';
+const {getStaticValue} = require('eslint-utils');
 const getDocumentationUrl = require('./utils/get-documentation-url');
 
 const MESSAGE_ID_MISSING_MESSAGE = 'constructorMissingMessage';
 const MESSAGE_ID_EMPTY_MESSAGE = 'emptyMessage';
+const MESSAGE_ID_NOT_STRING = 'message-is-not-a-string';
 const messages = {
-	[MESSAGE_ID_MISSING_MESSAGE]: 'Pass a message to the error constructor.',
-	[MESSAGE_ID_EMPTY_MESSAGE]: 'Error message should not be an empty string.'
+	[MESSAGE_ID_MISSING_MESSAGE]: 'Pass a message to the `{{constructor}}` constructor.',
+	[MESSAGE_ID_EMPTY_MESSAGE]: 'Error message should not be an empty string.',
+	[MESSAGE_ID_NOT_STRING]: 'Error message should be a string.'
 };
 
-const errorConstructors = new Set([
+const errorConstructors = [
 	// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error
 	'Error',
 	'EvalError',
@@ -18,84 +21,61 @@ const errorConstructors = new Set([
 	'TypeError',
 	'URIError',
 	'InternalError'
-]);
+];
 
-const isReferenceAssigned = expression => {
-	if (expression.type === 'AssignmentExpression') {
-		const assignedVariable = expression.left;
-		return assignedVariable.type === 'Identifier' && assignedVariable.name;
-	}
-
-	return false;
-};
-
-const findIdentifierValues = (identifierNode, context) => {
-	const scope = context.getScope(identifierNode);
-	const declarations = scope.set.get(identifierNode.name);
-	if (declarations === undefined) {
-		return [];
-	}
-
-	const expressions = declarations.references.map(reference => reference.identifier.parent);
-	const referenceValues = [];
-	for (const expression of expressions) {
-		if (isReferenceAssigned(expression)) {
-			referenceValues.push(expression.right);
-		} else if (expression.type === 'VariableDeclarator') {
-			referenceValues.push(expression.init);
-		}
-	}
+const selector = [
+	':matches(NewExpression, CallExpression)',
+	'[callee.type="Identifier"]',
+	`:matches(${errorConstructors.map(name => `[callee.name="${name}"]`).join(', ')})`
+].join('');
+const noArgumentsExpressionSelector = `${selector}[arguments.length=0]`;
+const errorMessageSelector = `${selector}[arguments.length>0]`;
 
-	return referenceValues;
-};
-
-const isEmptyMessageString = node => {
-	return (
-		node.arguments.length > 0 &&
-		node.arguments[0].type === 'Literal' &&
-		!node.arguments[0].value
-	);
-};
-
-const reportError = (expressionNode, context) => {
-	const error = expressionNode.callee;
-	if (errorConstructors.has(error.name)) {
-		if (expressionNode.arguments.length === 0) {
+const create = context => {
+	return {
+		[noArgumentsExpressionSelector](node) {
 			context.report({
-				node: expressionNode.parent,
-				messageId: MESSAGE_ID_MISSING_MESSAGE
+				node,
+				messageId: MESSAGE_ID_MISSING_MESSAGE,
+				data: {
+					constructor: node.callee.name
+				}
 			});
-		}
+		},
+		[errorMessageSelector](expression) {
+			const [node] = expression.arguments;
 
-		if (isEmptyMessageString(expressionNode)) {
-			context.report({
-				node: expressionNode.parent,
-				messageId: MESSAGE_ID_EMPTY_MESSAGE
-			});
-		}
-	}
-};
+			// These types can't be string, and `getStaticValue` may don't know the value
+			// Add more types, if issue reported
+			if (node.type === 'ArrayExpression' || node.type === 'ObjectExpression') {
+				context.report({
+					node,
+					messageId: MESSAGE_ID_NOT_STRING
+				});
+				return;
+			}
 
-const checkErrorMessage = (node, context) => {
-	if (node.type === 'Identifier') {
-		const identifierValues = findIdentifierValues(node, context);
-		for (const node of identifierValues) {
-			checkErrorMessage(node, context);
-		}
-	} else if (node.type === 'NewExpression' || node.type === 'CallExpression') {
-		reportError(node, context);
-	}
-};
+			const result = getStaticValue(node, context.getScope());
 
-const create = context => {
-	const throwStatements = [];
-	return {
-		'ThrowStatement'(throwStatement) {
-			throwStatements.push(throwStatement);
-		},
-		'Program:exit'() {
-			for (const throwStatement of throwStatements) {
-				checkErrorMessage(throwStatement.argument, context);
+			// We don't know the value of `message`
+			if (!result) {
+				return;
+			}
+
+			const {value} = result;
+			if (typeof value !== 'string') {
+				context.report({
+					node,
+					messageId: MESSAGE_ID_NOT_STRING
+				});
+				return;
+			}
+
+			if (value === '') {
+				context.report({
+					node,
+					messageId: MESSAGE_ID_EMPTY_MESSAGE
+				});
 			}
 		}
 	};

diff --git a/test/error-message.js b/test/error-message.js
@@ -1,77 +1,67 @@
 import {outdent} from 'outdent';
 import {test} from './utils/test';
 
-const MESSAGE_ID_MISSING_MESSAGE = 'constructorMissingMessage';
-const MESSAGE_ID_EMPTY_MESSAGE = 'emptyMessage';
-
-const emptyStringError = {
-	messageId: MESSAGE_ID_EMPTY_MESSAGE
-};
-
-const noMessageError = {
-	messageId: MESSAGE_ID_MISSING_MESSAGE
-};
-
 test({
-	// TODO: Check how should the last two tests work on default parserOptions
-	testerOptions: {
-		env: {
-			es6: true
-		},
-		parserOptions: undefined
-	},
 	valid: [
 		'throw new Error(\'error\')',
 		'throw new TypeError(\'error\')',
 		'throw new MyCustomError(\'error\')',
 		'throw new MyCustomError()',
 		'throw generateError()',
-		'throw new Error(lineNumber=2)',
-		'throw new Error([])',
 		'throw foo()',
 		'throw err',
 		'throw 1',
 		outdent`
 			const err = TypeError('error');
 			throw err;
+		`,
+		// Should not check other argument
+		'new Error("message", 0, 0)',
+		// We don't know the value
+		'new Error(foo)',
+		'new Error(...foo)',
+		// #915, not a issue anymore, we don't track `ThrowStatement`
+		outdent`
+			/* global x */
+			const a = x;
+			throw x;
 		`
 	],
-
 	invalid: [
-		{
-			code: 'throw new Error()',
-			errors: [noMessageError]
-		},
-		{
-			code: 'throw Error()',
-			errors: [noMessageError]
-		},
-		{
-			code: 'throw new Error(\'\')',
-			errors: [emptyStringError]
-		},
-		{
-			code: outdent`
-			const err = new Error();
-			throw err;
-			`,
-			errors: [noMessageError]
-		},
-		{
-			code: outdent`
-			let err = 1;
-			err = new Error();
-			throw err;
-			`,
-			errors: [noMessageError]
-		},
-		{
-			code: outdent`
-			let err = new Error();
-			err = 1;
-			throw err;
-			`,
-			errors: [noMessageError]
-		}
 	]
 });
+
+test.visualize([
+	'throw new Error()',
+	'throw Error()',
+	'throw new Error(\'\')',
+	'throw new Error(``)',
+	outdent`
+		const err = new Error();
+		throw err;
+	`,
+	outdent`
+		let err = 1;
+		err = new Error();
+		throw err;
+	`,
+	outdent`
+		let err = new Error();
+		err = 1;
+		throw err;
+	`,
+	'const foo = new TypeError()',
+	'const foo = new SyntaxError()',
+	outdent`
+		const errorMessage = Object.freeze({errorMessage: 1}).errorMessage;
+		throw new Error(errorMessage)
+	`,
+	'throw new Error([])',
+	'throw new Error([foo])',
+	'throw new Error([0][0])',
+	'throw new Error({})',
+	'throw new Error({foo})',
+	'throw new Error({foo: 0}.foo)',
+	'throw new Error(lineNumber=2)',
+	'const error = new RangeError;'
+]);
diff --git a/test/integration/config.js b/test/integration/config.js
@@ -24,10 +24,7 @@ module.exports = {
 		...enableAllRules,
 
 		// This rule crashing on replace string inside `jsx` or `Unicode escape sequence`
-		'unicorn/string-content': 'off',
-
-		// #922 will fix it
-		'unicorn/error-message': 'off'
+		'unicorn/string-content': 'off'
 	},
 	overrides: [
 		{