Batch of Template Type-checking fixes #34649

alxhub · 2020-01-06T18:38:19Z

This is a batch PR fixing several template type-checking issues that were discovered in the g3 codebase:

Fixes an issue with bindings which map to more than one field. @Input('x') x; @Input('x') y; for example (amazingly, this is not an error).
Fixes an issue with repeated bindings to fields ([x]="..." [x]="...").
Corrects the precedence of as any casts generated by the template type-checker in certain cases (a ? b : c as any was previously generated where the intent was (a ? b : c) as any).
Disables the aliasing system when generating template type-checking code, as it won't work.
Adds a new strictness flag for context generics, so Component<T> has T inferred as any unless strictTemplates is enabled.

JoostK

Some minor things that need some cleaning up.

JoostK · 2020-01-06T19:23:25Z

packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts

+
+        @Directive({
+          selector: '[dir]',
+          inputs: ['propertyName'],


Please see #34482 for a suggestion on how to make this setup more clear.

packages/compiler-cli/src/ngtsc/typecheck/src/ts_util.ts

packages/compiler-cli/src/ngtsc/typecheck/src/host.ts

packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts

aio/content/guide/template-typecheck.md

JoostK · 2020-01-06T19:46:34Z

packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts

+          node.typeParameters.map(param => ts.createTypeReferenceNode(param.name, undefined));
+    } else {
+      typeArguments = [];
+      for (let i = 0; i < node.typeParameters.length; i++) {


No .map here? :-)

It felt a little weird to use, since we're ignoring the value. Done though.

packages/compiler-cli/src/transformers/api.ts

JoostK

LGTM, but I'd like to see https://github.com/angular/angular/pull/34649/files#r363443363 addressed.

It's possible to declare multiple inputs for a directive/component which all map to the same property name. This is usually done in error, as only one of any bindings to the property will "win". In the template type-checker, an error was previously being raised as a result of this ambiguity. Specifically, a type constructor was produced which required a binding for each field, but only one of the fields had a value via the binding. TypeScript would (rightfully) error on missing values for the remaining fields. This ultimately was happening when the code which generated the default values for "unset" inputs belonging to directives or pipes used the final mapping from properties to fields as a source for field names. Instead, this commit uses the original list of fields to generate unset input values, which correctly provides values for fields which shared a property name but didn't receive the final binding.

Currently, the template type-checker gives an error if there are multiple bindings to the same input. This commit aligns the behavior of the template type-checker with the View Engine runtime: only the first binding to a field has any effect. The rest are ignored.

Previously, when generating template type-checking code, casts to 'any' were produced as `expr as any`, regardless of the expression. However, for certain expression types, this led to precedence issues with the cast. For example, `a !== b` is a `ts.BinaryExpression`, and wrapping it directly in the cast yields `a !== b as any`, which is semantically equivalent to `a !== (b as any)`. This is obviously not what is intended. Instead, this commit adds a list of expression types for which a "bare" wrapping is permitted. For other expressions, parentheses are added to ensure correct precedence: `(a !== b) as any`

Previously, `ReferenceEmitter.emit()` took an `ImportMode` enum value, where one value of the enum allowed forcing new imports to be generated when emitting a reference to some value or type. This commit refactors `ImportMode` to be an `ImportFlags` value instead. Using a bit field of flags will allow future customization of reference emitting.

alxhub · 2020-01-21T23:57:48Z

Done :)

FileToModuleHost aliasing supports compilation within environments that have two properties: 1. A `FileToModuleHost` exists which defines canonical module names for any given TS file. 2. Dependency restrictions exist which prevent the import of arbitrary files even if such files are within the .d.ts transitive closure of a compilation ("strictdeps"). In such an environment, generated imports can only go through import paths which are already present in the user program. The aliasing system supports the generation and consumption of such imports at runtime. `FileToModuleHost` aliasing does not emit re-exports in .d.ts files. This means that it's safe to rely on alias re-exports in generated .js code (they are guaranteed to exist at runtime) but not in template type-checking code (since TS will not be able to follow such imports). Therefore, non-aliased imports should be used in template type-checking code. This commit adds a `NoAliasing` flag to `ImportFlags` and sets it when generating imports in template type-checking code. The testing environment is also patched to support resolution of FileToModuleHost canonical paths within the template type-checking program, enabling testing of this change.

Previously, the template type-checker would always construct a generic template context type with correct bounds, even when strictTemplates was disabled. This meant that type-checking of expressions involving that type was stricter than View Engine. This commit introduces a 'strictContextGenerics' flag which behaves similarly to other 'strictTemplates' flags, and switches the inference of generic type parameters on the component context based on the value of this flag.

alxhub · 2020-01-22T00:06:08Z

Presubmit

…34649) It's possible to declare multiple inputs for a directive/component which all map to the same property name. This is usually done in error, as only one of any bindings to the property will "win". In the template type-checker, an error was previously being raised as a result of this ambiguity. Specifically, a type constructor was produced which required a binding for each field, but only one of the fields had a value via the binding. TypeScript would (rightfully) error on missing values for the remaining fields. This ultimately was happening when the code which generated the default values for "unset" inputs belonging to directives or pipes used the final mapping from properties to fields as a source for field names. Instead, this commit uses the original list of fields to generate unset input values, which correctly provides values for fields which shared a property name but didn't receive the final binding. PR Close #34649

Currently, the template type-checker gives an error if there are multiple bindings to the same input. This commit aligns the behavior of the template type-checker with the View Engine runtime: only the first binding to a field has any effect. The rest are ignored. PR Close #34649

Previously, when generating template type-checking code, casts to 'any' were produced as `expr as any`, regardless of the expression. However, for certain expression types, this led to precedence issues with the cast. For example, `a !== b` is a `ts.BinaryExpression`, and wrapping it directly in the cast yields `a !== b as any`, which is semantically equivalent to `a !== (b as any)`. This is obviously not what is intended. Instead, this commit adds a list of expression types for which a "bare" wrapping is permitted. For other expressions, parentheses are added to ensure correct precedence: `(a !== b) as any` PR Close #34649

Previously, `ReferenceEmitter.emit()` took an `ImportMode` enum value, where one value of the enum allowed forcing new imports to be generated when emitting a reference to some value or type. This commit refactors `ImportMode` to be an `ImportFlags` value instead. Using a bit field of flags will allow future customization of reference emitting. PR Close #34649

FileToModuleHost aliasing supports compilation within environments that have two properties: 1. A `FileToModuleHost` exists which defines canonical module names for any given TS file. 2. Dependency restrictions exist which prevent the import of arbitrary files even if such files are within the .d.ts transitive closure of a compilation ("strictdeps"). In such an environment, generated imports can only go through import paths which are already present in the user program. The aliasing system supports the generation and consumption of such imports at runtime. `FileToModuleHost` aliasing does not emit re-exports in .d.ts files. This means that it's safe to rely on alias re-exports in generated .js code (they are guaranteed to exist at runtime) but not in template type-checking code (since TS will not be able to follow such imports). Therefore, non-aliased imports should be used in template type-checking code. This commit adds a `NoAliasing` flag to `ImportFlags` and sets it when generating imports in template type-checking code. The testing environment is also patched to support resolution of FileToModuleHost canonical paths within the template type-checking program, enabling testing of this change. PR Close #34649

…34649) Previously, the template type-checker would always construct a generic template context type with correct bounds, even when strictTemplates was disabled. This meant that type-checking of expressions involving that type was stricter than View Engine. This commit introduces a 'strictContextGenerics' flag which behaves similarly to other 'strictTemplates' flags, and switches the inference of generic type parameters on the component context based on the value of this flag. PR Close #34649

Currently, the template type-checker gives an error if there are multiple bindings to the same input. This commit aligns the behavior of the template type-checker with the View Engine runtime: only the first binding to a field has any effect. The rest are ignored. PR Close #34649

Previously, when generating template type-checking code, casts to 'any' were produced as `expr as any`, regardless of the expression. However, for certain expression types, this led to precedence issues with the cast. For example, `a !== b` is a `ts.BinaryExpression`, and wrapping it directly in the cast yields `a !== b as any`, which is semantically equivalent to `a !== (b as any)`. This is obviously not what is intended. Instead, this commit adds a list of expression types for which a "bare" wrapping is permitted. For other expressions, parentheses are added to ensure correct precedence: `(a !== b) as any` PR Close #34649

Previously, `ReferenceEmitter.emit()` took an `ImportMode` enum value, where one value of the enum allowed forcing new imports to be generated when emitting a reference to some value or type. This commit refactors `ImportMode` to be an `ImportFlags` value instead. Using a bit field of flags will allow future customization of reference emitting. PR Close #34649

FileToModuleHost aliasing supports compilation within environments that have two properties: 1. A `FileToModuleHost` exists which defines canonical module names for any given TS file. 2. Dependency restrictions exist which prevent the import of arbitrary files even if such files are within the .d.ts transitive closure of a compilation ("strictdeps"). In such an environment, generated imports can only go through import paths which are already present in the user program. The aliasing system supports the generation and consumption of such imports at runtime. `FileToModuleHost` aliasing does not emit re-exports in .d.ts files. This means that it's safe to rely on alias re-exports in generated .js code (they are guaranteed to exist at runtime) but not in template type-checking code (since TS will not be able to follow such imports). Therefore, non-aliased imports should be used in template type-checking code. This commit adds a `NoAliasing` flag to `ImportFlags` and sets it when generating imports in template type-checking code. The testing environment is also patched to support resolution of FileToModuleHost canonical paths within the template type-checking program, enabling testing of this change. PR Close #34649

…34649) Previously, the template type-checker would always construct a generic template context type with correct bounds, even when strictTemplates was disabled. This meant that type-checking of expressions involving that type was stricter than View Engine. This commit introduces a 'strictContextGenerics' flag which behaves similarly to other 'strictTemplates' flags, and switches the inference of generic type parameters on the component context based on the value of this flag. PR Close #34649

…ngular#34649) It's possible to declare multiple inputs for a directive/component which all map to the same property name. This is usually done in error, as only one of any bindings to the property will "win". In the template type-checker, an error was previously being raised as a result of this ambiguity. Specifically, a type constructor was produced which required a binding for each field, but only one of the fields had a value via the binding. TypeScript would (rightfully) error on missing values for the remaining fields. This ultimately was happening when the code which generated the default values for "unset" inputs belonging to directives or pipes used the final mapping from properties to fields as a source for field names. Instead, this commit uses the original list of fields to generate unset input values, which correctly provides values for fields which shared a property name but didn't receive the final binding. PR Close angular#34649

Currently, the template type-checker gives an error if there are multiple bindings to the same input. This commit aligns the behavior of the template type-checker with the View Engine runtime: only the first binding to a field has any effect. The rest are ignored. PR Close angular#34649

Previously, when generating template type-checking code, casts to 'any' were produced as `expr as any`, regardless of the expression. However, for certain expression types, this led to precedence issues with the cast. For example, `a !== b` is a `ts.BinaryExpression`, and wrapping it directly in the cast yields `a !== b as any`, which is semantically equivalent to `a !== (b as any)`. This is obviously not what is intended. Instead, this commit adds a list of expression types for which a "bare" wrapping is permitted. For other expressions, parentheses are added to ensure correct precedence: `(a !== b) as any` PR Close angular#34649

Previously, `ReferenceEmitter.emit()` took an `ImportMode` enum value, where one value of the enum allowed forcing new imports to be generated when emitting a reference to some value or type. This commit refactors `ImportMode` to be an `ImportFlags` value instead. Using a bit field of flags will allow future customization of reference emitting. PR Close angular#34649

…34649) FileToModuleHost aliasing supports compilation within environments that have two properties: 1. A `FileToModuleHost` exists which defines canonical module names for any given TS file. 2. Dependency restrictions exist which prevent the import of arbitrary files even if such files are within the .d.ts transitive closure of a compilation ("strictdeps"). In such an environment, generated imports can only go through import paths which are already present in the user program. The aliasing system supports the generation and consumption of such imports at runtime. `FileToModuleHost` aliasing does not emit re-exports in .d.ts files. This means that it's safe to rely on alias re-exports in generated .js code (they are guaranteed to exist at runtime) but not in template type-checking code (since TS will not be able to follow such imports). Therefore, non-aliased imports should be used in template type-checking code. This commit adds a `NoAliasing` flag to `ImportFlags` and sets it when generating imports in template type-checking code. The testing environment is also patched to support resolution of FileToModuleHost canonical paths within the template type-checking program, enabling testing of this change. PR Close angular#34649

…ngular#34649) Previously, the template type-checker would always construct a generic template context type with correct bounds, even when strictTemplates was disabled. This meant that type-checking of expressions involving that type was stricter than View Engine. This commit introduces a 'strictContextGenerics' flag which behaves similarly to other 'strictTemplates' flags, and switches the inference of generic type parameters on the component context based on the value of this flag. PR Close angular#34649

angular-automatic-lock-bot · 2020-02-23T16:08:40Z

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

alxhub requested a review from a team as a code owner January 6, 2020 18:38

googlebot added the cla: yes label Jan 6, 2020

alxhub added the target: patch This PR is targeted for the next patch release label Jan 6, 2020

JoostK requested changes Jan 6, 2020

View reviewed changes

JoostK added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 6, 2020

alxhub force-pushed the ngtsc/ttc/g3-fixes branch 2 times, most recently from 39c94c6 to 6056a8f Compare January 8, 2020 00:15

atscott added the area: compiler Issues related to `ngc`, Angular's template compiler label Jan 9, 2020

ngbot bot added this to the needsTriage milestone Jan 9, 2020

alxhub removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Jan 21, 2020

alxhub force-pushed the ngtsc/ttc/g3-fixes branch from 6056a8f to 258d41b Compare January 21, 2020 22:21

alxhub requested a review from a team as a code owner January 21, 2020 22:21

alxhub requested a review from JoostK January 21, 2020 22:21

JoostK approved these changes Jan 21, 2020

View reviewed changes

alxhub added 4 commits January 21, 2020 15:57

alxhub force-pushed the ngtsc/ttc/g3-fixes branch from 258d41b to 4d59c99 Compare January 21, 2020 23:57

alxhub added 2 commits January 21, 2020 16:04

alxhub force-pushed the ngtsc/ttc/g3-fixes branch from 4d59c99 to fba022e Compare January 22, 2020 00:04

alxhub mentioned this pull request Jan 22, 2020

fix(ivy): type-checking of properties which map to multiple fields #34482

Closed

alxhub added the action: merge The PR is ready for merge by the caretaker label Jan 23, 2020

AndrewKushnir closed this in 22c957a Jan 23, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch of Template Type-checking fixes #34649

Batch of Template Type-checking fixes #34649

alxhub commented Jan 6, 2020

JoostK left a comment

JoostK Jan 6, 2020

JoostK Jan 6, 2020

alxhub Jan 7, 2020

JoostK left a comment

alxhub commented Jan 21, 2020

alxhub commented Jan 22, 2020

angular-automatic-lock-bot bot commented Feb 23, 2020

Batch of Template Type-checking fixes #34649

Batch of Template Type-checking fixes #34649

Conversation

alxhub commented Jan 6, 2020

JoostK left a comment

Choose a reason for hiding this comment

JoostK Jan 6, 2020

Choose a reason for hiding this comment

JoostK Jan 6, 2020

Choose a reason for hiding this comment

alxhub Jan 7, 2020

Choose a reason for hiding this comment

JoostK left a comment

Choose a reason for hiding this comment

alxhub commented Jan 21, 2020

alxhub commented Jan 22, 2020

angular-automatic-lock-bot bot commented Feb 23, 2020