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

Regression when using indexed type depending on function argument as return type #31672

Closed
AlCalzone opened this issue May 30, 2019 · 24 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@AlCalzone
Copy link
Contributor

TypeScript Version: 3.5.1

Search Terms:

Code

interface Foo {
	prop1: { value: number }[];
	prop2: string[];
}
function getFoo<T extends keyof Foo>(key: T): Foo[T] | undefined {
	if (key === "prop1") {
		return [{ value: 1 }]; // Error here
	}
}
const bar = getFoo("prop1"); // has correct type

Expected behavior:
No error. This used to work in 3.4.5

Actual behavior:

Type '{ value: number; }[]' is not assignable to type 'Foo[T]'.
  Type '{ value: number; }[]' is not assignable to type '{ value: number; }[] & string[]'.
    Type '{ value: number; }[]' is not assignable to type 'string[]'.
      Type '{ value: number; }' is not assignable to type 'string'.
@fatcerberus
Copy link

I'm thinking this is probably a result of #30769.

@AnyhowStep
Copy link
Contributor

I'm not sure if this is a bad thing at all. More inconvenient, for sure. But if you're sure it's correct, I think having an explicit type assertion (as T[K] or as unknown as T[K]) for unsound code is a good thing.

@fatcerberus
Copy link

fatcerberus commented May 30, 2019

Yeah, the new behavior is in general much more sound.

In this particular case the code looks okay since the key === "prop1" check narrows things down. The trouble arises because the type guard only affects the parameter key itself; Foo[T] is in a generic context so the compiler treats it like T is as wide as possible, i.e. as its constraint keyof Foo.

Too bad there wasn’t a way to make the compiler narrow type parameters like we can narrow variable and property types...

@RyanCavanaugh
Copy link
Member

This wasn't properly checked in 3.4:

interface Foo {
	prop1: { value: number }[];
	prop2: string[];
}
function getFoo<T extends keyof Foo>(key: T): Foo[T] | undefined {
  if (key === "prop1") {
        // Oops!
		return ["foo"];
	}
}
const bar = getFoo("prop1");

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label May 31, 2019
@fatcerberus
Copy link

Yep, might be nice if there was a way to narrow whole type parameters based on type guards, but that kind of thing is probably really challenging to implement in practice. Particularly in cases where there's more than one inference site for a type.

@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented May 31, 2019

You can't narrow a type parameter based on a value, because you can't know that the type parameter isn't wider than the value you saw:

interface Foo {
	prop1: { value: number }[];
	prop2: string[];
}
function getFoo<T extends keyof Foo>(key1: T, key2: T): Foo[T] | undefined {
  // checking key1 cannot tell you what key2 will be or what a valid return type is
  return null as any;
}
// T: "prop1" | "prop2"
const bar = getFoo("prop1", "prop2");

@jack-williams
Copy link
Collaborator

@fatcerberus

Even limiting it to types with one inference site is insufficient because one site can produce multiple values: Eg. () => T or T[].

@Validark
Copy link

Validark commented Jun 2, 2019

@RyanCavanaugh Don't you think cases like this should work though? Is this unsound?

interface ConfigMap {
	Number: number;
	String: string;
	Boolean: boolean;
}

const MyNumber: number = 0;
const MyString: string = "foo";
const MyBool: boolean = false;

export function GetConfiguration<K extends keyof ConfigMap>(key: K): ConfigMap[K] {
	if (key === "Number") {
		return MyNumber;
	} else if (key === "String") {
		return MyString;
	} else if (key === "Boolean") {
		return MyBool;
	}
}

const a = GetConfiguration("Boolean");
const b = GetConfiguration("Number");
const c = GetConfiguration("String");

@fatcerberus
Copy link

fatcerberus commented Jun 2, 2019

Counterexample:

function GetConfiguration<K extends keyof ConfigMap>(fakeKey: K, realKey: K): ConfigMap[K]
{
	if (fakeKey === "Number") {
		return MyNumber;
	} else if (fakeKey === "String") {
		return MyString;
	} else if (fakeKey === "Boolean") {
		return MyBool;
	}
}

The above code will be accepted by TS 3.4, but not by 3.5.

Basically the problem is this: K is a type. Inside the function, we don't know exactly what type it is other than it's some subset of keyof ConfigMap. key is a value of type K, but not the type itself. In general you can't prove anything about a type by checking the value of some example of the type. It just so happens that in this specific case:

  1. The type is a generic type parameter
  2. There is only one incoming value of that type
  3. The other example of the type, the return value, depends on the value of the input.

In the case where all conditions above are met, the code is sound. The compiler can prove 1 and 2, but without dependent types it can't prove 3, and because this is unsafe more often than not, you get a type error to warn you of that.

This is an example of contravariance at work, so I can see why it throws people off (contravariance is confusing!) - but it's there for a good reason 😄

@Validark
Copy link

Validark commented Jun 2, 2019

@fatcerberus I am not sure whether it is "unsafe more often than not", but I don't dispute that a case like the example you gave should make typescript error. I simply think it would be nice if TypeScript allowed cases where the types are sound, and restricted them when they are unsound.

@fatcerberus
Copy link

fatcerberus commented Jun 2, 2019

That's the point though - the compiler has no way to prove that it's safe (that is, it can't distinguish the safe cases from the unsafe ones). In my checklist above, it's only safe when all 3 of those conditions are met, and the compiler can't prove the third condition without dependent types. So the choice you have is either to let all the unsafe cases through (i.e. any case not meeting the 3 conditions, such as the code above), or restrict it entirely at the cost of a few specific examples.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Jun 2, 2019

Proposal: introduce one-of type parameter constraint.

declare function foo<T extends oneOf 1|2|3> (t : T) : void;
foo(1); //OK
foo(2); //OK
foo(3); //OK
foo(1 as 1|2); //Error

@AnyhowStep
Copy link
Contributor

You can sort of already force users to always pass in a single string literal that is not a union of other types,

type IsAny<T> = (
    0 extends (1 & T) ?
    true :
    false
);

type IsNever<T> = (
    [T] extends [never] ?
    (
        IsAny<T> extends true ?
        false :
        true
    ) :
    false
);

type IsStringLiteral<T extends string> = (
    IsAny<T> extends true ?
    false :
    IsNever<T> extends true ?
    false :
    string extends T ?
    false :
    true
);

type IsOneStringLiteral<T extends string> = (
    IsStringLiteral<T> extends true ?
    (
        {
            [k in T] : (
                Exclude<T, k> extends never ?
                true :
                false
            )
        }[T]
    ) :
    false
);
//false
type a = IsStringLiteral<any>;
//Err: Type 'unknown' does not satisfy the constraint 'string'.ts(2344)
type b = IsStringLiteral<unknown>;
//false
type c = IsStringLiteral<never>;
//false
type d = IsStringLiteral<string>;
//true
type e = IsStringLiteral<"x">;
//true
type f = IsStringLiteral<"x"|"y">;

//false
type _a = IsOneStringLiteral<any>;
//Err: Type 'unknown' does not satisfy the constraint 'string'.ts(2344)
type _b = IsOneStringLiteral<unknown>;
//false
type _c = IsOneStringLiteral<never>;
//false
type _d = IsOneStringLiteral<string>;
//true
type _e = IsOneStringLiteral<"x">;
//false
type _f = IsOneStringLiteral<"x"|"y">;


interface ConfigMap {
    Number: number;
    String: string;
    Boolean: boolean;
}

const MyNumber: number = 0;
const MyString: string = "foo";
const MyBool: boolean = false;

declare function GetConfiguration<K extends Extract<keyof ConfigMap, string>>(
    key : (
        IsOneStringLiteral<K> extends true ?
        K :
        [K, "is not a single string literal"]
    )
): ConfigMap[K];

const a = GetConfiguration("Boolean"); //boolean
const b = GetConfiguration("Number");  //number
const c = GetConfiguration("String");  //string

/*
    Argument of type '"Number" | "Boolean"' is not assignable to
    parameter of type '["Number" | "Boolean", "is not a single string literal"]'.

    Type '"Number"' is not assignable to
    type '["Number" | "Boolean", "is not a single string literal"]'.ts(2345)
*/
const err = GetConfiguration("Boolean" as "Boolean"|"Number");
/*
    Argument of type '"Number" | "String" | "Boolean"' is not assignable to
    parameter of type '["Number" | "String" | "Boolean", "is not a single string literal"]'.

    Type '"Number"' is not assignable to
    type '["Number" | "String" | "Boolean", "is not a single string literal"]'.ts(2345)
*/
const err2 = GetConfiguration("Boolean" as Extract<keyof ConfigMap, string>);

Playground

@AlCalzone
Copy link
Contributor Author

@RyanCavanaugh So if this is working as intended, is there any way to define the behavior I was trying to achieve here? So given the interface

interface Foo {
	prop1: { value: number }[];
	prop2: string[];
}

and a function with a single parameter with type keyof Foo.
I want to enforce a return type of:

  • Foo["prop1"] if the parameter is === "prop1"
  • Foo["prop2"] if the parameter is === "prop2"
  • undefined otherwise

I assume this should be possible somehow without resorting to as any hacks.

@AnyhowStep
Copy link
Contributor

AnyhowStep commented Jun 4, 2019

Here's a hack-y workaround that does not use as any,

interface Foo {
    prop1: { value: number }[];
    prop2: string[];
}
function getFoo<T extends keyof Foo>(key: T): Foo[T] | undefined {
    /*
        Works, but you must have a `Foo` instance
    */
    const tmp : Foo = {
        prop1 : [],
        prop2: [],
    };
    if (key === "prop1") {
        //I find it concerning that even strict equality (===)
        //DOES NOT narrow `key` to type `"prop1"`.
        //But I guess if it did narrow, then `tmp[key]` would fail again...
        //Unless TS can somehow realize that `key` is both `"prop1"`
        //and can still be used to satisfy `Foo[T]`
        //Set tmp.prop1 to be the return value you want.
        tmp.prop1 = [{value : 1}];
        return tmp[key];
    } else if (key === "prop2") {
        //Set tmp.prop2 to be the return value you want.
        tmp.prop2 = ["bye, world"];
        return tmp[key];
    } else {
        return undefined;
    }
}
const bar = getFoo("prop1"); // has correct type

If you don't want to keep instantiating the tmp object, you could make it a global and re-use it (lol)


I think using as any is the better workaround for now.

@AnyhowStep
Copy link
Contributor

Other proposal, automatically narrow key,

interface Foo {
    prop1: { value: number }[];
    prop2: string[];
}
function getFoo<T extends keyof Foo>(key: T): Foo[T] | undefined {
    /*
        Works, but you must have a `Foo` instance
    */
    const tmp : Foo = {
        prop1 : [],
        prop2: [],
    };
    if (key === "prop1") {
        //Can TS Make this narrowing automatically?
        const k = key as (T & "prop1");
        //Set tmp.prop1 to be the return value you want.
        tmp[k] = [{value : 1}];
        //This works
        return tmp[k];
    } else if (key === "prop2") {
        //Can TS Make this narrowing automatically?
        const k = key as (T & "prop2");
        //Set tmp.prop2 to be the return value you want.
        tmp[k] = ["bye, world"];
        return tmp[k];
    } else {
        return undefined;
    }
}
const bar = getFoo("prop1"); // has correct type

@AlCalzone
Copy link
Contributor Author

AlCalzone commented Jun 4, 2019

I think using as any is the better workaround for now.

I believe so too, at least compared to the workarounds you posted.

@rubenpieters
Copy link

@fatcerberus @RyanCavanaugh I agree that dependent types are the more sane solution to making these use cases work nicely, however I don't fully agree with:

You can't narrow a type parameter based on a value

or

you can't prove anything about a type by checking the value of some example of the type

Let's take a look at this example:

function func1<B extends "t" | "f">(
  b1: B,
  b2: B,
): B {
  if (b1 === "t") {
    return "t"; // typescript rejects this
  } else if (b1 === "f") {
    return "f"; // typescript rejects this
  }
  throw "satisfy exhaustiveness check";
}

Indeed, checking b1 cannot tell you what b2 might possibly be. However, it does tell you something about B. For example, if b1 === "t" then surely B cannot possibly be the type "f" (or never)? So in the b1 === "t"branch, extending the context with "t" <: B seems sound to me on first sight. Which should allow the above function to typecheck.

Unfortunately this observation does not help us further in the general case with indexed access types as far as I can see. However, together with an addition such as the oneOf operator by @AnyhowStep these use cases could work again.

@fatcerberus
Copy link

if b1 === "t" then surely B cannot possibly be the type "f" (or never)?

We have unions - B could still be "t" | "f" (and never is okay too since it extends everything, though would require a cast to actually call the function). Proving otherwise requires checking both parameters, and TS doesn’t track dependencies between values so that’s right out.

@rubenpieters
Copy link

rubenpieters commented Jun 6, 2019

Yes, B can be either "t" or "t" | "f" (so "t" <: B holds), but not "f" or never without manually introduced (unsafe) casts.

@fatcerberus
Copy link

I don’t think TypeScript currently has any way internally of representing a lower-bounded type. There was a proposal around here for a super constraint... let me see if I can find it.

Yeah, here it is: #14520

@jcalz
Copy link
Contributor

jcalz commented Jun 6, 2019

Something like #27808 (or maybe #25879) would help here... or even #25051. Anything that lets you treat a value of a union type of n constituents like "exactly one of these n types" and not "one of all 2n possible sets of these n types". Lots of Stack Overflow questions about this issue are coming.

@jack-williams
Copy link
Collaborator

I have an experimental PR (#30284) that starts on that track, but it would need explicit support for index access types.

@typescript-bot
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

9 participants