Fix bug in ensureProjectStructuresUpToDate that [maybe?] can lead to poisoned cache

[MarcCelani-at]

This PR may improve (but does not seem to fully fix) #47274

The return value of updateProjectIfDirty is not obvious to callers. It returns true if the project is dirty but when the set of files in the project stayed the same (Project.updateGraph returns true if set of files in the project stays the same and false - otherwise). I have to imagine that this is a bug, because this is a very nuanced return value.

There is only one call site of updateProjectIfDirty that checks the return value - in ensureProjectStructuresUptoDate. That code appears to assume that the function returns true if the project changed.

This PR changes updateProjectIfDirty to return boolean in similar fashion to updateGraph (true if set of files in the project stays the same, false otherwise). Update the call site to properly handle the return value.

I don't have a good sense of how to test this. Happy to add a unit test if someone can point me to another test that tests something similar.

Suspect that...

Fixes #47274

[typescript-bot]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[andrewbranch]

Thanks! Can we add a @returns JSDoc comment on that function that explains what the boolean means? It’s still a little hard to grok it without navigating all the way into Project#updateGraph.

[DanielRosenwasser]

Looks like we won't get this in in time for the beta.

Regardless I think we need to get a test case that failed earlier.

[MarcCelani-at]

I don't understand this code well enough to know what's going on here, this was honestly a shot in the dark. It doesn't fix the underlying issue for my repro (large checkout in a mono-repo). The existing code just looks suspicious given what the code is doing and the comments.

It would be great if someone more familiar with this code can look at it and maybe decide if it's a bug or not (I know this is a weird request haha)

Actually, a better request would be that...I'm very eager to pair-debug with someone from the typescript team. I have an extremely clear repro of the issue (#47274) in my mono-repo, which unfortunately I can't grant the typescript team access to, but I'd happy share screen over zoom and spend cycles on my side getting to the bottom of it. Sadly, this shot in the dark is the only thing I've spotted after probably a good 15+ hours debugging this issue.

[sheetalkamat]

This is incorrect change.. the intension here is to call updateGraph and get its return value only if it is marked as dirty.. So original code is correct in what it intends to do..

[MarcCelani-at]

Ok thanks for confirming. I'll abandon this PR. It anyway did not show any change in correctness. Though it's a little unfortunate that this change does not trigger any unit test failures.