Skip to content

Commit b08173b

Browse files
mavasanimavasani
authored
Merge pull request #44750 from mavasani/RelaxDedupeLogicInCodeFixService
Relax dedupe logic in code fix service
2 parents 56dc421 + 97d5d26 commit b08173b

File tree

2 files changed

+95
-18
lines changed

2 files changed

+95
-18
lines changed

‎src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -314,11 +314,17 @@ private async Task AppendFixesAsync(
314314

315315
var extensionManager = document.Project.Solution.Workspace.Services.GetService<IExtensionManager>();
316316

317-
// Run each CodeFixProvider to gather individual CodeFixes for reported diagnostics
318-
// Ensure that each diagnostic only has a unique registered code action for any given equivalance key.
317+
// Run each CodeFixProvider to gather individual CodeFixes for reported diagnostics.
318+
// Ensure that no diagnostic has registered code actions from different code fix providers with same equivalance key.
319319
// This prevents duplicate registered code actions from NuGet and VSIX code fix providers.
320320
// See https://github.com/dotnet/roslyn/issues/18818 for details.
321321
var uniqueDiagosticToEquivalenceKeysMap = new Dictionary<Diagnostic, PooledHashSet<string?>>();
322+
323+
// NOTE: For backward compatibility, we allow multiple registered code actions from the same code fix provider
324+
// to have the same equivalence key. See https://github.com/dotnet/roslyn/issues/44553 for details.
325+
// To ensure this, we track the fixer that first registered a code action to fix a diagnostic with a specific equivalence key.
326+
var diagnosticAndEquivalenceKeyToFixersMap = new Dictionary<(Diagnostic diagnostic, string? equivalenceKey), CodeFixProvider>();
327+
322328
try
323329
{
324330
foreach (var fixer in allFixers.Distinct())
@@ -338,11 +344,13 @@ await AppendFixesOrConfigurationsAsync(
338344
{
339345
var primaryDiagnostic = dxs.First();
340346
return GetCodeFixesAsync(document, primaryDiagnostic.Location.SourceSpan, fixer, isBlocking,
341-
ImmutableArray.Create(primaryDiagnostic), uniqueDiagosticToEquivalenceKeysMap, cancellationToken);
347+
ImmutableArray.Create(primaryDiagnostic), uniqueDiagosticToEquivalenceKeysMap,
348+
diagnosticAndEquivalenceKeyToFixersMap, cancellationToken);
342349
}
343350
else
344351
{
345-
return GetCodeFixesAsync(document, span, fixer, isBlocking, dxs, uniqueDiagosticToEquivalenceKeysMap, cancellationToken);
352+
return GetCodeFixesAsync(document, span, fixer, isBlocking, dxs,
353+
uniqueDiagosticToEquivalenceKeysMap, diagnosticAndEquivalenceKeyToFixersMap, cancellationToken);
346354
}
347355
}
348356
},
@@ -366,6 +374,7 @@ private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
366374
Document document, TextSpan span, CodeFixProvider fixer, bool isBlocking,
367375
ImmutableArray<Diagnostic> diagnostics,
368376
Dictionary<Diagnostic, PooledHashSet<string?>> uniqueDiagosticToEquivalenceKeysMap,
377+
Dictionary<(Diagnostic diagnostic, string? equivalenceKey), CodeFixProvider> diagnosticAndEquivalenceKeyToFixersMap,
369378
CancellationToken cancellationToken)
370379
{
371380
using var fixesDisposer = ArrayBuilder<CodeFix>.GetInstance(out var fixes);
@@ -377,7 +386,8 @@ private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
377386
lock (fixes)
378387
{
379388
// Filter out applicable diagnostics which already have a registered code action with same equivalence key.
380-
applicableDiagnostics = FilterApplicableDiagnostics(applicableDiagnostics, action.EquivalenceKey, uniqueDiagosticToEquivalenceKeysMap);
389+
applicableDiagnostics = FilterApplicableDiagnostics(applicableDiagnostics, action.EquivalenceKey,
390+
fixer, uniqueDiagosticToEquivalenceKeysMap, diagnosticAndEquivalenceKeyToFixersMap);
381391

382392
if (!applicableDiagnostics.IsEmpty)
383393
{
@@ -396,20 +406,33 @@ private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
396406
static ImmutableArray<Diagnostic> FilterApplicableDiagnostics(
397407
ImmutableArray<Diagnostic> applicableDiagnostics,
398408
string? equivalenceKey,
399-
Dictionary<Diagnostic, PooledHashSet<string?>> uniqueDiagosticToEquivalenceKeysMap)
409+
CodeFixProvider fixer,
410+
Dictionary<Diagnostic, PooledHashSet<string?>> uniqueDiagosticToEquivalenceKeysMap,
411+
Dictionary<(Diagnostic diagnostic, string? equivalenceKey), CodeFixProvider> diagnosticAndEquivalenceKeyToFixersMap)
400412
{
401413
using var disposer = ArrayBuilder<Diagnostic>.GetInstance(out var newApplicableDiagnostics);
402414
foreach (var diagnostic in applicableDiagnostics)
403415
{
404416
if (!uniqueDiagosticToEquivalenceKeysMap.TryGetValue(diagnostic, out var equivalenceKeys))
405417
{
418+
// First code action registered to fix this diagnostic with any equivalenceKey.
419+
// Record the equivalence key and the fixer that registered this action.
406420
equivalenceKeys = PooledHashSet<string?>.GetInstance();
407421
equivalenceKeys.Add(equivalenceKey);
408422
uniqueDiagosticToEquivalenceKeysMap[diagnostic] = equivalenceKeys;
423+
diagnosticAndEquivalenceKeyToFixersMap.Add((diagnostic, equivalenceKey), fixer);
424+
}
425+
else if (equivalenceKeys.Add(equivalenceKey))
426+
{
427+
// First code action registered to fix this diagnostic with the given equivalenceKey.
428+
// Record the the fixer that registered this action.
429+
diagnosticAndEquivalenceKeyToFixersMap.Add((diagnostic, equivalenceKey), fixer);
409430
}
410-
else if (!equivalenceKeys.Add(equivalenceKey))
431+
else if (diagnosticAndEquivalenceKeyToFixersMap[(diagnostic, equivalenceKey)] != fixer)
411432
{
412-
// Diagnostic already has a registered code action with same equivalence key.
433+
// Diagnostic already has a registered code action with same equivalence key from a different fixer.
434+
// Note that we allow same fixer to register multiple such code actions with the same equivalence key
435+
// for backward compatibility. See https://github.com/dotnet/roslyn/issues/44553 for details.
413436
continue;
414437
}
415438

‎src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs‎

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
using System;
88
using System.Collections.Generic;
99
using System.Collections.Immutable;
10-
using System.Composition;
1110
using System.Linq;
1211
using System.Threading;
1312
using System.Threading.Tasks;
@@ -97,33 +96,33 @@ public async Task TestGetFixesAsyncWithDuplicateDiagnostics()
9796
public async Task TestGetCodeFixWithExceptionInRegisterMethod()
9897
{
9998
await GetFirstDiagnosticWithFixAsync(new ErrorCases.ExceptionInRegisterMethod());
100-
await GetAddedFixesAsync(new ErrorCases.ExceptionInRegisterMethod());
99+
await GetAddedFixesWithExceptionValidationAsync(new ErrorCases.ExceptionInRegisterMethod());
101100
}
102101

103102
[Fact]
104103
public async Task TestGetCodeFixWithExceptionInRegisterMethodAsync()
105104
{
106105
await GetFirstDiagnosticWithFixAsync(new ErrorCases.ExceptionInRegisterMethodAsync());
107-
await GetAddedFixesAsync(new ErrorCases.ExceptionInRegisterMethodAsync());
106+
await GetAddedFixesWithExceptionValidationAsync(new ErrorCases.ExceptionInRegisterMethodAsync());
108107
}
109108

110109
[Fact]
111110
public async Task TestGetCodeFixWithExceptionInFixableDiagnosticIds()
112111
{
113112
await GetDefaultFixesAsync(new ErrorCases.ExceptionInFixableDiagnosticIds());
114-
await GetAddedFixesAsync(new ErrorCases.ExceptionInFixableDiagnosticIds());
113+
await GetAddedFixesWithExceptionValidationAsync(new ErrorCases.ExceptionInFixableDiagnosticIds());
115114
}
116115

117116
[Fact(Skip = "https://github.com/dotnet/roslyn/issues/21533")]
118117
public async Task TestGetCodeFixWithExceptionInFixableDiagnosticIds2()
119118
{
120119
await GetDefaultFixesAsync(new ErrorCases.ExceptionInFixableDiagnosticIds2());
121-
await GetAddedFixesAsync(new ErrorCases.ExceptionInFixableDiagnosticIds2());
120+
await GetAddedFixesWithExceptionValidationAsync(new ErrorCases.ExceptionInFixableDiagnosticIds2());
122121
}
123122

124123
[Fact]
125124
public async Task TestGetCodeFixWithExceptionInGetFixAllProvider()
126-
=> await GetAddedFixesAsync(new ErrorCases.ExceptionInGetFixAllProvider());
125+
=> await GetAddedFixesWithExceptionValidationAsync(new ErrorCases.ExceptionInGetFixAllProvider());
127126

128127
private async Task GetDefaultFixesAsync(CodeFixProvider codefix)
129128
{
@@ -136,7 +135,10 @@ private async Task GetDefaultFixesAsync(CodeFixProvider codefix)
136135
Assert.True(((TestErrorLogger)tuple.errorLogger).Messages.TryGetValue(codefix.GetType().Name, out var message));
137136
}
138137

139-
private async Task GetAddedFixesAsync(CodeFixProvider codefix)
138+
private Task<ImmutableArray<CodeFixCollection>> GetAddedFixesWithExceptionValidationAsync(CodeFixProvider codefix)
139+
=> GetAddedFixesAsync(codefix, diagnosticAnalyzer: new MockAnalyzerReference.MockDiagnosticAnalyzer(), exception: true);
140+
141+
private async Task<ImmutableArray<CodeFixCollection>> GetAddedFixesAsync(CodeFixProvider codefix, DiagnosticAnalyzer diagnosticAnalyzer, bool exception = false)
140142
{
141143
var tuple = ServiceSetup(codefix);
142144

@@ -145,13 +147,18 @@ private async Task GetAddedFixesAsync(CodeFixProvider codefix)
145147
GetDocumentAndExtensionManager(tuple.analyzerService, workspace, out var document, out var extensionManager);
146148
var incrementalAnalyzer = (IIncrementalAnalyzerProvider)tuple.analyzerService;
147149
var analyzer = incrementalAnalyzer.CreateIncrementalAnalyzer(workspace);
148-
var reference = new MockAnalyzerReference(codefix);
150+
var reference = new MockAnalyzerReference(codefix, ImmutableArray.Create(diagnosticAnalyzer));
149151
var project = workspace.CurrentSolution.Projects.Single().AddAnalyzerReference(reference);
150152
document = project.Documents.Single();
151153
var fixes = await tuple.codeFixService.GetFixesAsync(document, TextSpan.FromBounds(0, 0), includeConfigurationFixes: true, cancellationToken: CancellationToken.None);
152154

153-
Assert.True(extensionManager.IsDisabled(codefix));
154-
Assert.False(extensionManager.IsIgnored(codefix));
155+
if (exception)
156+
{
157+
Assert.True(extensionManager.IsDisabled(codefix));
158+
Assert.False(extensionManager.IsIgnored(codefix));
159+
}
160+
161+
return fixes;
155162
}
156163

157164
private async Task GetFirstDiagnosticWithFixAsync(CodeFixProvider codefix)
@@ -527,5 +534,52 @@ public override Task RegisterCodeFixesAsync(CodeFixContext context)
527534
return Task.CompletedTask;
528535
}
529536
}
537+
538+
[Theory, WorkItem(44553, "https://github.com/dotnet/roslyn/issues/44553")]
539+
[InlineData(null)]
540+
[InlineData("CodeFixProviderWithDuplicateEquivalenceKeyActions")]
541+
public async Task TestRegisteredCodeActionsWithSameEquivalenceKey(string? equivalenceKey)
542+
{
543+
var diagnosticId = "ID1";
544+
var analyzer = new MockAnalyzerReference.MockDiagnosticAnalyzer(ImmutableArray.Create(diagnosticId));
545+
var fixer = new CodeFixProviderWithDuplicateEquivalenceKeyActions(diagnosticId, equivalenceKey);
546+
547+
// Verify multiple code actions registered with same equivalence key are not de-duped.
548+
var fixes = (await GetAddedFixesAsync(fixer, analyzer)).SelectMany(fixCollection => fixCollection.Fixes).ToList();
549+
Assert.Equal(2, fixes.Count);
550+
}
551+
552+
private sealed class CodeFixProviderWithDuplicateEquivalenceKeyActions : CodeFixProvider
553+
{
554+
private readonly string _diagnosticId;
555+
private readonly string? _equivalenceKey;
556+
557+
public CodeFixProviderWithDuplicateEquivalenceKeyActions(string diagnosticId, string? equivalenceKey)
558+
{
559+
_diagnosticId = diagnosticId;
560+
_equivalenceKey = equivalenceKey;
561+
}
562+
563+
public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create(_diagnosticId);
564+
565+
public override Task RegisterCodeFixesAsync(CodeFixContext context)
566+
{
567+
// Register duplicate code actions with same equivalence key, but different title.
568+
RegisterCodeFix(context, titleSuffix: "1");
569+
RegisterCodeFix(context, titleSuffix: "2");
570+
571+
return Task.CompletedTask;
572+
}
573+
574+
private void RegisterCodeFix(CodeFixContext context, string titleSuffix)
575+
{
576+
context.RegisterCodeFix(
577+
CodeAction.Create(
578+
nameof(CodeFixProviderWithDuplicateEquivalenceKeyActions) + titleSuffix,
579+
ct => Task.FromResult(context.Document),
580+
_equivalenceKey),
581+
context.Diagnostics);
582+
}
583+
}
530584
}
531585
}

0 commit comments

Comments
 (0)