From 3a7832f9c45354401a67abedc8b7926caf95247e Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Tue, 18 Feb 2020 11:28:30 -0800 Subject: [PATCH 1/2] Add code fix service unit test for duplicate NuGet and VSIX code fix provider --- .../Test/CodeFixes/CodeFixServiceTests.cs | 236 +++++++++++++++++- 1 file changed, 226 insertions(+), 10 deletions(-) diff --git a/src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs b/src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs index ec26ed861c737..459aa8f38ebb8 100644 --- a/src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs +++ b/src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs @@ -7,9 +7,11 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Composition; using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeActions; using Microsoft.CodeAnalysis.CodeFixes; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Editor.Shared.Utilities; @@ -17,6 +19,7 @@ using Microsoft.CodeAnalysis.ErrorLogger; using Microsoft.CodeAnalysis.Extensions; using Microsoft.CodeAnalysis.Host.Mef; +using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.SolutionCrawler; using Microsoft.CodeAnalysis.Test.Utilities; using Microsoft.CodeAnalysis.Text; @@ -56,7 +59,7 @@ public async Task TestGetFirstDiagnosticWithFixAsync() var unused = await fixService.GetMostSevereFixableDiagnosticAsync(document, TextSpan.FromBounds(0, 0), cancellationToken: CancellationToken.None); var fixer1 = (MockFixer)fixers.Single().Value; - var fixer2 = (MockFixer)reference.Fixer; + var fixer2 = (MockFixer)reference.Fixer!; // check to make sure both of them are called. Assert.True(fixer1.Called); @@ -222,13 +225,13 @@ public sealed override Task RegisterCodeFixesAsync(CodeFixContext context) private class MockAnalyzerReference : AnalyzerReference, ICodeFixProviderFactory { - public readonly CodeFixProvider Fixer; + public readonly CodeFixProvider? Fixer; public readonly ImmutableArray Analyzers; private static readonly CodeFixProvider s_defaultFixer = new MockFixer(); private static readonly ImmutableArray s_defaultAnalyzers = ImmutableArray.Create(new MockDiagnosticAnalyzer()); - public MockAnalyzerReference(CodeFixProvider fixer, ImmutableArray analyzers) + public MockAnalyzerReference(CodeFixProvider? fixer, ImmutableArray analyzers) { Fixer = fixer; Analyzers = analyzers; @@ -239,7 +242,7 @@ public MockAnalyzerReference() { } - public MockAnalyzerReference(CodeFixProvider fixer) + public MockAnalyzerReference(CodeFixProvider? fixer) : this(fixer, s_defaultAnalyzers) { } @@ -280,26 +283,43 @@ public override ImmutableArray GetAnalyzersForAllLanguages() public ImmutableArray GetFixers() { - return ImmutableArray.Create(Fixer); + return Fixer != null ? ImmutableArray.Create(Fixer) : ImmutableArray.Empty; } public class MockDiagnosticAnalyzer : DiagnosticAnalyzer { - private readonly DiagnosticDescriptor _descriptor = new DiagnosticDescriptor(MockFixer.Id, "MockDiagnostic", "MockDiagnostic", "InternalCategory", DiagnosticSeverity.Warning, isEnabledByDefault: true); + public MockDiagnosticAnalyzer(ImmutableArray reportedDiagnosticIds) + { + SupportedDiagnostics = CreateSupportedDiagnostics(reportedDiagnosticIds); + } + + public MockDiagnosticAnalyzer() + : this(ImmutableArray.Create(MockFixer.Id)) + { + } - public override ImmutableArray SupportedDiagnostics + private static ImmutableArray CreateSupportedDiagnostics(ImmutableArray reportedDiagnosticIds) { - get + var builder = ArrayBuilder.GetInstance(); + foreach (var diagnosticId in reportedDiagnosticIds) { - return ImmutableArray.Create(_descriptor); + var descriptor = new DiagnosticDescriptor(diagnosticId, "MockDiagnostic", "MockDiagnostic", "InternalCategory", DiagnosticSeverity.Warning, isEnabledByDefault: true); + builder.Add(descriptor); } + + return builder.ToImmutableAndFree(); } + public override ImmutableArray SupportedDiagnostics { get; } + public override void Initialize(AnalysisContext context) { context.RegisterSyntaxTreeAction(c => { - c.ReportDiagnostic(Diagnostic.Create(_descriptor, c.Tree.GetLocation(TextSpan.FromBounds(0, 0)))); + foreach (var descriptor in SupportedDiagnostics) + { + c.ReportDiagnostic(Diagnostic.Create(descriptor, c.Tree.GetLocation(TextSpan.FromBounds(0, 0)))); + } }); } } @@ -319,5 +339,201 @@ private static string ToLogFormat(Exception exception) return exception.Message + Environment.NewLine + exception.StackTrace; } } + + [Fact, WorkItem(18818, "https://github.com/dotnet/roslyn/issues/18818")] + public async Task TestNuGetAndVsixCodeFixersAsync() + { + // No NuGet or VSIX code fix provider + // Verify no code action registered + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: null, + expectedNuGetFixerCodeActionWasRegistered: false, + vsixFixer: null, + expectedVsixFixerCodeActionWasRegistered: false); + + // Only NuGet code fix provider + // Verify only NuGet fixer's code action registered + var fixableDiagnosticIds = ImmutableArray.Create(MockFixer.Id); + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: new NuGetCodeFixProvider(fixableDiagnosticIds), + expectedNuGetFixerCodeActionWasRegistered: true, + vsixFixer: null, + expectedVsixFixerCodeActionWasRegistered: false); + + // Only Vsix code fix provider + // Verify only Vsix fixer's code action registered + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: null, + expectedNuGetFixerCodeActionWasRegistered: false, + vsixFixer: new VsixCodeFixProvider(fixableDiagnosticIds), + expectedVsixFixerCodeActionWasRegistered: true); + + // Both NuGet and Vsix code fix provider + // Verify only NuGet fixer's code action registered + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: new NuGetCodeFixProvider(fixableDiagnosticIds), + expectedNuGetFixerCodeActionWasRegistered: true, + vsixFixer: new VsixCodeFixProvider(fixableDiagnosticIds), + expectedVsixFixerCodeActionWasRegistered: false); + } + + private async Task TestNuGetAndVsixCodeFixersCoreAsync( + NuGetCodeFixProvider? nugetFixer, + bool expectedNuGetFixerCodeActionWasRegistered, + VsixCodeFixProvider? vsixFixer, + bool expectedVsixFixerCodeActionWasRegistered, + MockAnalyzerReference.MockDiagnosticAnalyzer? diagnosticAnalyzer = null) + { + var fixes = await GetNuGetAndVsixCodeFixersCoreAsync(nugetFixer, vsixFixer, diagnosticAnalyzer); + + var fixTitles = fixes.SelectMany(fixCollection => fixCollection.Fixes).Select(f => f.Action.Title).ToHashSet(); + Assert.Equal(expectedNuGetFixerCodeActionWasRegistered, fixTitles.Contains(nameof(NuGetCodeFixProvider))); + Assert.Equal(expectedVsixFixerCodeActionWasRegistered, fixTitles.Contains(nameof(VsixCodeFixProvider))); + } + + [Fact, WorkItem(18818, "https://github.com/dotnet/roslyn/issues/18818")] + public async Task TestNuGetAndVsixCodeFixersWithMultipleFixableDiagnosticIdsAsync() + { + const string id1 = "ID1"; + const string id2 = "ID2"; + var reportedDiagnosticIds = ImmutableArray.Create(id1, id2); + var diagnosticAnalyzer = new MockAnalyzerReference.MockDiagnosticAnalyzer(reportedDiagnosticIds); + + // Only NuGet code fix provider which fixes both reported diagnostic IDs. + // Verify only NuGet fixer's code actions registered and they fix all IDs. + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: new NuGetCodeFixProvider(reportedDiagnosticIds), + expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer: reportedDiagnosticIds, + vsixFixer: null, + expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer: ImmutableArray.Empty, + diagnosticAnalyzer); + + // Only Vsix code fix provider which fixes both reported diagnostic IDs. + // Verify only Vsix fixer's code action registered and they fix all IDs. + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: null, + expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer: ImmutableArray.Empty, + vsixFixer: new VsixCodeFixProvider(reportedDiagnosticIds), + expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer: reportedDiagnosticIds, + diagnosticAnalyzer); + + // Both NuGet and Vsix code fix provider register same fixable IDs. + // Verify only NuGet fixer's code actions registered. + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: new NuGetCodeFixProvider(reportedDiagnosticIds), + expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer: reportedDiagnosticIds, + vsixFixer: new VsixCodeFixProvider(reportedDiagnosticIds), + expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer: ImmutableArray.Empty, + diagnosticAnalyzer); + + // Both NuGet and Vsix code fix provider register different fixable IDs. + // Verify both NuGet and Vsix fixer's code actions registered. + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: new NuGetCodeFixProvider(ImmutableArray.Create(id1)), + expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer: ImmutableArray.Create(id1), + vsixFixer: new VsixCodeFixProvider(ImmutableArray.Create(id2)), + expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer: ImmutableArray.Create(id2), + diagnosticAnalyzer); + + // NuGet code fix provider registers subset of Vsix code fix provider fixable IDs. + // Verify both NuGet and Vsix fixer's code actions registered, + // there are no duplicates and NuGet ones are preferred for duplicates. + await TestNuGetAndVsixCodeFixersCoreAsync( + nugetFixer: new NuGetCodeFixProvider(ImmutableArray.Create(id1)), + expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer: ImmutableArray.Create(id1), + vsixFixer: new VsixCodeFixProvider(reportedDiagnosticIds), + expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer: ImmutableArray.Create(id2), + diagnosticAnalyzer); + } + + private async Task TestNuGetAndVsixCodeFixersCoreAsync( + NuGetCodeFixProvider? nugetFixer, + ImmutableArray expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer, + VsixCodeFixProvider? vsixFixer, + ImmutableArray expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer, + MockAnalyzerReference.MockDiagnosticAnalyzer diagnosticAnalyzer) + { + var fixes = (await GetNuGetAndVsixCodeFixersCoreAsync(nugetFixer, vsixFixer, diagnosticAnalyzer)) + .SelectMany(fixCollection => fixCollection.Fixes); + + var nugetFixerRegisteredActions = fixes.Where(f => f.Action.Title == nameof(NuGetCodeFixProvider)); + var actualDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer = nugetFixerRegisteredActions.SelectMany(a => a.Diagnostics).Select(d => d.Id); + Assert.True(actualDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer.SetEquals(expectedDiagnosticIdsWithRegisteredCodeActionsByNuGetFixer)); + + var vsixFixerRegisteredActions = fixes.Where(f => f.Action.Title == nameof(VsixCodeFixProvider)); + var actualDiagnosticIdsWithRegisteredCodeActionsByVsixFixer = vsixFixerRegisteredActions.SelectMany(a => a.Diagnostics).Select(d => d.Id); + Assert.True(actualDiagnosticIdsWithRegisteredCodeActionsByVsixFixer.SetEquals(expectedDiagnosticIdsWithRegisteredCodeActionsByVsixFixer)); + } + + private async Task> GetNuGetAndVsixCodeFixersCoreAsync( + NuGetCodeFixProvider? nugetFixer, + VsixCodeFixProvider? vsixFixer, + MockAnalyzerReference.MockDiagnosticAnalyzer? diagnosticAnalyzer = null) + { + var code = @"class C { }"; + var diagnosticService = new TestDiagnosticAnalyzerService(DiagnosticExtensions.GetCompilerDiagnosticAnalyzersMap()); + + var vsixFixers = vsixFixer != null + ? SpecializedCollections.SingletonEnumerable(new Lazy(() => vsixFixer, new CodeChangeProviderMetadata(name: nameof(VsixCodeFixProvider), languages: LanguageNames.CSharp))) + : SpecializedCollections.EmptyEnumerable>(); + + using var workspace = TestWorkspace.CreateCSharp(code, openDocuments: true); + + var logger = SpecializedCollections.SingletonEnumerable(new Lazy(() => workspace.Services.GetRequiredService())); + var fixService = new CodeFixService( + workspace.ExportProvider.GetExportedValue(), + diagnosticService, logger, vsixFixers, SpecializedCollections.EmptyEnumerable>()); + + var incrementalAnalyzer = (IIncrementalAnalyzerProvider)diagnosticService; + + // register diagnostic engine to solution crawler + var analyzer = incrementalAnalyzer.CreateIncrementalAnalyzer(workspace); + + diagnosticAnalyzer ??= new MockAnalyzerReference.MockDiagnosticAnalyzer(); + var analyzers = ImmutableArray.Create(diagnosticAnalyzer); + var reference = new MockAnalyzerReference(nugetFixer, analyzers); + var project = workspace.CurrentSolution.Projects.Single().AddAnalyzerReference(reference); + + var document = project.Documents.Single(); + return await fixService.GetFixesAsync(document, TextSpan.FromBounds(0, 0), includeConfigurationFixes: false, cancellationToken: CancellationToken.None); + } + + [ExportCodeFixProvider(LanguageNames.CSharp), Shared] + private sealed class NuGetCodeFixProvider : AbstractNuGetOrVsixCodeFixProvider + { + public NuGetCodeFixProvider(ImmutableArray fixableDiagnsoticIds) + : base(fixableDiagnsoticIds, nameof(NuGetCodeFixProvider)) + { + } + } + + [ExportCodeFixProvider(LanguageNames.CSharp), Shared] + private sealed class VsixCodeFixProvider : AbstractNuGetOrVsixCodeFixProvider + { + public VsixCodeFixProvider(ImmutableArray fixableDiagnsoticIds) + : base(fixableDiagnsoticIds, nameof(VsixCodeFixProvider)) + { + } + } + + private abstract class AbstractNuGetOrVsixCodeFixProvider : CodeFixProvider + { + private readonly string _name; + + protected AbstractNuGetOrVsixCodeFixProvider(ImmutableArray fixableDiagnsoticIds, string name) + { + FixableDiagnosticIds = fixableDiagnsoticIds; + _name = name; + } + + public override ImmutableArray FixableDiagnosticIds { get; } + + public override Task RegisterCodeFixesAsync(CodeFixContext context) + { + var fixableDiagnostics = context.Diagnostics.WhereAsArray(d => FixableDiagnosticIds.Contains(d.Id)); + context.RegisterCodeFix(CodeAction.Create(_name, ct => Task.FromResult(context.Document)), fixableDiagnostics); + return Task.CompletedTask; + } + } } } From e38d3a1eeb94d60172c4209a6883e04723d6ed9e Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Tue, 18 Feb 2020 16:52:35 -0800 Subject: [PATCH 2/2] Implement filtering logic for duplicate/overlapping code actions from Nuget and VSIX. Implements the code fix de-duping design from #18818 --- .../CodeFixes/CodeFixService.cs | 118 +++++++++++++----- 1 file changed, 85 insertions(+), 33 deletions(-) diff --git a/src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs b/src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs index f9b4f7f7c5d96..1cbc7972dcaeb 100644 --- a/src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs +++ b/src/EditorFeatures/Core/Implementation/CodeFixes/CodeFixService.cs @@ -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 priorityMap = _fixerPriorityMap[document.Project.Language].Value; + ImmutableDictionary priorityMap = fixersForLanguage.Value; result.Sort((d1, d2) => GetValue(d1).CompareTo(GetValue(d2))); int GetValue(CodeFixCollection c) @@ -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) @@ -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(); - // 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>(); + 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> GetCodeFixesAsync( Document document, TextSpan span, CodeFixProvider fixer, bool isBlocking, - ImmutableArray diagnostics, CancellationToken cancellationToken) + ImmutableArray diagnostics, + Dictionary> uniqueDiagosticToEquivalenceKeysMap, + CancellationToken cancellationToken) { using var fixesDisposer = ArrayBuilder.GetInstance(out var fixes); var context = new CodeFixContext(document, span, diagnostics, @@ -355,7 +373,13 @@ private async Task> 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, @@ -365,6 +389,34 @@ private async Task> GetCodeFixesAsync( var task = fixer.RegisterCodeFixesAsync(context) ?? Task.CompletedTask; await task.ConfigureAwait(false); return fixes.ToImmutable(); + + static ImmutableArray FilterApplicableDiagnostics( + ImmutableArray applicableDiagnostics, + string equivalenceKey, + Dictionary> uniqueDiagosticToEquivalenceKeysMap) + { + using var disposer = ArrayBuilder.GetInstance(out var newApplicableDiagnostics); + foreach (var diagnostic in applicableDiagnostics) + { + if (!uniqueDiagosticToEquivalenceKeysMap.TryGetValue(diagnostic, out var equivalenceKeys)) + { + equivalenceKeys = PooledHashSet.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(