Skip to content

Commit

Permalink
fix(no-multiple-resolved): false positives when the last expression i…
Browse files Browse the repository at this point in the history 
…n a try block is a call to resolve (#384)
  • Loading branch information
@ota-meshi
ota-meshi authored Oct 19, 2022
1 parent 70f0012 commit dc51b1c
Show file tree
Hide file tree
Showing 2 changed files with 247 additions and 12 deletions.
163 changes: 163 additions & 0 deletions __tests__/no-multiple-resolved.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,74 @@ ruleTester.run('no-multiple-resolved', rule, {
resolve(value)
})
})`,
`new Promise(async (resolve, reject) => {
try {
await foo();
resolve();
} catch (error) {
reject(error);
}
})`,
`new Promise(async (resolve, reject) => {
try {
const r = await foo();
resolve(r);
} catch (error) {
reject(error);
}
})`,
`new Promise(async (resolve, reject) => {
try {
const r = await foo();
resolve(r());
} catch (error) {
reject(error);
}
})`,
`new Promise(async (resolve, reject) => {
try {
const r = await foo();
resolve(r.foo);
} catch (error) {
reject(error);
}
})`,
`new Promise(async (resolve, reject) => {
try {
const r = await foo();
resolve(new r());
} catch (error) {
reject(error);
}
})`,
`new Promise(async (resolve, reject) => {
try {
const r = await foo();
resolve(import(r));
} catch (error) {
reject(error);
}
})`,
`new Promise((resolve, reject) => {
fn(async function * () {
try {
const r = await foo();
resolve(yield r);
} catch (error) {
reject(error);
}
})
})`,
`new Promise(async (resolve, reject) => {
let a;
try {
const r = await foo();
resolve();
if(r) return;
} catch (error) {
reject(error);
}
})`,
],

invalid: [
Expand Down Expand Up @@ -306,5 +374,100 @@ ruleTester.run('no-multiple-resolved', rule, {
},
],
},
{
code: `new Promise(async (resolve, reject) => {
try {
const r = await foo();
resolve();
r();
} catch (error) {
reject(error);
}
})`,
errors: [
{
message:
'Promise should not be resolved multiple times. Promise is potentially resolved on line 4.',
line: 7,
},
],
},
{
code: `new Promise(async (resolve, reject) => {
let a;
try {
const r = await foo();
resolve();
a = r.foo;
} catch (error) {
reject(error);
}
})`,
errors: [
{
message:
'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.',
line: 8,
},
],
},
{
code: `new Promise(async (resolve, reject) => {
let a;
try {
const r = await foo();
resolve();
a = new r();
} catch (error) {
reject(error);
}
})`,
errors: [
{
message:
'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.',
line: 8,
},
],
},
{
code: `new Promise(async (resolve, reject) => {
let a;
try {
const r = await foo();
resolve();
import(r);
} catch (error) {
reject(error);
}
})`,
errors: [
{
message:
'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.',
line: 8,
},
],
},
{
code: `new Promise((resolve, reject) => {
fn(async function * () {
try {
const r = await foo();
resolve();
yield r;
} catch (error) {
reject(error);
}
})
})`,
errors: [
{
message:
'Promise should not be resolved multiple times. Promise is potentially resolved on line 5.',
line: 8,
},
],
},
],
})
96 changes: 84 additions & 12 deletions rules/no-multiple-resolved.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,25 @@ const {

/**
* @typedef {import('estree').Node} Node
* @typedef {import('estree').Expression} Expression
* @typedef {import('estree').Identifier} Identifier
* @typedef {import('estree').FunctionExpression} FunctionExpression
* @typedef {import('estree').ArrowFunctionExpression} ArrowFunctionExpression
* @typedef {import('estree').SimpleCallExpression} CallExpression
* @typedef {import('estree').MemberExpression} MemberExpression
* @typedef {import('estree').NewExpression} NewExpression
* @typedef {import('estree').ImportExpression} ImportExpression
* @typedef {import('estree').YieldExpression} YieldExpression
* @typedef {import('eslint').Rule.CodePath} CodePath
* @typedef {import('eslint').Rule.CodePathSegment} CodePathSegment
*/

/**
* An expression that can throw an error.
* see https://github.com/eslint/eslint/blob/e940be7a83d0caea15b64c1e1c2785a6540e2641/lib/linter/code-path-analysis/code-path-analyzer.js#L639-L643
* @typedef {CallExpression | MemberExpression | NewExpression | ImportExpression | YieldExpression} ThrowableExpression
*/

/**
* Iterate all previous path segments.
* @param {CodePathSegment} segment
Expand Down Expand Up @@ -150,14 +161,18 @@ class CodePathInfo {

/**
* Check all paths and return paths resolved multiple times.
* @param {PromiseCodePathContext} promiseCodePathContext
* @returns {Iterable<AlreadyResolvedData & { node: Identifier }>}
*/
*iterateReports() {
*iterateReports(promiseCodePathContext) {
const targets = [...this.segmentInfos.values()].filter(
(info) => info.resolved
)
for (const segmentInfo of targets) {
const result = this._getAlreadyResolvedData(segmentInfo.segment)
const result = this._getAlreadyResolvedData(
segmentInfo.segment,
promiseCodePathContext
)
if (result) {
yield {
node: segmentInfo.resolved,
Expand All @@ -170,14 +185,18 @@ class CodePathInfo {
/**
* Compute the previously resolved path.
* @param {CodePathSegment} segment
* @param {PromiseCodePathContext} promiseCodePathContext
* @returns {AlreadyResolvedData | null}
*/
_getAlreadyResolvedData(segment) {
if (segment.prevSegments.length === 0) {
_getAlreadyResolvedData(segment, promiseCodePathContext) {
const prevSegments = segment.prevSegments.filter(
(prev) => !promiseCodePathContext.isResolvedTryBlockCodePathSegment(prev)
)
if (prevSegments.length === 0) {
return null
}
const prevSegmentInfos = segment.prevSegments.map((prev) =>
this._getProcessedSegmentInfo(prev)
const prevSegmentInfos = prevSegments.map((prev) =>
this._getProcessedSegmentInfo(prev, promiseCodePathContext)
)
if (prevSegmentInfos.every((info) => info.resolved)) {
// If the previous paths are all resolved, the next path is also resolved.
Expand Down Expand Up @@ -246,16 +265,20 @@ class CodePathInfo {
}
/**
* @param {CodePathSegment} segment
* @param {PromiseCodePathContext} promiseCodePathContext
*/
_getProcessedSegmentInfo(segment) {
_getProcessedSegmentInfo(segment, promiseCodePathContext) {
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)
const alreadyResolvedData = this._getAlreadyResolvedData(
segment,
promiseCodePathContext
)
if (alreadyResolvedData) {
if (alreadyResolvedData.kind === 'certain') {
newInfo.resolved = alreadyResolvedData.resolved
Expand Down Expand Up @@ -291,6 +314,21 @@ class CodePathSegmentInfo {
}
}

class PromiseCodePathContext {
constructor() {
/** @type {Set<string>} */
this.resolvedSegmentIds = new Set()
}
/** @param {CodePathSegment} */
addResolvedTryBlockCodePathSegment(segment) {
this.resolvedSegmentIds.add(segment.id)
}
/** @param {CodePathSegment} */
isResolvedTryBlockCodePathSegment(segment) {
return this.resolvedSegmentIds.has(segment.id)
}
}

module.exports = {
meta: {
type: 'problem',
Expand All @@ -308,6 +346,7 @@ module.exports = {
/** @param {import('eslint').Rule.RuleContext} context */
create(context) {
const reported = new Set()
const promiseCodePathContext = new PromiseCodePathContext()
/**
* @param {Identifier} node
* @param {Identifier} resolved
Expand All @@ -327,9 +366,14 @@ module.exports = {
},
})
}
/** @param {CodePathInfo} codePathInfo */
function verifyMultipleResolvedPath(codePathInfo) {
for (const { node, resolved, kind } of codePathInfo.iterateReports()) {
/**
* @param {CodePathInfo} codePathInfo
* @param {PromiseCodePathContext} promiseCodePathContext
*/
function verifyMultipleResolvedPath(codePathInfo, promiseCodePathContext) {
for (const { node, resolved, kind } of codePathInfo.iterateReports(
promiseCodePathContext
)) {
report(node, resolved, kind)
}
}
Expand All @@ -338,6 +382,8 @@ module.exports = {
const codePathInfoStack = []
/** @type {Set<Identifier>[]} */
const resolverReferencesStack = [new Set()]
/** @type {ThrowableExpression | null} */
let lastThrowableExpression = null
return {
/** @param {FunctionExpression | ArrowFunctionExpression} node */
'FunctionExpression, ArrowFunctionExpression'(node) {
Expand Down Expand Up @@ -376,7 +422,33 @@ module.exports = {
onCodePathEnd() {
const codePathInfo = codePathInfoStack.shift()
if (codePathInfo.resolvedCount > 1) {
verifyMultipleResolvedPath(codePathInfo)
verifyMultipleResolvedPath(codePathInfo, promiseCodePathContext)
}
},
/** @param {ThrowableExpression} node */
'CallExpression, MemberExpression, NewExpression, ImportExpression, YieldExpression:exit'(
node
) {
lastThrowableExpression = node
},
/**
* @param {CodePathSegment} segment
* @param {Node} node
*/
onCodePathSegmentEnd(segment, node) {
if (
node.type === 'CatchClause' &&
lastThrowableExpression &&
lastThrowableExpression.type === 'CallExpression' &&
node.parent.type === 'TryStatement' &&
node.parent.range[0] <= lastThrowableExpression.range[0] &&
lastThrowableExpression.range[1] <= node.parent.range[1]
) {
const resolverReferences = resolverReferencesStack[0]
if (resolverReferences.has(lastThrowableExpression.callee)) {
// Mark a segment if the last expression in the try block is a call to resolve.
promiseCodePathContext.addResolvedTryBlockCodePathSegment(segment)
}
}
},
/** @type {Identifier} */
Expand Down

0 comments on commit dc51b1c

Please sign in to comment.