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

noUncheckedIndexedAccess does not seem to work on the type level #46273

Closed
asbjornh opened this issue Oct 8, 2021 · 4 comments
Closed

noUncheckedIndexedAccess does not seem to work on the type level #46273

asbjornh opened this issue Oct 8, 2021 · 4 comments
Labels
Duplicate An existing issue was already created

Comments

@asbjornh
Copy link

asbjornh commented Oct 8, 2021

Bug Report

Let me preface this by saying that I realize that this is likely a conscious design decision (seeing as this behavior seems to have existed ever since noUncheckedIndexedAccess was introduced). I do think it's highly undesirable though. And sorry in advance if this has already been answered (although I didn't find anything through search)

🔎 Search Terms

noUncheckedIndexedAccess

🕗 Version & Regression Information

It seems like this has been the behavior ever since noUnchekedIndexedAccess was introduced.

⏯ Playground Link

Playground link

💻 Code

type A = string[];

/** These are type errors but shouldn't be, as `A[number]` should include `undefined` */
const x: A[number] = undefined;
const y: A[0] = undefined;
const z: A[1000] = undefined;

The following illustrates how authors might unintentionally introduce unsoundness by including a return type annotation:

/** Without return type annotation */
const get = <O, K extends keyof O>(o: O, k: K) => o[k];

/** With return type annotation */
const get2 = <O, K extends keyof O>(o: O, k: K): O[K] => o[k];

/** A type error as expected */
const a: number = get([1], 1);

/** No type error, because `get2` happens to include a return type annotation */
const b: number = get2([1], 1);

🙁 Actual behavior

When doing indexed access on array types (f. ex. (number[])[number])), the resulting type does not include undefined. This can erode the type safety that noUncheckedIndexedAccess provides, without the author realizing it.

This is also the case for Record but I haven't included an example for that.

🙂 Expected behavior

Property access on an indexed type should resolve to a type that includes undefined

@MartinJohns
Copy link
Contributor

MartinJohns commented Oct 8, 2021

Duplicate of #42471. And I still strongly disagree with the expected behaviour described in this issue. The type should be whatever is stored in the array. According to the type undefined is not stored in the array, regardless of noUncheckedIndexedAccess. Wouldn't you expect to be able to push A[number] values to the array? If the type would include undefined it wouldn't be possible anymore for arrays not storing undefined values.

As per #39560 this is also the intended behaviour and not a bug.

#43729 is somewhat related: the ability to differentiate between "read" and "write" access on the type-level.

@asbjornh
Copy link
Author

asbjornh commented Oct 8, 2021

Thanks for pointing me to those issues.

I didn't consider the case of adding values to arrays or dictionaries. That makes sense.

I do think there's a legitimate issue here though, where adding a type annotation that seems correct introduces the potential for sneaky runtime crashes. As far as I can tell, there's currently no syntax to express property access that might fail (except for manually always including undefined in the type, which is incorrect for non-indexed types).

Would this make sense as a new feature request? I guess this could potentially be covered by #43729, but that issue seems only tangentially related

@DanielRosenwasser DanielRosenwasser added the Duplicate An existing issue was already created label Oct 8, 2021
@DanielRosenwasser
Copy link
Member

I can sympathize wanting to be safer when writing declaration files, but I would personally rather keep things simple. I don't exactly know what the rule would be - maybe that in return positions, an indexed access with a concrete property type always has to be coupled with an | undefined?

interface Array<T> {
    pop(): this[number] // ❌
    pop(): this[number] | undefined// ✅

    [index: number]: T;
}

interface MapLike<V>
    [key: string]: V;

    get<K extends string>(key: K): this[K] // ✅ (? - this is questionable)
}

Honestly, I would suggest a lint rule that enforces this on certain return types, and only in .d.ts files.

@asbjornh
Copy link
Author

asbjornh commented Oct 11, 2021

After messing around for a long while I've found a userland workaround. Here it is if anyone ever lands here in the future:

type Get<O, K extends keyof O> =
	O extends any[]
	? number extends O['length'] // If `number` is a subtype of `O.length` then `O` is _not_ a tuple, since a tuple has a specific length
		? O[K] | undefined
		: O[K]
	: number extends keyof O // If O is not an array and `number` is a subtype of `keyof O` then `O` is a `Record<number, any>`
	? O[K] | undefined
	: string extends keyof O // If `string` is a subtype of `keyof O` then `O` is a `Record<string, any>`
	? O[K] | undefined
	: O[K];

I'm happy with this. Thanks for nudging me towards figuring this out and sorry for creating noise ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants