Compute fixes by order of fixers, so we don't have to sort them after#59584
Compute fixes by order of fixers, so we don't have to sort them after#59584CyrusNajmabadi merged 16 commits intodotnet:mainfrom
Conversation
| => priorityMap.TryGetValue((CodeFixProvider)c.Provider, out var value) ? value : int.MaxValue; | ||
| await AppendFixesAsync( | ||
| document, aggregatedDiagnostics, fixAllForInSpan: false, | ||
| priority, options, result, addOperationScope, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
this is the meat of hte change. previously we would go through each span/diagnostic asked, adn then (internally) go through all fixers to get the fixes for them. Then, after that we'd come and sort the results by codefixprovider to get the final result. This meant we couldn't return anything until they had all computed and we then ordered by fixer.
Now, we just order by fixers first, and run each fixer against the span/diags they can fix.
| @@ -42,7 +42,7 @@ internal partial class CodeFixService : ICodeFixService | |||
| private readonly ImmutableArray<Lazy<CodeFixProvider, CodeChangeProviderMetadata>> _fixers; | |||
There was a problem hiding this comment.
this PR should be viewed with whitespace off.
|
|
||
| // If the fixers specify an explicit ordering between each other, then respect that. | ||
| if (_priorityMap.TryGetValue(x, out var xOrder) && | ||
| _priorityMap.TryGetValue(y, out var yOrder)) |
There was a problem hiding this comment.
this is teh same issue as https://github.com/dotnet/roslyn/pull/59586/files#diff-5f6cf0794827f40d6b8b8abae0080ef94419f1cffc0d7343a0c21fcf6c6ceb9fR483
Specifically, our tests end up creating a CodeFixService with codefixproviders that have no CodeFixMetadata on them. So we end up not knowing anything about them and these later bits of code have to be resilient to that. I don't know if htis is intentional and the real codefixservice is supposed to be able to handle thsi. Or if this is jsut a case of badly behaving tests doing the wrong thing.
|
@jasonmalinowski @mavasani this is ready for review. |
jasonmalinowski
left a comment
There was a problem hiding this comment.
Signed off, with same caveat as you had for the other one: maybe those tests need to be fixed.
| if (diagnostic.IsSuppressed) | ||
| continue; | ||
|
|
||
| cancellationToken.ThrowIfCancellationRequested(); |
There was a problem hiding this comment.
Didn't want to keep the cancellation token? More not sure if anything else is checking it around here.
There was a problem hiding this comment.
so in this case the change of the CT firing here is practically nill. this is basically just taking a small list of values and converting to a dictiony (without any processing work).
i will add a CT check on all method entrypoints though.
Co-authored-by: Jason Malinowski <jason@jason-m.com>
Followup to #59582
Teh current approach computes fixes in random order of fixers. It then sorts the result by fixer-order (e.g. fixers can order themselves against other fixers).
Because of this, we cannot return fixes until all fixers have run. There is a larger piece of work i want to do here where the CodeFixService returns an IAsyncEnumerable. To be able to do that, and have it return resutls in a sensible order that can be used immediately, we need to compute things in the order that they would be returned in.
The benefit of this is that we will now have a common path both for the code that computes all teh fixes for a lightbulb, and the code that wants to determine if a lightbulb should show only needs to pull on the first item of the IAsyncEnumerable.
So now, we determien the fixers we want to run and sort them first. Then we compute the results per fixer. Thsi will allow us in the future to just return the result once the fixer has computed it.