-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [prefer-nullish-coalescing] add option ignoreBooleanCoercion
#9924
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin): [prefer-nullish-coalescing] add option ignoreBooleanCoercion
#9924
Conversation
Thanks for the PR, @developer-bandi! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
❌ Deploy Preview for typescript-eslint failed.
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 52c3596. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9924 +/- ##
==========================================
- Coverage 86.51% 86.51% -0.01%
==========================================
Files 430 430
Lines 15098 15123 +25
Branches 4384 4400 +16
==========================================
+ Hits 13062 13083 +21
- Misses 1679 1683 +4
Partials 357 357
Flags with carried forward coverage won't be shown. Click here to find out more.
|
(current.type === AST_NODE_TYPES.CallExpression && | ||
current.callee.type === AST_NODE_TYPES.Identifier && | ||
// eslint-disable-next-line @typescript-eslint/internal/prefer-ast-types-enum | ||
current.callee.name === 'Boolean') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Bug] Folks sometimes write their own function Boolean( ... ) { ... }
. This rule will need to handle that well.
Previous example: #10005 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you. I modified it by referring to the code written by @yeonjuan .
* `if (() => a || b) {}` | ||
* `if (function () { return a || b }) {}` | ||
*/ | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Testing] Good to see that the cases tripping this up are known 😄.
Also to test are other unusual things in if
s:
class
expressionsnew
expressions- Function decorators
- ...I'm drawing a blank on more spooky things at the moment, but there's a good chance more exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Because I was copying the isConditionalTest function, I forgot to change the example case. In practice, an example would be
Boolean(()=> a || b)
.
Therefore, it seems that the situations you mentioned should actually apply to the isConditionalTest function as well. Should the work be done in this pr? -
Actually, I did not exactly understand the three cases you mentioned. Could you please give an example if possible?
-
The additional case I was considering is an object. Should cases like
Boolean({test:a||b})
also be included?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey sorry for the long wait! This is a good start, but I think has some edge case handling and testing to go. 🚀
@@ -172,6 +172,34 @@ foo ?? 'a string'; | |||
|
|||
Also, if you would like to ignore all primitives types, you can set `ignorePrimitives: true`. It is equivalent to `ignorePrimitives: { string: true, number: true, bigint: true, boolean: true }`. | |||
|
|||
### `ignoreMakeBoolean` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Docs] "make" is a general word. Let's go with ignoreBooleanCoercion
:
### `ignoreMakeBoolean` | |
### `ignoreBooleanCoercion` |
Whether to ignore any make boolean type value like `Boolan`, `!` . | ||
|
||
like !, Boolean, etc. You can use this when you do not want to apply the rule to cases that make the value a boolean. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whether to ignore any make boolean type value like `Boolan`, `!` . | |
like !, Boolean, etc. You can use this when you do not want to apply the rule to cases that make the value a boolean. | |
Whether to ignore expressions that coerce a value into a boolean: `Boolean(...)` and `!(...)`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this option include allowing arguments to the !
operator? The discussion in the issue explicitly contradicts this #9080 (comment)
This is clearly my mistake. I misunderstood that the Not operator should be included in the process of understanding the context. I will remove all related code and test code. |
function isMakeBoolean( | ||
node: TSESTree.Node, | ||
context: Readonly<TSESLint.RuleContext<MessageIds, Options>>, | ||
): boolean { | ||
const parents = new Set<TSESTree.Node | null>([node]); | ||
let current = node.parent; | ||
while (current) { | ||
parents.add(current); | ||
|
||
if ( | ||
current.type === AST_NODE_TYPES.CallExpression && | ||
isBuiltInBooleanCall(current, context) | ||
) { | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the algorithm in this function definitely has some problems....
For example, the following code will be ignored by the new option
let a: string | true | undefined;
let b: string | boolean | undefined;
declare function f(x: unknown): unknown;
const x = Boolean(f(a || b));
However, I'm also noticing that the existing code works quite similarly....
typescript-eslint/packages/eslint-plugin/src/rules/prefer-nullish-coalescing.ts
Lines 414 to 449 in 723d0a8
function isConditionalTest(node: TSESTree.Node): boolean { | |
const parents = new Set<TSESTree.Node | null>([node]); | |
let current = node.parent; | |
while (current) { | |
parents.add(current); | |
if ( | |
(current.type === AST_NODE_TYPES.ConditionalExpression || | |
current.type === AST_NODE_TYPES.DoWhileStatement || | |
current.type === AST_NODE_TYPES.IfStatement || | |
current.type === AST_NODE_TYPES.ForStatement || | |
current.type === AST_NODE_TYPES.WhileStatement) && | |
parents.has(current.test) | |
) { | |
return true; | |
} | |
if ( | |
[ | |
AST_NODE_TYPES.ArrowFunctionExpression, | |
AST_NODE_TYPES.FunctionExpression, | |
].includes(current.type) | |
) { | |
/** | |
* This is a weird situation like: | |
* `if (() => a || b) {}` | |
* `if (function () { return a || b }) {}` | |
*/ | |
return false; | |
} | |
current = current.parent; | |
} | |
return false; | |
} |
So I'm wondering whether that has bugs too....
Without that context, I'd think we'd want to model this algorithm closer to what's in other rules, for example, no-floating-promises or no-extra-boolean-cast. Those rules recurse specifically on logical expressions, ??
expressions, ternaries, chain expressions, sequence expressions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
upon checking, it appears that similar code is ignored when the ignoreConditionalTests option is true playground
declare let a: string | null;
declare const b: string | null;
declare function f(x: unknown): unknown;
if (f(a || b)) {}
while (f(a || b)) {}
so i think isConditionalTest function need to modified. Would it be a good idea to open a new issue for this problem?
to modify algorithm, I looked at the no-extra-boolean-cast
code you mentioned.
considering what you mentioned, it seems like you can improve it by referring to the algorithm of the function below, but I am wondering if my understanding is correct.
https://github.com/eslint/eslint/blob/main/lib/rules/no-extra-boolean-cast.js#L117-L167
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What fun, haha. Thanks for checking on this!
I think file an issue no matter what, and it's up to you if you feel motivated and want to fix it in this PR or prefer to not worry about it in this work. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering what you mentioned, it seems like you can improve it by referring to the algorithm of the function below, but I am wondering if my understanding is correct.
Yeah, exactly, that's the pattern I'm suggesting to study 👍 (just the first thing that came to my mind, since I have previously touched that code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What fun, haha. Thanks for checking on this!
I think file an issue no matter what, and it's up to you if you feel motivated and want to fix it in this PR or
prefer to not worry about it in this work. 👍
It is assumed that similar logic will be used in isConditionalTest Fn, so �i will work on it together in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
considering what you mentioned, it seems like you can improve it by referring to the algorithm of the function below, but I am wondering if my understanding is correct.
Yeah, exactly, that's the pattern I'm suggesting to study 👍 (just the first thing that came to my mind, since I have previously touched that code)
thank you, I will look at the code in more detail and then proceed with the work.
i edit the These functions have been modified in almost the same way, changing from recursively searching if a parent exists to recursively searching only if the logical operation conditions are satisfied. The logical operation conditions are the same for both functions, but recursive search is allowed only when the result of the logical operation is used as the value. recursive is not allowed
**recursive is allowed **
For more examples, please refer to the test code. |
ignoreBooleanCoercion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for going above and beyond by fixing issues in the existing code in addition to the added functionality!
@@ -103,6 +104,11 @@ export default createRule<Options, MessageIds>({ | |||
'Whether to ignore any ternary expressions that could be simplified by using the nullish coalescing operator.', | |||
type: 'boolean', | |||
}, | |||
ignoreBooleanCoercion: { | |||
description: | |||
'Whether to ignore any make boolean type value like `Boolean`', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'Whether to ignore any make boolean type value like `Boolean`', | |
'Whether to ignore arguments to the `Boolean` constructor', |
…eslint into feature/prefer-nullish-coalescing
added commit to resolve conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
##### [v8.13.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8130-2024-11-04) ##### 🚀 Features - **eslint-plugin:** \[only-throw-error] add allow option ([#10221](typescript-eslint/typescript-eslint#10221)) - **eslint-plugin:** \[prefer-nullish-coalescing] add option `ignoreBooleanCoercion` ([#9924](typescript-eslint/typescript-eslint#9924)) - **eslint-plugin:** disable `no-class-assign` rule in `eslint-recommended` config ([#10250](typescript-eslint/typescript-eslint#10250)) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] add support for covering a missing property with `undefined` ([#10232](typescript-eslint/typescript-eslint#10232)) - **eslint-plugin:** \[consistent-type-definitions] don't leave trailing parens when fixing type to interface ([#10235](typescript-eslint/typescript-eslint#10235)) - **eslint-plugin:** \[no-deprecated] report when exported class implements/extends deprecated entity ([#10259](typescript-eslint/typescript-eslint#10259)) - **eslint-plugin:** \[no-deprecated] report on deprecated variables used inside dynamic imports ([#10261](typescript-eslint/typescript-eslint#10261)) - **eslint-plugin:** \[no-unnecessary-condition] falsey bigint should be falsey ([#10205](typescript-eslint/typescript-eslint#10205)) ##### ❤️ Thank You - auvred [@auvred](https://github.com/auvred) - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Mark de Dios [@peanutenthusiast](https://github.com/peanutenthusiast) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.12.2 | 8.13.0 | | npm | @typescript-eslint/parser | 8.12.2 | 8.13.0 | ## [v8.13.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8130-2024-11-04) ##### 🚀 Features - **eslint-plugin:** \[only-throw-error] add allow option ([#10221](typescript-eslint/typescript-eslint#10221)) - **eslint-plugin:** \[prefer-nullish-coalescing] add option `ignoreBooleanCoercion` ([#9924](typescript-eslint/typescript-eslint#9924)) - **eslint-plugin:** disable `no-class-assign` rule in `eslint-recommended` config ([#10250](typescript-eslint/typescript-eslint#10250)) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] add support for covering a missing property with `undefined` ([#10232](typescript-eslint/typescript-eslint#10232)) - **eslint-plugin:** \[consistent-type-definitions] don't leave trailing parens when fixing type to interface ([#10235](typescript-eslint/typescript-eslint#10235)) - **eslint-plugin:** \[no-deprecated] report when exported class implements/extends deprecated entity ([#10259](typescript-eslint/typescript-eslint#10259)) - **eslint-plugin:** \[no-deprecated] report on deprecated variables used inside dynamic imports ([#10261](typescript-eslint/typescript-eslint#10261)) - **eslint-plugin:** \[no-unnecessary-condition] falsey bigint should be falsey ([#10205](typescript-eslint/typescript-eslint#10205)) ##### ❤️ Thank You - auvred [@auvred](https://github.com/auvred) - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Mark de Dios [@peanutenthusiast](https://github.com/peanutenthusiast) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.12.2 | 8.13.0 | | npm | @typescript-eslint/parser | 8.12.2 | 8.13.0 | ## [v8.13.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8130-2024-11-04) ##### 🚀 Features - **eslint-plugin:** \[only-throw-error] add allow option ([#10221](typescript-eslint/typescript-eslint#10221)) - **eslint-plugin:** \[prefer-nullish-coalescing] add option `ignoreBooleanCoercion` ([#9924](typescript-eslint/typescript-eslint#9924)) - **eslint-plugin:** disable `no-class-assign` rule in `eslint-recommended` config ([#10250](typescript-eslint/typescript-eslint#10250)) ##### 🩹 Fixes - **eslint-plugin:** \[switch-exhaustiveness-check] add support for covering a missing property with `undefined` ([#10232](typescript-eslint/typescript-eslint#10232)) - **eslint-plugin:** \[consistent-type-definitions] don't leave trailing parens when fixing type to interface ([#10235](typescript-eslint/typescript-eslint#10235)) - **eslint-plugin:** \[no-deprecated] report when exported class implements/extends deprecated entity ([#10259](typescript-eslint/typescript-eslint#10259)) - **eslint-plugin:** \[no-deprecated] report on deprecated variables used inside dynamic imports ([#10261](typescript-eslint/typescript-eslint#10261)) - **eslint-plugin:** \[no-unnecessary-condition] falsey bigint should be falsey ([#10205](typescript-eslint/typescript-eslint#10205)) ##### ❤️ Thank You - auvred [@auvred](https://github.com/auvred) - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Kirk Waiblinger [@kirkwaiblinger](https://github.com/kirkwaiblinger) - Mark de Dios [@peanutenthusiast](https://github.com/peanutenthusiast) - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.
PR Checklist
ignoreConditionalTests: true
#10153Overview
I read the comments on the issue, and it seemed better to create a new option rather than adding a function to ignoreConditionalTests, so I created a new option.
The reason is that I thought it was reasonable to say that there is no case in typescript-eslint where the content raised in the issue is judged as a condition/boolean.