Skip to content

Make handleDtsMayChangeOf void-returning #44322

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.

Sign up for GitHub

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

Merged
amcasey merged 2 commits into from
Jun 11, 2021
Merged

Make handleDtsMayChangeOf void-returning #44322

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ad62b75
Make handleDtsMayChangeOf void-returning
amcasey May 28, 2021
c7399b8
Make forEachFileAndExportsOfFile void-returning
amcasey May 28, 2021
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
42 changes: 16 additions & 26 deletions src/compiler/builder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,6 @@ namespace ts {
}
}
}

return false;
}

/**
Expand All @@ -517,7 +515,7 @@ namespace ts {
/**
* Iterate on referencing modules that export entities from affected file
*/
function forEachReferencingModulesOfExportOfAffectedFile(state: BuilderProgramState, affectedFile: SourceFile, fn: (state: BuilderProgramState, filePath: Path) => boolean) {
function forEachReferencingModulesOfExportOfAffectedFile(state: BuilderProgramState, affectedFile: SourceFile, fn: (state: BuilderProgramState, filePath: Path) => void) {
// If there was change in signature (dts output) for the changed file,
// then only we need to handle pending file emit
if (!state.exportedModulesMap || !state.changedFilesSet.has(affectedFile.resolvedPath)) {
Expand All @@ -536,8 +534,8 @@ namespace ts {
const currentPath = queue.pop()!;
if (!seenFileNamesMap.has(currentPath)) {
seenFileNamesMap.set(currentPath, true);
const result = fn(state, currentPath);
if (result && isChangedSignature(state, currentPath)) {
fn(state, currentPath);
if (isChangedSignature(state, currentPath)) {
const currentSourceFile = Debug.checkDefined(state.program).getSourceFileByPath(currentPath)!;
Copy link
Member Author

@amcasey amcasey May 28, 2021

Choose a reason for hiding this comment

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

I'm pretty sure never entering this branch was a bug.

queue.push(...BuilderState.getReferencedByPaths(state, currentSourceFile.resolvedPath));
Copy link
Member Author

@amcasey amcasey May 28, 2021

Choose a reason for hiding this comment

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

If no one objects, I'd kind of like to rename queue to "stack", since it's manipulated with push and pop.

}
Expand All @@ -549,13 +547,11 @@ namespace ts {
const seenFileAndExportsOfFile = new Set<string>();
// Go through exported modules from cache first
// If exported modules has path, all files referencing file exported from are affected
if (forEachEntry(state.currentAffectedFilesExportedModulesMap, (exportedModules, exportedFromPath) =>
forEachEntry(state.currentAffectedFilesExportedModulesMap, (exportedModules, exportedFromPath) =>
Copy link
Member

@sheetalkamat sheetalkamat Jun 1, 2021

Choose a reason for hiding this comment

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

If this queues files , shouldnt we stop here and not do next step.. its duplicated work?

Copy link
Member Author

@amcasey amcasey Jun 1, 2021

Choose a reason for hiding this comment

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

I'm not sure what it should do, but I believe this is just a clearer way to write what it already does do. I don't understand the code very well and may have missed a case where it does return true and exit early.

Copy link
Member Author

@amcasey amcasey Jun 1, 2021

Choose a reason for hiding this comment

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

Specifically, I'm less confident in the second commit than the first. AFAICT, it could only return true if a recursive call to itself returned true and all of the recursive base cases returned false.

exportedModules &&
exportedModules.has(affectedFile.resolvedPath) &&
forEachFilesReferencingPath(state, exportedFromPath, seenFileAndExportsOfFile, fn)
)) {
return;
}
);

// If exported from path is not from cache and exported modules has path, all files referencing file exported from are affected
forEachEntry(state.exportedModulesMap, (exportedModules, exportedFromPath) =>
Expand All @@ -568,47 +564,41 @@ namespace ts {
/**
* Iterate on files referencing referencedPath
*/
function forEachFilesReferencingPath(state: BuilderProgramState, referencedPath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => boolean) {
return forEachEntry(state.referencedMap!, (referencesInFile, filePath) =>
function forEachFilesReferencingPath(state: BuilderProgramState, referencedPath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => void) {
forEachEntry(state.referencedMap!, (referencesInFile, filePath) =>
referencesInFile.has(referencedPath) && forEachFileAndExportsOfFile(state, filePath, seenFileAndExportsOfFile, fn)
);
}

/**
* fn on file and iterate on anything that exports this file
*/
function forEachFileAndExportsOfFile(state: BuilderProgramState, filePath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => boolean): boolean {
function forEachFileAndExportsOfFile(state: BuilderProgramState, filePath: Path, seenFileAndExportsOfFile: Set<string>, fn: (state: BuilderProgramState, filePath: Path) => void) {
if (!tryAddToSet(seenFileAndExportsOfFile, filePath)) {
return false;
return;
}

if (fn(state, filePath)) {
// If there are no more diagnostics from old cache, done
return true;
}
fn(state, filePath);

Debug.assert(!!state.currentAffectedFilesExportedModulesMap);
// Go through exported modules from cache first
// If exported modules has path, all files referencing file exported from are affected
if (forEachEntry(state.currentAffectedFilesExportedModulesMap, (exportedModules, exportedFromPath) =>
forEachEntry(state.currentAffectedFilesExportedModulesMap, (exportedModules, exportedFromPath) =>
exportedModules &&
exportedModules.has(filePath) &&
forEachFileAndExportsOfFile(state, exportedFromPath, seenFileAndExportsOfFile, fn)
)) {
return true;
}
);

// If exported from path is not from cache and exported modules has path, all files referencing file exported from are affected
if (forEachEntry(state.exportedModulesMap!, (exportedModules, exportedFromPath) =>
forEachEntry(state.exportedModulesMap!, (exportedModules, exportedFromPath) =>
!state.currentAffectedFilesExportedModulesMap!.has(exportedFromPath) && // If we already iterated this through cache, ignore it
exportedModules.has(filePath) &&
forEachFileAndExportsOfFile(state, exportedFromPath, seenFileAndExportsOfFile, fn)
)) {
return true;
}
);

// Remove diagnostics of files that import this file (without going to exports of referencing files)
return !!forEachEntry(state.referencedMap!, (referencesInFile, referencingFilePath) =>

forEachEntry(state.referencedMap!, (referencesInFile, referencingFilePath) =>
referencesInFile.has(filePath) &&
!seenFileAndExportsOfFile.has(referencingFilePath) && // Not already removed diagnostic file
fn(state, referencingFilePath) // Dont add to seen since this is not yet done with the export removal
Expand Down