fix(ngcc): switch ngcc over to use the `TraitCompiler` #34889

JoostK · 2020-01-21T23:08:44Z

fix(ngcc): do not attempt compilation when analysis fails

In #34288, ngtsc was refactored to separate the result of the analysis
and resolve phase for more granular incremental rebuilds. In this model,
any errors in one phase transition the trait into an error state, which
prevents it from being ran through subsequent phases. The ngcc compiler
on the other hand did not adopt this strict error model, which would
cause incomplete metadata—due to errors in earlier phases—to be offered
for compilation that could result in a hard crash.

This commit updates ngcc to take advantage of ngtsc's TraitCompiler,
that internally manages all Ivy classes that are part of the
compilation. This effectively replaces ngcc's own AnalyzedFile and
AnalyzedClass types, together with all of the logic to drive the
DecoratorHandlers. All of this is now handled in the TraitCompiler,
benefiting from its explicit state transitions of Traits so that the
ngcc crash is a thing of the past.

Fixes #34500
Resolves FW-1788

JoostK · 2020-01-21T23:15:51Z

This PR is a replacement for #34652, with a different approach. Instead of making use of the Trait types from within ngcc, this change goes one step further to drive the compilation using the TraitCompiler. The benefit of this is that the protocol around the calling of DecoratorHandlers isn't duplicated between ngtsc and ngcc, with the drawback of introducing a larger diff with more risk involved.

petebacondarwin

Nice refactoring! It is great to see so much code removed from ngcc.
Can you add the NgccTraitCompiler spec and simplify the DefaultMigrationHost spec as mentioned below.
Otherwise LGTM!

packages/compiler-cli/ngcc/src/analysis/decoration_analyzer.ts

packages/compiler-cli/ngcc/src/analysis/ngcc_trait_compiler.ts

packages/compiler-cli/ngcc/src/rendering/dts_renderer.ts

packages/compiler-cli/ngcc/test/analysis/decoration_analyzer_spec.ts

petebacondarwin · 2020-01-23T08:40:52Z

packages/compiler-cli/ngcc/test/analysis/migration_host_spec.ts

 import {createComponentDecorator} from '../../src/migrations/utils';
+import {EntryPointBundle} from '../../src/packages/entry_point_bundle';
+import {MockLogger} from '../helpers/mock_logger';
+import {makeTestEntryPointBundle} from '../helpers/utils';

 runInEachFileSystem(() => {
  describe('DefaultMigrationHost', () => {


It seems to me that most of this spec file is actually testing the NgccTraitCompiler. The DefaultMigrationHost mostly just delegates to that.

How about simplifying this spec just to check that the delegation occurs as expected, and then create a spec for NgccTraitCompiler that exercises the tests here?

I'm really not fond of testing just delegation behaviour, as doing so starts to be testing an implementation rather than some desired behaviour. I do see however where you're coming from here, and have moved most of the injectSyntheticDecorator and getAllDecorators tests to an NgccTraitCompiler spec file, while retaining some in the DefaultMigrationHost specs to test the logic in the context of the migration host.

packages/compiler-cli/src/ngtsc/transform/src/api.ts

packages/compiler-cli/src/ngtsc/transform/src/compilation.ts

This syntax is invalid in these source files and does result in compilation errors as the constructor parameters could not be resolved. This hasn't been an issue until now as those errors were ignored in the tests, but future work to introduce the Trait system of ngtsc into ngcc will cause these errors to prevent compilation, resulting in broken tests.

In angular#34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`, that internally manages all Ivy classes that are part of the compilation. This effectively replaces ngcc's own `AnalyzedFile` and `AnalyzedClass` types, together with all of the logic to drive the `DecoratorHandler`s. All of this is now handled in the `TraitCompiler`, benefiting from its explicit state transitions of `Trait`s so that the ngcc crash is a thing of the past. Fixes angular#34500 Resolves FW-1788

JoostK · 2020-01-23T20:18:46Z

This is green on CI: angular/ngcc-validation#792

@petebacondarwin PTAL

petebacondarwin

Grand!

JoostK · 2020-01-23T21:47:10Z

merge-assistence: Alex has approved on behalf of fw-compiler when it was still draft.

AndrewKushnir · 2020-01-23T21:50:22Z

Presubmit

In #34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`, that internally manages all Ivy classes that are part of the compilation. This effectively replaces ngcc's own `AnalyzedFile` and `AnalyzedClass` types, together with all of the logic to drive the `DecoratorHandler`s. All of this is now handled in the `TraitCompiler`, benefiting from its explicit state transitions of `Trait`s so that the ngcc crash is a thing of the past. Fixes #34500 Resolves FW-1788 PR Close #34889

…34889) This syntax is invalid in these source files and does result in compilation errors as the constructor parameters could not be resolved. This hasn't been an issue until now as those errors were ignored in the tests, but future work to introduce the Trait system of ngtsc into ngcc will cause these errors to prevent compilation, resulting in broken tests. PR Close #34889

In #34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`, that internally manages all Ivy classes that are part of the compilation. This effectively replaces ngcc's own `AnalyzedFile` and `AnalyzedClass` types, together with all of the logic to drive the `DecoratorHandler`s. All of this is now handled in the `TraitCompiler`, benefiting from its explicit state transitions of `Trait`s so that the ngcc crash is a thing of the past. Fixes #34500 Resolves FW-1788 PR Close #34889

…ngular#34889) This syntax is invalid in these source files and does result in compilation errors as the constructor parameters could not be resolved. This hasn't been an issue until now as those errors were ignored in the tests, but future work to introduce the Trait system of ngtsc into ngcc will cause these errors to prevent compilation, resulting in broken tests. PR Close angular#34889

) In angular#34288, ngtsc was refactored to separate the result of the analysis and resolve phase for more granular incremental rebuilds. In this model, any errors in one phase transition the trait into an error state, which prevents it from being ran through subsequent phases. The ngcc compiler on the other hand did not adopt this strict error model, which would cause incomplete metadata—due to errors in earlier phases—to be offered for compilation that could result in a hard crash. This commit updates ngcc to take advantage of ngtsc's `TraitCompiler`, that internally manages all Ivy classes that are part of the compilation. This effectively replaces ngcc's own `AnalyzedFile` and `AnalyzedClass` types, together with all of the logic to drive the `DecoratorHandler`s. All of this is now handled in the `TraitCompiler`, benefiting from its explicit state transitions of `Trait`s so that the ngcc crash is a thing of the past. Fixes angular#34500 Resolves FW-1788 PR Close angular#34889

angular-automatic-lock-bot · 2020-02-23T16:09:26Z

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

JoostK added type: bug/fix action: review The PR is still awaiting reviews from at least one requested reviewer severity3: broken target: patch This PR is targeted for the next patch release risk: medium labels Jan 21, 2020

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

googlebot added the cla: yes label Jan 21, 2020

alxhub approved these changes Jan 22, 2020

View reviewed changes

JoostK marked this pull request as ready for review January 23, 2020 06:26

JoostK requested review from a team as code owners January 23, 2020 06:26

JoostK requested a review from petebacondarwin January 23, 2020 06:26

petebacondarwin requested changes Jan 23, 2020

View reviewed changes

JoostK mentioned this pull request Jan 23, 2020

fix(ngcc): do not attempt compilation when analysis fails #34652

Closed

JoostK added 3 commits January 23, 2020 19:35

fixup! fix(ngcc): do not attempt compilation when analysis fails

daa2f30

JoostK force-pushed the ngcc-with-trait-compiler branch from 6342500 to daa2f30 Compare January 23, 2020 18:35

JoostK added a commit to JoostK/ngcc-validation that referenced this pull request Jan 23, 2020

preview angular/angular#34889

6771c94

JoostK added a commit to JoostK/ngcc-validation that referenced this pull request Jan 23, 2020

preview angular/angular#34889

4e71f7e

JoostK added a commit to JoostK/ngcc-validation that referenced this pull request Jan 23, 2020

preview angular/angular#34889

6a365fb

petebacondarwin approved these changes Jan 23, 2020

View reviewed changes

JoostK added the action: presubmit The PR is in need of a google3 presubmit label Jan 23, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Jan 23, 2020

AndrewKushnir closed this in ba82532 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

fix(ngcc): switch ngcc over to use the `TraitCompiler` #34889

fix(ngcc): switch ngcc over to use the `TraitCompiler` #34889

JoostK commented Jan 21, 2020

JoostK commented Jan 21, 2020

petebacondarwin left a comment

petebacondarwin Jan 23, 2020

JoostK Jan 23, 2020

JoostK commented Jan 23, 2020

petebacondarwin left a comment

JoostK commented Jan 23, 2020

AndrewKushnir commented Jan 23, 2020

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

fix(ngcc): switch ngcc over to use the TraitCompiler #34889

fix(ngcc): switch ngcc over to use the TraitCompiler #34889

Conversation

JoostK commented Jan 21, 2020

JoostK commented Jan 21, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

petebacondarwin Jan 23, 2020

Choose a reason for hiding this comment

JoostK Jan 23, 2020

Choose a reason for hiding this comment

JoostK commented Jan 23, 2020

petebacondarwin left a comment

Choose a reason for hiding this comment

JoostK commented Jan 23, 2020

AndrewKushnir commented Jan 23, 2020

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

fix(ngcc): switch ngcc over to use the `TraitCompiler` #34889

fix(ngcc): switch ngcc over to use the `TraitCompiler` #34889