Fix throw in generator comparer #76769
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
All nodes either passed in a default comparer or null, which was stored in the node table. Modifications then had the option to supply a seperate comparer to do the modification. In all cases the comparer passed into the modify call was the same as the one passed when creating the table, so we can remove the call from modify and just use the table instance. Rather than each node passing it its own default when it doesn't have a comparer, just pass in null and let the table control creating the default.
chsienki
force-pushed
the
fix_throw_in_generator_comparer
branch
from
330ad77 to
1d077d9
Compare
|
@dotnet/roslyn-compiler for review please. |
| @@ -25,6 +25,8 @@ internal sealed class WrappedUserComparer<T> : IEqualityComparer<T> | |||
| { | |||
| private readonly IEqualityComparer<T> _inner; | |||
|
|
|||
| public static WrappedUserComparer<T> Default { get; } = new WrappedUserComparer<T>(EqualityComparer<T>.Default); | |||
|
|
|||
| public WrappedUserComparer(IEqualityComparer<T> inner) | |||
There was a problem hiding this comment.
When you originally wrote this rather than a ctor did you consider the following?
public static WrappedUserComparer Create(IEqualityComparer<T> comparer) =>
comparer is WrappedUserComparer<T> w ? w : new WrappedUserComparer(comparer);One concern I had when looking through this code was if we were inadvertently creating nested wraps.
There was a problem hiding this comment.
It's an internal class and you can only create it via an extension method that doesn't return it, so it shouldn't be possible to actually get an instance of it that can be passed to the ctor as an external user.
| @@ -72,7 +72,7 @@ public CombineNode(IIncrementalGeneratorNode<TInput1> input1, IIncrementalGenera | |||
| }; | |||
|
|
|||
| var entry = (entry1.Item, input2); | |||
| if (state != EntryState.Modified || _comparer is null || !tableBuilder.TryModifyEntry(entry, _comparer, stopwatch.Elapsed, stepInputs, state)) | |||
| if (state != EntryState.Modified || _comparer is null || !tableBuilder.TryModifyEntry(entry, stopwatch.Elapsed, stepInputs, state)) | |||
There was a problem hiding this comment.
What does the check _comparer is null serve here? The null case means it's effectively the default comparer so why are we not trying to use that for TryModifyEntry?
There was a problem hiding this comment.
It's an optimization. If we've already determined that one of the two sides is modified then the default comparer will always find the same result. However if there's a custom comparer it's possible that will override the result and return cached.
| @@ -48,7 +48,7 @@ public Builder(PredicateSyntaxStrategy<T> owner, object key, StateTableStore tab | |||
| _name = name; | |||
| _comparer = comparer; | |||
| _key = key; | |||
| _filterTable = table.GetStateTableOrEmpty<SyntaxNode>(_owner._filterKey).ToBuilder(stepName: null, trackIncrementalSteps); | |||
| _filterTable = table.GetStateTableOrEmpty<SyntaxNode>(_owner._filterKey).ToBuilder(stepName: null, trackIncrementalSteps, equalityComparer: Roslyn.Utilities.ReferenceEqualityComparer.Instance); | |||
There was a problem hiding this comment.
T is not constrainted to class so why do we use a reference equality comparer vs. simply the default comparer?
There was a problem hiding this comment.
This was code written by Cyrus originally, I'm refactoring it to keep it semantically the same.
| @@ -46,12 +46,12 @@ public NodeStateTable<TOutput> UpdateStateTable(DriverStateTable.Builder graphSt | |||
| this.LogTables(stepName, s_tableType, previousTable, previousTable, sourceTable); | |||
| if (graphState.DriverState.TrackIncrementalSteps) | |||
| { | |||
| return previousTable.CreateCachedTableWithUpdatedSteps(sourceTable, stepName, EqualityComparer<TOutput>.Default); | |||
| return previousTable.CreateCachedTableWithUpdatedSteps(sourceTable, stepName, equalityComparer: null); | |||
There was a problem hiding this comment.
This creates a new NodeStateTable that drops the comparer from previousTable. Is that intended?
There was a problem hiding this comment.
Yes, if it's all cached the comparer would never be used. Later on, if there's a modification the table will be re-created and the real comparer will be passed to it.
| @@ -35,7 +35,7 @@ public InputNode(Func<DriverStateTable.Builder, ImmutableArray<T>> getInput, IEq | |||
| private InputNode(Func<DriverStateTable.Builder, ImmutableArray<T>> getInput, Action<IIncrementalGeneratorOutputNode>? registerOutput, IEqualityComparer<T>? inputComparer = null, IEqualityComparer<T>? comparer = null, string? name = null) | |||
| { | |||
| _getInput = getInput; | |||
| _comparer = comparer ?? EqualityComparer<T>.Default; | |||
| _comparer = comparer; | |||
| _inputComparer = inputComparer ?? EqualityComparer<T>.Default; | |||
There was a problem hiding this comment.
Should _inputComparer be handled similarly?
There was a problem hiding this comment.
No, we control the input comparer and all the inputs it compares. If that's throwing it's a real product bug and we don't want to falsely attribute it to a generator.
|
@dotnet/roslyn-compiler for any further feedback, thanks! |
When checking for generator item modification it's possible for an object to throw when calling
.Equals(). User supplied comparers are wrapped in a special comparer that catches the exception and wraps it in aUserFunctionExceptionthat is used by the generator driver to handle the error and attribute it to the offending generator.However, if no comparer is supplied the generator uses the default comparer which does not translate exceptions, meaning it can crash the driver.
Whilst fixing this I noticed that we were passing a lot of default comparers around and passing the same comparer to modify calls as we did in construction of the state table, none of which is required. By centralizing the comparer creation and passing we only need to change the default comparer to a wrapped comparer in a single place.
Commit by Commit review makes this simpler to see what / why I did.
Fixes #76765