TreeFS, RootPathUtils: consistently preserve trailing separators in l…

…ookups

Summary:
Currently, `metro-file-map` has little/no consideration of paths with trailing slashes. These come up as we consider hierarchical lookups from module candidate paths with trailing slashes (but in any case, ought to have defined behaviour).

This defines a consistent handling where input paths have trailing slashes, for `RootPathUtils` and `FileSystem` (`TreeFS`).

 - `absoluteToNormal` and `relativeToNormal` will *preserve* trailing slashes, except where the normal path is the project root (`''`), so we formalise that a `normalPath` may have a trailing slash.
 - A `lookup()` on `/foo/bar/` should succeed iff `/foo/bar` is a directory or a symlink to a directory (i.e., not if `/foo/bar` is a file), consistent with posix `ls`, `stat`, `realpath`, etc., which fail with `"Not a directory"`.
 - The returned `realPath` should drop any trailing slash (except for the fs root), as posix `realpath` does. In `metro-file-map`, `canonicalPath` never has a trailing slash, and maps 1-1 with a tree node.

Reviewed By: GijsWeterings

Differential Revision: D60773938

fbshipit-source-id: 77d1bac7caf27e0d125e7cd9f3d6190cd1305bef

packages/metro-file-map/src/lib/RootPathUtils.js

@@ -27,6 +27,12 @@ import * as path from 'path';
  * Output and input paths are at least well-formed (normal where indicated by
  * naming).
  *
+ * Trailing path separators are preserved, except for fs roots in
+ * normalToAbsolute (fs roots always have a trailing separator), and the
+ * project root in absoluteToNormal and relativeToNormal (the project root is
+ * always the empty string, and is always a directory, so a trailing separator
+ * is redundant).
+ *
  * As of Node 20, absoluteToNormal is ~8x faster than `path.relative` and
  * `normalToAbsolute` is ~20x faster than `path.resolve`, benchmarked on the
  * real inputs from building FB's product graph. Some well-formed inputs
@@ -108,10 +114,18 @@ export class RootPathUtils {
         absolutePath,
         endOfMatchingPrefix,
         upIndirectionsToPrepend,
-      ) ?? path.relative(this.#rootDir, absolutePath)
+      ) ?? this.#slowAbsoluteToNormal(absolutePath)
     );
   }
 
+  #slowAbsoluteToNormal(absolutePath: string): string {
+    const endsWithSep = absolutePath.endsWith(path.sep);
+    const result = path.relative(this.#rootDir, absolutePath);
+    return endsWithSep && !result.endsWith(path.sep)
+      ? result + path.sep
+      : result;
+  }
+
   // `normalPath` is assumed to be normal (root-relative, no redundant
   // indirection), per the definition above.
   normalToAbsolute(normalPath: string): string {
@@ -199,10 +213,21 @@ export class RootPathUtils {
             break;
           }
         }
-        const right = fullPath.slice(pos);
+        // After collapsing we may have no more segments remaining (following
+        // '..' indirections). Ensure that we don't drop or add a trailing
+        // separator in this case by taking .slice(pos-1). In any other case,
+        // we know that fullPath[pos] is a separator.
+        if (pos >= fullPath.length) {
+          return totalUpIndirections > 0
+            ? UP_FRAGMENT_SEP.repeat(totalUpIndirections - 1) +
+                '..' +
+                fullPath.slice(pos - 1)
+            : '';
+        }
+        const right = pos > 0 ? fullPath.slice(pos) : fullPath;
         if (
-          right === '' ||
-          (right === '..' && totalUpIndirections >= this.#rootParts.length - 1)
+          right === '..' &&
+          totalUpIndirections >= this.#rootParts.length - 1
         ) {
           // If we have no right side (or an indirection that would take us
           // below the root), just ensure we don't include a trailing separtor.

packages/metro-file-map/src/lib/TreeFS.js

@@ -31,8 +31,8 @@ type MixedNode = FileNode | DirectoryNode;
 //
 // mixedPath - a root-relative or absolute path
 // relativePath - a root-relative path
-// normalPath - a root-relative, normalised path (no extraneous '.' or '..')
-// canonicalPath - a root-relative, normalised, real path (no symlinks in dirname)
+// normalPath - a root-relative, normalised path (no extraneous '.' or '..'), may have a trailing slash
+// canonicalPath - a root-relative, normalised, real path (no symlinks in dirname), no trailing slash
 
 export default class TreeFS implements MutableFileSystem {
   +#cachedNormalSymlinkTargets: WeakMap<FileNode, Path> = new WeakMap();
@@ -441,17 +441,23 @@ export default class TreeFS implements MutableFileSystem {
         }
       }
 
-      // If there are no more '/' to come, we're done unless this is a symlink
-      // we must follow.
+      // We are done if...
       if (
-        isLastSegment &&
-        (segmentNode instanceof Map ||
-          segmentNode[H.SYMLINK] === 0 ||
-          opts.followLeaf === false)
+        // ...at a directory node and the only subsequent character is `/`, or
+        (nextSepIdx === targetNormalPath.length - 1 &&
+          segmentNode instanceof Map) ||
+        // there are no subsequent `/`, and this node is anything but a symlink
+        // we're required to resolve due to followLeaf.
+        (isLastSegment &&
+          (segmentNode instanceof Map ||
+            segmentNode[H.SYMLINK] === 0 ||
+            opts.followLeaf === false))
       ) {
         return {
           ancestorOfRootIdx,
-          canonicalPath: targetNormalPath,
+          canonicalPath: isLastSegment
+            ? targetNormalPath
+            : targetNormalPath.slice(0, -1), // remove trailing `/`
           exists: true,
           node: segmentNode,
           parentNode,

packages/metro-file-map/src/lib/__tests__/RootPathUtils-test.js

@@ -14,7 +14,7 @@ import type {RootPathUtils as RootPathUtilsT} from '../RootPathUtils';
 let mockPathModule;
 jest.mock('path', () => mockPathModule);
 
-describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => {
+describe.each([['win32'], ['posix']])('RootPathUtils on %s', platform => {
   // Convenience function to write paths with posix separators but convert them
   // to system separators
   const p: string => string = filePath =>
@@ -25,10 +25,12 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => {
   let RootPathUtils: Class<RootPathUtilsT>;
   let pathUtils: RootPathUtilsT;
   let pathRelative: JestMockFn<[string, string], string>;
+  let sep: string;
 
   beforeEach(() => {
     jest.resetModules();
     mockPathModule = jest.requireActual<{}>('path')[platform];
+    sep = mockPathModule.sep;
     pathRelative = jest.spyOn(mockPathModule, 'relative');
     RootPathUtils = require('../RootPathUtils').RootPathUtils;
   });
@@ -38,21 +40,30 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => {
     p('/project/root/../root2/foobar'),
     p('/project/root/../../project2/foo'),
     p('/project/root/../../project/foo'),
+    p('/project/root/../../project/foo/'),
     p('/project/root/../../project/root'),
+    p('/project/root/../../project/root/'),
     p('/project/root/../../project/root/foo.js'),
     p('/project/bar'),
+    p('/project/bar/'),
     p('/project/../outside/bar'),
     p('/project/baz/foobar'),
     p('/project/rootfoo/baz'),
     p('/project'),
+    p('/project/'),
     p('/'),
     p('/outside'),
-  ])(`absoluteToNormal('%s') is correct and optimised`, normalPath => {
+    p('/outside/'),
+  ])(`absoluteToNormal('%s') is correct and optimised`, absolutePath => {
     const rootDir = p('/project/root');
     pathUtils = new RootPathUtils(rootDir);
-    const expected = mockPathModule.relative(rootDir, normalPath);
+    let expected = mockPathModule.relative(rootDir, absolutePath);
+    // Unlike path.relative, we expect to preserve trailing separators.
+    if (absolutePath.endsWith(sep) && expected !== '') {
+      expected += sep;
+    }
     pathRelative.mockClear();
-    expect(pathUtils.absoluteToNormal(normalPath)).toEqual(expected);
+    expect(pathUtils.absoluteToNormal(absolutePath)).toEqual(expected);
     expect(pathRelative).not.toHaveBeenCalled();
   });
 
@@ -68,43 +79,65 @@ describe.each([['win32'], ['posix']])('pathUtilsForRoot on %s', platform => {
       p('/project/root/a./../foo'),
       p('/project/root/../a./foo'),
       p('/project/root/.././foo'),
-    ])(`absoluteToNormal('%s') falls back to path.relative`, normalPath => {
-      const expected = mockPathModule.relative(rootDir, normalPath);
+      p('/project/root/.././foo/'),
+    ])(`absoluteToNormal('%s') falls back to path.relative`, absolutePath => {
+      let expected = mockPathModule.relative(rootDir, absolutePath);
+      // Unlike path.relative, we expect to preserve trailing separators.
+      if (absolutePath.endsWith(sep) && !expected.endsWith(sep)) {
+        expected += sep;
+      }
       pathRelative.mockClear();
-      expect(pathUtils.absoluteToNormal(normalPath)).toEqual(expected);
+      expect(pathUtils.absoluteToNormal(absolutePath)).toEqual(expected);
       expect(pathRelative).toHaveBeenCalled();
     });
 
     test.each([
       p('..'),
       p('../..'),
+      p('../../'),
       p('normal/path'),
+      p('normal/path/'),
       p('../normal/path'),
+      p('../normal/path/'),
       p('../../normal/path'),
       p('../../../normal/path'),
     ])(`normalToAbsolute('%s') matches path.resolve`, normalPath => {
-      expect(pathUtils.normalToAbsolute(normalPath)).toEqual(
-        mockPathModule.resolve(rootDir, normalPath),
-      );
+      let expected = mockPathModule.resolve(rootDir, normalPath);
+      // Unlike path.resolve, we expect to preserve trailing separators.
+      if (normalPath.endsWith(sep) && !expected.endsWith(sep)) {
+        expected += sep;
+      }
+      expect(pathUtils.normalToAbsolute(normalPath)).toEqual(expected);
     });
 
     test.each([
       p('..'),
       p('../root'),
       p('../root/path'),
       p('../project'),
+      p('../project/'),
       p('../../project/root'),
+      p('../../project/root/'),
       p('../../../normal/path'),
+      p('../../../normal/path/'),
       p('../../..'),
     ])(
       `relativeToNormal('%s') matches path.resolve + path.relative`,
       relativePath => {
-        expect(pathUtils.relativeToNormal(relativePath)).toEqual(
-          mockPathModule.relative(
-            rootDir,
-            mockPathModule.resolve(rootDir, relativePath),
-          ),
+        let expected = mockPathModule.relative(
+          rootDir,
+          mockPathModule.resolve(rootDir, relativePath),
         );
+        // Unlike native path.resolve + path.relative, we expect to preserve
+        // trailing separators. (Consistent with path.normalize.)
+        if (
+          relativePath.endsWith(sep) &&
+          !expected.endsWith(sep) &&
+          expected !== ''
+        ) {
+          expected += sep;
+        }
+        expect(pathUtils.relativeToNormal(relativePath)).toEqual(expected);
       },
     );
   });

packages/metro-file-map/src/lib/__tests__/TreeFS-test.js

@@ -132,6 +132,7 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
 
     test.each([
       [p('/project/bar.js/bad-parent'), [], p('/project/bar.js')],
+      [p('/project/bar.js/'), [], p('/project/bar.js')],
       [
         p('/project/link-to-nowhere'),
         [p('/project/link-to-nowhere')],
@@ -152,13 +153,16 @@ describe.each([['win32'], ['posix']])('TreeFS on %s', platform => {
         }),
     );
 
-    test.each([[p('/project/foo')], [p('/project/root/outside')]])(
-      'returns type: d for %s',
-      givenPath =>
-        expect(tfs.lookup(givenPath)).toMatchObject({
-          exists: true,
-          type: 'd',
-        }),
+    test.each([
+      [p('/project/foo'), p('/project/foo')],
+      [p('/project/foo/'), p('/project/foo')],
+      [p('/project/root/outside'), p('/outside')],
+    ])('returns type: d for %s', (givenPath, expectedRealPath) =>
+      expect(tfs.lookup(givenPath)).toMatchObject({
+        exists: true,
+        type: 'd',
+        realPath: expectedRealPath,
+      }),
     );
 
     test('traversing the same symlink multiple times does not imply a cycle', () => {