-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Improve checking of in
operator combined with noUncheckedIndexedAccess
#50936
Conversation
…affected by `noUncheckedIndexedAccess` at all
Thanks! This is exactly what's preventing us from using |
…edIndexedAccess # Conflicts: # src/compiler/checker.ts
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.
Small thing missing, but other than that, it looks good to me. Thanks 😊
src/compiler/checker.ts
Outdated
if (!compilerOptions.noUncheckedIndexedAccess) { | ||
return filtered; | ||
} | ||
return mapType(filtered, t => { |
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.
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"
}
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.
great catch! thanks. I've pushed out the fix for this
@ahejlsberg could you confirm we're ok with the solution proposed here of adding extra intersections with |
First, I very much like the idea of narrowing indexed accesses guarded by 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 |
Closing as superseded by #51653 |
follow up to #50666 , mostly piggy-backing on the logic introduced there
fixes #43614
cc @ahejlsberg @andreialecu