Skip to content
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

Improve checking of in operator combined with noUncheckedIndexedAccess #50936

Closed

Conversation

Andarist
Copy link
Contributor

follow up to #50666 , mostly piggy-backing on the logic introduced there

fixes #43614

cc @ahejlsberg @andreialecu

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Sep 24, 2022
src/compiler/checker.ts Outdated Show resolved Hide resolved
…affected by `noUncheckedIndexedAccess` at all
@sandersn sandersn added this to Not started in PR Backlog Oct 3, 2022
@sandersn sandersn moved this from Not started to Waiting on reviewers in PR Backlog Oct 17, 2022
@JakobBruenker
Copy link

Thanks! This is exactly what's preventing us from using noUncheckedIndexedAccess, so I'm looking forward to seeing this merged

…edIndexedAccess

# Conflicts:
#	src/compiler/checker.ts
Copy link
Member

@gabritto gabritto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small thing missing, but other than that, it looks good to me. Thanks 😊

if (!compilerOptions.noUncheckedIndexedAccess) {
return filtered;
}
return mapType(filtered, t => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should only do this if assumeTrue is true (maybe have the check above be if (!compilerOptions.noUncheckedIndexedAccess || !assumeTrue) ...).

Might be nice to add that to the tests as well, something like:

function f2(obj: Record<string, string>) {
  if ("test" in obj) {
    return obj.test
  }
  else {
    obj.test // Here it should be `string | undefined` still
  }
  return "default"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great catch! thanks. I've pushed out the fix for this

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Nov 25, 2022
@gabritto
Copy link
Member

@ahejlsberg could you confirm we're ok with the solution proposed here of adding extra intersections with Record<propName, propType> during narrowing when noUncheckedIndexedAccess is true?

@Andarist Andarist requested review from gabritto and removed request for ahejlsberg November 26, 2022 07:23
@ahejlsberg
Copy link
Member

First, I very much like the idea of narrowing indexed accesses guarded by in checks since we already do that for optional properties in --exactOptionalPropertyTypes mode. But, by that same reasoning, I think it should be done through the same mechanism as --exactOptionalPropertyTypes, i.e. use of the special missingType that indicates an undefined originating in a missing property. Specifically, we should use missingType as the undefined that is added when reading from an index signature and then rely on our existing CFA infrastructure to remove that missingType in code guarded by an in check. This would also get us support for narrowing through obj.hasOwnProperty("foo") checks, which is already in place for optional properties.

I realize that it is possible to get a similar effect using intersections, but it leads to more complex types and is oddly inconsistent with what we do for optional properties. And, really, index signatures are just another form of optional properties.

I will put up a PR in the next day or two to show what I mean. We may need to do a bit of optimizing of the logic that detects multiple kinds of undefined in unions and intersections, but we'll see what the performance suites reveal.

@Andarist
Copy link
Contributor Author

Closing as superseded by #51653

@Andarist Andarist closed this Nov 27, 2022
PR Backlog automation moved this from Waiting on author to Done Nov 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
PR Backlog
  
Done
Development

Successfully merging this pull request may close these issues.

noUncheckedIndexedAccess does not narrow properly with "prop" in obj
5 participants