no-instanceof-builtin-object: Rename to no-instanceof-builtins and use suggestions for unsafe cases (#2537)

Author: fisker

docs/deprecated-rules.md

@@ -14,7 +14,7 @@ This rule was renamed to [`no-array-callback-reference`](rules/no-array-callback
 
 ## no-instanceof-array
 
-Replaced by [`no-instanceof-builtin-object`](rules/no-instanceof-builtin-object) which covers more cases.
+Replaced by [`no-instanceof-builtins`](rules/no-instanceof-builtins.md) which covers more cases.
 
 ## no-reduce
 

docs/rules/no-instanceof-builtin-object.md renamed to docs/rules/no-instanceof-builtins.md

@@ -2,7 +2,7 @@
 
 💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/sindresorhus/eslint-plugin-unicorn#preset-configs-eslintconfigjs).
 
-🔧 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix).
+🔧💡 This rule is automatically fixable by the [`--fix` CLI option](https://eslint.org/docs/latest/user-guide/command-line-interface#--fix) and manually fixable by [editor suggestions](https://eslint.org/docs/latest/use/core-concepts#rule-suggestions).
 
 <!-- end auto-generated rule header -->
 <!-- Do not manually modify this header. Run: `npm run fix:eslint-docs` -->
@@ -73,7 +73,7 @@ The matching strategy:
 - `'strict'` - Matches all built-in constructors.
 
 ```js
-"unicorn/no-instanceof-builtin-object": [
+"unicorn/no-instanceof-builtins": [
 	"error",
 	{
 		"strategy": "strict"
@@ -89,7 +89,7 @@ Default: `[]`
 Specify the constructors that should be validated.
 
 ```js
-"unicorn/no-instanceof-builtin-object": [
+"unicorn/no-instanceof-builtins": [
 	"error",
 	{
 		"include": [
@@ -108,7 +108,7 @@ Default: `[]`
 Specifies the constructors that should be excluded, with this rule taking precedence over others.
 
 ```js
-"unicorn/no-instanceof-builtin-object": [
+"unicorn/no-instanceof-builtins": [
 	"error",
 	{
 		"exclude": [
@@ -127,7 +127,7 @@ Default: `false`
 Specifies using [`Error.isError()`](https://github.com/tc39/proposal-is-error) to determine whether it is an error object.
 
 ```js
-"unicorn/no-instanceof-builtin-object": [
+"unicorn/no-instanceof-builtins": [
 	"error",
 	{
 		"strategy": "strict",

index.js

@@ -5,7 +5,7 @@ import packageJson from './package.json' with {type: 'json'};
 
 const deprecatedRules = createDeprecatedRules({
 	// {ruleId: ReplacementRuleId | ReplacementRuleId[]}, if no replacement, use `{ruleId: []}`
-	'no-instanceof-array': 'unicorn/no-instanceof-builtin-object',
+	'no-instanceof-array': 'unicorn/no-instanceof-builtins',
 });
 
 const externalRules = {

package.json

@@ -34,6 +34,7 @@
 		"lint:js": "eslint",
 		"lint:markdown": "markdownlint \"**/*.md\"",
 		"lint:package-json": "npmPkgJsonLint .",
+		"rename-rule": "node ./scripts/rename-rule.js && npm run create-rules-index-file && npm run fix:eslint-docs",
 		"run-rules-on-codebase": "eslint --config=./eslint.dogfooding.config.js",
 		"smoke": "eslint-remote-tester --config ./test/smoke/eslint-remote-tester.config.js",
 		"test": "npm-run-all --continue-on-error lint test:*",

readme.md

@@ -87,7 +87,7 @@ export default [
 | [no-empty-file](docs/rules/no-empty-file.md)                                                     | Disallow empty files.                                                                                                                                                                                             | ✅  |    |    |
 | [no-for-loop](docs/rules/no-for-loop.md)                                                         | Do not use a `for` loop that can be replaced with a `for-of` loop.                                                                                                                                                | ✅  | 🔧 | 💡 |
 | [no-hex-escape](docs/rules/no-hex-escape.md)                                                     | Enforce the use of Unicode escapes instead of hexadecimal escapes.                                                                                                                                                | ✅  | 🔧 |    |
-| [no-instanceof-builtin-object](docs/rules/no-instanceof-builtin-object.md)                       | Disallow `instanceof` with built-in objects                                                                                                                                                                       | ✅  | 🔧 |    |
+| [no-instanceof-builtins](docs/rules/no-instanceof-builtins.md)                                   | Disallow `instanceof` with built-in objects                                                                                                                                                                       | ✅  | 🔧 | 💡 |
 | [no-invalid-fetch-options](docs/rules/no-invalid-fetch-options.md)                               | Disallow invalid options in `fetch()` and `new Request()`.                                                                                                                                                        | ✅  |    |    |
 | [no-invalid-remove-event-listener](docs/rules/no-invalid-remove-event-listener.md)               | Prevent calling `EventTarget#removeEventListener()` with the result of an expression.                                                                                                                             | ✅  |    |    |
 | [no-keyword-prefix](docs/rules/no-keyword-prefix.md)                                             | Disallow identifiers starting with `new` or `class`.                                                                                                                                                              |    |    |    |

rules/index.js

@@ -32,7 +32,7 @@ import noDocumentCookie from './no-document-cookie.js';
 import noEmptyFile from './no-empty-file.js';
 import noForLoop from './no-for-loop.js';
 import noHexEscape from './no-hex-escape.js';
-import noInstanceofBuiltinObject from './no-instanceof-builtin-object.js';
+import noInstanceofBuiltins from './no-instanceof-builtins.js';
 import noInvalidFetchOptions from './no-invalid-fetch-options.js';
 import noInvalidRemoveEventListener from './no-invalid-remove-event-listener.js';
 import noKeywordPrefix from './no-keyword-prefix.js';
@@ -160,7 +160,7 @@ const rules = {
 	'no-empty-file': createRule(noEmptyFile, 'no-empty-file'),
 	'no-for-loop': createRule(noForLoop, 'no-for-loop'),
 	'no-hex-escape': createRule(noHexEscape, 'no-hex-escape'),
-	'no-instanceof-builtin-object': createRule(noInstanceofBuiltinObject, 'no-instanceof-builtin-object'),
+	'no-instanceof-builtins': createRule(noInstanceofBuiltins, 'no-instanceof-builtins'),
 	'no-invalid-fetch-options': createRule(noInvalidFetchOptions, 'no-invalid-fetch-options'),
 	'no-invalid-remove-event-listener': createRule(noInvalidRemoveEventListener, 'no-invalid-remove-event-listener'),
 	'no-keyword-prefix': createRule(noKeywordPrefix, 'no-keyword-prefix'),

rules/no-instanceof-builtin-object.js renamed to rules/no-instanceof-builtins.js

@@ -6,22 +6,22 @@ import typedArray from './shared/typed-array.js';
 
 const isInstanceofToken = token => token.value === 'instanceof' && token.type === 'Keyword';
 
-const MESSAGE_ID = 'no-instanceof-builtin-object';
+const MESSAGE_ID = 'no-instanceof-builtins';
+const MESSAGE_ID_SWITCH_TO_TYPE_OF = 'switch-to-type-of';
 const messages = {
 	[MESSAGE_ID]: 'Avoid using `instanceof` for type checking as it can lead to unreliable results.',
+	[MESSAGE_ID_SWITCH_TO_TYPE_OF]: 'Switch to `typeof … === \'{{type}}\'`.',
 };
 
-const looseStrategyConstructors = new Set([
+const primitiveWrappers = new Set([
 	'String',
 	'Number',
 	'Boolean',
 	'BigInt',
 	'Symbol',
-	'Array',
-	'Function',
 ]);
 
-const strictStrategyConstructors = new Set([
+const strictStrategyConstructors = [
 	// Error types
 	...builtinErrors,
 
@@ -51,7 +51,7 @@ const strictStrategyConstructors = new Set([
 	'Date',
 	'SharedArrayBuffer',
 	'FinalizationRegistry',
-]);
+];
 
 const replaceWithFunctionCall = (node, sourceCode, functionName) => function * (fixer) {
 	const {tokenStore, instanceofToken} = getInstanceOfToken(sourceCode, node);
@@ -111,6 +111,12 @@ const create = context => {
 		exclude = [],
 	} = context.options[0] ?? {};
 
+	const forbiddenConstructors = new Set(
+		strategy === 'strict'
+			? [...strictStrategyConstructors, ...include]
+			: include,
+	);
+
 	const {sourceCode} = context;
 
 	return {
@@ -122,9 +128,7 @@ const create = context => {
 				return;
 			}
 
-			if (!looseStrategyConstructors.has(right.name) && !strictStrategyConstructors.has(right.name) && !include.includes(right.name)) {
-				return;
-			}
+			const constructorName = right.name;
 
 			/** @type {import('eslint').Rule.ReportDescriptor} */
 			const problem = {
@@ -133,22 +137,31 @@ const create = context => {
 			};
 
 			if (
-				right.name === 'Array'
-				|| (right.name === 'Error' && useErrorIsError)
+				constructorName === 'Array'
+				|| (constructorName === 'Error' && useErrorIsError)
 			) {
-				const functionName = right.name === 'Array' ? 'Array.isArray' : 'Error.isError';
+				const functionName = constructorName === 'Array' ? 'Array.isArray' : 'Error.isError';
 				problem.fix = replaceWithFunctionCall(node, sourceCode, functionName);
 				return problem;
 			}
 
-			// Loose strategy by default
-			if (looseStrategyConstructors.has(right.name)) {
+			if (constructorName === 'Function') {
 				problem.fix = replaceWithTypeOfExpression(node, sourceCode);
 				return problem;
 			}
 
-			// Strict strategy
-			if (strategy !== 'strict' && include.length === 0) {
+			if (primitiveWrappers.has(constructorName)) {
+				problem.suggest = [
+					{
+						messageId: MESSAGE_ID_SWITCH_TO_TYPE_OF,
+						data: {type: constructorName.toLowerCase()},
+						fix: replaceWithTypeOfExpression(node, sourceCode),
+					},
+				];
+				return problem;
+			}
+
+			if (!forbiddenConstructors.has(constructorName)) {
 				return;
 			}
 
@@ -204,6 +217,7 @@ const config = {
 			include: [],
 			exclude: [],
 		}],
+		hasSuggestions: true,
 		messages,
 	},
 };

scripts/rename-rule.js

@@ -75,7 +75,7 @@ const run = async () => {
 		return;
 	}
 
-	if (Reflect.has(rules, ruleId)) {
+	if (rules.includes(ruleId)) {
 		console.log(`${ruleId} already exists.`);
 		return;
 	}

test/no-instanceof-builtin-object.js renamed to test/no-instanceof-builtins.js

@@ -33,7 +33,7 @@ const RULES_WITHOUT_PASS_FAIL_SECTIONS = new Set([
 	'prefer-math-min-max',
 	'consistent-existence-index-check',
 	'prefer-global-this',
-	'no-instanceof-builtin-object',
+	'no-instanceof-builtins',
 	'no-named-default',
 	'consistent-assert',
 	'no-accessor-recursion',