Estimate file versions safely #2753
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For a long time,
defineEarlyCutoffhas been accessing theValuesstore directlyto compute
GetModificationTimevalues instead of callinguse, breaking theinvariant. The values are used to associate the rule result to a file version,
which gets recorded in the
Valueas well as used as the key in the Diagnosticsstore.
The problem here is that the
GetModificationTimerule computes a new version andmutates the Values store, so if
defineEarlyCutoffpeeks in the store beforeGetModificationTimehas run, it will grab the old version. This leads to lostdiagnostics and potentially to misversioned
Values.Fixing the problem is tricky, because we cannot simply use
GetModificationTimeinside
defineEarlyCutofffor all rules. There are three issues with this:GetModificationTime: if everything depends on it,then we lose the ability to do early cutoff
GetModificationTimehasdependencies itself. Because hls-graph doesn't implement cycle detection (Shake
did), it is a nightmare to debug these cycles.
GetModificationTimecalls the file system for nonFOIs and in the past this was very expensive for projects with large cartesian
product of module paths and source folders
To work around these I had to introduce a new hls-graph primitive,
applyWithoutDependency, as well as add a bunch more fragile type tests on the keytype to decide on whether to use
GetModificationTimeor peek into the valuesstore. The type casts could be cleaned up by introducing a type class, but I'm
not sure the end result would be any better.
To understand the issue and debug the implementation of the fix, I added a
number of opentelemety traces which I'm leaving in place in case they could be
useful in the future.