fix(ngcc): correctly invalidate cache when moving/removing files/directories (#35106)

One particular scenario where this was causing problems was when the
[BackupFileCleaner][1] restored a file (such as a `.d.ts` file) by
[moving the backup file][2] to its original location, but the modified
content was kept in the cache.

[1]: https://github.com/angular/angular/blob/4d36b2f6e/packages/compiler-cli/ngcc/src/writing/cleaning/cleaning_strategies.ts#L54
[2]: https://github.com/angular/angular/blob/4d36b2f6e/packages/compiler-cli/ngcc/src/writing/cleaning/cleaning_strategies.ts#L61

Fixes #35095

PR Close #35106

integration/ngcc/mock-ngcc-version-marker.js

@@ -0,0 +1,50 @@
+/**
+ * @license
+ * Copyright Google Inc. All Rights Reserved.
+ *
+ * Use of this source code is governed by an MIT-style license that can be
+ * found in the LICENSE file at https://angular.io/license
+ */
+
+/**
+ * **Usage:**
+ * ```sh
+ * node mock-ngcc-version-marker <entry-point> <mock-ngcc-version>
+ *
+ * # Example:
+ * # node mock-ngcc-version-marker @angular/material/button 3.0.0
+ * ```
+ *
+ * Ngcc marks entry-points as processed by adding marker properties in their `package.json`. It uses
+ * a version as a marker in order to be able to distinguish entry-points processed by a different
+ * ngcc version (e.g. an older version after an update) and perform the necessary clean-up.
+ *
+ * This script replaces the ngcc version marker in an entry-point's `package.json` to make it appear
+ * as if it were processed by a different version of ngcc. This allows testing the clean-up logic
+ * without the overhead of actually installing a different ngcc version, compiling the dependencies,
+ * then installing the current version and compiling again.
+ */
+
+const {writeFileSync} = require('fs');
+const {basename} = require('path');
+
+const entryPointName = process.argv[2];
+const mockNgccVersion = process.argv[3];
+const actualNgccVersion = require('@angular/compiler-cli/package.json').version;
+
+if (!entryPointName || !mockNgccVersion) {
+  throw new Error(
+      'Missing required argument(s).\n' +
+      `Usage: node ${basename(__filename)} <entry-point> <mock-ngcc-version>\n`);
+}
+
+const entryPointPkgJsonPath = require.resolve(`${entryPointName}/package.json`);
+const entryPointPkgJson = require(entryPointPkgJsonPath);
+const processedMarkers = entryPointPkgJson.__processed_by_ivy_ngcc__;
+
+Object.keys(processedMarkers).forEach(key => processedMarkers[key] = mockNgccVersion);
+writeFileSync(entryPointPkgJsonPath, JSON.stringify(entryPointPkgJson, null, 2));
+
+console.log(
+    `Successfully mocked ngcc version marker in '${entryPointName}': ` +
+    `${actualNgccVersion} --> ${mockNgccVersion}`);

integration/ngcc/test.sh

@@ -3,7 +3,7 @@
 # Do not immediately exit on error to allow the `assertSucceeded` function to handle the error.
 #
 # NOTE:
-# Each statement should be followed by an `assertSucceeded`/`assertFailed` or `exit 1` statement.
+# Each statement should be followed by an `assert*` or `exit 1` statement.
 set +e -x
 
 PATH=$PATH:$(npm bin)
@@ -22,6 +22,26 @@ function assertSucceeded {
   fi
 }
 
+function assertEquals {
+  local value1=$1;
+  local value2=$2;
+
+  if [[ "$value1" != "$value2" ]]; then
+    echo "FAIL: Expected '$value1' to equal '$value2'."
+    exit 1;
+  fi
+}
+
+function assertNotEquals {
+  local value1=$1;
+  local value2=$2;
+
+  if [[ "$value1" == "$value2" ]]; then
+    echo "FAIL: Expected '$value1' not to equal '$value2'."
+    exit 1;
+  fi
+}
+
 
 ngcc --help
 assertSucceeded "Expected 'ngcc --help' to succeed."
@@ -140,6 +160,27 @@ assertSucceeded "Expected 'ngcc' to log 'Compiling'."
   assertSucceeded "Expected 'ngcc' to not add trailing commas to factory function parameters in UMD."
 
 
+# Can it correctly clean up and re-compile when dependencies are already compiled by a different version?
+  readonly actualNgccVersion=`node --print "require('@angular/compiler-cli/package.json').version"`
+  readonly mockNgccVersion="3.0.0"
+
+  # Mock the ngcc version marker on a package to make it appear as if it is compiled by a different ngcc version.
+  node mock-ngcc-version-marker @angular/material/button $mockNgccVersion
+  assertSucceeded "Expected to successfully mock the 'ngcc' version marker in '@angular/material/button'."
+  assertEquals $mockNgccVersion `node --print "require('@angular/material/button/package.json').__processed_by_ivy_ngcc__.main"`
+  assertEquals 1 `cat node_modules/@angular/material/button/button.d.ts | grep 'import \* as ɵngcc0' | wc -l`
+
+  # Re-compile packages (which requires cleaning up those compiled by a different ngcc version).
+  # (Use sync mode to ensure all tasks share the same `CachedFileSystem` instance.)
+  ngcc --no-async --properties main
+  assertSucceeded "Expected 'ngcc' to successfully re-compile the packages."
+
+  # Ensure previously compiled packages were correctly cleaned up (i.e. no multiple
+  # `import ... ɵngcc0` statements) and re-compiled by the current ngcc version.
+  assertEquals $actualNgccVersion `node --print "require('@angular/material/button/package.json').__processed_by_ivy_ngcc__.main"`
+  assertEquals 1 `cat node_modules/@angular/material/button/button.d.ts | grep 'import \* as ɵngcc0' | wc -l`
+
+
 # Can it compile `@angular/platform-server` in UMD + typings without errors?
 # (The CLI prefers the `main` property (which maps to UMD) over `module` when compiling `@angular/platform-server`.
 # See https://github.com/angular/angular-cli/blob/e36853338/packages/angular_devkit/build_angular/src/angular-cli-files/models/webpack-configs/server.ts#L34)
@@ -151,6 +192,7 @@ assertSucceeded "Expected 'ngcc' to log 'Compiling'."
   ngcc --properties main --target @angular/platform-server
   assertSucceeded "Expected 'ngcc' to successfully compile '@angular/platform-server' (main)."
 
+
 # Can it be safely run again (as a noop)?
 # And check that it logged skipping compilation as expected
 ngcc -l debug | grep 'Skipping'

packages/compiler-cli/src/ngtsc/file_system/src/cached_file_system.ts

@@ -72,12 +72,16 @@ export class CachedFileSystem implements FileSystem {
 
   moveFile(from: AbsoluteFsPath, to: AbsoluteFsPath): void {
     this.delegate.moveFile(from, to);
+
     this.existsCache.set(from, false);
+    this.existsCache.set(to, true);
+
     if (this.readFileCache.has(from)) {
       this.readFileCache.set(to, this.readFileCache.get(from));
       this.readFileCache.delete(from);
+    } else {
+      this.readFileCache.delete(to);
     }
-    this.existsCache.set(to, true);
   }
 
   ensureDir(path: AbsoluteFsPath): void {
@@ -90,10 +94,18 @@ export class CachedFileSystem implements FileSystem {
 
   removeDeep(path: AbsoluteFsPath): void {
     this.delegate.removeDeep(path);
-    // Clear out all children of this directory from the exists cache.
+
+    // Clear out this directory and all its children from the `exists` cache.
     for (const p of this.existsCache.keys()) {
       if (p.startsWith(path)) {
-        this.existsCache.set(path, false);
+        this.existsCache.set(p, false);
+      }
+    }
+
+    // Clear out this directory and all its children from the `readFile` cache.
+    for (const p of this.readFileCache.keys()) {
+      if (p.startsWith(path)) {
+        this.readFileCache.delete(p);
       }
     }
   }

packages/compiler-cli/src/ngtsc/file_system/test/cached_file_system_spec.ts

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 import {CachedFileSystem} from '../src/cached_file_system';
-import {absoluteFrom, setFileSystem} from '../src/helpers';
+import {absoluteFrom, join, setFileSystem} from '../src/helpers';
 import {NodeJSFileSystem} from '../src/node_js_file_system';
 import {AbsoluteFsPath, FileSystem} from '../src/types';
 
@@ -236,7 +236,7 @@ describe('CachedFileSystem', () => {
       expect(readFileSpy).toHaveBeenCalledWith(abcPath);
     });
 
-    it('should update the `to` "readFile" cache', () => {
+    it('should update the `to` "readFile" cache (if `from` was cached)', () => {
       spyOn(delegate, 'moveFile');
       const readFileSpy = spyOn(delegate, 'readFile');
 
@@ -252,6 +252,24 @@ describe('CachedFileSystem', () => {
       expect(fs.readFile(xyzPath)).toEqual('abc content');
       expect(readFileSpy).not.toHaveBeenCalled();
     });
+
+    it('should delete the `to` "readFile" cache (if `from` was not cached)', () => {
+      spyOn(delegate, 'moveFile');
+      const readFileSpy = spyOn(delegate, 'readFile');
+
+      // Fill the xyz "readFile" cache
+      readFileSpy.and.returnValue('xyz content');
+      fs.readFile(xyzPath);
+      readFileSpy.calls.reset();
+
+      // Move the file
+      fs.moveFile(abcPath, xyzPath);
+
+      // Show that the cache was not hit for the xyz file
+      readFileSpy.and.returnValue('abc content');
+      expect(fs.readFile(xyzPath)).toBe('abc content');
+      expect(readFileSpy).toHaveBeenCalledWith(xyzPath);
+    });
   });
 
   describe('ensureDir()', () => {
@@ -281,14 +299,41 @@ describe('CachedFileSystem', () => {
     });
 
     it('should update the exists cache', () => {
+      spyOn(delegate, 'writeFile');
       spyOn(delegate, 'removeDeep');
       const existsSpy = spyOn(delegate, 'exists').and.returnValue(true);
       expect(fs.exists(abcPath)).toBe(true);
       existsSpy.calls.reset();
 
+      // Create a file inside `/a/b/c`.
+      const abcdPath = join(abcPath, 'd');
+      fs.writeFile(abcdPath, 'content');
+      expect(fs.exists(abcdPath)).toBe(true);
+      expect(existsSpy).not.toHaveBeenCalled();
+
+      // Remove the `/a/b/c` directory and ensure it is removed from cache (along with its content).
       fs.removeDeep(abcPath);
       expect(fs.exists(abcPath)).toBeFalsy();
+      expect(fs.exists(abcdPath)).toBeFalsy();
       expect(existsSpy).not.toHaveBeenCalled();
     });
+
+    it('should update the readFile cache', () => {
+      spyOn(delegate, 'writeFile');
+      spyOn(delegate, 'removeDeep');
+      spyOn(delegate, 'lstat').and.throwError('ENOENT: no such file or directory');
+      const readFileSpy = spyOn(delegate, 'readFile');
+
+      // Create a file inside `/a/b/c`.
+      const abcdPath = join(abcPath, 'd');
+      fs.writeFile(abcdPath, 'content from cache');
+      expect(fs.readFile(abcdPath)).toBe('content from cache');
+      expect(readFileSpy).not.toHaveBeenCalled();
+
+      // Remove the `/a/b/c` directory and ensure it is removed from cache (along with its content).
+      fs.removeDeep(abcPath);
+      expect(() => fs.readFile(abcdPath)).toThrowError('ENOENT: no such file or directory');
+      expect(() => fs.readFile(abcPath)).toThrowError('ENOENT: no such file or directory');
+    });
   });
 });