From c01f9ce2ed7fdec8858b742a152bb2f933322b5e Mon Sep 17 00:00:00 2001 From: yosuke ota Date: Sat, 1 Oct 2022 23:42:27 +0900 Subject: [PATCH 1/3] feat: add `no-multiple-resolved` rule --- __tests__/no-multiple-resolved.js | 310 +++++++++++++++++++++ docs/rules/no-multiple-resolved.md | 32 +++ rules/lib/is-promise-constructor.js | 48 ++++ rules/no-multiple-resolved.js | 403 ++++++++++++++++++++++++++++ rules/param-names.js | 5 +- 5 files changed, 797 insertions(+), 1 deletion(-) create mode 100644 __tests__/no-multiple-resolved.js create mode 100644 docs/rules/no-multiple-resolved.md create mode 100644 rules/lib/is-promise-constructor.js create mode 100644 rules/no-multiple-resolved.js diff --git a/__tests__/no-multiple-resolved.js b/__tests__/no-multiple-resolved.js new file mode 100644 index 00000000..b49acb0f --- /dev/null +++ b/__tests__/no-multiple-resolved.js @@ -0,0 +1,310 @@ +'use strict' + +const rule = require('../rules/no-multiple-resolved') +const RuleTester = require('eslint').RuleTester +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 2020, + }, +}) + +ruleTester.run('no-multiple-resolved', rule, { + valid: [ + `new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } else { + resolve(value) + } + }) + })`, + `new Promise((resolve, reject) => { + if (error) { + reject(error) + } else { + resolve(value) + } + })`, + `new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + return + } + resolve(value) + }) + })`, + `new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } + if (!error) { + resolve(value) + } + }) + })`, + `new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } + if (error) { + return + } + resolve(value) + }) + })`, + ` + new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } + if (!error) { + // process + } else { + // process + } + if(!error) { + resolve(value) + } + }) + })`, + ` + new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + return + } + if (!error) { + // process + } else { + // process + } + + resolve(value) + }) + })`, + ], + + invalid: [ + { + code: ` + new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } + + resolve(value) + }) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.', + line: 8, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + if (error) { + reject(error) + } + + resolve(value) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 4.', + line: 7, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + reject(error) + resolve(value) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is already resolved on line 3.', + line: 4, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } + if (!error) { + // process + } else { + // process + } + + resolve(value) + }) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.', + line: 13, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + if (foo) { + if (bar) reject(error) + } + } + + resolve(value) + }) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 6.', + line: 10, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } else { + return + } + + resolve(value) + }) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is already resolved on line 5.', + line: 10, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + if(foo) { + if (error) { + reject(error) + } else { + return + } + resolve(value) + } + + resolve(value) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is already resolved on line 5.', + line: 9, + }, + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 9.', + line: 12, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + if (foo) { + reject(error) + } else { + resolve(value) + } + if(bar) { + resolve(value) + } + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is already resolved on line 4.', + line: 9, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + while (error) { + reject(error) + } + resolve(value) + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 4.', + line: 6, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + try { + reject(error) + } finally { + resolve(value) + } + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is already resolved on line 4.', + line: 6, + }, + ], + }, + { + code: ` + new Promise((resolve, reject) => { + try { + if (error) { + reject(error) + } + } finally { + resolve(value) + } + })`, + errors: [ + { + message: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.', + line: 8, + }, + ], + }, + ], +}) diff --git a/docs/rules/no-multiple-resolved.md b/docs/rules/no-multiple-resolved.md new file mode 100644 index 00000000..7f4a66fc --- /dev/null +++ b/docs/rules/no-multiple-resolved.md @@ -0,0 +1,32 @@ +# Disallow creating new promises with paths that resolve multiple times (no-multiple-resolved) + +This rule warns of paths that resolve multiple times in executor functions that +Promise constructors. + +#### Valid + +```js +new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } else { + resolve(value) + } + }) +}) +``` + +#### Invalid + +```js +new Promise((resolve, reject) => { + fn((error, value) => { + if (error) { + reject(error) + } + + resolve(value) // Both `reject` and `resolve` may be called. + }) +}) +``` diff --git a/rules/lib/is-promise-constructor.js b/rules/lib/is-promise-constructor.js new file mode 100644 index 00000000..cbf3d3d3 --- /dev/null +++ b/rules/lib/is-promise-constructor.js @@ -0,0 +1,48 @@ +/** + * Library: isPromiseConstructor + * Makes sure that an Expression node is new Promise(). + */ +'use strict' + +/** + * @typedef {import('estree').Node} Node + * @typedef {import('estree').Expression} Expression + * @typedef {import('estree').NewExpression} NewExpression + * @typedef {import('estree').FunctionExpression} FunctionExpression + * @typedef {import('estree').ArrowFunctionExpression} ArrowFunctionExpression + * + * @typedef {NewExpression & { callee: { type: 'Identifier', name: 'Promise' } }} NewPromise + * @typedef {NewPromise & { arguments: [FunctionExpression | ArrowFunctionExpression] }} NewPromiseWithInlineExecutor + * + */ +/** + * Checks whether the given node is new Promise(). + * @param {Node} node + * @returns {node is NewPromise} + */ +function isPromiseConstructor(node) { + return ( + node.type === 'NewExpression' && + node.callee.type === 'Identifier' && + node.callee.name === 'Promise' + ) +} + +/** + * Checks whether the given node is new Promise(() => {}). + * @param {Node} node + * @returns {node is NewPromiseWithInlineExecutor} + */ +function isPromiseConstructorWithInlineExecutor(node) { + return ( + isPromiseConstructor(node) && + node.arguments.length === 1 && + (node.arguments[0].type === 'FunctionExpression' || + node.arguments[0].type === 'ArrowFunctionExpression') + ) +} + +module.exports = { + isPromiseConstructor, + isPromiseConstructorWithInlineExecutor, +} diff --git a/rules/no-multiple-resolved.js b/rules/no-multiple-resolved.js new file mode 100644 index 00000000..cd6e64c6 --- /dev/null +++ b/rules/no-multiple-resolved.js @@ -0,0 +1,403 @@ +/** + * Rule: no-multiple-resolved + * Disallow creating new promises with paths that resolve multiple times + */ + +'use strict' + +const getDocsUrl = require('./lib/get-docs-url') +const { + isPromiseConstructorWithInlineExecutor, +} = require('./lib/is-promise-constructor') + +/** + * @typedef {import('estree').Node} Node + * @typedef {import('estree').Identifier} Identifier + * @typedef {import('estree').FunctionExpression} FunctionExpression + * @typedef {import('estree').ArrowFunctionExpression} ArrowFunctionExpression + * @typedef {import('estree').SimpleCallExpression} CallExpression + * @typedef {import('eslint').Rule.CodePath} CodePath + * @typedef {import('eslint').Rule.CodePathSegment} CodePathSegment + */ + +/** + * Iterate all previous path segments. + * @param {CodePathSegment} segment + * @returns {Iterable} + */ +function* iterateAllPrevPathSegments(segment) { + yield* iterate(segment, []) + + /** + * @param {CodePathSegment} segment + * @param {CodePathSegment[]} processed + */ + function* iterate(segment, processed) { + if (processed.includes(segment)) { + return + } + const nextProcessed = [segment, ...processed] + + for (const prev of segment.prevSegments) { + if (prev.prevSegments.length === 0) { + yield [prev] + } else { + for (const segments of iterate(prev, nextProcessed)) { + yield [prev, ...segments] + } + } + } + } +} +/** + * Iterate all next path segments. + * @param {CodePathSegment} segment + * @returns {Iterable} + */ +function* iterateAllNextPathSegments(segment) { + yield* iterate(segment, []) + + /** + * @param {CodePathSegment} segment + * @param {CodePathSegment[]} processed + */ + function* iterate(segment, processed) { + if (processed.includes(segment)) { + return + } + const nextProcessed = [segment, ...processed] + + for (const next of segment.nextSegments) { + if (next.nextSegments.length === 0) { + yield [next] + } else { + for (const segments of iterate(next, nextProcessed)) { + yield [next, ...segments] + } + } + } + } +} + +/** + * Finds the same route path from the given path following previous path segments. + * @param {CodePathSegment} segment + * @returns {CodePathSegment | null} + */ +function findSameRoutePathSegment(segment) { + /** @type {Set} */ + const routeSegments = new Set() + for (const route of iterateAllPrevPathSegments(segment)) { + if (routeSegments.size === 0) { + for (const seg of route) { + routeSegments.add(seg) + } + continue + } + for (const seg of routeSegments) { + if (!route.includes(seg)) { + routeSegments.delete(seg) + } + } + } + // istanbul ignore next -- Usually always present. + if (routeSegments.size === 0) { + return null + } + for (const routeSegment of routeSegments) { + let hasUnreached = false + for (const segments of iterateAllNextPathSegments(routeSegment)) { + if (!segments.includes(segment)) { + // It has a route that does not reach the given path. + hasUnreached = true + break + } + } + if (!hasUnreached) { + return routeSegment + } + } + return null +} + +class CodePathInfo { + /** + * @param {CodePath} path + */ + constructor(path) { + this.path = path + /** @type {Map} */ + this.segmentInfos = new Map() + this.resolvedCount = 0 + /** @type {CodePathSegment[]} */ + this.allSegments = [] + } + + getCurrentSegmentInfos() { + return this.path.currentSegments.map((segment) => { + const info = this.segmentInfos.get(segment) + if (info) { + return info + } + const newInfo = new CodePathSegmentInfo(this, segment) + this.segmentInfos.set(segment, newInfo) + return newInfo + }) + } + /** + * @typedef {object} AlreadyResolvedData + * @property {Identifier} resolved + * @property {'certain' | 'potential'} kind + */ + + /** + * Check all paths and return paths resolved multiple times. + * @returns {Iterable} + */ + *iterateReports() { + const targets = [...this.segmentInfos.values()].filter( + (info) => info.resolved + ) + for (const segmentInfo of targets) { + const result = this._getAlreadyResolvedData(segmentInfo.segment) + if (result) { + yield { + node: segmentInfo.resolved, + resolved: result.resolved, + kind: result.kind, + } + } + } + } + /** + * Compute the previously resolved path. + * @param {CodePathSegment} segment + * @returns {AlreadyResolvedData | null} + */ + _getAlreadyResolvedData(segment) { + if (segment.prevSegments.length === 0) { + return null + } + const prevSegmentInfos = segment.prevSegments.map((prev) => + this._getProcessedSegmentInfo(prev) + ) + if (prevSegmentInfos.every((info) => info.resolved)) { + // If the previous paths are all resolved, the next path is also resolved. + return { + resolved: prevSegmentInfos[0].resolved, + kind: 'certain', + } + } + + for (const prevSegmentInfo of prevSegmentInfos) { + if (prevSegmentInfo.resolved) { + // If the previous path is partially resolved, + // then the next path is potentially resolved. + return { + resolved: prevSegmentInfo.resolved, + kind: 'potential', + } + } + if (prevSegmentInfo.potentiallyResolved) { + let potential = false + if (prevSegmentInfo.segment.nextSegments.length === 1) { + // If the previous path is potentially resolved and there is one next path, + // then the next path is potentially resolved. + potential = true + } else { + // This is necessary, for example, if `resolve()` in the finally section. + const segmentInfo = this.segmentInfos.get(segment) + if (segmentInfo && segmentInfo.resolved) { + if ( + prevSegmentInfo.segment.nextSegments.every((next) => { + const nextSegmentInfo = this.segmentInfos.get(next) + return ( + nextSegmentInfo && + nextSegmentInfo.resolved === segmentInfo.resolved + ) + }) + ) { + // If the previous path is potentially resolved and + // the next paths all point to the same resolved node, + // then the next path is potentially resolved. + potential = true + } + } + } + + if (potential) { + return { + resolved: prevSegmentInfo.potentiallyResolved, + kind: 'potential', + } + } + } + } + + const sameRoute = findSameRoutePathSegment(segment) + if (sameRoute) { + const sameRouteSegmentInfo = this._getProcessedSegmentInfo(sameRoute) + if (sameRouteSegmentInfo.potentiallyResolved) { + return { + resolved: sameRouteSegmentInfo.potentiallyResolved, + kind: 'potential', + } + } + } + return null + } + /** + * @param {CodePathSegment} segment + */ + _getProcessedSegmentInfo(segment) { + const segmentInfo = this.segmentInfos.get(segment) + if (segmentInfo) { + return segmentInfo + } + const newInfo = new CodePathSegmentInfo(this, segment) + this.segmentInfos.set(segment, newInfo) + + const alreadyResolvedData = this._getAlreadyResolvedData(segment) + if (alreadyResolvedData) { + if (alreadyResolvedData.kind === 'certain') { + newInfo.resolved = alreadyResolvedData.resolved + } else { + newInfo.potentiallyResolved = alreadyResolvedData.resolved + } + } + return newInfo + } +} + +class CodePathSegmentInfo { + /** + * @param {CodePathInfo} pathInfo + * @param {CodePathSegment} segment + */ + constructor(pathInfo, segment) { + this.pathInfo = pathInfo + this.segment = segment + /** @type {Identifier | null} */ + this._resolved = null + /** @type {Identifier | null} */ + this.potentiallyResolved = null + } + + get resolved() { + return this._resolved + } + /** @type {Identifier} */ + set resolved(identifier) { + this._resolved = identifier + this.pathInfo.resolvedCount++ + } +} + +module.exports = { + meta: { + type: 'problem', + docs: { + url: getDocsUrl('no-multiple-resolved'), + }, + messages: { + alreadyResolved: + 'Promise should not be resolved multiple times. Promise is already resolved on line {{line}}.', + potentiallyAlreadyResolved: + 'Promise should not be resolved multiple times. Promise is potentially resolved on line {{line}}.', + }, + schema: [], + }, + /** @param {import('eslint').Rule.RuleContext} context */ + create(context) { + const reported = new Set() + /** + * @param {Identifier} node + * @param {Identifier} resolved + * @param {'certain' | 'potential'} kind + */ + function report(node, resolved, kind) { + if (reported.has(node)) { + return + } + reported.add(node) + context.report({ + node: node.parent, + messageId: + kind === 'certain' ? 'alreadyResolved' : 'potentiallyAlreadyResolved', + data: { + line: resolved.loc.start.line, + }, + }) + } + /** @param {CodePathInfo} codePathInfo */ + function verifyMultipleResolvedPath(codePathInfo) { + for (const { node, resolved, kind } of codePathInfo.iterateReports()) { + report(node, resolved, kind) + } + } + + /** @type {CodePathInfo[]} */ + const codePathInfoStack = [] + /** @type {Set[]} */ + const resolverReferencesStack = [new Set()] + return { + /** @param {FunctionExpression | ArrowFunctionExpression} node */ + 'FunctionExpression, ArrowFunctionExpression'(node) { + if (!isPromiseConstructorWithInlineExecutor(node.parent)) { + return + } + // Collect and stack `resolve` and `reject` references. + /** @type {Set} */ + const resolverReferences = new Set() + const resolvers = node.params.filter( + /** @returns {node is Identifier} */ + (node) => node && node.type === 'Identifier' + ) + for (const resolver of resolvers) { + const variable = context.getScope().set.get(resolver.name) + // istanbul ignore next -- Usually always present. + if (!variable) continue + for (const reference of variable.references) { + resolverReferences.add(reference.identifier) + } + } + + resolverReferencesStack.unshift(resolverReferences) + }, + /** @param {FunctionExpression | ArrowFunctionExpression} node */ + 'FunctionExpression, ArrowFunctionExpression:exit'(node) { + if (!isPromiseConstructorWithInlineExecutor(node.parent)) { + return + } + resolverReferencesStack.shift() + }, + /** @param {CodePath} path */ + onCodePathStart(path) { + codePathInfoStack.unshift(new CodePathInfo(path)) + }, + onCodePathEnd() { + const codePathInfo = codePathInfoStack.shift() + if (codePathInfo.resolvedCount > 1) { + verifyMultipleResolvedPath(codePathInfo) + } + }, + /** @type {Identifier} */ + 'CallExpression > Identifier.callee'(node) { + const codePathInfo = codePathInfoStack[0] + const resolverReferences = resolverReferencesStack[0] + if (!resolverReferences.has(node)) { + return + } + for (const segmentInfo of codePathInfo.getCurrentSegmentInfos()) { + // If a resolving path is found, report if the path is already resolved. + // Store the information if it is not already resolved. + if (segmentInfo.resolved) { + report(node, segmentInfo.resolved, 'certain') + continue + } + segmentInfo.resolved = node + } + }, + } + }, +} diff --git a/rules/param-names.js b/rules/param-names.js index ab53a52e..354a78ce 100644 --- a/rules/param-names.js +++ b/rules/param-names.js @@ -1,6 +1,9 @@ 'use strict' const getDocsUrl = require('./lib/get-docs-url') +const { + isPromiseConstructorWithInlineExecutor, +} = require('./lib/is-promise-constructor') module.exports = { meta: { @@ -13,7 +16,7 @@ module.exports = { create(context) { return { NewExpression(node) { - if (node.callee.name === 'Promise' && node.arguments.length === 1) { + if (isPromiseConstructorWithInlineExecutor(node)) { const params = node.arguments[0].params if (!params || !params.length) { From 1c618e577d524e170bbe622c7d7a4670b40b00eb Mon Sep 17 00:00:00 2001 From: yosuke ota Date: Sat, 1 Oct 2022 23:44:25 +0900 Subject: [PATCH 2/3] fix index.js --- index.js | 1 + 1 file changed, 1 insertion(+) diff --git a/index.js b/index.js index 08008d29..40b81054 100644 --- a/index.js +++ b/index.js @@ -16,6 +16,7 @@ module.exports = { 'no-new-statics': require('./rules/no-new-statics'), 'no-return-in-finally': require('./rules/no-return-in-finally'), 'valid-params': require('./rules/valid-params'), + 'no-multiple-resolved': require('./rules/no-multiple-resolved'), }, rulesConfig: { 'param-names': 1, From 7574293174d6857bad55bce9f9c053de0474364d Mon Sep 17 00:00:00 2001 From: yosuke ota Date: Sun, 2 Oct 2022 00:07:50 +0900 Subject: [PATCH 3/3] minor refactor --- rules/no-multiple-resolved.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/rules/no-multiple-resolved.js b/rules/no-multiple-resolved.js index cd6e64c6..8e6f9f64 100644 --- a/rules/no-multiple-resolved.js +++ b/rules/no-multiple-resolved.js @@ -89,6 +89,7 @@ function findSameRoutePathSegment(segment) { const routeSegments = new Set() for (const route of iterateAllPrevPathSegments(segment)) { if (routeSegments.size === 0) { + // First for (const seg of route) { routeSegments.add(seg) } @@ -100,10 +101,7 @@ function findSameRoutePathSegment(segment) { } } } - // istanbul ignore next -- Usually always present. - if (routeSegments.size === 0) { - return null - } + for (const routeSegment of routeSegments) { let hasUnreached = false for (const segments of iterateAllNextPathSegments(routeSegment)) {