Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert changes to matchFiles/readDirectory made since 4.3 #46787

Merged
merged 4 commits into from
Nov 11, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions src/compiler/sys.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1266,7 +1266,6 @@ namespace ts {
let activeSession: import("inspector").Session | "stopping" | undefined;
let profilePath = "./profile.cpuprofile";

let hitSystemWatcherLimit = false;

const Buffer: {
new (input: string, encoding?: string): any;
Expand Down Expand Up @@ -1620,12 +1619,6 @@ namespace ts {
options = { persistent: true };
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand a full revert is safer at this point in the cycle, but it would be nice to keep (or quickly restore) this part of the change.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you review the latest commit and see if this is what you had in mind?

if (hitSystemWatcherLimit) {
sysLog(`sysLog:: ${fileOrDirectory}:: Defaulting to fsWatchFile`);
return watchPresentFileSystemEntryWithFsWatchFile();
}

try {
const presentWatcher = _fs.watch(
fileOrDirectory,
Expand All @@ -1642,8 +1635,6 @@ namespace ts {
// Catch the exception and use polling instead
// Eg. on linux the number of watches are limited and one could easily exhaust watches and the exception ENOSPC is thrown when creating watcher at that point
// so instead of throwing error, use fs.watchFile
hitSystemWatcherLimit ||= e.code === "ENOSPC";
sysLog(`sysLog:: ${fileOrDirectory}:: Changing to fsWatchFile`);
return watchPresentFileSystemEntryWithFsWatchFile();
}
}
Expand All @@ -1665,6 +1656,7 @@ namespace ts {
* Eg. on linux the number of watches are limited and one could easily exhaust watches and the exception ENOSPC is thrown when creating watcher at that point
*/
function watchPresentFileSystemEntryWithFsWatchFile(): FileWatcher {
sysLog(`sysLog:: ${fileOrDirectory}:: Changing to fsWatchFile`);
return watchFile(
fileOrDirectory,
createFileWatcherCallback(callback),
Expand Down Expand Up @@ -1804,7 +1796,7 @@ namespace ts {
}

function readDirectory(path: string, extensions?: readonly string[], excludes?: readonly string[], includes?: readonly string[], depth?: number): string[] {
return matchFiles(path, extensions, excludes, includes, useCaseSensitiveFileNames, process.cwd(), depth, getAccessibleFileSystemEntries, realpath, directoryExists);
return matchFiles(path, extensions, excludes, includes, useCaseSensitiveFileNames, process.cwd(), depth, getAccessibleFileSystemEntries, realpath);
}

function fileSystemEntryExists(path: string, entryKind: FileSystemEntryKind): boolean {
Expand Down
30 changes: 14 additions & 16 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6629,7 +6629,7 @@ namespace ts {
includeFilePattern: getRegularExpressionForWildcard(includes, absolutePath, "files"),
includeDirectoryPattern: getRegularExpressionForWildcard(includes, absolutePath, "directories"),
excludePattern: getRegularExpressionForWildcard(excludes, absolutePath, "exclude"),
basePaths: getBasePaths(absolutePath, includes, useCaseSensitiveFileNames)
basePaths: getBasePaths(path, includes, useCaseSensitiveFileNames)
};
}

Expand All @@ -6638,7 +6638,7 @@ namespace ts {
}

/** @param path directory of the tsconfig.json */
export function matchFiles(path: string, extensions: readonly string[] | undefined, excludes: readonly string[] | undefined, includes: readonly string[] | undefined, useCaseSensitiveFileNames: boolean, currentDirectory: string, depth: number | undefined, getFileSystemEntries: (path: string) => FileSystemEntries, realpath: (path: string) => string, directoryExists: (path: string) => boolean): string[] {
export function matchFiles(path: string, extensions: readonly string[] | undefined, excludes: readonly string[] | undefined, includes: readonly string[] | undefined, useCaseSensitiveFileNames: boolean, currentDirectory: string, depth: number | undefined, getFileSystemEntries: (path: string) => FileSystemEntries, realpath: (path: string) => string): string[] {
path = normalizePath(path);
currentDirectory = normalizePath(currentDirectory);

Expand All @@ -6653,22 +6653,20 @@ namespace ts {
const results: string[][] = includeFileRegexes ? includeFileRegexes.map(() => []) : [[]];
const visited = new Map<string, true>();
const toCanonical = createGetCanonicalFileName(useCaseSensitiveFileNames);
for (const absoluteBasePath of patterns.basePaths) {
if (directoryExists(absoluteBasePath)) {
visitDirectory(absoluteBasePath, depth);
}
for (const basePath of patterns.basePaths) {
visitDirectory(basePath, combinePaths(currentDirectory, basePath), depth);
}

return flatten(results);

function visitDirectory(absolutePath: string, depth: number | undefined) {
function visitDirectory(path: string, absolutePath: string, depth: number | undefined) {
const canonicalPath = toCanonical(realpath(absolutePath));
if (visited.has(canonicalPath)) return;
visited.set(canonicalPath, true);
const { files, directories } = getFileSystemEntries(absolutePath);
const { files, directories } = getFileSystemEntries(path);

for (const current of sort<string>(files, compareStringsCaseSensitive)) {
const name = combinePaths(absolutePath, current);
const name = combinePaths(path, current);
const absoluteName = combinePaths(absolutePath, current);
if (extensions && !fileExtensionIsOneOf(name, extensions)) continue;
if (excludeRegex && excludeRegex.test(absoluteName)) continue;
Expand All @@ -6691,32 +6689,32 @@ namespace ts {
}

for (const current of sort<string>(directories, compareStringsCaseSensitive)) {
const name = combinePaths(path, current);
const absoluteName = combinePaths(absolutePath, current);
if ((!includeDirectoryRegex || includeDirectoryRegex.test(absoluteName)) &&
(!excludeRegex || !excludeRegex.test(absoluteName))) {
visitDirectory(absoluteName, depth);
visitDirectory(name, absoluteName, depth);
}
}
}
}

/**
* Computes the unique non-wildcard base paths amongst the provided include patterns.
* @returns Absolute directory paths
*/
function getBasePaths(absoluteTsconfigPath: string, includes: readonly string[] | undefined, useCaseSensitiveFileNames: boolean): string[] {
function getBasePaths(path: string, includes: readonly string[] | undefined, useCaseSensitiveFileNames: boolean): string[] {
// Storage for our results in the form of literal paths (e.g. the paths as written by the user).
const basePaths: string[] = [absoluteTsconfigPath];
const basePaths: string[] = [path];

if (includes) {
// Storage for literal base paths amongst the include patterns.
const includeBasePaths: string[] = [];
for (const include of includes) {
// We also need to check the relative paths by converting them to absolute and normalizing
// in case they escape the base path (e.g "..\somedirectory")
const absoluteIncludePath: string = isRootedDiskPath(include) ? include : normalizePath(combinePaths(absoluteTsconfigPath, include));
const absolute: string = isRootedDiskPath(include) ? include : normalizePath(combinePaths(path, include));
// Append the literal and canonical candidate base paths.
includeBasePaths.push(getIncludeBasePath(absoluteIncludePath));
includeBasePaths.push(getIncludeBasePath(absolute));
}

// Sort the offsets array using either the literal or canonical path representations.
Expand All @@ -6725,7 +6723,7 @@ namespace ts {
// Iterate over each include base path and include unique base paths that are not a
// subpath of an existing base path
for (const includeBasePath of includeBasePaths) {
if (every(basePaths, basePath => !containsPath(basePath, includeBasePath, absoluteTsconfigPath, !useCaseSensitiveFileNames))) {
if (every(basePaths, basePath => !containsPath(basePath, includeBasePath, path, !useCaseSensitiveFileNames))) {
basePaths.push(includeBasePath);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/watchUtilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ namespace ts {
const rootResult = tryReadDirectory(rootDir, rootDirPath);
let rootSymLinkResult: FileSystemEntries | undefined;
if (rootResult !== undefined) {
return matchFiles(rootDir, extensions, excludes, includes, useCaseSensitiveFileNames, currentDirectory, depth, getFileSystemEntries, realpath, directoryExists);
return matchFiles(rootDir, extensions, excludes, includes, useCaseSensitiveFileNames, currentDirectory, depth, getFileSystemEntries, realpath);
}
return host.readDirectory!(rootDir, extensions, excludes, includes, depth);

Expand Down
2 changes: 1 addition & 1 deletion src/harness/fakesHosts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ namespace fakes {
}

public readDirectory(path: string, extensions?: readonly string[], exclude?: readonly string[], include?: readonly string[], depth?: number): string[] {
return ts.matchFiles(path, extensions, exclude, include, this.useCaseSensitiveFileNames, this.getCurrentDirectory(), depth, path => this.getAccessibleFileSystemEntries(path), path => this.realpath(path), path => this.directoryExists(path));
return ts.matchFiles(path, extensions, exclude, include, this.useCaseSensitiveFileNames, this.getCurrentDirectory(), depth, path => this.getAccessibleFileSystemEntries(path), path => this.realpath(path));
}

public getAccessibleFileSystemEntries(path: string): ts.FileSystemEntries {
Expand Down
5 changes: 2 additions & 3 deletions src/harness/harnessIO.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1438,15 +1438,14 @@ namespace Harness {
}

const referenceDir = referencePath(relativeFileBase, opts && opts.Baselinefolder, opts && opts.Subfolder);
let existing = IO.readDirectory(referenceDir, referencedExtensions || [extension]); // always an _absolute_ path
let existing = IO.readDirectory(referenceDir, referencedExtensions || [extension]);
if (extension === ".ts" || referencedExtensions && referencedExtensions.indexOf(".ts") > -1 && referencedExtensions.indexOf(".d.ts") === -1) {
// special-case and filter .d.ts out of .ts results
existing = existing.filter(f => !ts.endsWith(f, ".d.ts"));
}
const missing: string[] = [];
const absoluteTestDir = `${process.cwd()}/${referenceDir}`;
for (const name of existing) {
const localCopy = name.substring(absoluteTestDir.length - relativeFileBase.length);
const localCopy = name.substring(referenceDir.length - relativeFileBase.length);
if (!writtenFiles.has(localCopy)) {
missing.push(localCopy);
}
Expand Down
2 changes: 1 addition & 1 deletion src/harness/virtualFileSystemWithWatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -922,7 +922,7 @@ interface Array<T> { length: number; [n: number]: T; }`
});
}
return { directories, files };
}, path => this.realpath(path), path => this.directoryExists(path));
}, path => this.realpath(path));
}

createHash(s: string): string {
Expand Down
31 changes: 0 additions & 31 deletions src/testRunner/unittests/publicApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,34 +182,3 @@ describe("unittests:: Public APIs:: getChild* methods on EndOfFileToken with JSD
assert.equal(endOfFileToken.getChildCount(), 1);
assert.notEqual(endOfFileToken.getChildAt(0), /*expected*/ undefined);
});

describe("unittests:: Public APIs:: sys", () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth keeping?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We’ll bring it back when we decide what the correct thing to assert is

it("readDirectory", () => {
// #45990, testing passing a non-absolute path
// `sys.readDirectory` is just `matchFiles` plugged into the real FS
const read = ts.matchFiles(
/*path*/ "",
/*extensions*/ [".ts", ".tsx"],
/*excludes*/ ["node_modules", "dist"],
/*includes*/ ["**/*"],
/*useCaseSensitiveFileNames*/ true,
/*currentDirectory*/ "/",
/*depth*/ undefined,
/*getFileSystemEntries*/ path => {
switch (path) {
case "/": return { directories: [], files: ["file.ts"] };
default: return { directories: [], files: [] };
}
},
/*realpath*/ ts.identity,
/*directoryExists*/ path => {
switch (path) {
case "/": return true;
default: return false;
}
}
);

assert.deepEqual(read, ["/file.ts"]);
});
});