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,
defineEarlyCutoff
has been accessing theValues
store directlyto compute
GetModificationTime
values instead of callinguse
, breaking theinvariant. The values are used to associate the rule result to a file version,
which gets recorded in the
Value
as well as used as the key in the Diagnosticsstore.
The problem here is that the
GetModificationTime
rule computes a new version andmutates the Values store, so if
defineEarlyCutoff
peeks in the store beforeGetModificationTime
has 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
GetModificationTime
inside
defineEarlyCutoff
for all rules. There are three issues with this:GetModificationTime
: if everything depends on it,then we lose the ability to do early cutoff
GetModificationTime
hasdependencies itself. Because hls-graph doesn't implement cycle detection (Shake
did), it is a nightmare to debug these cycles.
GetModificationTime
calls 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
GetModificationTime
or 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.