Add no-thenable rule (#1616)

Author: fisker

configs/recommended.js

@@ -40,6 +40,7 @@ module.exports = {
 		'unicorn/no-object-as-default-parameter': 'error',
 		'unicorn/no-process-exit': 'error',
 		'unicorn/no-static-only-class': 'error',
+		'unicorn/no-thenable': 'error',
 		'unicorn/no-this-assignment': 'error',
 		'unicorn/no-unreadable-array-destructuring': 'error',
 		'unicorn/no-unsafe-regex': 'off',

docs/rules/no-thenable.md

@@ -0,0 +1,101 @@
+# Disallow `then` property
+
+✅ *This rule is part of the [recommended](https://github.com/sindresorhus/eslint-plugin-unicorn#recommended-config) config.*
+
+If an object is defined as "thenable", once it's accidentally used in an await expression, it may cause problems:
+
+```js
+const foo = {
+	unicorn: 1,
+	then() {},
+};
+
+const {unicorn} = await foo;
+
+console.log('after'); //<- This will never execute
+```
+
+```js
+const foo = {
+	unicorn: 1,
+	then() {
+		throw new Error('You shouldn’t have called me')
+	},
+};
+
+const {unicorn} = await foo;
+// Error: You shouldn’t have called me
+```
+
+If a module has an export named `then`, dynamic `import()` may not work as expected:
+
+```js
+// foo.js
+
+export function then () {
+	throw new Error('You shouldn’t have called me')
+}
+```
+
+```js
+// bar.js
+
+const foo = await import('./foo.js');
+// Error: You shouldn’t have called me
+```
+
+## Fail
+
+```js
+export {then};
+```
+
+```js
+const foo = {
+	then() {}
+};
+```
+
+```js
+const foo = {
+	get then() {}
+};
+```
+
+```js
+foo.then = function () {}
+```
+
+```js
+class Foo {
+	then() {}
+}
+```
+
+```js
+class Foo {
+	static then() {}
+}
+```
+
+## Pass
+
+```js
+export {then as success};
+```
+
+```js
+const foo = {
+	success() {}
+};
+```
+
+```js
+class Foo {
+	success() {}
+}
+```
+
+```js
+const foo = bar.then;
+```

readme.md

@@ -74,6 +74,7 @@ Configure it in `package.json`.
 			"unicorn/no-object-as-default-parameter": "error",
 			"unicorn/no-process-exit": "error",
 			"unicorn/no-static-only-class": "error",
+			"unicorn/no-thenable": "error",
 			"unicorn/no-this-assignment": "error",
 			"unicorn/no-unreadable-array-destructuring": "error",
 			"unicorn/no-unsafe-regex": "off",
@@ -187,6 +188,7 @@ Each rule has emojis denoting:
 | [no-object-as-default-parameter](docs/rules/no-object-as-default-parameter.md) | Disallow the use of objects as default parameters. | ✅ |  |  |
 | [no-process-exit](docs/rules/no-process-exit.md) | Disallow `process.exit()`. | ✅ |  |  |
 | [no-static-only-class](docs/rules/no-static-only-class.md) | Forbid classes that only have static members. | ✅ | 🔧 |  |
+| [no-thenable](docs/rules/no-thenable.md) | Disallow `then` property. | ✅ |  |  |
 | [no-this-assignment](docs/rules/no-this-assignment.md) | Disallow assigning `this` to a variable. | ✅ |  |  |
 | [no-unreadable-array-destructuring](docs/rules/no-unreadable-array-destructuring.md) | Disallow unreadable array destructuring. | ✅ | 🔧 |  |
 | [no-unsafe-regex](docs/rules/no-unsafe-regex.md) | Disallow unsafe regular expressions. |  |  |  |

rules/no-thenable.js

@@ -0,0 +1,129 @@
+'use strict';
+const {getStaticValue} = require('eslint-utils');
+const {methodCallSelector} = require('./selectors/index.js');
+const getPropertyName = require('./utils/get-property-name.js');
+const getKeyName = require('./utils/get-key-name.js');
+
+const MESSAGE_ID_OBJECT = 'no-thenable-object';
+const MESSAGE_ID_EXPORT = 'no-thenable-export';
+const MESSAGE_ID_CLASS = 'no-thenable-class';
+const messages = {
+	[MESSAGE_ID_OBJECT]: 'Do not add `then` to an object.',
+	[MESSAGE_ID_EXPORT]: 'Do not export `then`.',
+	[MESSAGE_ID_CLASS]: 'Do not add `then` to a class.',
+};
+
+const isStringThen = (node, context) => {
+	const result = getStaticValue(node, context.getScope());
+
+	return result && result.value === 'then';
+};
+
+const cases = [
+	// `{then() {}}`,
+	// `{get then() {}}`,
+	// `{[computedKey]() {}}`,
+	// `{get [computedKey]() {}}`,
+	{
+		selector: 'ObjectExpression > Property.properties > .key',
+		test: (node, context) => getKeyName(node.parent, context.getScope()) === 'then',
+		messageId: MESSAGE_ID_OBJECT,
+	},
+	// `class Foo {then}`,
+	// `class Foo {static then}`,
+	// `class Foo {get then() {}}`,
+	// `class Foo {static get then() {}}`,
+	{
+		selector: ':matches(PropertyDefinition, MethodDefinition) > .key',
+		test: (node, context) => getKeyName(node.parent, context.getScope()) === 'then',
+		messageId: MESSAGE_ID_CLASS,
+	},
+	// `foo.then = …`
+	// `foo[computedKey] = …`
+	{
+		selector: 'AssignmentExpression > MemberExpression.left > .property',
+		test: (node, context) => getPropertyName(node.parent, context.getScope()) === 'then',
+		messageId: MESSAGE_ID_OBJECT,
+	},
+	// `Object.defineProperty(foo, 'then', …)`
+	// `Reflect.defineProperty(foo, 'then', …)`
+	{
+		selector: [
+			methodCallSelector({
+				objects: ['Object', 'Reflect'],
+				method: 'defineProperty',
+				minimumArguments: 3,
+			}),
+			'[arguments.0.type!="SpreadElement"]',
+			' > .arguments:nth-child(2)',
+		].join(''),
+		test: isStringThen,
+		messageId: MESSAGE_ID_OBJECT,
+	},
+	// `Object.fromEntries(['then', …])`
+	{
+		selector: [
+			methodCallSelector({
+				object: 'Object',
+				method: 'fromEntries',
+				argumentsLength: 1,
+			}),
+			' > ArrayExpression.arguments:nth-child(1)',
+			' > .elements:nth-child(1)',
+		].join(''),
+		test: isStringThen,
+		messageId: MESSAGE_ID_OBJECT,
+	},
+	// `export {then}`
+	{
+		selector: 'ExportSpecifier.specifiers > Identifier.exported[name="then"]',
+		messageId: MESSAGE_ID_EXPORT,
+	},
+	// `export function then() {}`,
+	// `export class then {}`,
+	{
+		selector: 'ExportNamedDeclaration > :matches(FunctionDeclaration, ClassDeclaration).declaration > Identifier[name="then"].id',
+		messageId: MESSAGE_ID_EXPORT,
+	},
+	// `export const … = …`;
+	{
+		selector: 'ExportNamedDeclaration > VariableDeclaration.declaration',
+		messageId: MESSAGE_ID_EXPORT,
+		getNodes: (node, context) => context.getDeclaredVariables(node).flatMap(({name, identifiers}) => name === 'then' ? identifiers : []),
+	},
+];
+
+/** @param {import('eslint').Rule.RuleContext} context */
+const create = context => Object.fromEntries(
+	cases.map(({selector, test, messageId, getNodes}) => [
+		selector,
+		function * (node) {
+			if (getNodes) {
+				for (const problematicNode of getNodes(node, context)) {
+					yield {node: problematicNode, messageId};
+				}
+
+				return;
+			}
+
+			if (test && !test(node, context)) {
+				return;
+			}
+
+			yield {node, messageId};
+		},
+	]),
+);
+
+/** @type {import('eslint').Rule.RuleModule} */
+module.exports = {
+	create,
+	meta: {
+		type: 'problem',
+		docs: {
+			description: 'Disallow `then` property.',
+		},
+		schema: [],
+		messages,
+	},
+};

rules/utils/get-key-name.js

@@ -0,0 +1,37 @@
+'use strict';
+const {getStaticValue} = require('eslint-utils');
+
+// TODO[@fisker]: Merge this with `./get-property-name.js`
+
+/**
+Get the key value of a node.
+
+@param {Node} node - The node.
+@param {Scope} [scope] - The scope to start finding the variable. Optional. If this scope was given, it tries to resolve identifier references which are in the given node as much as possible.
+*/
+function getKeyName(node, scope) {
+	const {type, key, computed} = node;
+
+	/* istanbul ignore next - Prevent unexpected case */
+	if (
+		type !== 'Property'
+		&& type !== 'PropertyDefinition'
+		&& type !== 'MethodDefinition'
+	) {
+		return;
+	}
+
+	if (!computed) {
+		if (key.type === 'Identifier') {
+			return key.name;
+		}
+
+		/* istanbul ignore next: It could be `PrivateIdentifier`(ESTree) or `PrivateName`(Babel) when it's in `class` */
+		return;
+	}
+
+	const result = getStaticValue(key, scope);
+	return result && result.value;
+}
+
+module.exports = getKeyName;