fix(compiler-cli): ensure file_system handles mixed Windows drives (angular#38030)

The `fs.relative()` method assumed that the file-system is a single tree,
which is not the case in Windows, where you can have multiple drives,
e.g. `C:`, `D:` etc.

This commit changes `fs.relative()` so that it no longer forces the result
to be a `PathSegment` and then flows that refactoring through the rest of
the compiler-cli (and ngcc).  The main difference is that now, in some cases,
we needed to check whether the result is "rooted", i.e an `AbsoluteFsPath`,
rather than a `PathSegment`, before using it.

Fixes angular#36777

PR Close angular#38030

Author: petebacondarwin

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

@@ -5,12 +5,12 @@
  * 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
  */
-import {AbsoluteFsPath, relative} from '../../../src/ngtsc/file_system';
+import {AbsoluteFsPath, isLocalRelativePath, relative} from '../../../src/ngtsc/file_system';
 import {DependencyTracker} from '../../../src/ngtsc/incremental/api';
 
 export function isWithinPackage(packagePath: AbsoluteFsPath, filePath: AbsoluteFsPath): boolean {
   const relativePath = relative(packagePath, filePath);
-  return !relativePath.startsWith('..') && !relativePath.startsWith('node_modules/');
+  return isLocalRelativePath(relativePath) && !relativePath.startsWith('node_modules/');
 }
 
 class NoopDependencyTracker implements DependencyTracker {

packages/compiler-cli/ngcc/src/entry_point_finder/tracing_entry_point_finder.ts

@@ -193,7 +193,7 @@ export abstract class TracingEntryPointFinder implements EntryPointFinder {
    * Split the given `path` into path segments using an FS independent algorithm.
    * @param path The path to split.
    */
-  private splitPath(path: PathSegment) {
+  private splitPath(path: PathSegment|AbsoluteFsPath) {
     const segments = [];
     while (path !== '.') {
       segments.unshift(this.fs.basename(path));

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

@@ -9,7 +9,7 @@ import {Statement} from '@angular/compiler';
 import MagicString from 'magic-string';
 import * as ts from 'typescript';
 
-import {absoluteFromSourceFile, AbsoluteFsPath, dirname, relative} from '../../../src/ngtsc/file_system';
+import {absoluteFromSourceFile, AbsoluteFsPath, dirname, relative, toRelativeImport} from '../../../src/ngtsc/file_system';
 import {NOOP_DEFAULT_IMPORT_RECORDER, Reexport} from '../../../src/ngtsc/imports';
 import {Import, ImportManager, translateStatement} from '../../../src/ngtsc/translator';
 import {isDtsPath} from '../../../src/ngtsc/util/src/typescript';
@@ -57,8 +57,9 @@ export class EsmRenderingFormatter implements RenderingFormatter {
 
       if (from) {
         const basePath = stripExtension(from);
-        const relativePath = './' + relative(dirname(entryPointBasePath), basePath);
-        exportFrom = entryPointBasePath !== basePath ? ` from '${relativePath}'` : '';
+        const relativePath = relative(dirname(entryPointBasePath), basePath);
+        const relativeImport = toRelativeImport(relativePath);
+        exportFrom = entryPointBasePath !== basePath ? ` from '${relativeImport}'` : '';
       }
 
       const exportStr = `\nexport {${e.identifier}}${exportFrom};`;
@@ -197,10 +198,10 @@ export class EsmRenderingFormatter implements RenderingFormatter {
       const ngModuleName = info.ngModule.node.name.text;
       const declarationFile = absoluteFromSourceFile(info.declaration.getSourceFile());
       const ngModuleFile = absoluteFromSourceFile(info.ngModule.node.getSourceFile());
+      const relativePath = relative(dirname(declarationFile), ngModuleFile);
+      const relativeImport = toRelativeImport(relativePath);
       const importPath = info.ngModule.ownedByModuleGuess ||
-          (declarationFile !== ngModuleFile ?
-               stripExtension(`./${relative(dirname(declarationFile), ngModuleFile)}`) :
-               null);
+          (declarationFile !== ngModuleFile ? stripExtension(relativeImport) : null);
       const ngModule = generateImportString(importManager, importPath, ngModuleName);
 
       if (info.declaration.type) {

packages/compiler-cli/ngcc/src/writing/new_entry_point_file_writer.ts

@@ -6,7 +6,7 @@
  * 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
  */
-import {absoluteFromSourceFile, AbsoluteFsPath, dirname, FileSystem, join, relative} from '../../../src/ngtsc/file_system';
+import {absoluteFromSourceFile, AbsoluteFsPath, dirname, FileSystem, isLocalRelativePath, join, relative, resolve} from '../../../src/ngtsc/file_system';
 import {isDtsPath} from '../../../src/ngtsc/util/src/typescript';
 import {Logger} from '../logging/logger';
 import {EntryPoint, EntryPointJsonProperty} from '../packages/entry_point';
@@ -70,9 +70,9 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
       bundle: EntryPointBundle, packagePath: AbsoluteFsPath, ngccFolder: AbsoluteFsPath) {
     bundle.src.program.getSourceFiles().forEach(sourceFile => {
       const relativePath = relative(packagePath, absoluteFromSourceFile(sourceFile));
-      const isOutsidePackage = relativePath.startsWith('..');
-      if (!sourceFile.isDeclarationFile && !isOutsidePackage) {
-        const newFilePath = join(ngccFolder, relativePath);
+      const isInsidePackage = isLocalRelativePath(relativePath);
+      if (!sourceFile.isDeclarationFile && isInsidePackage) {
+        const newFilePath = resolve(ngccFolder, relativePath);
         this.fs.ensureDir(dirname(newFilePath));
         this.fs.copyFile(absoluteFromSourceFile(sourceFile), newFilePath);
       }
@@ -86,7 +86,7 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
       super.writeFileAndBackup(file);
     } else {
       const relativePath = relative(packagePath, file.path);
-      const newFilePath = join(ngccFolder, relativePath);
+      const newFilePath = resolve(ngccFolder, relativePath);
       this.fs.ensureDir(dirname(newFilePath));
       this.fs.writeFile(newFilePath, file.contents);
     }
@@ -98,7 +98,7 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
       super.revertFileAndBackup(filePath);
     } else if (this.fs.exists(filePath)) {
       const relativePath = relative(packagePath, filePath);
-      const newFilePath = join(packagePath, NGCC_DIRECTORY, relativePath);
+      const newFilePath = resolve(packagePath, NGCC_DIRECTORY, relativePath);
       this.fs.removeFile(newFilePath);
     }
   }
@@ -117,8 +117,9 @@ export class NewEntryPointFileWriter extends InPlaceFileWriter {
     // All format properties point to the same format-path.
     const oldFormatProp = formatProperties[0]!;
     const oldFormatPath = packageJson[oldFormatProp]!;
-    const oldAbsFormatPath = join(entryPoint.path, oldFormatPath);
-    const newAbsFormatPath = join(ngccFolder, relative(entryPoint.packagePath, oldAbsFormatPath));
+    const oldAbsFormatPath = resolve(entryPoint.path, oldFormatPath);
+    const newAbsFormatPath =
+        resolve(ngccFolder, relative(entryPoint.packagePath, oldAbsFormatPath));
     const newFormatPath = relative(entryPoint.path, newAbsFormatPath);
 
     // Update all properties in `package.json` (both in memory and on disk).

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

@@ -6,7 +6,7 @@
  * found in the LICENSE file at https://angular.io/license
  */
 export {NgtscCompilerHost} from './src/compiler_host';
-export {absoluteFrom, absoluteFromSourceFile, basename, dirname, getFileSystem, isRoot, isRooted, join, relative, relativeFrom, resolve, setFileSystem} from './src/helpers';
+export {absoluteFrom, absoluteFromSourceFile, basename, dirname, getFileSystem, isLocalRelativePath, isRoot, isRooted, join, relative, relativeFrom, resolve, setFileSystem, toRelativeImport} from './src/helpers';
 export {LogicalFileSystem, LogicalProjectPath} from './src/logical';
 export {NodeJSFileSystem} from './src/node_js_file_system';
 export {AbsoluteFsPath, FileStats, FileSystem, PathSegment, PathString} from './src/types';

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

@@ -83,7 +83,7 @@ export function isRooted(path: string): boolean {
 /**
  * Static access to `relative`.
  */
-export function relative<T extends PathString>(from: T, to: T): PathSegment {
+export function relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
   return fs.relative(from, to);
 }
 
@@ -93,3 +93,23 @@ export function relative<T extends PathString>(from: T, to: T): PathSegment {
 export function basename(filePath: PathString, extension?: string): PathSegment {
   return fs.basename(filePath, extension) as PathSegment;
 }
+
+/**
+ * Returns true if the given path is locally relative.
+ *
+ * This is used to work out if the given path is relative (i.e. not absolute) but also is not
+ * escaping the current directory.
+ */
+export function isLocalRelativePath(relativePath: string): boolean {
+  return !isRooted(relativePath) && !relativePath.startsWith('..');
+}
+
+/**
+ * Converts a path to a form suitable for use as a relative module import specifier.
+ *
+ * In other words it adds the `./` to the path if it is locally relative.
+ */
+export function toRelativeImport(relativePath: PathSegment|AbsoluteFsPath): PathSegment|
+    AbsoluteFsPath {
+  return isLocalRelativePath(relativePath) ? `./${relativePath}` as PathSegment : relativePath;
+}

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

@@ -82,7 +82,7 @@ export class InvalidFileSystem implements FileSystem {
   isRooted(path: string): boolean {
     throw makeError();
   }
-  relative<T extends PathString>(from: T, to: T): PathSegment {
+  relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
     throw makeError();
   }
   basename(filePath: string, extension?: string): PathSegment {

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

@@ -7,7 +7,7 @@
  */
 import * as ts from 'typescript';
 
-import {absoluteFrom, dirname, relative, resolve} from './helpers';
+import {absoluteFrom, dirname, isLocalRelativePath, relative, resolve, toRelativeImport} from './helpers';
 import {AbsoluteFsPath, BrandedPath, PathSegment} from './types';
 import {stripExtension} from './util';
 
@@ -29,11 +29,8 @@ export const LogicalProjectPath = {
    * importing from `to`.
    */
   relativePathBetween: function(from: LogicalProjectPath, to: LogicalProjectPath): PathSegment {
-    let relativePath = relative(dirname(resolve(from)), resolve(to));
-    if (!relativePath.startsWith('../')) {
-      relativePath = ('./' + relativePath) as PathSegment;
-    }
-    return relativePath as PathSegment;
+    const relativePath = relative(dirname(resolve(from)), resolve(to));
+    return toRelativeImport(relativePath) as PathSegment;
   },
 };
 
@@ -122,5 +119,5 @@ export class LogicalFileSystem {
  * E.g. `foo/bar/zee` is within `foo/bar` but not within `foo/car`.
  */
 function isWithinBasePath(base: AbsoluteFsPath, path: AbsoluteFsPath): boolean {
-  return !relative(base, path).startsWith('..');
+  return isLocalRelativePath(relative(base, path));
 }

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

@@ -9,7 +9,7 @@
 import * as fs from 'fs';
 import * as fsExtra from 'fs-extra';
 import * as p from 'path';
-import {absoluteFrom, relativeFrom} from './helpers';
+import {absoluteFrom} from './helpers';
 import {AbsoluteFsPath, FileStats, FileSystem, PathSegment, PathString} from './types';
 
 /**
@@ -93,8 +93,8 @@ export class NodeJSFileSystem implements FileSystem {
   isRooted(path: string): boolean {
     return p.isAbsolute(path);
   }
-  relative<T extends PathString>(from: T, to: T): PathSegment {
-    return relativeFrom(this.normalize(p.relative(from, to)));
+  relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath {
+    return this.normalize(p.relative(from, to)) as PathSegment | AbsoluteFsPath;
   }
   basename(filePath: string, extension?: string): PathSegment {
     return p.basename(filePath, extension) as PathSegment;

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

@@ -57,7 +57,14 @@ export interface FileSystem {
   resolve(...paths: string[]): AbsoluteFsPath;
   dirname<T extends PathString>(file: T): T;
   join<T extends PathString>(basePath: T, ...paths: string[]): T;
-  relative<T extends PathString>(from: T, to: T): PathSegment;
+  /**
+   * Compute the relative path between `from` and `to`.
+   *
+   * In file-systems that can have multiple file trees the returned path may not actually be
+   * "relative" (i.e. `PathSegment`). For example, Windows can have multiple drives :
+   * `relative('c:/a/b', 'd:/a/c')` would be `d:/a/c'.
+   */
+  relative<T extends PathString>(from: T, to: T): PathSegment|AbsoluteFsPath;
   basename(filePath: string, extension?: string): PathSegment;
   realpath(filePath: AbsoluteFsPath): AbsoluteFsPath;
   getDefaultLibLocation(): AbsoluteFsPath;