Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): [no-unnecessary-condition] handle left-hand optional with exactOptionalPropertyTypes option#8249
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@yeonjuan! 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJan 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
f4a4bfc to00ff844Comparefcfa73f tof2145b1CompareThere 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.
The functionality & tests look good to me, thanks!
Just requesting changes on handling computed values. LMK if the prompt I gave isn't enough info to go off of?
Uh oh!
There was an error while loading.Please reload this page.
| // TODO - figure out how to handle when computed key used. | ||
| /* | ||
| declare const foo: { bar: { baz?: number }; }; | ||
| declare const baz: 'baz'; | ||
| foo.bar[baz] ??= 1; | ||
| */ |
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.
[Feature] Yeah we'll have to do something here to make this PR mergable.
Have you triedgetStaticValue? That's kind of the standard utility we've been going with.
Forreally complex cases where folks are doing funky stuff like(1 * 1) + "other" then it's fine to not handle them.
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.
Hi@JoshuaKGoldberg, While looking into handling the computed key case, I found another false positive which occurs regardless of theexactOptionalPropertyTypes.
enumKeys{A='A',B='B',}typeFoo={[Keys.A]:number|null;[Keys.B]:number;};declareconstfoo:Foo;declareconstkey:Keys;foo[key]??=1;// Unnecessary conditional, expected left-hand side of `??` operator to be possibly null or undefined. 11:1 - 11:9letbar=foo[key];bar??=1;// No Error
I think this false positive and the#6635 can be handled together, without branching based onexactOptionalPropertyTypes.
I'd like to come back to it after it's been triaged, what do you think about this false positive?
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.
Yeah that makes sense to me. Nice find!
codecovbot commentedJan 31, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@## main #8249 +/- ##==========================================+ Coverage 87.70% 87.71% +0.01%========================================== Files 397 397 Lines 13950 13963 +13 Branches 4055 4061 +6 ==========================================+ Hits 12235 12248 +13 Misses 1403 1403 Partials 312 312
Flags with carried forward coverage won't be shown.Click here to find out more.
|
…nal with exactOptionalPropertyTypes option
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.
This looks great - some tricky logic to handle, nice job getting it working! 👏
Just one typo in a function name. I'll go ahead and apply it myself to get this merged & unblock followups.
Uh oh!
There was an error while loading.Please reload this page.
…nal with exactOptionalPropertyTypes option (typescript-eslint#8249)* fix(eslint-plugin): [no-unnecessary-condition] handle left-hand optional with exactOptionalPropertyTypes option* typo: remove the 's'---------Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>
PR Checklist
??=operator andexactOptionalPropertyTypescompiler option #6635Overview
This PR fixes a false positive issue that occurs when using the exactOptionalPropertyTypes option.
Playground
tsconfig.json
{"compilerOptions": {"exactOptionalPropertyTypes":true,"strictNullChecks":true }}code.ts
When the exactOptionalPropertyTypes option is true, typescript removes
undefinedtype of the expression on the left-hand side in assignment. This seems to be the way typescript is intended to provide the functionality ofexactOptionalPropertyTypes.Playground
In this PR, I added code to get the property's Symbol from the member expression's object type and check if whether it's optional.
This implementation has a limitation in that it cannot handle well for using computed key. If it should be handled, I'll need to modify the code a bit more, and I'd love to hear feedback.