Skip to content

Implement de-duplication of code actions from NuGet and VSIX code fixers #41768

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 18, 2020
Merged

Implement de-duplication of code actions from NuGet and VSIX code fixers #41768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a7832f
Add code fix service unit test for duplicate NuGet and VSIX code fix …
mavasani Feb 18, 2020
e38d3a1
Implement filtering logic for duplicate/overlapping code actions from…
mavasani Feb 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
118 changes: 85 additions & 33 deletions src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,10 @@ await AppendFixesAsync(
result, addOperationScope, cancellationToken).ConfigureAwait(false);
}

if (result.Count > 0)
if (result.Count > 0 && _fixerPriorityMap.TryGetValue(document.Project.Language, out var fixersForLanguage))
{
// sort the result to the order defined by the fixers
ImmutableDictionary<CodeFixProvider, int> priorityMap = _fixerPriorityMap[document.Project.Language].Value;
ImmutableDictionary<CodeFixProvider, int> priorityMap = fixersForLanguage.Value;
result.Sort((d1, d2) => GetValue(d1).CompareTo(GetValue(d2)));

int GetValue(CodeFixCollection c)
Expand Down Expand Up @@ -290,6 +290,13 @@ private async Task AppendFixesAsync(
{
cancellationToken.ThrowIfCancellationRequested();

// Prioritize NuGet based project code fixers over VSIX based workspace code fixers.
if (hasAnyProjectFixer && projectFixersMap.TryGetValue(diagnosticId, out var projectFixers))
{
Debug.Assert(!isInteractive);
allFixers.AddRange(projectFixers);
}

if (hasAnySharedFixer && fixerMap.Value.TryGetValue(diagnosticId, out var workspaceFixers))
{
if (isInteractive)
Expand All @@ -301,51 +308,62 @@ private async Task AppendFixesAsync(
allFixers.AddRange(workspaceFixers);
}
}

if (hasAnyProjectFixer && projectFixersMap.TryGetValue(diagnosticId, out var projectFixers))
{
Debug.Assert(!isInteractive);
allFixers.AddRange(projectFixers);
}
}

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

// run each CodeFixProvider to gather individual CodeFixes for reported diagnostics
foreach (var fixer in allFixers.Distinct())
// Run each CodeFixProvider to gather individual CodeFixes for reported diagnostics
// Ensure that each diagnostic only has a unique registered code action for any given equivalance key.
// This prevents duplicate registered code actions from NuGet and VSIX code fix providers.
// See https://github.com/dotnet/roslyn/issues/18818 for details.
var uniqueDiagosticToEquivalenceKeysMap = new Dictionary<Diagnostic, PooledHashSet<string>>();
try
{
cancellationToken.ThrowIfCancellationRequested();
foreach (var fixer in allFixers.Distinct())
{
cancellationToken.ThrowIfCancellationRequested();

await AppendFixesOrConfigurationsAsync(
document, span, diagnostics, fixAllForInSpan, result, fixer,
hasFix: d => this.GetFixableDiagnosticIds(fixer, extensionManager).Contains(d.Id),
getFixes: dxs =>
{
var fixerName = fixer.GetType().Name;
using (addOperationScope(fixerName))
using (RoslynEventSource.LogInformationalBlock(FunctionId.CodeFixes_GetCodeFixesAsync, fixerName, cancellationToken))
await AppendFixesOrConfigurationsAsync(
document, span, diagnostics, fixAllForInSpan, result, fixer,
hasFix: d => this.GetFixableDiagnosticIds(fixer, extensionManager).Contains(d.Id),
getFixes: dxs =>
{
if (fixAllForInSpan)
{
var primaryDiagnostic = dxs.First();
return GetCodeFixesAsync(document, primaryDiagnostic.Location.SourceSpan, fixer, isBlocking, ImmutableArray.Create(primaryDiagnostic), cancellationToken);
}
else
var fixerName = fixer.GetType().Name;
using (addOperationScope(fixerName))
using (RoslynEventSource.LogInformationalBlock(FunctionId.CodeFixes_GetCodeFixesAsync, fixerName, cancellationToken))
{
return GetCodeFixesAsync(document, span, fixer, isBlocking, dxs, cancellationToken);
if (fixAllForInSpan)
{
var primaryDiagnostic = dxs.First();
return GetCodeFixesAsync(document, primaryDiagnostic.Location.SourceSpan, fixer, isBlocking,
ImmutableArray.Create(primaryDiagnostic), uniqueDiagosticToEquivalenceKeysMap, cancellationToken);
}
else
{
return GetCodeFixesAsync(document, span, fixer, isBlocking, dxs, uniqueDiagosticToEquivalenceKeysMap, cancellationToken);
}
}
}
},
cancellationToken: cancellationToken).ConfigureAwait(false);
},
cancellationToken: cancellationToken).ConfigureAwait(false);

// Just need the first result if we are doing fix all in span
if (fixAllForInSpan && result.Any()) return;
// Just need the first result if we are doing fix all in span
if (fixAllForInSpan && result.Any()) return;
}
}
finally
{
foreach (var pooledSet in uniqueDiagosticToEquivalenceKeysMap.Values)
{
pooledSet.Free();
}
}
}

private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
Document document, TextSpan span, CodeFixProvider fixer, bool isBlocking,
ImmutableArray<Diagnostic> diagnostics, CancellationToken cancellationToken)
ImmutableArray<Diagnostic> diagnostics,
Dictionary<Diagnostic, PooledHashSet<string>> uniqueDiagosticToEquivalenceKeysMap,
CancellationToken cancellationToken)
{
using var fixesDisposer = ArrayBuilder<CodeFix>.GetInstance(out var fixes);
var context = new CodeFixContext(document, span, diagnostics,
Expand All @@ -355,7 +373,13 @@ private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
// Serialize access for thread safety - we don't know what thread the fix provider will call this delegate from.
lock (fixes)
{
fixes.Add(new CodeFix(document.Project, action, applicableDiagnostics));
// Filter out applicable diagnostics which already have a registered code action with same equivalence key.
applicableDiagnostics = FilterApplicableDiagnostics(applicableDiagnostics, action.EquivalenceKey, uniqueDiagosticToEquivalenceKeysMap);

if (!applicableDiagnostics.IsEmpty)
{
fixes.Add(new CodeFix(document.Project, action, applicableDiagnostics));
}
}
},
verifyArguments: false,
Expand All @@ -365,6 +389,34 @@ private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
var task = fixer.RegisterCodeFixesAsync(context) ?? Task.CompletedTask;
await task.ConfigureAwait(false);
return fixes.ToImmutable();

static ImmutableArray<Diagnostic> FilterApplicableDiagnostics(
ImmutableArray<Diagnostic> applicableDiagnostics,
string equivalenceKey,
Dictionary<Diagnostic, PooledHashSet<string>> uniqueDiagosticToEquivalenceKeysMap)
{
using var disposer = ArrayBuilder<Diagnostic>.GetInstance(out var newApplicableDiagnostics);
Copy link
Contributor

@sharwell sharwell Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using var disposer = ArrayBuilder<Diagnostic>.GetInstance(out var newApplicableDiagnostics);
using var _ = ArrayBuilder<Diagnostic>.GetInstance(out var newApplicableDiagnostics);

Copy link
Contributor Author

@mavasani mavasani Mar 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will do this as part of a follow-up change.

foreach (var diagnostic in applicableDiagnostics)
{
if (!uniqueDiagosticToEquivalenceKeysMap.TryGetValue(diagnostic, out var equivalenceKeys))
{
equivalenceKeys = PooledHashSet<string>.GetInstance();
equivalenceKeys.Add(equivalenceKey);
uniqueDiagosticToEquivalenceKeysMap[diagnostic] = equivalenceKeys;
}
else if (!equivalenceKeys.Add(equivalenceKey))
{
// Diagnostic already has a registered code action with same equivalence key.
continue;
}

newApplicableDiagnostics.Add(diagnostic);
}

return newApplicableDiagnostics.Count == applicableDiagnostics.Length
? applicableDiagnostics
: newApplicableDiagnostics.ToImmutable();
}
}

private async Task AppendConfigurationsAsync(
Expand Down
Loading