Fix a couple of bugs in SG #55032
Fix a couple of bugs in SG #55032
Conversation
- Ensure that .Collect() reports as cached if all the inputs are cached - Don't use user comparer for matching inputs
|
@dotnet/roslyn-compiler for review please :) |
| @@ -38,7 +38,7 @@ public NodeStateTable<T> UpdateStateTable(DriverStateTable.Builder graphState, N | |||
| var inputItems = _getInput(graphState); | |||
|
|
|||
| // create a mutable hashset of the new items we can check against | |||
| HashSet<T> itemsSet = new HashSet<T>(_comparer); | |||
| HashSet<T> itemsSet = new HashSet<T>(); | |||
There was a problem hiding this comment.
Why aren't we using the custom comparer here?
There was a problem hiding this comment.
That's the bug. Because these are 'input' items (compilations / syntax trees / additional files etc) we control their lifetime. If the user comparer is used they can say that two additional files are different when they are not; that breaks our internal assumptions around comparers that they can only be used to change a modified value back to a cached value.
Basically the only time the comparer should be used is if we think the item has changed, the user has to the ability to override it.
|
|
||
| AssertTableEntries(table, new[] { (1, EntryState.Cached), (2, EntryState.Cached), (3, EntryState.Cached) }); | ||
| } | ||
|
|
| int index = 0; | ||
| foreach (var entry in table) | ||
| { | ||
| Assert.Equal(expected[index].item, (IEnumerable<T>)entry.item); |
There was a problem hiding this comment.
Consider using the AssertEx helpers that do nice output diffing to make failures more diagnosable.
|
@chsienki do you still plan on merging this for p3? |
ghost
modified the milestones:
.Collect()reports as cached if all the inputs are cachedFixes #54855