Don't widen return types of function expressions
RyanCavanaugh opened this issue · 35 comments
This change courtesy @JsonFreeman who is trying it out
(context elided: widening of function expression return types)
The problem with this widening is observable in type argument inference and no implicit any. For type argument inference, we are prone to infer an any, where we should not:
function f<T>(x: T, y: T): T { }
f(1, null); // returns number
f(() => 1, () => null); // returns () => any, but should return () => numberSo after we get to parity, I propose we do the following. We do not widen function expressions. A function is simply given a function type. However, a function declaration (and a named function expression) introduces a name whose type is the widened form of the type of the function. Very simple to explain and simple to implement.
I’ve been told that this would be a breaking change, because types that used to be any are now more specific types. But here are some reasons why it would be okay:
- In the places where you actually need the type to be any (because there are no other inference candidates), you would still get any as a result
- In places where there was a better (more specific) type to infer, you’d get the better type.
- With the noImplicitAny flag, you’d get fewer errors because there are actually fewer implicit anys
Questions:
Is a principle of design changes going forward to not switch from 'any' to a more precise type because it can be a breaking change?
Going with 'not a breaking change' here because this is unlikely to break working code, but we need to verify this.
Would this manufacture two types?
In essence, we already have two types: The original and the widened type. So by that measure this is not really a change
Has someone tried it?
Jason willing to try it out and report back
Bumping this since we ran into a situation with a leaked any in our own compiler. Minimal example:
function forEach<T, U>(array: T[], callback: (element: T, index: number) => U): U {
if (array) {
for (let i = 0, len = array.length; i < len; i++) {
let result = callback(array[i], i);
if (result) {
return result;
}
}
}
return undefined;
}
forEach([1, 2, 3, 4], () => {
// do stuff
if (blah) {
// bam! accidental any
return undefined;
}
});We need an implementation proposal for this one.
What if we just did this:
- Don't widen when you type a function body
- When you widen a type, and it's a function type, you widen the return type
- When you resolveName and it's a function expression or function declaration, you widen its type
I've arrived here via #7220. Would appreciate some guidance on whether the following is expected behaviour or not (playground):
interface G<K, R> {
(v: K): R;
}
function F<T>(v: T): T {
return v;
}
interface A {
__Type: "A";
}
interface B {
__Type: "B";
}
let a: A;
let b: B;
a = b; // OK: error as expected, B is not assignable to A
b = a; // OK: error as expected, A is not assignable to B
let x: G<A, B> = F; // How is this possible?It's the final assignment I cannot understand... because the compiler does not complain.
During assignment checking, type parameters are replaced with any. So the effective signature of F here is (a: any) => any
Time to take this back up since it's affecting a lot of people writing reducers
Any updated status on this?
bump
I came here by Daniel's reference from #20859. I'm having trouble writing typesafe libraries where users can provide callbacks which should follow the type contract of the library function.
And, as shown in my examples in #20859, this affects not only generic funtions (as depicted here) but also non-generic strictly typed functions. Basically, the return of a callback function is type unsafe, always, since the callback is a variable, and any assigning of a function to a variable (of a function type) ignores the return value of the r-value.
A lot of safety is missed by the current behavior.
This is another one I've been having to work around. I've added helper types so that users of Fabric don't have to duplicate entire function signatures for type safety, like so:
const circularTokens: IButtonComponent['tokens'] = (props, theme) => { };However, this loses some (EDIT: not all) type safety on the return type, which is really important for having us make sure Fabric users are doing the right thing in their implementations. So for now I've had to make helper types:
export type IButtonTokenReturnType = ReturnType<Extract<IButtonComponent['tokens'], Function>>;Which requires users to do this to get full type safety:
const circularTokens: IButtonComponent['tokens'] = (props, theme): IButtonTokenReturnType => { };Which is a bit frictiony and error prone. I'd love to see this issue fixed!
I assume by "safety" you mean excess property checking?
I guess the way I'm looking at it is I want the same level of type safety for the return type, whether it's explicitly specified or applied via functional type as shown here:
// Error based on return type:
const function1 = (input: number): { testKey?: number } => { return { testKey: 42, invalidKey: 42 } }
// Same return type via functional signature but no error:
type TestFunction = (input: number) => { testKey?: number };
const function2: TestFunction = (input) => { return { testKey: 42, invalidKey: 42 } }Is MUI team right saying the following is a TypeScript widen issue? I think TS is just right (see my test here).
Type '{ display: string; flexDirection: string; }' is not assignable to type 'CSSProperties'.
Types of property 'flexDirection' are incompatible.
Type 'string' is not assignable to type '"column" | "-moz-initial" | "inherit" | "initial" | "revert" | "unset" | "column-reverse" | "row" | "row-reverse" | undefined'.
Just ran into this trying to capture a subtype of an object using spread but no error is thrown on excess properties.
Example
interface Props extends ThirdPartyOptions {
bar: "baz",
}
const foo = (props: Readonly<Props>): ThirdPartyOptions => {
const { ...rest } = props;
return rest;
};EDIT: Found a workaround for my case:
function keysTyped<T>(obj: T): Array<keyof T> {
return Object.keys(obj) as Array<keyof T>;
}
interface Props extends ThirdPartyOptions {
bar: "baz",
}
const foo = (props: Readonly<Props>): ThirdPartyOptions => {
const { ...rest } = props;
const test: Array<keyof ThirdPartyOptions> = keysTyped(rest); // Error
return rest;
};I filed #33908 based on behaviour a coworker found in NGRX where NGRX's ActionReducer<S, A> interface has a callable signature of (state: S | undefined, action: A) => S but the widening issue allows for subtle bugs as extra keys can be provided by the reducer that don't match the provided interface.
This may factor into whether this is a breaking change or not, given how popular NGRX is amongst the Angular community.
I filed #33908 based on behaviour a coworker found in NGRX where NGRX's
ActionReducer<S, A>interface has a callable signature of(state: S | undefined, action: A) => Sbut the widening issue allows for subtle bugs as extra keys can be provided by the reducer that don't match the provided interface.This may factor into whether this is a breaking change or not, given how popular NGRX is amongst the Angular community.
Also to add on, it would make it very difficult to catch typos on existing keys.
ie.
const initialState: SomeState = {
someID: number
}
createReducer(
initialState,
on(someAction, (state, { id }) => {
return {
...state,
someId: id // this typo wouldn't throw errors
}
}
)"Fixing" this would be useful for my SQL query builder library.
tsql.from(myTable)
.select(columns => [columns])
.insertInto(
connection,
otherTable,
columns => {
return {
otherTableColumn0 : columns.myTableColumn0,
otherTableColumn1 : columns.myTableColumn1,
//Should give a compile error because `otherTable`
//does not have column `doesNotExist`
doesNotExist : 32,
};
}
);If "exact types" were implemented, I could use that for this constraint, too
declare function foo(
callback: () => {x:string}
): void
foo(() => {
return {
x: "hi",
y: "bye", //should error
};
})Related,
#12632
export interface Demo {
items?: () => { demo?: string };
}
export const dd: Demo = {
items: (): ReturnType<Required<Demo>['items']> => ({ demo: '1', str: 'error' }),
};We've noticed the same issue after some of the reducers returned garbage. Big surprise.
function reducer(state: IState, action: IAction): IState {
switch (action.type) {
case 'FOOL_TYPESCRIPT':
return {
...state,
...{ ANY_NON_EXISTING_FIELD_GETS_SWALLOWED_WITH_NO_ERROR: "BOOM" },
};
default:
return state;
}
}This problem affects any factory function that is intended to create a specific type. (From the form of #12632 )
interface OnlyThis {
field: number
}
type FactoryFunc = () => OnlyThis
function makeOnly(ctor: FactoryFunc) { }
const test = makeOnly(()=> ({
field: 123,
extra: 456, // expected to fail, but does not.
}))
There's no way to prevent makeOnly from accepting a function returning an object with excess fields. It will detect missing fields, but not excess fields. Thus the user of makeOnly will no know that their values are being ignored.
@mortoray yes, this is my main issue with this. Currently, the only workaround is to explicitly type the arrow func, which kinda defeats the purpose:
const test = makeOnly((): OnlyThis => ({
field: 123,
extra: 456, // error
}))
Here is another minimal example of the problem:
const getFoo2: () => Foo = {
foo: {
bar: number
}
} => ({
foo: {
bar: 1,
buz: 2, // Doesnt throw an excessive property error
}
})The weird thing is, its not like the function is untyped. Changing the name of foo, or adding an excessive property to the main return object, works as expected. It only fails on the nested object.
I believe my StackOverflow question relates to this issue: https://stackoverflow.com/questions/63258306/how-to-force-typescript-excess-property-check-on-function-return-value
Hey everyone, I opened #40311 a couple of days ago which I think should fix most of the issues that have brought people here. If you'd like to give it a try, we have a build available over at #40311 (comment).
Hey @DanielRosenwasser, what's the status of your pull request? I suppose I don't have to list any reasons for wanting this feature (there are many discussions), so I'm only curious if there's been any progress recently
+1 to consider this or adding it as a compiler flag.
I ran into problems with some .map() calls, which seems related to the issue described.
Is this still being worked on?
Hi!
Is return type widening a possible cause for the lack of errors in this case?
type T = { foo: number };
type F = () => T
// no type error
const a: F = () => ({
foo: 1,
bar: 2,
})Or is it because of the covariance rule on function subtyping?
Happens for .map()
interface IUser {
id: number;
}
// No errors
const users: Array<IUser> = [].map<IUser>(() => ({
id: 1,
unknownProperty: 'unknownProperty',
}));
// Property 'id' is missing in type '{ unknownProperty: string; }' but required in type 'IUser'.ts(2741)
const users1: Array<IUser> = [].map<IUser>(() => ({
unknownProperty: 'unknownProperty',
}));
console.log(users);
console.log(users1);This issue is also present when writing GraphQL resolvers:
const resolvers: Resolvers = {
Query: {
someField: () => ({
requiredField: 'some value',
// @ts-expect-error
optionalFieldWithTyppo: 'some value',
})
}
}As we can see in this example TS is not doing its job of catching the type issue introduced by the typo on the optional field name.
AFAIK there is no real workaround (except explicitly putting the return type on the function which defeats the purpose of putting the type in the first place). Is this issue still on the radar of the TypeScript team?
This issue seems to be the cause of this error:
type MyResponse = {
my: string;
response: string;
};
const myFunc = (param: string): MyResponse => {
const myResponse = {
my: 'my',
response: 'response',
extra: 'extra', // no error here
};
return myResponse;
};
const myFunc2 = (param: string): MyResponse => {
return {
my: 'my',
response: 'response',
extra: 'extra', // error here
};
};What are the action items here?
Having same issue as @yamcodes
@RyanCavanaugh @DanielRosenwasser I also noticed that when using object spread in the return statement of a function, no error is reported for additional properties that don’t exist in the target type.
For example:
✅ Type error is correctly raised here:
type MyResponse = {
my: string;
response: string;
};
const myFunc1 = (param: string): MyResponse => {
return {
my: 'my',
response: 'response',
extra: 'extra', // error here, as 'extra' is not part of 'MyResponse'
};
};❌ However, no error is raised in the following cases:
- When assigning the object to a variable first:
const myFunc2 = (param: string): MyResponse => {
const myResponse = {
my: 'my',
response: 'response',
extra: 'extra', // no error here, even though 'extra' is not part of 'MyResponse'
};
return myResponse;
};- When using object spread:
const myFunc3 = (param: string): MyResponse => {
const myResponse = {
my: 'my',
response: 'response',
extra: 'extra', // no error here either
};
return { ...myResponse };
};In both scenarios, TypeScript is allowing extra properties without raising an error even though it knows the final shape is inconsistent with the type annotations.
Extra properties are allowed in typescript. Excess property checks only apply to the type in effect at the point of declaration (if any). This is generally considered a linting feature to catch typos, not a type correctness feature.
Outside that specific scenario, extra properties are not an error. Consider if you wanted to assign a value {a: A, B: B} to a variable typed {a: A}. The value satisfies the type.
When the constant declaration is a step removed from where the value is used, EPC does not come into play.