fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

mattlewis92 · 2019-06-27T23:27:16Z

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

Incremental compilation fails with ivy:

ERROR in Cannot read property 'has' of null
ℹ ｢wdm｣: Failed to compile.
/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/incremental/src/state.js:120
                .some(function (resourcePath) { return _this.modifiedResourceFiles.has(resourcePath); });
                                                                                   ^

TypeError: Cannot read property 'has' of null
    at /Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/incremental/src/state.js:120:84
    at Array.some (<anonymous>)
    at IncrementalState.hasChangedResourceDependencies (/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/incremental/src/state.js:120:18)
    at IncrementalState.safeToSkip (/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/incremental/src/state.js:70:57)
    at IvyCompilation.analyze (/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/transform/src/compilation.js:156:39)
    at IvyCompilation.analyzeSync (/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/transform/src/compilation.js:71:76)
    at /Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/program.js:257:39
    at Array.forEach (<anonymous>)
    at NgtscProgram.ensureAnalyzed (/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/program.js:255:22)
    at NgtscProgram.getNgSemanticDiagnostics (/Users/mattlewis/Code/clickup/frontend/node_modules/@angular/compiler-cli/src/ngtsc/program.js:167:36)

What is the new behavior?

Incremental compilation works

Does this PR introduce a breaking change?

Yes
No

Other information

I applied this patch locally and everything worked smoothly after! 😄

alfaproject · 2019-06-28T06:16:02Z

packages/compiler-cli/src/ngtsc/incremental/src/state.ts

@@ -112,7 +112,8 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
  }

  private hasChangedResourceDependencies(sf: ts.SourceFile): boolean {
-    if (this.modifiedResourceFiles === undefined || !this.metadata.has(sf)) {
+    if (this.modifiedResourceFiles === undefined || this.modifiedResourceFiles === null ||


Suggested change

if (this.modifiedResourceFiles === undefined || this.modifiedResourceFiles === null ||

if (this.modifiedResourceFiles == null ||

The type doesn't even allow for undefined, so I guess you could just check for null with triple equals or fix the type if it can really be undefined.

I'm not too sure, I guess that undefined check is there for a reason, let's see what @alxhub / @petebacondarwin say

Ah! This is my mistake. Sorry.

In the original draft of the PR the parameter was optional, which meant that it could be undefined or defined (not null) but then I made it non-optional and switched to using null to indicate that it is explicitly not needed.

So we should be able ditch the undefined check in favour of the null check.

We should also add tests for this bug.

petebacondarwin

@mattlewis92 - thanks for catching this.

Please just replace the === undefined with === null.

Do you fancy trying to add a test for this case to https://github.com/angular/angular/blob/master/packages/compiler-cli/test/ngtsc/incremental_spec.ts ?

If not, let me know and I can take over this PR and add the test.

mattlewis92 · 2019-06-28T12:34:52Z

Yeah sure, I can try writing a test. If I understand the problem right, I can copy from this test: https://github.com/angular/angular/blob/master/packages/compiler-cli/test/ngtsc/incremental_spec.ts#L79-L104 but then make a change to component1.ts instead and that should trigger the error?

petebacondarwin · 2019-06-28T12:46:23Z

@mattlewis92 - yes copy the test not modify ;-) and make the change as you suggest. Please check that this will fail before the fix is applied. Get in touch if you have difficulty running the tests or getting it to fail. Thanks!

mattlewis92 · 2019-06-28T13:25:37Z

@petebacondarwin I'm struggling to get this to fail as I can never get modifiedResourceFiles to be null, everything I try it always ends up being an empty Set. Any ideas?

petebacondarwin · 2019-06-28T13:44:53Z

😱 do you know what trigger in your project causes the error?

mattlewis92 · 2019-06-28T13:57:38Z

It was the initial load of the app, before the incremental compilation would have even triggered, which makes sense actually.

petebacondarwin · 2019-06-28T14:01:18Z

But the CLI implementation of that function always returns a Set<> not null nor undefined...
https://github.com/angular/angular-cli/blob/70a4cbe306e50af56e9c41a1b788991abb85687b/packages/ngtools/webpack/src/compiler_host.ts#L358

petebacondarwin · 2019-06-28T14:05:28Z

OK, so ngcc converts any falsy value to null:

    this.modifiedResourceFiles =
        this.host.getModifiedResourceFiles && this.host.getModifiedResourceFiles() || null;

So perhaps the this.host in your case did not have a getModifiedResourceFiles method?

petebacondarwin · 2019-06-28T14:08:32Z

Ahah! The test is at fault. See

angular/packages/compiler-cli/test/ngtsc/env.ts

Line 171 in 7186f9c

this.changedResources);

changedResources is of type Set<string>|undefined = undefined;

mattlewis92 · 2019-06-28T14:09:27Z

But the CLI implementation of that function always returns a Set<> not null nor undefined...
angular/angular-cli:packages/ngtools/webpack/src/compiler_host.ts@70a4cbe#L358

Ah, I'm using the 8.0.x version of the cli which doesn't have that method implemented so I guess it falls back to the default one provided by ngtsc, which would return undefined on the first run and then get converted to null by this:

this.modifiedResourceFiles =
this.host.getModifiedResourceFiles && this.host.getModifiedResourceFiles() || null;

petebacondarwin · 2019-06-28T14:11:52Z

If we change that param to null it will probably fail without any new tests!

petebacondarwin · 2019-06-28T14:13:05Z

So CLI is doing the right thing (even 8.0.0 version).
And your fix (to test for null rather than undefined) is correct.
The problem with getting a failing test is that the driveMain was not following the null type correctly.

mattlewis92 · 2019-06-28T14:30:39Z

I tried changing it to null but the issue is that enableMultipleCompilations then initialises it to a new set:

angular/packages/compiler-cli/test/ngtsc/env.ts

Line 97 in 7186f9c

this.changedResources = new Set();

and that's called on every spec run:

angular/packages/compiler-cli/test/ngtsc/incremental_spec.ts

Line 22 in 7186f9c

env.enableMultipleCompilations();

petebacondarwin · 2019-06-28T14:39:41Z

I'm just playing with it now...

petebacondarwin · 2019-06-28T14:50:41Z

try these changes:

diff --git a/packages/compiler-cli/src/main.ts b/packages/compiler-cli/src/main.ts
index 13ca9d5c22..7e03a4ecb3 100644
--- a/packages/compiler-cli/src/main.ts
+++ b/packages/compiler-cli/src/main.ts
@@ -26,7 +26,7 @@ export function main(
     config?: NgcParsedConfiguration, customTransformers?: api.CustomTransformers, programReuse?: {
       program: api.Program | undefined,
     },
-    modifiedResourceFiles?: Set<string>): number {
+    modifiedResourceFiles: Set<string>| null = null): number {
   let {project, rootNames, options, errors: configErrors, watch, emitFlags} =
       config || readNgcCommandLineAndConfiguration(args);
   if (configErrors.length) {
diff --git a/packages/compiler-cli/src/perform_compile.ts b/packages/compiler-cli/src/perform_compile.ts
index 4d27b2b323..f7ea7bbb59 100644
--- a/packages/compiler-cli/src/perform_compile.ts
+++ b/packages/compiler-cli/src/perform_compile.ts
@@ -224,7 +224,7 @@ export function exitCodeFromResult(diags: Diagnostics | undefined): number {
 export function performCompilation(
     {rootNames, options, host, oldProgram, emitCallback, mergeEmitResultsCallback,
      gatherDiagnostics = defaultGatherDiagnostics, customTransformers,
-     emitFlags = api.EmitFlags.Default, modifiedResourceFiles}: {
+     emitFlags = api.EmitFlags.Default, modifiedResourceFiles = null}: {
       rootNames: string[],
       options: api.CompilerOptions,
       host?: api.CompilerHost,
@@ -234,7 +234,7 @@ export function performCompilation(
       gatherDiagnostics?: (program: api.Program) => Diagnostics,
       customTransformers?: api.CustomTransformers,
       emitFlags?: api.EmitFlags,
-      modifiedResourceFiles?: Set<string>,
+      modifiedResourceFiles?: Set<string>| null,
     }): PerformCompilationResult {
   let program: api.Program|undefined;
   let emitResult: ts.EmitResult|undefined;
diff --git a/packages/compiler-cli/test/ngtsc/env.ts b/packages/compiler-cli/test/ngtsc/env.ts
index efb1fe6793..1f1639688d 100644
--- a/packages/compiler-cli/test/ngtsc/env.ts
+++ b/packages/compiler-cli/test/ngtsc/env.ts
@@ -26,7 +26,7 @@ import {setWrapHostForTest} from '../../src/transformers/compiler_host';
 export class NgtscTestEnvironment {
   private multiCompileHostExt: MultiCompileHostExt|null = null;
   private oldProgram: Program|null = null;
-  private changedResources: Set<string>|undefined = undefined;
+  private changedResources: Set<string>|null = null;
 
   private constructor(
       private fs: FileSystem, readonly outDir: AbsoluteFsPath, readonly basePath: AbsoluteFsPath) {}
@@ -107,6 +107,12 @@ export class NgtscTestEnvironment {
     this.multiCompileHostExt.flushWrittenFileTracking();
   }
 
+  /**
+   * Older versions of the CLI do not provide the `CompilerHost.getModifiedResourceFiles()` method.
+   * This results in the `changedResources` set being `null`.
+   */
+  simulateLegacyCLICompilerHost() { this.changedResources = null; }
+
   getFilesWrittenSinceLastFlush(): Set<string> {
     if (this.multiCompileHostExt === null) {
       throw new Error(`Not tracking written files - call enableMultipleCompilations()`);
diff --git a/packages/compiler-cli/test/ngtsc/incremental_spec.ts b/packages/compiler-cli/test/ngtsc/incremental_spec.ts
index 9cf9198feb..b4daca54ff 100644
--- a/packages/compiler-cli/test/ngtsc/incremental_spec.ts
+++ b/packages/compiler-cli/test/ngtsc/incremental_spec.ts
@@ -23,6 +23,24 @@ runInEachFileSystem(() => {
       env.tsconfig();
     });
 
+    it('should not crash if CLI does not provide getModifiedResourceFiles()', () => {
+      env.write('component1.ts', `
+      import {Component} from '@angular/core';
+
+      @Component({selector: 'cmp', templateUrl: './component1.template.html'})
+      export class Cmp1 {}
+    `);
+      env.write('component1.template.html', 'cmp1');
+      env.driveMain();
+
+      // Simulate a change to `component1.html`
+      env.flushWrittenFileTracking();
+      env.invalidateCachedFile('component1.html');
+      env.simulateLegacyCLICompilerHost();
+      env.driveMain();
+    });
+
     it('should skip unchanged services', () => {
       env.write('service.ts', `
       import {Injectable} from '@angular/core';

mattlewis92 · 2019-06-28T15:31:57Z

Awesome, that got it! Thanks so much for your help! 😄

petebacondarwin · 2019-07-10T09:25:49Z

Thanks for this @mattlewis92 - @alxhub is away at the moment but once he is back, we can get his approval and get this merged.

alxhub · 2019-07-11T11:42:55Z

Hi @mattlewis92,

This looks good to me! The only thing that I would ask is that you add some detail to the commit message describing the bug and the fix. In particular I'm interested in how this manages to escape type checking - how do we end up with modifiedResourceFiles being null when the types should not allow that?

alxhub · 2019-07-11T12:24:54Z

Presubmit

mattlewis92 · 2019-07-16T17:12:49Z

Hi @mattlewis92,

This looks good to me! The only thing that I would ask is that you add some detail to the commit message describing the bug and the fix. In particular I'm interested in how this manages to escape type checking - how do we end up with modifiedResourceFiles being null when the types should not allow that?

@petebacondarwin please can you help me out with what to put for the commit message? Thanks! 😄

petebacondarwin · 2019-07-17T15:26:50Z

packages/compiler-cli/src/ngtsc/incremental/src/state.ts

@@ -112,7 +112,7 @@ export class IncrementalState implements DependencyTracker, MetadataReader, Meta
  }

  private hasChangedResourceDependencies(sf: ts.SourceFile): boolean {
-    if (this.modifiedResourceFiles === undefined || !this.metadata.has(sf)) {
+    if (this.modifiedResourceFiles === null || !this.metadata.has(sf)) {


@alxhub this is the key change. The IncrementalState class takes a Set<string>|null for its modifiedResourceFiles parameter. The modifiedResourceFiles property could never be undefined, instead it could be a Set<string> or it could be null.

To tighten up the types across the system we changed everything else so that all properties and variables that could hold this modified resource files value could also never by undefined.

An alternative approach would have been to change every type to Set<string>|undefined instead. But my understanding is that you prefer to use null when something is explicitly not being provided.

petebacondarwin · 2019-07-17T15:53:48Z

@mattlewis92 - I would suggest something like the following for the commit message...

fix(ivy): support older CLI versions that do not pass a list of changed files

Versions of CLI prior to angular/angular-cli@0e339ee did not expose the host.getModifiedResourceFiles() method.

This meant that null was being passed through to the IncrementalState.reconcile() method
to indicate that there were either no changes or the host didn't support that method.

This commit fixes a bug where we were checking for undefined rather than null when
deciding whether any resource files had changed, causing a null reference error to be thrown.

This bug was not caught by the unit testing because the tests set up the changed files
via a slightly different process, not having access to the CompilerHost, and these test
were making the erroneous assumption that undefined indicated that there were no
changed files.

…ed files Versions of CLI prior to angular/angular-cli@0e339ee did not expose the host.getModifiedResourceFiles() method. This meant that null was being passed through to the IncrementalState.reconcile() method to indicate that there were either no changes or the host didn't support that method. This commit fixes a bug where we were checking for undefined rather than null when deciding whether any resource files had changed, causing a null reference error to be thrown. This bug was not caught by the unit testing because the tests set up the changed files via a slightly different process, not having access to the CompilerHost, and these test were making the erroneous assumption that undefined indicated that there were no changed files.

mattlewis92 · 2019-07-17T16:28:15Z

Awesome, thanks @petebacondarwin! I've just made that change now.

alxhub · 2019-07-18T20:07:11Z

Presubmit

stefanocke · 2019-07-30T11:08:08Z

I got the same error, but not with CLI, but just by calling ngc in watch mode (with "enableIvy": true).
Interestingly, it only happens on every second change.

mattlewis92 · 2019-07-30T11:50:50Z

@stefanocke ivy fixes don't get backported to 8.1.x, so you need to be on 8.2.0-rc.0 for this fix to be included. ng update @angular/core --next --force can do it for you automatically.

…ed files (angular#31322) Versions of CLI prior to angular/angular-cli@0e339ee did not expose the host.getModifiedResourceFiles() method. This meant that null was being passed through to the IncrementalState.reconcile() method to indicate that there were either no changes or the host didn't support that method. This commit fixes a bug where we were checking for undefined rather than null when deciding whether any resource files had changed, causing a null reference error to be thrown. This bug was not caught by the unit testing because the tests set up the changed files via a slightly different process, not having access to the CompilerHost, and these test were making the erroneous assumption that undefined indicated that there were no changed files. PR Close angular#31322

angular-automatic-lock-bot · 2019-09-15T08:52:41Z

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

mattlewis92 requested a review from a team as a code owner June 27, 2019 23:27

googlebot added the cla: yes label Jun 27, 2019

alfaproject reviewed Jun 28, 2019

View reviewed changes

petebacondarwin requested changes Jun 28, 2019

View reviewed changes

ngbot bot added this to the Backlog milestone Jun 28, 2019

petebacondarwin self-assigned this Jun 28, 2019

mattlewis92 force-pushed the fix-incremental-compilaton branch 2 times, most recently from 0b6a0b1 to a8ada6b Compare June 28, 2019 15:29

mattlewis92 force-pushed the fix-incremental-compilaton branch from a8ada6b to 7e6c38c Compare June 28, 2019 15:31

petebacondarwin added the comp: ngcc label Jul 10, 2019

petebacondarwin approved these changes Jul 10, 2019

View reviewed changes

alxhub approved these changes Jul 11, 2019

View reviewed changes

petebacondarwin reviewed Jul 17, 2019

View reviewed changes

mattlewis92 force-pushed the fix-incremental-compilaton branch from 7e6c38c to c470579 Compare July 17, 2019 16:27

alxhub approved these changes Jul 18, 2019

View reviewed changes

petebacondarwin added the action: merge The PR is ready for merge by the caretaker label Jul 18, 2019

mhevery closed this in 4aecf92 Jul 18, 2019

stefanocke mentioned this pull request Jul 30, 2019

Type Error during incremental compilation stefanocke/angular-cli-app-with-rollup#3

Closed

stefanocke mentioned this pull request Aug 3, 2019

ngc --watch ignores changes in templates (ivy) #31982

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

mattlewis92 commented Jun 27, 2019

alfaproject Jun 28, 2019

alfaproject Jun 28, 2019

mattlewis92 Jun 28, 2019

petebacondarwin Jun 28, 2019

petebacondarwin left a comment

mattlewis92 commented Jun 28, 2019 •

edited

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019 •

edited

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019 •

edited

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019 •

edited

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jul 10, 2019

alxhub commented Jul 11, 2019

alxhub commented Jul 11, 2019

mattlewis92 commented Jul 16, 2019

petebacondarwin Jul 17, 2019

petebacondarwin Jul 17, 2019

alxhub Jul 18, 2019

petebacondarwin commented Jul 17, 2019 •

edited

mattlewis92 commented Jul 17, 2019

alxhub commented Jul 18, 2019

stefanocke commented Jul 30, 2019 •

edited

mattlewis92 commented Jul 30, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

	if (this.modifiedResourceFiles === undefined \|\| this.modifiedResourceFiles === null \|\|
	if (this.modifiedResourceFiles == null \|\|

fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

fix(ivy): handle modifiedResourceFiles being null on incremental compilation #31322

Conversation

mattlewis92 commented Jun 27, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

alfaproject Jun 28, 2019

Choose a reason for hiding this comment

alfaproject Jun 28, 2019

Choose a reason for hiding this comment

mattlewis92 Jun 28, 2019

Choose a reason for hiding this comment

petebacondarwin Jun 28, 2019

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

mattlewis92 commented Jun 28, 2019 • edited

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019 • edited

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019 • edited

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019

petebacondarwin commented Jun 28, 2019 • edited

mattlewis92 commented Jun 28, 2019

petebacondarwin commented Jul 10, 2019

alxhub commented Jul 11, 2019

alxhub commented Jul 11, 2019

mattlewis92 commented Jul 16, 2019

petebacondarwin Jul 17, 2019

Choose a reason for hiding this comment

petebacondarwin Jul 17, 2019

Choose a reason for hiding this comment

alxhub Jul 18, 2019

Choose a reason for hiding this comment

petebacondarwin commented Jul 17, 2019 • edited

mattlewis92 commented Jul 17, 2019

alxhub commented Jul 18, 2019

stefanocke commented Jul 30, 2019 • edited

mattlewis92 commented Jul 30, 2019

angular-automatic-lock-bot bot commented Sep 15, 2019

mattlewis92 commented Jun 28, 2019 •

edited

mattlewis92 commented Jun 28, 2019 •

edited

petebacondarwin commented Jun 28, 2019 •

edited

petebacondarwin commented Jun 28, 2019 •

edited

petebacondarwin commented Jul 17, 2019 •

edited

stefanocke commented Jul 30, 2019 •

edited