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

FormBuilder#group incorrectly infers type if union type is passed in #45912

Closed
bgotink opened this issue May 6, 2022 · 3 comments
Closed

FormBuilder#group incorrectly infers type if union type is passed in #45912

bgotink opened this issue May 6, 2022 · 3 comments

Comments

@bgotink
Copy link

bgotink commented May 6, 2022

Which @angular/* package(s) are the source of the bug?

forms

Is this a regression?

No

Description

The group function expands passed-in union types, which it shouldn't do:

const groupControl = formBuilder.group({
    value: '' as string | number,
});

// Expected type:
//    FormControl<string | number>
// Actual type:
//    FormControl<string> | FormControl<number>
const valueControl = group.controls.value;

image

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/angular-ivy-ao4sne?file=src%2Fapp%2Fapp.component.ts

Please provide the exception or error you saw

n/a

Please provide the environment you discovered this bug in (run ng version)

Unable to run ng version in stackblitz

Happens in 14.0.0-next.16

Anything else?

This is caused by the type mapping in

/**
* FormBuilder accepts values in various container shapes, as well as raw values.
* Element returns the appropriate corresponding model class, given the container T.
* The flag N, if not never, makes the resulting `FormControl` have N in its type.
*/
export type ɵElement<T, N extends null> =
T extends FormControl<infer U> ? FormControl<U> :
T extends FormGroup<infer U> ? FormGroup<U> :
T extends FormArray<infer U> ? FormArray<U> :
T extends AbstractControl<infer U> ? AbstractControl<U> :
T extends FormControlState<infer U> ? FormControl<U|N> :
T extends ControlConfig<infer U> ? FormControl<U|N> :
// ControlConfig can be too much for the compiler to infer in the wrapped case. This is
// not surprising, since it's practically death-by-polymorphism (e.g. the optional validators
// members that might be arrays). Watch for ControlConfigs that might fall through.
T extends Array<infer U|ValidatorFn|ValidatorFn[]|AsyncValidatorFn|AsyncValidatorFn[]> ? FormControl<U|N> :
// Fallthough case: T is not a container type; use it directly as a value.
FormControl<T|N>;

Typescript expands the type union in the type map, because different parts of the type union might end up in different branches of the type map.
Here's a minimal example (playground)

type IsNumber<T> = T extends number ? true : false;

// false
type Test1 =  IsNumber<string>;

// true
type Test2 = IsNumber<number>;

// boolean
type Test3 = IsNumber<string | number>;

This can be solved by wrapping the type in the map with [] (playground):

type IsExactlyNumber<T> = [T] extends [number] ? true : false;

// false
type Test1 =  IsExactlyNumber<string>;

// true
type Test2 = IsExactlyNumber<number>;

// false
type Test3 = IsExactlyNumber<string | number>;
@dylhunn
Copy link
Contributor

dylhunn commented May 10, 2022

(removed, working on a patch to fix this)

dylhunn added a commit to dylhunn/angular that referenced this issue May 10, 2022
…ypes.

Previously, using `FormBuilder` with a union type would produce unions of *controls*:

```
// `foo` has type `FormControl<string>|FormControl<number>`.
const c = fb.group({foo: 'bar' as string | number});
```

This actually works in many cases, due to how sophisticated Typescript's distributed types are (e.g. `value` still has type `string|number`), but it is subtly incorrect. Here is a code example that exposes the reason the inference is incorrect:

```
let fc = c.controls.foo;
// Error: Type 'FormControl<string | number>' is not assignable to type 'FormControl<string> | FormControl<number>'.ts
fc = new FormControl<string|number>('');
```

Instead, we want the union to apply to the *values*:

```
// `foo` should have type `FormControl<string|number>`.
const c = fb.group({foo: 'bar' as string | number});
```

Essentially, we want to prevent Typescript from distributing the type. [As specified in the handbook](https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types):

> Typically, distributivity is the desired behavior. To avoid that behavior, you can surround each side of the extends keyword with square brackets.

This PR applies this suggestion to `FormBuilder`'s type inference.

Fixes angular#45912.
dylhunn added a commit to dylhunn/angular that referenced this issue May 10, 2022
…ypes.

Previously, using `FormBuilder` with a union type would produce unions of *controls*:

```
// `foo` has type `FormControl<string>|FormControl<number>`.
const c = fb.nonNullable.group({foo: 'bar' as string | number});
```

This actually works in many cases, due to how extraordinarily powerful Typescript's distributive types are (e.g. `value` still has type `string|number`), but it is subtly incorrect. Here is a code example that exposes the reason the inference is incorrect. It exploits the fact that Typescript will not "un-distribute" a type, producing an obviously spurious error:

```
// fc gets an inferred distributive union type `FormControl<string> | FormControl<number>`
let fc = c.controls.foo;
// Error: Type 'FormControl<string | number>' is not assignable to type 'FormControl<string> | FormControl<number>'.
fc = new FormControl<string|number>('', {initialValueIsDefault: true});
```

Instead, we want the union to apply to the *values*:

```
// `foo` should have type `FormControl<string|number>`.
const c = fb.nonNullable.group({foo: 'bar' as string | number});
```

Essentially, we want to prevent Typescript from distributing the type. [As specified in the handbook](https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types):

> Typically, distributivity is the desired behavior. To avoid that behavior, you can surround each side of the extends keyword with square brackets.

This PR applies this suggestion to `FormBuilder`'s type inference.

Fixes angular#45912.
@dylhunn
Copy link
Contributor

dylhunn commented May 10, 2022

I believe #45942 should fix the problem. Thanks for catching this.

Typescript's support for unifying distributive types is shockingly good. Even with the previous bug, this still compiled:

const c = fb.nonNullable.group({foo: 'bar' as string | number});
let newCtrl: FormControl<string|number> = c.controls.foo;

because Typescript will perform the distribution when assigning to FormControl<string|number>. Hence, this is actually really hard to catch in action causing problems. The problematic cases do exist though:

// fc gets an inferred distributive union type `FormControl<string> | FormControl<number>`
let fc = c.controls.foo;
// Error: Type 'FormControl<string | number>' is not assignable to type 'FormControl<string> | FormControl<number>'.
fc = new FormControl<string|number>('', {initialValueIsDefault: true});

AndrewKushnir pushed a commit that referenced this issue May 10, 2022
…ypes. (#45942)

Previously, using `FormBuilder` with a union type would produce unions of *controls*:

```
// `foo` has type `FormControl<string>|FormControl<number>`.
const c = fb.nonNullable.group({foo: 'bar' as string | number});
```

This actually works in many cases, due to how extraordinarily powerful Typescript's distributive types are (e.g. `value` still has type `string|number`), but it is subtly incorrect. Here is a code example that exposes the reason the inference is incorrect. It exploits the fact that Typescript will not "un-distribute" a type, producing an obviously spurious error:

```
// fc gets an inferred distributive union type `FormControl<string> | FormControl<number>`
let fc = c.controls.foo;
// Error: Type 'FormControl<string | number>' is not assignable to type 'FormControl<string> | FormControl<number>'.
fc = new FormControl<string|number>('', {initialValueIsDefault: true});
```

Instead, we want the union to apply to the *values*:

```
// `foo` should have type `FormControl<string|number>`.
const c = fb.nonNullable.group({foo: 'bar' as string | number});
```

Essentially, we want to prevent Typescript from distributing the type. [As specified in the handbook](https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types):

> Typically, distributivity is the desired behavior. To avoid that behavior, you can surround each side of the extends keyword with square brackets.

This PR applies this suggestion to `FormBuilder`'s type inference.

Fixes #45912.

PR Close #45942
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants