Skip to content

Fix noUncheckedIndexedAccess with tuple rest types and generic index types #40681

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

Merged

Conversation

andrewbranch
Copy link
Member

(each commit fixes a separate bug)

Fixes #40657
Fixes #40666

Comment on lines 13995 to 13999
if (!type) {
indexedAccessTypes.set(id, type = createIndexedAccessType(objectType, indexType, aliasSymbol, aliasTypeArguments));
indexedAccessTypes.set(id, type = createIndexedAccessType(objectType, indexType, aliasSymbol, aliasTypeArguments, accessFlags));
}

return shouldIncludeUndefined ? getUnionType([type, undefinedType]) : type;
return type;
Copy link
Member Author

Choose a reason for hiding this comment

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

At this point, we don’t yet know whether the index type will be constrained to something that resolves to property signatures or index signatures, so we can’t return a union with undefined. But when we come back to do the deferred checking of the indexed access type we saved, we’ve lost the accessFlags that indicate that this is a position where undefined should be included in the type.

As I’m writing this, I’m realizing that this is probably broken for generic indexed access types that get used both for reading and for writing, so converting back to draft.

Copy link
Member Author

Choose a reason for hiding this comment

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

The only easy way I could find to fix this was creating separate IndexedAccessTypes (and keying the cache) depending on whether it was an expression where --noUncheckedIndexedAccess could have an effect or not. So in effect, in

function<T extends string>(key: T) {
  obj[key] = undefined;
  return obj[key];
}

the two usages of obj[key] have two separate types. (This only happens when the object type and/or the index type is generic.) I think this is technically possible to avoid, but would involve a lot of inexpedient threading of accessFlags (or some parameter based on it) through a bunch of functions.

@andrewbranch andrewbranch marked this pull request as draft September 21, 2020 22:39
@andrewbranch andrewbranch marked this pull request as ready for review September 22, 2020 00:01
const type = <IndexedAccessType>createType(TypeFlags.IndexedAccess);
type.objectType = objectType;
type.indexType = indexType;
type.aliasSymbol = aliasSymbol;
type.aliasTypeArguments = aliasTypeArguments;
type.noUncheckedIndexedAccessCandidate = shouldIncludeUndefined;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should just be named shouldIncludeUndefined?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did that at first, but I felt like it was confusing because that’s only true if the type resolves to an index signature. Indexed access types often resolve to an actual property, where they should not include undefined. Happy to hear pithier suggestions that don’t imply the unconditional presence of undefined though.

@sandersn
Copy link
Member

@andrewbranch is this ready to merge, or is it waiting on a review from @weswigham? I guess it's the latter but I want to make sure.

@andrewbranch
Copy link
Member Author

Ryan suggested waiting on a review from @weswigham if possible.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

My comment is mostly stylistic - this looks OK implementation-wise (noUncheckedIndexedAccessCandidate is super wordy, though 😛 )

@andrewbranch
Copy link
Member Author

@weswigham and @DanielRosenwasser both expressed distaste toward the name; I don't like it either, but I like it better than shouldIncludeUndefined for the reason I outlined here. Open to suggestions before merging.

@andrewbranch andrewbranch merged commit 5fbe980 into microsoft:master Oct 1, 2020
@andrewbranch andrewbranch deleted the bug/noUncheckedIndexedAccess branch October 1, 2020 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

noUncheckedIndexedAccess: unexpected error when indexing with generic constrained to keyof noUncheckedIndexedAccess and variadic tuples
6 participants