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

Safer callback parameter checking #16726

Closed
mquandalle opened this issue Jun 23, 2017 · 5 comments
Closed

Safer callback parameter checking #16726

mquandalle opened this issue Jun 23, 2017 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@mquandalle
Copy link

mquandalle commented Jun 23, 2017

TypeScript Version: 2.4.1-insiders.20170615


Edit: I've written a more minimal reproduction of the issue below


Following up on the examples of #12769, we have the following code which is working as expected:

type UpdateSpec<T> = { [P in keyof T]?: UpdateSpec<T[P]> };

declare function update<T>(obj: T, spec: UpdateSpec<T>): T;

declare let foo: { a: number };

// As expected, this is working
update(foo, { a: 1 });

// As expected, this is an error
update(foo, { a: 1, b: 2 });

However if we change the update signature to accept a spec factory, ie a parameter of type () => UpdateSpec<T> instead of just UpdateSpec<T>, the above error isn't triggered anymore:

declare function update2<T>(obj: T, specFactory: () => UpdateSpec<T>): T;

// As expected, this is still working
update2(foo, () => ({ a: 1 }));

// Unexpected, this should be an error but the compiler is silent
update2(foo, () => ({ a: 1, b: 2 }));
@mquandalle
Copy link
Author

Actually, it's easy to build a more minimal reproduction of the issue here:

declare let foo: { a: number };
declare function bar<T>(a: T, b: () => T): void;

bar(foo, () => ({ a: 1 }));        // As expected, this is working
bar(foo, () => ({ a: 1, b: 2 }));  // Unexpected, this should be an error

So it seems that generics aren't passed down to callbacks in TS 1.4? Maybe there is an issue already open for that?

@mquandalle mquandalle changed the title Deepmerge factory Generic callback Jun 23, 2017
@kitsonk
Copy link
Contributor

kitsonk commented Jun 24, 2017

This has to do with the assignability of types... When you have a generic like this, that is in to positions being inferred, TypeScript has to try to figure out what the minimum type is and if both the positions are assignable. This in this case they are both assignable to the type { a: number; }.

If either of the values were not assignable to each other, then you would have an error:

declare let foo: { a: number };
declare function bar<T>(a: T, b: () => T): void;

bar(foo, () => ({ a: 1 }));        // As expected, this is working
bar(foo, () => ({ a: 1, b: 2 }));  // Not an error because types are assignable to each other
bar(foo, () => ({ b: 1 }));        // Throws an error

The excess properties are somewhat "surprising" but are valid given the structural typing system. TypeScript used to play a bit looser with this, but now detects excess properties when there is a direct type assignment. For example, this fails:

bar(foo, (): { a: number; } => ({ a: 1, b: 2 })); // Type not inferred, throws error

If you let TypeScript guess at your intent, sometimes TypeScript may surprise you.

@mquandalle
Copy link
Author

Thank you for the answer. I'm still wondering if there is room for improvements here, or if this behavior is a fundamental consequence of Typescript structural type system.

What I find puzzling is that the minimal example is working perfectly fine without the indirection of the callback:

interface Foo { a: number }
declare function bar(a: Foo): void
declare function barWithCallback(a: () => Foo): void

example

I find confusing that replacing a value of type Foo by a callback that also returns a value of type Foo will change the actual set of values accepted by the type system.

As you point out @kitsonk, it's possible to enforce a stricter object type checking by putting an explicit annotation when calling the barWithCallback function, but shouldn't this be automatically inferred from the barWithCallback signature? Maybe this was a design decision to avoid breaking existing base code? If yes, could there be a way to enforce the expected behavior without having to re-type the callback argument every time we use the barWithCallback function?

@mquandalle mquandalle changed the title Generic callback Safer callback parameter checking Jun 27, 2017
@RyanCavanaugh
Copy link
Member

@mquandalle any difference between this and #12632 ?

@mquandalle
Copy link
Author

@RyanCavanaugh Thanks for pointing out this issue, I wasn't able to find it when searching for it.

Closing in favor of #12632.

@RyanCavanaugh RyanCavanaugh added the Duplicate An existing issue was already created label Jun 27, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants