Make handleDtsMayChangeOf void-returning #44322
Conversation
Right now, it always returns false. This seems important, since otherwise it would stop graph traversals prematurely. It took me a while to figure that out though and I thought it might be clearer if it were simply void-returning. I've kept the behavior the same, except in `forEachReferencingModulesOfExportOfAffectedFile`, where it seemed like never enqueueing new references was a mistake.
typescript-bot
added
Author: Team
For Uncommitted Bug
| 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)!; |
There was a problem hiding this comment.
I'm pretty sure never entering this branch was a bug.
| 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)!; | ||
| queue.push(...BuilderState.getReferencedByPaths(state, currentSourceFile.resolvedPath)); |
There was a problem hiding this comment.
If no one objects, I'd kind of like to rename queue to "stack", since it's manipulated with push and pop.
As far as I can tell, it could only return `false`.
|
It's probably easier to review the commits separately. |
| @@ -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) => | |||
There was a problem hiding this comment.
If this queues files , shouldnt we stop here and not do next step.. its duplicated work?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
FYI: OK i remembered why it was returning boolean instead of void. removeSemanticDiagnosticsOf had intent that if we have deleted everything, we dont need to iterate on files any more. But after we added emit for d.ts is also needed part of the equation we couldnt use that any more (because you may or may not copy semantic diagnostics per your builder options) so we couldnt break the loop like that and had to ensure to iterate through everything and thats where the returning true part was lost and hence not needed any more. |
Right now, it always returns false. This seems important, since otherwise it would stop graph traversals prematurely. It took me a while to figure that out though and I thought it might be clearer if it were simply void-returning.
I've kept the behavior the same, except in
forEachReferencingModulesOfExportOfAffectedFile, where it seemed like never enqueueing new references was a mistake.