fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

Josh-Cena · 2023-03-24T21:16:57Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unnecessary-condition] warns about conditional assignment to index signature #6632
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

typescript-eslint · 2023-03-24T21:17:00Z

Thanks for the PR, @Josh-Cena!

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.

netlify · 2023-03-24T21:17:15Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`fd1488a`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/641e13f5c2740d0008b88042
😎 Deploy Preview	https://deploy-preview-6762--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

… signatures

nx-cloud · 2023-03-24T21:19:47Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit fd1488a. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 25 targets

Sent with 💌 from NxCloud.

Josh-Cena · 2023-03-24T21:21:13Z

packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts

-function getElem(dict: Record<string, { foo: string }>, key: string) {
-  if (dict[key]) {
-    return dict[key].foo;
-  } else {
-    return '';
-  }
-}


This was arguably a bug fix in the existing test case, because it shouldn't be reported either.

This actually should be reported because the fixture config doesn't use noUncheckedIndexAccess!

I think we should probably guard the new code around whether or not the flag is on.

@bradzacher This code is the exact same principle as dictionary[key] ??= defaultVal. If we want to ignore that, we have to ignore others as well.

They are and they aren't. You're correct in that they're both record index access in a logical position - so they're semantically the same case. However TS reports the types differently for the two cases.

In this instance TS will report the read type, meaning that if noUncheckedIndexAccess is turned on then TS will report the type as T | undefimed to us.

However in x ?? y, TS will report the write type, meaning it reports just T - no matter whether or not noUncheckedIndexAccess is on or not.

This is the fundamental difference between the two cases and why we are false positiving on the assignment operators, but not on this case.

However, we already ignore array index access, even without noUncheckedIndexAccess. See this test case:

declare const arr: { foo: number }[]; const bar = arr[42]?.foo ?? 0;

I find it quite natural to extend the scope to all index signatures, since there isn't something special about array indices.

The reason we did it for arrays and not objects is because defining arrays as (T | undefined)[] is similar to Record<string, T | undefined>, but far, far less ergonomic syntactically and is arguably worse given how common it is to iterate arrays directly using an iterator method or for/of.

For objects it's pretty natural to union in undefined because most usecases for reading is an index signature - the direct iteration methods (Object.entries / Object.values) are both rarer usecases so the undefined doesn't get in the way as much.

Now that we have noUncheckedIndexAccess - we don't really need the logic at all, but I understand that some people think the compiler option goes too far and can get in the way (which is true to a degree).

The motivation behind this change isn't bringing the two sets up to parity (that's a much larger change that we should discuss as a group to decide if we think we should treat objects the same as arrays in this regard) - instead it's just fixing the current false positives we have due to the logical operator handling we added in #6594

codecov · 2023-03-24T21:33:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (main@bc31078). Click here to learn what that means.
Report is 873 commits behind head on main.

❗ Current head 4ac41b6 differs from pull request most recent head fd1488a. Consider uploading reports for the commit fd1488a to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6762   +/-   ##
=======================================
  Coverage        ?   87.30%           
=======================================
  Files           ?      384           
  Lines           ?    13148           
  Branches        ?     3863           
=======================================
  Hits            ?    11479           
  Misses          ?     1302           
  Partials        ?      367

Flag	Coverage Δ
unittest	`87.30% <100.00%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...slint-plugin/src/rules/no-unnecessary-condition.ts	`98.45% <100.00%> (ø)`

bradzacher

Thanks for getting straight on this!
Almost there I think!

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

bradzacher · 2023-03-26T09:18:46Z

packages/eslint-plugin/tests/rules/no-unnecessary-condition.test.ts

-function getElem(dict: Record<string, { foo: string }>, key: string) {
-  if (dict[key]) {
-    return dict[key].foo;
-  } else {
-    return '';
-  }
-}


This actually should be reported because the fixture config doesn't use noUncheckedIndexAccess!

I think we should probably guard the new code around whether or not the flag is on.

Ref: typescript-eslint/typescript-eslint#6762

* Update dependency upgrades - non-major * Require valid disables with explanations * Temporarily disable typescript-eslint rule Ref: typescript-eslint/typescript-eslint#6762 --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Karl Horky <karl.horky@gmail.com>

JoshuaKGoldberg · 2023-07-16T23:48:23Z

👋 @Josh-Cena just checking in, is this still something you have time for?

Josh-Cena · 2023-07-17T06:41:03Z

Yes! I'll get to it. Sorry for the wait. I still don't know what the best way to fix this is, because I feel like accounting for index signature optionality, even without the compiler option, should be acceptable.

hbiede · 2023-08-15T15:49:48Z

Hoping this will be available in the near-term future. I'd love to use this rule without having to use dozens of casts

karlhorky · 2023-08-16T13:44:52Z

It seems like the TypeScript PR microsoft/TypeScript#54777 was merged, which closed the issue in the TypeScript repo:

exactOptionalPropertyTypes does not include undefined in the type for assignment operators like ??= microsoft/TypeScript#53305 (comment)

Does that mean that this PR is now obsolete, once the TS PR changes are released in a version of TypeScript (eg. maybe 5.2)?

Josh-Cena · 2023-08-16T14:51:49Z

@karlhorky That PR changes the quick info of the write type so it excludes undefined (and the programmatic API always excludes it), which makes this PR even more necessary :)

Josh-Cena · 2023-10-01T18:51:57Z

@bradzacher Sorry it's been a really long time and I think neither of us are up to speed on this now. Let's recap—what (potential) false-positives should we fix and what should we continue erroring on? Currently we ignore all object[index] where object has index signatures, because that would always be potentially undefined.

bradzacher

originally I was against us bringing arrays and objects into parity.
however given the state noUncheckedIndexAccess is in esp in regards to ??= - I shall acquiesce here and we can loosen the rule to just ignore really anything with an index signature.

A few comments but otherwise looking good.

bradzacher · 2023-11-10T02:27:48Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+      if (checker.isTupleType(objType)) {
+        // If indexing a tuple with a literal, the type will be sound
+        return node.property.type !== AST_NODE_TYPES.Literal;
+      }


This check is actually no longer sound with modern TS.

Tuples can be variadic now - (eg [T1, ...T2[]], [...T1[], T2], [T1, ...T2[], T3], [...T1[]]) - so they're no more safe than an array.

I.e. for const foo: [T1, ...T2[]] - foo[1] is possibly undefined

playground

bradzacher · 2023-11-10T02:55:29Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+      }
+      if (node.computed) {
+        const indexType = getNodeType(node.property);
+        if (isTypeAnyType(indexType)) {


question: do we want to handle the never case here?
I think it's okay not to just figured I'd flag it for completeness sake

bradzacher · 2023-11-10T02:57:50Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+          // No need to check anything about any
+          return true;
+        }
+        if (isTypeFlagSet(indexType, ts.TypeFlags.NumberLike)) {


This doens't handle "nominal" number types here.
EG type T = number & { __brand: unknown } would work in the signature but we would not match it here.

I think it's okay that we ignore it here - it's a super niche usecase.
Just commenting for completeness's sake.

bradzacher · 2023-11-10T02:58:28Z

packages/eslint-plugin/src/rules/no-unnecessary-condition.ts

+        if (isTypeFlagSet(indexType, ts.TypeFlags.NumberLike)) {
+          return (
+            checker.getIndexTypeOfType(objType, ts.IndexKind.Number) !==
+            undefined
+          );
+        }
+      }


we should probably also handle symbol indexes as well.
They're super rare but it should just be one extra if

declare const obj: {[k: symbol]: string}; declare const key: symbol; const res = obj[key]; res?.length // should not report

JoshuaKGoldberg · 2024-01-08T16:09:08Z

👋 ping @Josh-Cena, do you still have time for this one?

JoshuaKGoldberg · 2024-01-29T16:47:56Z

Closing this PR as it's been stale for a while without activity. If anybody wants to drive it forward, please do post your own PR - and if you use this as a start, consider adding a co-author attribution at the end of your PR description. Thanks! 😊

fix(eslint-plugin): [no-unnec-cond] ignore optionally accessing index…

fd1488a

… signatures

Josh-Cena force-pushed the nuc-index-signature branch from 4ac41b6 to fd1488a Compare March 24, 2023 21:19

Josh-Cena commented Mar 24, 2023

View reviewed changes

bradzacher changed the title ~~fix(eslint-plugin): [no-unnec-cond] ignore optionally accessing index signatures~~ fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures Mar 26, 2023

bradzacher requested changes Mar 26, 2023

View reviewed changes

bradzacher added bug Something isn't working awaiting response Issues waiting for a reply from the OP or another party labels Mar 27, 2023

karlhorky mentioned this pull request Apr 21, 2023

Bug: [no-unnecessary-condition] warns about conditional assignment to index signature #6632

Open

4 tasks

karlhorky added a commit to upleveled/ical-move-events that referenced this pull request Apr 22, 2023

Temporarily disable typescript-eslint rule

f8b14f7

Ref: typescript-eslint/typescript-eslint#6762

JoshuaKGoldberg requested a review from bradzacher October 18, 2023 02:06

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Oct 18, 2023

bradzacher requested changes Nov 10, 2023

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Nov 10, 2023

bradzacher mentioned this pull request Nov 29, 2023

chore: finish enabling no-unnecessary-condition internally #8004

Merged

3 tasks

bradzacher added the stale PRs or Issues that are at risk of being or have been closed due to inactivity for a prolonged period label Jan 27, 2024

JoshuaKGoldberg closed this Jan 29, 2024

github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2024

Josh-Cena deleted the nuc-index-signature branch June 2, 2024 09:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

Josh-Cena commented Mar 24, 2023 •

edited

Loading

typescript-eslint bot commented Mar 24, 2023

netlify bot commented Mar 24, 2023 •

edited

Loading

nx-cloud bot commented Mar 24, 2023 •

edited

Loading

Josh-Cena Mar 24, 2023

bradzacher Mar 26, 2023

Josh-Cena Mar 26, 2023

bradzacher Mar 26, 2023

Josh-Cena Mar 26, 2023

bradzacher Mar 27, 2023

codecov bot commented Mar 24, 2023 •

edited

Loading

bradzacher left a comment

bradzacher Mar 26, 2023

JoshuaKGoldberg commented Jul 16, 2023

Josh-Cena commented Jul 17, 2023

hbiede commented Aug 15, 2023

karlhorky commented Aug 16, 2023 •

edited

Loading

Josh-Cena commented Aug 16, 2023

Josh-Cena commented Oct 1, 2023

bradzacher left a comment

bradzacher Nov 10, 2023

bradzacher Nov 10, 2023

bradzacher Nov 10, 2023

bradzacher Nov 10, 2023

JoshuaKGoldberg commented Jan 8, 2024

JoshuaKGoldberg commented Jan 29, 2024

fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

fix(eslint-plugin): [no-unnecessary-condition] ignore optionally accessing index signatures #6762

Conversation

Josh-Cena commented Mar 24, 2023 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Mar 24, 2023

netlify bot commented Mar 24, 2023 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Mar 24, 2023 • edited Loading

☁️ Nx Cloud Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 24, 2023 • edited Loading

Codecov Report

bradzacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jul 16, 2023

Josh-Cena commented Jul 17, 2023

hbiede commented Aug 15, 2023

karlhorky commented Aug 16, 2023 • edited Loading

Josh-Cena commented Aug 16, 2023

Josh-Cena commented Oct 1, 2023

bradzacher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoshuaKGoldberg commented Jan 8, 2024

JoshuaKGoldberg commented Jan 29, 2024

Josh-Cena commented Mar 24, 2023 •

edited

Loading

netlify bot commented Mar 24, 2023 •

edited

Loading

nx-cloud bot commented Mar 24, 2023 •

edited

Loading

codecov bot commented Mar 24, 2023 •

edited

Loading

karlhorky commented Aug 16, 2023 •

edited

Loading