From ca7cc0ff07897e913c53c6fc7fd2ace267a15046 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Fri, 6 Sep 2024 21:17:44 +0200 Subject: [PATCH 1/7] feat: ignoreAssignmentVariable --- __tests__/always-return.js | 23 ++++++++++++ docs/rules/always-return.md | 42 +++++++++++++++++++-- rules/always-return.js | 73 +++++++++++++++++++++++++++++++++++++ 3 files changed, 135 insertions(+), 3 deletions(-) diff --git a/__tests__/always-return.js b/__tests__/always-return.js index cb888fd6..efed3f15 100644 --- a/__tests__/always-return.js +++ b/__tests__/always-return.js @@ -105,6 +105,19 @@ ruleTester.run('always-return', rule, { .finally(() => console.error('end'))`, options: [{ ignoreLastCallback: true }], }, + `hey.then(x => { window[x] = x })`, + `hey.then(x => { window.a = x })`, + `hey.then(x => { window.a.n = x })`, + `hey.then(x => { window[12] = x })`, + `hey.then(x => { window['12']["test"] = x })`, + { + code: `hey.then(x => { globalThis[x] = x })`, + options: [{ ignoreAssignmentVariable: ['globalThis'] }], + }, + { + code: `hey.then(x => { globalThis.a.x = x })`, + options: [{ ignoreAssignmentVariable: ['globalThis'] }], + }, ], invalid: [ @@ -230,5 +243,15 @@ ruleTester.run('always-return', rule, { options: [{ ignoreLastCallback: true }], errors: [{ message }], }, + { + code: `hey.then(x => { windo[x] = x })`, + options: [{ ignoreAssignmentVariable: ['window'] }], + errors: [{ message }], + }, + { + code: `hey.then(x => { windows[x] = x })`, + options: [{ ignoreAssignmentVariable: ['window'] }], + errors: [{ message }], + }, ], }) diff --git a/docs/rules/always-return.md b/docs/rules/always-return.md index d778771b..7d8e1f74 100644 --- a/docs/rules/always-return.md +++ b/docs/rules/always-return.md @@ -49,9 +49,9 @@ myPromise.then((b) => { ##### `ignoreLastCallback` -You can pass an `{ ignoreLastCallback: true }` as an option to this rule to the -last `then()` callback in a promise chain does not warn if it does not have a -`return`. Default is `false`. +You can pass an `{ ignoreLastCallback: true }` as an option to this rule so that +the last `then()` callback in a promise chain does not warn if it does not have +a `return`. Default is `false`. ```js // OK @@ -92,3 +92,39 @@ function foo() { }) } ``` + +##### `ignoreAssignmentVariable` + +You can pass an `{ ignoreAssignmentVariable: [] }` as an option to this rule +with a list of variable names so that the last `then()` callback in a promise +chain does not warn if it does an assignment to a global variable. Default is +`["window"]`. + +```js +/* eslint promise/always-return: ["error", { ignoreAssignmentVariable: ["window", "globalThis"] }] */ + +// OK +promise.then((x) => { + window.x = x +}) + +// OK +promise.then((x) => { + window.x.y = x +}) + +// OK +promise.then((x) => { + globalThis.x = x +}) + +// OK +promise.then((x) => { + globalThis.modules.x = x +}) + +// NG +promise.then((x) => { + let a = x +}) +``` diff --git a/rules/always-return.js b/rules/always-return.js index 093ea33a..8b5f4f1f 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -124,6 +124,57 @@ function peek(arr) { return arr[arr.length - 1] } +/** + * Gets the full object name for a MemberExpression or Identifier + * @param {import('estree').MemberExpression | import('estree').Identifier} node + * @returns {string} + */ +function getFullObjectName(node) { + if (node.type === 'Identifier') { + return node.name + } + const objectName = getFullObjectName(node.object) + const propertyName = node.property.name + return `${objectName}${node.computed ? '[' : '.'}${propertyName}${node.computed ? ']' : ''}` +} + +/** + * Checks if the given name starts with an ignored variable + * @param {string} name + * @param {string[]} ignoredVars + * @returns {boolean} + */ +function startsWithIgnoredVar(name, ignoredVars) { + return ignoredVars.some( + (ignoredVar) => + name === ignoredVar || + (name.startsWith(ignoredVar) && + (name.charAt(ignoredVar.length) === '.' || + name.charAt(ignoredVar.length) === '[')), + ) +} + +/** + * Checks if the node is an assignment to an ignored variable + * @param {Node} node + * @param {string[]} ignoredVars + * @returns {boolean} + */ +function isIgnoredAssignment(node, ignoredVars) { + if (node.type !== 'ExpressionStatement') return false + const expr = node.expression + if (expr.type !== 'AssignmentExpression') return false + const left = expr.left + // istanbul ignore else + if (left.type === 'MemberExpression') { + const fullName = getFullObjectName(left.object) + return startsWithIgnoredVar(fullName, ignoredVars) + } + // istanbul ignore next + // fallback + return false +} + module.exports = { meta: { type: 'problem', @@ -139,6 +190,14 @@ module.exports = { ignoreLastCallback: { type: 'boolean', }, + ignoreAssignmentVariable: { + type: 'array', + items: { + type: 'string', + pattern: '^[\\w]+$', + }, + uniqueItems: true, + }, }, additionalProperties: false, }, @@ -150,6 +209,9 @@ module.exports = { create(context) { const options = context.options[0] || {} const ignoreLastCallback = !!options.ignoreLastCallback + const ignoreAssignmentVariable = options.ignoreAssignmentVariable || [ + 'window', + ] /** * @typedef {object} FuncInfo * @property {string[]} branchIDStack This is a stack representing the currently @@ -244,6 +306,17 @@ module.exports = { return } + if (ignoreAssignmentVariable.length && isLastCallback(node)) { + // istanbul ignore else + if (node.body?.type === 'BlockStatement') { + for (const statement of node.body.body) { + if (isIgnoredAssignment(statement, ignoreAssignmentVariable)) { + return + } + } + } + } + path.finalSegments.forEach((segment) => { const id = segment.id const branch = funcInfo.branchInfoMap[id] From e45f2207e75191bd4513b53790cea852cb062c1c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Wed, 11 Sep 2024 09:11:26 +0200 Subject: [PATCH 2/7] apply suggestions --- __tests__/always-return.js | 41 +++++++++++++++++-------- docs/rules/always-return.md | 22 +++++++------ rules/always-return.js | 61 +++++++++++++------------------------ 3 files changed, 62 insertions(+), 62 deletions(-) diff --git a/__tests__/always-return.js b/__tests__/always-return.js index efed3f15..dcd9260e 100644 --- a/__tests__/always-return.js +++ b/__tests__/always-return.js @@ -105,18 +105,15 @@ ruleTester.run('always-return', rule, { .finally(() => console.error('end'))`, options: [{ ignoreLastCallback: true }], }, - `hey.then(x => { window[x] = x })`, - `hey.then(x => { window.a = x })`, - `hey.then(x => { window.a.n = x })`, - `hey.then(x => { window[12] = x })`, - `hey.then(x => { window['12']["test"] = x })`, - { - code: `hey.then(x => { globalThis[x] = x })`, - options: [{ ignoreAssignmentVariable: ['globalThis'] }], - }, - { - code: `hey.then(x => { globalThis.a.x = x })`, - options: [{ ignoreAssignmentVariable: ['globalThis'] }], + `hey.then(x => { globalThis = x })`, + `hey.then(x => { globalThis[a] = x })`, + `hey.then(x => { globalThis.a = x })`, + `hey.then(x => { globalThis.a.n = x })`, + `hey.then(x => { globalThis[12] = x })`, + `hey.then(x => { globalThis['12']["test"] = x })`, + { + code: `hey.then(x => { window['x'] = x })`, + options: [{ ignoreAssignmentVariable: ['window'] }], }, ], @@ -249,7 +246,25 @@ ruleTester.run('always-return', rule, { errors: [{ message }], }, { - code: `hey.then(x => { windows[x] = x })`, + code: `hey.then(x => { invalid = x })`, + errors: [{ message }], + }, + { + code: `hey.then(x => { invalid['x'] = x })`, + errors: [{ message }], + }, + { + code: `hey.then(x => { windo['x'] = x })`, + options: [{ ignoreAssignmentVariable: ['window'] }], + errors: [{ message }], + }, + { + code: `hey.then(x => { windows['x'] = x })`, + options: [{ ignoreAssignmentVariable: ['window'] }], + errors: [{ message }], + }, + { + code: `hey.then(x => { x() })`, options: [{ ignoreAssignmentVariable: ['window'] }], errors: [{ message }], }, diff --git a/docs/rules/always-return.md b/docs/rules/always-return.md index 7d8e1f74..81899d96 100644 --- a/docs/rules/always-return.md +++ b/docs/rules/always-return.md @@ -98,33 +98,37 @@ function foo() { You can pass an `{ ignoreAssignmentVariable: [] }` as an option to this rule with a list of variable names so that the last `then()` callback in a promise chain does not warn if it does an assignment to a global variable. Default is -`["window"]`. +`["globalThis"]`. ```js -/* eslint promise/always-return: ["error", { ignoreAssignmentVariable: ["window", "globalThis"] }] */ +/* eslint promise/always-return: ["error", { ignoreAssignmentVariable: ["globalThis"] }] */ // OK promise.then((x) => { - window.x = x + globalThis = x }) -// OK promise.then((x) => { - window.x.y = x + globalThis.x = x }) // OK promise.then((x) => { - globalThis.x = x + globalThis.x.y = x }) -// OK +// NG +promise.then((x) => { + anyOtherVariable = x +}) + +// NG promise.then((x) => { - globalThis.modules.x = x + anyOtherVariable.x = x }) // NG promise.then((x) => { - let a = x + x() }) ``` diff --git a/rules/always-return.js b/rules/always-return.js index 8b5f4f1f..c20c263d 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -125,37 +125,22 @@ function peek(arr) { } /** - * Gets the full object name for a MemberExpression or Identifier - * @param {import('estree').MemberExpression | import('estree').Identifier} node - * @returns {string} + * Gets the root object name for a MemberExpression or Identifier. + * @param {Node} node + * @returns {string | undefined} */ -function getFullObjectName(node) { +function getRootObjectName(node) { if (node.type === 'Identifier') { return node.name } - const objectName = getFullObjectName(node.object) - const propertyName = node.property.name - return `${objectName}${node.computed ? '[' : '.'}${propertyName}${node.computed ? ']' : ''}` -} - -/** - * Checks if the given name starts with an ignored variable - * @param {string} name - * @param {string[]} ignoredVars - * @returns {boolean} - */ -function startsWithIgnoredVar(name, ignoredVars) { - return ignoredVars.some( - (ignoredVar) => - name === ignoredVar || - (name.startsWith(ignoredVar) && - (name.charAt(ignoredVar.length) === '.' || - name.charAt(ignoredVar.length) === '[')), - ) + // istanbul ignore else (fallback) + if (node.type === 'MemberExpression') { + return getRootObjectName(node.object) + } } /** - * Checks if the node is an assignment to an ignored variable + * Checks if the node is an assignment to an ignored variable. Use getRootObjectName to get the variable name. * @param {Node} node * @param {string[]} ignoredVars * @returns {boolean} @@ -165,14 +150,8 @@ function isIgnoredAssignment(node, ignoredVars) { const expr = node.expression if (expr.type !== 'AssignmentExpression') return false const left = expr.left - // istanbul ignore else - if (left.type === 'MemberExpression') { - const fullName = getFullObjectName(left.object) - return startsWithIgnoredVar(fullName, ignoredVars) - } - // istanbul ignore next - // fallback - return false + const rootName = getRootObjectName(left) + return ignoredVars.includes(rootName) } module.exports = { @@ -210,8 +189,9 @@ module.exports = { const options = context.options[0] || {} const ignoreLastCallback = !!options.ignoreLastCallback const ignoreAssignmentVariable = options.ignoreAssignmentVariable || [ - 'window', + 'globalThis', ] + /** * @typedef {object} FuncInfo * @property {string[]} branchIDStack This is a stack representing the currently @@ -306,13 +286,14 @@ module.exports = { return } - if (ignoreAssignmentVariable.length && isLastCallback(node)) { - // istanbul ignore else - if (node.body?.type === 'BlockStatement') { - for (const statement of node.body.body) { - if (isIgnoredAssignment(statement, ignoreAssignmentVariable)) { - return - } + if ( + ignoreAssignmentVariable.length && + isLastCallback(node) && + node.body?.type === 'BlockStatement' + ) { + for (const statement of node.body.body) { + if (isIgnoredAssignment(statement, ignoreAssignmentVariable)) { + return } } } From 430ef10454343feec3f1bc364f117894b9dce287 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Wed, 11 Sep 2024 09:14:47 +0200 Subject: [PATCH 3/7] reorder tests --- __tests__/always-return.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/__tests__/always-return.js b/__tests__/always-return.js index dcd9260e..c6f6b325 100644 --- a/__tests__/always-return.js +++ b/__tests__/always-return.js @@ -241,16 +241,16 @@ ruleTester.run('always-return', rule, { errors: [{ message }], }, { - code: `hey.then(x => { windo[x] = x })`, - options: [{ ignoreAssignmentVariable: ['window'] }], + code: `hey.then(x => { invalid = x })`, errors: [{ message }], }, { - code: `hey.then(x => { invalid = x })`, + code: `hey.then(x => { invalid['x'] = x })`, errors: [{ message }], }, { - code: `hey.then(x => { invalid['x'] = x })`, + code: `hey.then(x => { windo[x] = x })`, + options: [{ ignoreAssignmentVariable: ['window'] }], errors: [{ message }], }, { From 1105a4abfe8720341dbb199d888adfab6e8723ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Wed, 11 Sep 2024 09:22:49 +0200 Subject: [PATCH 4/7] add test --- __tests__/always-return.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/__tests__/always-return.js b/__tests__/always-return.js index c6f6b325..7cc74de1 100644 --- a/__tests__/always-return.js +++ b/__tests__/always-return.js @@ -113,7 +113,7 @@ ruleTester.run('always-return', rule, { `hey.then(x => { globalThis['12']["test"] = x })`, { code: `hey.then(x => { window['x'] = x })`, - options: [{ ignoreAssignmentVariable: ['window'] }], + options: [{ ignoreAssignmentVariable: ['globalThis', 'window'] }], }, ], From cfa4a5eba73ff9c5ae3e23550cf25acb955d3b93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Wed, 11 Sep 2024 09:25:59 +0200 Subject: [PATCH 5/7] improve comment --- rules/always-return.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/always-return.js b/rules/always-return.js index c20c263d..0ba8df55 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -140,7 +140,7 @@ function getRootObjectName(node) { } /** - * Checks if the node is an assignment to an ignored variable. Use getRootObjectName to get the variable name. + * Checks if the node is an assignment to an ignored variable. * @param {Node} node * @param {string[]} ignoredVars * @returns {boolean} From 859a4be0a4d1dd93ee8ea1060a778e20aca74d51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?G=C3=BCrg=C3=BCn=20Day=C4=B1o=C4=9Flu?= Date: Wed, 11 Sep 2024 09:52:53 +0200 Subject: [PATCH 6/7] apply scagood's suggestion Co-authored-by: Sebastian Good <2230835+scagood@users.noreply.github.com> --- rules/always-return.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/always-return.js b/rules/always-return.js index 0ba8df55..3a9c103e 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -173,7 +173,7 @@ module.exports = { type: 'array', items: { type: 'string', - pattern: '^[\\w]+$', + pattern: '^\\w+$', }, uniqueItems: true, }, From 064637aaa784de119aad6ca77343b42335373d5b Mon Sep 17 00:00:00 2001 From: Brett Zamir Date: Thu, 17 Oct 2024 04:03:37 +0800 Subject: [PATCH 7/7] Update rules/always-return.js --- rules/always-return.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules/always-return.js b/rules/always-return.js index 3a9c103e..7d9784b8 100644 --- a/rules/always-return.js +++ b/rules/always-return.js @@ -173,7 +173,7 @@ module.exports = { type: 'array', items: { type: 'string', - pattern: '^\\w+$', + pattern: '^[\\w$]+$', }, uniqueItems: true, },