Ivy compiler incorrectly allows undefined to be passed to any component input even without explicit undefined in input type when strictNullChecks is on #32690

compeek · 2019-09-16T04:31:47Z

🐞 bug report

Affected Package

Not exactly sure, but probably:
@angular/compiler
@angular/compiler-cli

Is this a regression?

No, from my experience, the old compiler didn't enforce strictNullChecks in templates at all, so this is new to Ivy.

Description

With strictNullChecks turned on, if you have a component input with a certain type that does not explicitly include undefined, the Ivy compiler does not complain if you pass undefined to that input from another component. This defeats a lot of the purpose of using strictNullChecks. (Note that it does complain about passing null if null isn't explicitly in the type, so I know strictNullChecks is enabled properly.)

I believe this is due to the use of Partial<Pick<...>> in the type check block for the template as described here: https://blog.angularindepth.com/type-checking-templates-in-angular-viewengine-and-ivy-77f8536359f5

Partial<...> causes each property to be made optional, which in effect adds undefined to the type of each property as well, as described in the TypeScript docs: Optional parameters and properties automatically have undefined added to their types, even when their type annotations don’t specifically include undefined.. (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-0.html#optional-parameters-and-properties)

So, what seems to be happening is the Ivy compiler is adding undefined to the type of every input when considering the bindings, which means it will never complain about passing undefined to the input of any component. That really makes strictNullChecks a lot less useful.

Please understand that although nobody in practice would probably pass a hardcoded undefined to a component input, this is an actual problem any time you use undefined to represent the absence of a value being passed to a component. In my case, I have a store I subscribe to using the async pipe, and then some of the state gets passed into child components from there. Various properties on my state have types like SomeModel | undefined. undefined could mean the model is not yet loaded, for example. Although some people may use null instead, there seems to be a significant group that prefers to use undefined in all cases. Wherever you fall on that debate, my point is that this definitely comes into play in real projects, and in fact I'm having the issue after switching to Ivy in my own project. I can't have a lot of confidence in what I'm passing to my components' inputs if undefined is always allowed by the compiler regardless of the actual type of the inputs.

🔬 Minimal Reproduction

https://github.com/compeek/angular-ivy-input-undefined-issue

If you compile this project, you should only see a compiler error about null and not about undefined as well. See index.html for some more explanation of the example.

You can reproduce this easily in any project with strictNullChecks turned on and the Ivy compiler enabled with these two components (from my minimal reproduction project):

@Component({
  selector: 'app-root',
  template: `<app-child [stringOnlyInput1]="thisPropertyIsUndefined" [stringOnlyInput2]="thisPropertyIsNull"></app-child>`
})
export class AppComponent {
  thisPropertyIsUndefined: undefined = undefined;
  thisPropertyIsNull: null = null;
}

@Component({
  selector: 'app-child',
  template: `
    {{ 'stringOnlyInput1: ' + stringOnlyInput1 }}
    <br>
    {{ 'stringOnlyInput2: ' + stringOnlyInput2 }}
  `
})
export class ChildComponent {
  @Input() stringOnlyInput1: string = '';
  @Input() stringOnlyInput2: string = '';
}

The compiler should complain about the values passed to both inputs, but in fact it only complains about the null value and not undefined.

🔥 Exception or Error

I would expect an error like:
Type 'undefined' is not assignable to type 'string'.

But none is thrown.

And in fact if you do it with null, the error looks like this:
Type 'null' is not assignable to type 'string | undefined'.

Notice how it says it's not assignable to string | undefined even though the input type is just string.

🌍 Your Environment

Angular Version:



Angular CLI: 9.0.0-next.4
Node: 10.16.3
OS: win32 x64
Angular: 9.0.0-next.6
... animations, common, compiler, compiler-cli, core
... language-service, platform-browser, platform-browser-dynamic
... router

Package                           Version
-----------------------------------------------------------
@angular-devkit/architect         0.900.0-next.4
@angular-devkit/build-angular     0.900.0-next.4
@angular-devkit/build-optimizer   0.900.0-next.4
@angular-devkit/build-webpack     0.900.0-next.4
@angular-devkit/core              9.0.0-next.4
@angular-devkit/schematics        9.0.0-next.4
@angular/cli                      9.0.0-next.4
@ngtools/webpack                  9.0.0-next.4
@schematics/angular               9.0.0-next.4
@schematics/update                0.900.0-next.4
rxjs                              6.5.3
typescript                        3.5.3
webpack                           4.39.3

Anything else relevant?

The text was updated successfully, but these errors were encountered:

alxhub · 2019-10-08T22:19:53Z

This is https://angular-team.atlassian.net/browse/FW-1606. @JoostK has a fix :)

compeek · 2019-10-09T01:58:58Z

This is https://angular-team.atlassian.net/browse/FW-1606. Joost Koehoorn has a fix :)

Unfortunately I do not have access to that, so I'm not sure what you're referencing. :( However if there's a fix, that's great! Any idea when it will be shipped?

JoostK · 2019-10-09T04:01:11Z

@compeek the idea is that we will remove the Partial mapped type and instead ensure to generate all possible inputs in the object literal, where any input that is not used is set to null as any. This achieves the same goal of allowing not all inputs to be present, without undesirably introducing undefined for all inputs.

Expect a PR to be out later this week.

compeek · 2019-10-09T04:21:53Z

@JoostK, when you say any input that is not used is set to null as any, do you mean that any input not specified in the template will automatically get a value of null passed to it? If so, wouldn't that override any default values set on those input properties?

JoostK · 2019-10-09T04:33:06Z

Oh these changes will all be in the generated code that's used for type checking, which will never actually execute. The generated code that the compiler generates for runtime execution will not be affected.

compeek · 2019-10-09T04:36:40Z

Oh, I see now. Excellent. Thank you for the information! I look forward to this fix coming out in a "next" release so I can start using it in my project.

Prior to this change, the template type checker would always allow a value of type `undefined` to be passed into a directive's inputs, even if the input's type did not allow for it. This was due to how the type constructor for a directive was generated, where a `Partial` mapped type was used to allow for inputs to be unset. This essentially introduces the `undefined` type as acceptable type for all inputs. This commit removes the `Partial` type from the type constructor, which means that we can no longer omit any properties that were unset. Instead, any properties that are not set will still be included in the type constructor call, having their value assigned to `any`. Before: ```typescript class NgForOf<T> { static ngTypeCtor<T>(init: Partial<Pick<NgForOf<T>, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf<T>; } NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']}); ``` After: ```typescript class NgForOf<T> { static ngTypeCtor<T>(init: Pick<NgForOf<T>, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf<T>; } NgForOf.ngTypeCtor(init: { ngForOf: ['foo', 'bar'], ngForTrackBy: null as any, ngForTemplate: null as any, }); ``` This change only affects generated type check code, the generated runtime code is not affected. Fixes angular#32690 Resolves FW-1606

…lar#33066) Prior to this change, the template type checker would always allow a value of type `undefined` to be passed into a directive's inputs, even if the input's type did not allow for it. This was due to how the type constructor for a directive was generated, where a `Partial` mapped type was used to allow for inputs to be unset. This essentially introduces the `undefined` type as acceptable type for all inputs. This commit removes the `Partial` type from the type constructor, which means that we can no longer omit any properties that were unset. Instead, any properties that are not set will still be included in the type constructor call, having their value assigned to `any`. Before: ```typescript class NgForOf<T> { static ngTypeCtor<T>(init: Partial<Pick<NgForOf<T>, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>>): NgForOf<T>; } NgForOf.ngTypeCtor(init: {ngForOf: ['foo', 'bar']}); ``` After: ```typescript class NgForOf<T> { static ngTypeCtor<T>(init: Pick<NgForOf<T>, 'ngForOf'|'ngForTrackBy'|'ngForTemplate'>): NgForOf<T>; } NgForOf.ngTypeCtor(init: { ngForOf: ['foo', 'bar'], ngForTrackBy: null as any, ngForTemplate: null as any, }); ``` This change only affects generated type check code, the generated runtime code is not affected. Fixes angular#32690 Resolves FW-1606 PR Close angular#33066

angular-automatic-lock-bot · 2019-11-14T16:10:06Z

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.}

AndrewKushnir added area: compiler Issues related to `ngc`, Angular's template compiler comp: ivy labels Sep 16, 2019

ngbot bot added this to the needsTriage milestone Sep 16, 2019

compeek closed this as completed Oct 9, 2019

compeek reopened this Oct 9, 2019

JoostK mentioned this issue Oct 9, 2019

fix(ivy): do not always accept undefined for directive inputs #33066

Closed

mhevery closed this as completed in 50bf17a Oct 14, 2019

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ivy compiler incorrectly allows undefined to be passed to any component input even without explicit undefined in input type when strictNullChecks is on #32690

Ivy compiler incorrectly allows undefined to be passed to any component input even without explicit undefined in input type when strictNullChecks is on #32690

compeek commented Sep 16, 2019 •

edited

alxhub commented Oct 8, 2019

compeek commented Oct 9, 2019 •

edited

JoostK commented Oct 9, 2019

compeek commented Oct 9, 2019 •

edited

JoostK commented Oct 9, 2019

compeek commented Oct 9, 2019 •

edited

angular-automatic-lock-bot bot commented Nov 14, 2019

Ivy compiler incorrectly allows undefined to be passed to any component input even without explicit undefined in input type when strictNullChecks is on #32690

Ivy compiler incorrectly allows undefined to be passed to any component input even without explicit undefined in input type when strictNullChecks is on #32690

Comments

compeek commented Sep 16, 2019 • edited

🐞 bug report

Affected Package

Is this a regression?

Description

🔬 Minimal Reproduction

🔥 Exception or Error

🌍 Your Environment

alxhub commented Oct 8, 2019

compeek commented Oct 9, 2019 • edited

JoostK commented Oct 9, 2019

compeek commented Oct 9, 2019 • edited

JoostK commented Oct 9, 2019

compeek commented Oct 9, 2019 • edited

angular-automatic-lock-bot bot commented Nov 14, 2019

compeek commented Sep 16, 2019 •

edited

compeek commented Oct 9, 2019 •

edited

compeek commented Oct 9, 2019 •

edited

compeek commented Oct 9, 2019 •

edited