microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.34k stars 12.4k forks source link

Numeric template type can't be used to index tuple in mapped type (despite actually working anyway) #48739

Closed Jamesernator closed 1 year ago

Jamesernator commented 2 years ago

Bug Report

🔎 Search Terms

numeric string, number template, tuple

🕗 Version & Regression Information

Note

This is a followup to another issue I raised where similar issues arose, however here the exact cause is different and more obviously a bug rather than intended behaviour.

This might still be a design limitation, but it's unclear to me how, without this, one can map a tuple type to something with a generic constraint.

⏯ Playground Link

Playground link with relevant code

💻 Code

class ValType {
    #isValType = true;
}

class I32 extends ValType {
    #isI32 = true;
}

class Externref extends ValType {
    #isExternref = true;
}

class Local<T extends ValType> {
    readonly #type: T;

    constructor(type: T) {
        this.#type = type;
    }

    get type(): T {
        return this.#type;
    }
}

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [K in keyof Type]: K extends number ? Local<Type[K]>
        // Type 'Type[K]' does not satisfy the constraint 'ValType'.
        //   Type 'Type[`${number}` & string]' is not assignable to type 'ValType'.(2344)
        : K extends `${ number }` ? Local<Type[K]>
        : Type[K]
};

// This is still the correct type, despite the above error, so clearly
// the keys "0" and "1" are successfully interpreted as `${ number }` in
// the conditional type
const locals: LocalsFor<[I32, Externref]> = [
    new Local(new I32()),
    new Local(new Externref()),
];

🙁 Actual behavior

It produces an error, despite the fact K extends `${ number }` should be treated as a tuple key. (As per the other issue I raised numbers can't be used to index tuples, so a numeric string is the only way to distinguish tuple entry keys from methods for mapping tuple types).

The final bit of code, shows that the constraint does work, however it's not treated as a valid key for ReadonlyArray<ValType> despite subtypes being potentially tuples.

Note that because of the type constraint, mapped tuple types do not work.

🙂 Expected behavior

This should not have an error, as it otherwise works and the constraint should filter the tuple member keys (and not the methods).

Jamesernator commented 2 years ago

Actually I might close this and open an issue on a more basic failure in mapped types, the fact that constraints don't work despite mapped types ignoring methods in array/tuple types:

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [K in keyof Type]: Local<Type[K]>
};
Jamesernator commented 2 years ago

As the other issue is supposedly a working as intended, I'm gonna reopen this one.

RyanCavanaugh commented 2 years ago

I guess what I'm not understanding is why you don't write this type

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [K in number & keyof Type]: Local<Type[K]>;
};

which seems to do what you're looking for?

Adding more ad-hoc rules around e.g. K extends ${number} is just going to make mapped types more "bizarre", per your comment in the other thread, so I'm not sure what avenue to pursue here except for using the above definition.

Jamesernator commented 2 years ago

which seems to do what you're looking for?

The problem with that is it deletes all the other properties (and without negated types, we can't do [K in keyof Type & not number] to merge in the rest):

const locals: LocalsFor<[I32, Externref]> = [
    new Local(new I32()),
    new Local(new Externref()),
];

// Property 'length' does not exist on type 'LocalsFor<[I32, Externref]>'.
console.log(locals.length);

Adding more ad-hoc rules around e.g. K extends ${number} is just going to make mapped types more "bizarre"

This wouldn't really be ad-hoc, like "0" is clearly ${ number }, this is just true. What's weirder is that [K in number & keyof Type] works, given that usually K extends number is false?:

type CheckNumKeys<Type extends ReadonlyArray<any>> = {
    [K in keyof Type]: K extends number ? "key-is-number" : "key-not-number"
};

// ["key-not-number", "key-not-number"]
type X = CheckNums<["a", "b"]>;

Like why would number & keyof Type be expected to be something other than never if for tuples keyof doesn't return the keys as number to begin with? Except that it actually magically does in this very specific situation? (Which again isn't documented in the handbook, so how is anyone supposed to figure out that would work, given that the logical combination of parts suggests it wouldn't?)

Jamesernator commented 2 years ago

Also just to be clear, the example in the initial comment almost works, like the type is mapped just fine (as LocalsFor<[I32, Externref]> DOES correctly map the "0" and "1" keys through the condition K extends `${ number }`). However what is incorrect is that TypeScript doesn't consider Type[`${ number }`] to be ValType DESPITE the fact it does consider Type[number] (as in the first conditional).

As you've pointed out in the previous issue, the keys aren't exposed as numbers, because well they aren't numbers. However neither are Array keys, so Type[`${ number }`] should work when indexing the array Type (and again, this technically already works for tuple keys).

Jamesernator commented 2 years ago

Okay so funnily enough, just adding the signature myself actually works:

declare global {
    interface ReadonlyArray<T> {
        [k: `${ number }`]: T;
    }
}

This should probably still be in the standard types that TypeScript ships as this is, well, how arrays in JS work. But at least I have an actually good workaround now.

RyanCavanaugh commented 2 years ago

This should probably still be in the standard types that TypeScript ships as this is, well, how arrays in JS work.

No, because 0.2 is a valid ${number} but not an array index.

We're still left with the problem identified in the other issue

type M = ReadonlyArray<ValType> & { "0.2": boolean };
type G = LocalsFor<M>;
type X = G["0.2"]; // Local<boolean>, a constraint violation

WLOG (constrained to your scenario where presumably you don't care about that example, which is super valid) you can write

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [K in keyof Type]: Local<Type[K] & ValType>;
};

or

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [K in keyof Type]: Type[K] extends ValType ? Local<Type[K]> : never;
};

depending on what you want to happen to out-of-band values.

Jamesernator commented 2 years ago

No, because 0.2 is a valid ${number} but not an array index.

I mean given that TypeScript already behaves like this for the arrays:

const arr = [1,2,3];

// x is type number
const x = arr[0.2]

It wouldn't be that inconsistent. If people really want to attach non(positive)-integer numbers to arrays, they already do so at their own peril today anyway.

Like if TypeScript ever fixed array[number] to be array[positiveInteger], then obviously the string indexer would be array[`${positiveInteger}`]. But from what I've seen TS has already considered int types out of scope. So I don't see why TypeScript would care that ReadonlyArray[`${ number }`] is technically wrong when ReadonlyArray[number] already is.

WLOG (constrained to your scenario where presumably you don't care about that example, which is super valid) you can write

This one suffers the problem of deleting types again, although even if it's changed to:

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [K in keyof Type]: Type[K] extends ValType ? Local<Type[K]> : Type[K];
};

It just breaks the type in ways I'm not entirely clear on, and I'm not sure what a minimal reproduction actually is:

Screenshot from 2022-04-19 15-17-22

I suspect it has to do with the extra indirection through more generics, my suggested fix doesn't suffer this problem though and instead there is no type error there, so the actual array parts are preserved properly with my fix.

or

This suffers exactly the same problem as the previous suggestion.

@RyanCavanaugh RyanCavanaugh added the Unactionable label 1 hour ago

I'm not clear why this is the case, adding the signature I suggested just above would basically be consistent with how TypeScript already treats array[number] today.

RyanCavanaugh commented 2 years ago

I'm not clear why this is the case, adding the signature I suggested just above would basically be consistent with how TypeScript already treats array[number] today.

The cure has to be better than the disease. Adding that declaration would permit things like

declare const arr: ReadonlyArray<boolean>;
// j: boolean
let j = arr["4.0"];

I'm calling this unactionable because not liking how mapped type are/aren't homomorhpic or do/don't deal with numeric-like keys isn't something we can change in a way that doesn't cause someone else to not like it instead, with the added drawback of being a breaking change.

Jamesernator commented 2 years ago

The cure has to be better than the disease

Why were int types rejected as a suggestion then? Because if array[number] is considered a "disease", then that was/is the obvious solution. (And then array[`${int}`] or whatever could be added safely because that would be correct).

This is something that has been frustrating me with TypeScript for quite a long time now, and that's that there is a lot of historical baggage that has fixes outright rejected (like the aforementioned int-types) or have been suggested for years with no indication of fixing either way (there are way too many of these I've encountered to even begin listing).

It's a shame because there really just aren't major competitors to TypeScript (Flow doesn't really compete in the same space, as it's more a subset of JS that is strictly typed, rather than adding types to JS, and they announced a fairly significant direction change recently to non-standard JS features). There is one ridiculously excellent type system in Hegel, however it lacks one main feature which is why I've stayed with TypeScript and that's suggestions/completions.

Andarist commented 2 years ago

By the way... using & number in the key part of the mapped type is somewhat suboptimal because such a type won't be usable with spreads: TS playground

The problem here is that the result of this produces an object type with an index signature. If we really want this then there is no point in using a mapped type here at all because we will end up with a single key~ anyway. This could be rewritten to:

type LocalsFor<Type extends ReadonlyArray<ValType>> = {
    [x: number]: Local<Type[number]>
}

TS playground with the code above

So it seems that a better approach is to actually use & number in the template part of the mapped type: TS playground

That on the other hand... works somewhat accidentally. The problem in here is that when mapping over a tuple we'll get keys like '0' | '1' | '2' | .... So why does it even work? After all string & never is... never. It works because never can be used to access an index signature. TS playground demonstrating that.

The problem is... this still doesn't really work as expected. Because we've accessed an index signature we got a union back of all possible value types from this tuple and most likely the intention was to map each position separately. It is possible to type this index by index by for what seems to be a pretty standard operation... it's pretty complex and requires using nested conditional types or the new infer extends combo: TS playground

I'm also discussing this problem in the comments here: https://github.com/microsoft/TypeScript/pull/48599

Andarist commented 2 years ago

@Jamesernator @RyanCavanaugh I believe that the issue here was addressed by https://github.com/microsoft/TypeScript/pull/48837

github-actions[bot] commented 1 year ago

This issue has been marked as 'Unactionable' and has seen no recent activity. It has been automatically closed for house-keeping purposes.