From 0c46ddf7cc9f1676642b1ddf8389ce7636bf80ad Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 9 May 2025 13:10:56 -0700 Subject: [PATCH 1/4] Collect data about which code fixes end up making changes that conflict with other code fixes --- .../Portable/Copilot/CopilotChangeAnalysis.cs | 4 +- .../Copilot/CopilotChangeAnalysisUtilities.cs | 1 + .../Copilot/ICopilotChangeAnalysisService.cs | 52 ++++++++++++++----- .../Core/Collections/IntervalTreeHelpers.cs | 10 ++-- .../Core/Collections/MutableIntervalTree`1.cs | 8 ++- .../SimpleMutableIntervalTree`2.cs | 4 +- 6 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/Features/Core/Portable/Copilot/CopilotChangeAnalysis.cs b/src/Features/Core/Portable/Copilot/CopilotChangeAnalysis.cs index 4ac58c7f4bd79..8df5f9a04b589 100644 --- a/src/Features/Core/Portable/Copilot/CopilotChangeAnalysis.cs +++ b/src/Features/Core/Portable/Copilot/CopilotChangeAnalysis.cs @@ -46,6 +46,7 @@ internal readonly record struct CopilotDiagnosticAnalysis( /// Mapping from diagnostic id to the total time taken to fix diagnostics with that id. /// Mapping from diagnostic id to the name of the provider that provided the fix. /// Mapping from provider name to the total time taken to fix diagnostics with that provider. +/// Mapping from provider name to whether or not that provider conflicted with another provider in producing a fix. [DataContract] internal readonly record struct CopilotCodeFixAnalysis( [property: DataMember(Order = 0)] TimeSpan TotalComputationTime, @@ -53,4 +54,5 @@ internal readonly record struct CopilotCodeFixAnalysis( [property: DataMember(Order = 2)] Dictionary DiagnosticIdToCount, [property: DataMember(Order = 3)] Dictionary DiagnosticIdToApplicationTime, [property: DataMember(Order = 4)] Dictionary> DiagnosticIdToProviderName, - [property: DataMember(Order = 5)] Dictionary ProviderNameToApplicationTime); + [property: DataMember(Order = 5)] Dictionary ProviderNameToApplicationTime, + [property: DataMember(Order = 6)] Dictionary ProviderNameToHasConflict); diff --git a/src/Features/Core/Portable/Copilot/CopilotChangeAnalysisUtilities.cs b/src/Features/Core/Portable/Copilot/CopilotChangeAnalysisUtilities.cs index d239a642fd393..bddb3dd6c747e 100644 --- a/src/Features/Core/Portable/Copilot/CopilotChangeAnalysisUtilities.cs +++ b/src/Features/Core/Portable/Copilot/CopilotChangeAnalysisUtilities.cs @@ -49,6 +49,7 @@ public static IDisposable LogCopilotChangeAnalysis( d["CodeFixAnalysis_DiagnosticIdToApplicationTime"] = StringifyDictionary(analysisResult.CodeFixAnalysis.DiagnosticIdToApplicationTime); d["CodeFixAnalysis_DiagnosticIdToProviderName"] = StringifyDictionary(analysisResult.CodeFixAnalysis.DiagnosticIdToProviderName); d["CodeFixAnalysis_ProviderNameToApplicationTime"] = StringifyDictionary(analysisResult.CodeFixAnalysis.ProviderNameToApplicationTime); + d["CodeFixAnalysis_ProviderNameToHasConflict"] = StringifyDictionary(analysisResult.CodeFixAnalysis.ProviderNameToHasConflict); }, args: (featureId, accepted, proposalId, analysisResult)), cancellationToken); } diff --git a/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs b/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs index 31348615b97a6..c084324092101 100644 --- a/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs +++ b/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs @@ -14,7 +14,6 @@ using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Host; using Microsoft.CodeAnalysis.Host.Mef; -using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.PooledObjects; using Microsoft.CodeAnalysis.Remote; using Microsoft.CodeAnalysis.Shared; @@ -43,6 +42,7 @@ Task AnalyzeChangeAsync( [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] internal sealed class DefaultCopilotChangeAnalysisService( ICodeFixService codeFixService) : ICopilotChangeAnalysisService + { private const string RoslynPrefix = "Microsoft.CodeAnalysis."; @@ -255,13 +255,14 @@ private async Task ComputeCodeFixAnalysisAsync( var diagnosticIdToApplicationTime = new Dictionary(); var diagnosticIdToProviderName = new Dictionary>(); var providerNameToApplicationTime = new Dictionary(); + var providerNameToHasConflict = new Dictionary(); var totalApplicationTimeStopWatch = SharedStopwatch.StartNew(); await ProducerConsumer<(CodeFixCollection collection, TimeSpan elapsedTime)>.RunParallelAsync( codeFixCollections, produceItems: static async (codeFixCollection, callback, args, cancellationToken) => { - var (@this, solution, _, _, _, _) = args; + var (@this, solution, _, _, _, _, _) = args; var firstAction = GetFirstAction(codeFixCollection.Fixes[0]); var applicationTimeStopWatch = SharedStopwatch.StartNew(); @@ -270,19 +271,41 @@ private async Task ComputeCodeFixAnalysisAsync( }, consumeItems: static async (values, args, cancellationToken) => { - var (@this, solution, diagnosticIdToCount, diagnosticIdToApplicationTime, diagnosticIdToProviderName, providerNameToApplicationTime) = args; + var (@this, solution, diagnosticIdToCount, diagnosticIdToApplicationTime, diagnosticIdToProviderName, providerNameToApplicationTime, providerNameToHasConflict) = args; + + var intervalTree = new SimpleMutableIntervalTree(new CodeFixCollectionIntervalIntrospector()); + await foreach (var (codeFixCollection, applicationTime) in values) { var diagnosticId = codeFixCollection.FirstDiagnostic.Id; - var providerName = codeFixCollection.Provider.GetType().FullName![RoslynPrefix.Length..]; + var providerName = GetProviderName(codeFixCollection); IncrementCount(diagnosticIdToCount, diagnosticId); IncrementElapsedTime(diagnosticIdToApplicationTime, diagnosticId, applicationTime); diagnosticIdToProviderName.MultiAdd(diagnosticId, providerName); IncrementElapsedTime(providerNameToApplicationTime, providerName, applicationTime); + + intervalTree.AddIntervalInPlace(codeFixCollection); + } + + using var intersectingCollections = TemporaryArray.Empty; + foreach (var codeFixCollection in intervalTree) + { + intersectingCollections.Clear(); + intervalTree.FillWithIntervalsThatIntersectWith( + codeFixCollection.TextSpan.Start, + codeFixCollection.TextSpan.Length, + ref intersectingCollections.AsRef()); + + var providerName = GetProviderName(codeFixCollection); + + var newHasConflictValue = intersectingCollections.Count > 0; + var storedHasConflictValue = providerNameToHasConflict.TryGetValue(providerName, out var currentHasConflictValue) && currentHasConflictValue; + + providerNameToHasConflict[providerName] = storedHasConflictValue || newHasConflictValue; } }, - args: (@this: this, newDocument.Project.Solution, diagnosticIdToCount, diagnosticIdToApplicationTime, diagnosticIdToProviderName, providerNameToApplicationTime), + args: (@this: this, newDocument.Project.Solution, diagnosticIdToCount, diagnosticIdToApplicationTime, diagnosticIdToProviderName, providerNameToApplicationTime, providerNameToHasConflict), cancellationToken).ConfigureAwait(false); var totalApplicationTime = totalApplicationTimeStopWatch.Elapsed; @@ -292,7 +315,8 @@ private async Task ComputeCodeFixAnalysisAsync( diagnosticIdToCount, diagnosticIdToApplicationTime, diagnosticIdToProviderName, - providerNameToApplicationTime); + providerNameToApplicationTime, + providerNameToHasConflict); Task> ComputeCodeFixCollectionsAsync() { @@ -300,8 +324,6 @@ Task> ComputeCodeFixCollectionsAsync() newSpans, static async (span, callback, args, cancellationToken) => { - var intervalTree = new TextSpanMutableIntervalTree(); - var (@this, newDocument) = args; await foreach (var codeFixCollection in @this._codeFixService.StreamFixesAsync( newDocument, span, cancellationToken).ConfigureAwait(false)) @@ -316,11 +338,6 @@ static async (span, callback, args, cancellationToken) => IsVisibleDiagnostic(codeFix.PrimaryDiagnostic.IsSuppressed, codeFix.PrimaryDiagnostic.Severity) && (codeFixCollection.Provider.GetType().Namespace ?? "").StartsWith(RoslynPrefix)) { - // The first for a particular span is the one we would apply. Ignore others that fix the same span. - if (intervalTree.HasIntervalThatOverlapsWith(codeFixCollection.TextSpan)) - continue; - - intervalTree.AddIntervalInPlace(codeFixCollection.TextSpan); callback(codeFixCollection); } } @@ -328,5 +345,14 @@ static async (span, callback, args, cancellationToken) => args: (@this: this, newDocument), cancellationToken); } + + static string GetProviderName(CodeFixCollection codeFixCollection) + => codeFixCollection.Provider.GetType().FullName![RoslynPrefix.Length..]; + } + + private readonly struct CodeFixCollectionIntervalIntrospector : IIntervalIntrospector + { + public TextSpan GetSpan(CodeFixCollection value) + => value.TextSpan; } } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTreeHelpers.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTreeHelpers.cs index dcc6d66424f90..b6ede2a21c306 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTreeHelpers.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTreeHelpers.cs @@ -19,12 +19,12 @@ namespace Microsoft.CodeAnalysis.Shared.Collections; internal interface IIntervalTreeWitness where TIntervalTree : IIntervalTree { - public bool TryGetRoot(TIntervalTree tree, [NotNullWhen(true)] out TNode? root); - public bool TryGetLeftNode(TIntervalTree tree, TNode node, [NotNullWhen(true)] out TNode? leftNode); - public bool TryGetRightNode(TIntervalTree tree, TNode node, [NotNullWhen(true)] out TNode? rightNode); + bool TryGetRoot(TIntervalTree tree, [NotNullWhen(true)] out TNode? root); + bool TryGetLeftNode(TIntervalTree tree, TNode node, [NotNullWhen(true)] out TNode? leftNode); + bool TryGetRightNode(TIntervalTree tree, TNode node, [NotNullWhen(true)] out TNode? rightNode); - public T GetValue(TIntervalTree tree, TNode node); - public TNode GetMaxEndNode(TIntervalTree tree, TNode node); + T GetValue(TIntervalTree tree, TNode node); + TNode GetMaxEndNode(TIntervalTree tree, TNode node); } /// diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs index b209ea59ee183..c7051137ae101 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs @@ -23,13 +23,19 @@ internal partial class MutableIntervalTree : IIntervalTree protected Node? root; + //public void Add(in TIntrospector introspector, T value) + // where TIntrospector : struct, IIntervalIntrospector + //{ + // this.root = Insert(this.root, new Node(value), in introspector); + //} + public static MutableIntervalTree Create(in TIntrospector introspector, IEnumerable values) where TIntrospector : struct, IIntervalIntrospector { var result = new MutableIntervalTree(); foreach (var value in values) - result.root = Insert(result.root, new Node(value), in introspector); + result.Add(introspector, value); return result; } diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/SimpleMutableIntervalTree`2.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/SimpleMutableIntervalTree`2.cs index fe1fd19c494f8..c42a6b2646820 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/SimpleMutableIntervalTree`2.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/SimpleMutableIntervalTree`2.cs @@ -12,16 +12,14 @@ internal class SimpleMutableIntervalTree : MutableIntervalTree { private readonly TIntrospector _introspector; - public SimpleMutableIntervalTree(in TIntrospector introspector, IEnumerable? values) + public SimpleMutableIntervalTree(in TIntrospector introspector, IEnumerable? values = null) { _introspector = introspector; if (values != null) { foreach (var value in values) - { root = Insert(root, new Node(value), introspector); - } } } From 159b10ada3fc610fc37194a9073e5d19a807a8e9 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 9 May 2025 13:22:49 -0700 Subject: [PATCH 2/4] docs --- .../Portable/Copilot/ICopilotChangeAnalysisService.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs b/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs index c084324092101..34812113a0dc2 100644 --- a/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs +++ b/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs @@ -273,6 +273,10 @@ private async Task ComputeCodeFixAnalysisAsync( { var (@this, solution, diagnosticIdToCount, diagnosticIdToApplicationTime, diagnosticIdToProviderName, providerNameToApplicationTime, providerNameToHasConflict) = args; + // Track which text span each code fix says it will be fixing. We can use this to efficiently determine + // which codefixes 'conflict' with some other codefix (in that that multiple features think they can fix + // the same span of code). We would need some mechanism to determine which we would prefer to take in + // order to have a good experience in such a case. var intervalTree = new SimpleMutableIntervalTree(new CodeFixCollectionIntervalIntrospector()); await foreach (var (codeFixCollection, applicationTime) in values) @@ -288,6 +292,7 @@ private async Task ComputeCodeFixAnalysisAsync( intervalTree.AddIntervalInPlace(codeFixCollection); } + // Now go over the fixed spans and see which intersect with other spans using var intersectingCollections = TemporaryArray.Empty; foreach (var codeFixCollection in intervalTree) { @@ -299,7 +304,9 @@ private async Task ComputeCodeFixAnalysisAsync( var providerName = GetProviderName(codeFixCollection); - var newHasConflictValue = intersectingCollections.Count > 0; + // >= 2 because we want to see how many total fixers fix a particular span, and we only care if + // we're seeing multiple. + var newHasConflictValue = intersectingCollections.Count >= 2; var storedHasConflictValue = providerNameToHasConflict.TryGetValue(providerName, out var currentHasConflictValue) && currentHasConflictValue; providerNameToHasConflict[providerName] = storedHasConflictValue || newHasConflictValue; From 04c73611d30b0ae23c81319df208d9b88b29bcfe Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 9 May 2025 13:25:37 -0700 Subject: [PATCH 3/4] Revert --- .../Compiler/Core/Collections/MutableIntervalTree`1.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs index c7051137ae101..b209ea59ee183 100644 --- a/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs +++ b/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/MutableIntervalTree`1.cs @@ -23,19 +23,13 @@ internal partial class MutableIntervalTree : IIntervalTree protected Node? root; - //public void Add(in TIntrospector introspector, T value) - // where TIntrospector : struct, IIntervalIntrospector - //{ - // this.root = Insert(this.root, new Node(value), in introspector); - //} - public static MutableIntervalTree Create(in TIntrospector introspector, IEnumerable values) where TIntrospector : struct, IIntervalIntrospector { var result = new MutableIntervalTree(); foreach (var value in values) - result.Add(introspector, value); + result.root = Insert(result.root, new Node(value), in introspector); return result; } From 5e4eb708056432dd34cf920eb04463228106f956 Mon Sep 17 00:00:00 2001 From: Cyrus Najmabadi Date: Fri, 9 May 2025 13:55:18 -0700 Subject: [PATCH 4/4] lint --- .../Core/Portable/Copilot/ICopilotChangeAnalysisService.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs b/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs index 34812113a0dc2..f5019cf430543 100644 --- a/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs +++ b/src/Features/Core/Portable/Copilot/ICopilotChangeAnalysisService.cs @@ -42,7 +42,6 @@ Task AnalyzeChangeAsync( [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] internal sealed class DefaultCopilotChangeAnalysisService( ICodeFixService codeFixService) : ICopilotChangeAnalysisService - { private const string RoslynPrefix = "Microsoft.CodeAnalysis.";