Merge pull request #14667 from CyrusNajmabadi/suggestedActionCleanup2

Report better telemetry for fix-all code actions.

Fixes: #13911

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

@@ -220,34 +220,37 @@ public async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(Document docu
             {
                 cancellationToken.ThrowIfCancellationRequested();
 
-                Func<Diagnostic, bool> hasFix = (d) => this.GetFixableDiagnosticIds(fixer, extensionManager).Contains(d.Id);
-                Func<ImmutableArray<Diagnostic>, Task<ImmutableArray<CodeFix>>> getFixes =
-                    async (dxs) =>
-                    {
-                        var fixes = ArrayBuilder<CodeFix>.GetInstance();
-                        var context = new CodeFixContext(document, span, dxs,
-                            // TODO: Can we share code between similar lambdas that we pass to this API in BatchFixAllProvider.cs, CodeFixService.cs and CodeRefactoringService.cs?
-                            (action, applicableDiagnostics) =>
-                            {
-                                // 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));
-                                }
-                            },
-                            verifyArguments: false,
-                            cancellationToken: cancellationToken);
-
-                        var task = fixer.RegisterCodeFixesAsync(context) ?? SpecializedTasks.EmptyTask;
-                        await task.ConfigureAwait(false);
-                        return fixes.ToImmutableAndFree();
-                    };
-
-                await AppendFixesOrSuppressionsAsync(document, span, diagnostics, result, fixer,
-                    hasFix, getFixes, cancellationToken).ConfigureAwait(false);
+                await AppendFixesOrSuppressionsAsync(
+                    document, span, diagnostics, result, fixer,
+                    hasFix: d => this.GetFixableDiagnosticIds(fixer, extensionManager).Contains(d.Id),
+                    getFixes: dxs => GetCodeFixesAsync(document, span, fixer, dxs, cancellationToken),
+                    cancellationToken: cancellationToken).ConfigureAwait(false);
             }
         }
 
+        private async Task<ImmutableArray<CodeFix>> GetCodeFixesAsync(
+            Document document, TextSpan span, CodeFixProvider fixer,
+            ImmutableArray<Diagnostic> diagnostics, CancellationToken cancellationToken)
+        {
+            var fixes = ArrayBuilder<CodeFix>.GetInstance();
+            var context = new CodeFixContext(document, span, diagnostics,
+                // TODO: Can we share code between similar lambdas that we pass to this API in BatchFixAllProvider.cs, CodeFixService.cs and CodeRefactoringService.cs?
+                (action, applicableDiagnostics) =>
+                {
+                    // 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));
+                    }
+                },
+                verifyArguments: false,
+                cancellationToken: cancellationToken);
+
+            var task = fixer.RegisterCodeFixesAsync(context) ?? SpecializedTasks.EmptyTask;
+            await task.ConfigureAwait(false);
+            return fixes.ToImmutableAndFree();
+        }
+
         private async Task AppendSuppressionsAsync(
             Document document, TextSpan span, IEnumerable<DiagnosticData> diagnostics, 
             ArrayBuilder<CodeFixCollection> result, CancellationToken cancellationToken)
@@ -258,13 +261,12 @@ public async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(Document docu
                 return;
             }
 
-            Func<Diagnostic, bool> hasFix = (d) => lazySuppressionProvider.Value.CanBeSuppressedOrUnsuppressed(d);
-            Func<ImmutableArray<Diagnostic>, Task<ImmutableArray<CodeFix>>> getFixes = 
-                dxs => lazySuppressionProvider.Value.GetSuppressionsAsync(
-                    document, span, dxs, cancellationToken);
             await AppendFixesOrSuppressionsAsync(
                 document, span, diagnostics, result, lazySuppressionProvider.Value, 
-                hasFix, getFixes, cancellationToken).ConfigureAwait(false);
+                hasFix: d => lazySuppressionProvider.Value.CanBeSuppressedOrUnsuppressed(d),
+                getFixes: dxs => lazySuppressionProvider.Value.GetSuppressionsAsync(
+                    document, span, dxs, cancellationToken), 
+                cancellationToken: cancellationToken).ConfigureAwait(false);
         }
 
         private async Task AppendFixesOrSuppressionsAsync(
@@ -277,7 +279,10 @@ public async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(Document docu
             Func<ImmutableArray<Diagnostic>, Task<ImmutableArray<CodeFix>>> getFixes,
             CancellationToken cancellationToken)
         {
-            var diagnostics = (await diagnosticsWithSameSpan.OrderByDescending(d => d.Severity).ToDiagnosticsAsync(document.Project, cancellationToken).ConfigureAwait(false)).Where(d => hasFix(d)).ToImmutableArray();
+            var allDiagnostics = 
+                await diagnosticsWithSameSpan.OrderByDescending(d => d.Severity)
+                                             .ToDiagnosticsAsync(document.Project, cancellationToken).ConfigureAwait(false);
+            var diagnostics = allDiagnostics.WhereAsArray(hasFix);
             if (diagnostics.Length <= 0)
             {
                 // this can happen for suppression case where all diagnostics can't be suppressed
@@ -289,28 +294,30 @@ public async Task<ImmutableArray<CodeFixCollection>> GetFixesAsync(Document docu
                 () => getFixes(diagnostics),
                 defaultValue: ImmutableArray<CodeFix>.Empty).ConfigureAwait(false);
 
-            if (fixes != null && fixes.Any())
+            if (fixes.IsDefaultOrEmpty)
             {
-                // If the fix provider supports fix all occurrences, then get the corresponding FixAllProviderInfo and fix all context.
-                var fixAllProviderInfo = extensionManager.PerformFunction(fixer, () => ImmutableInterlocked.GetOrAdd(ref _fixAllProviderMap, fixer, FixAllProviderInfo.Create), defaultValue: null);
+                return;
+            }
 
-                FixAllState fixAllState = null;
-                IEnumerable<FixAllScope> supportedScopes = null;
-                if (fixAllProviderInfo != null)
-                {
-                    var codeFixProvider = (fixer as CodeFixProvider) ?? new WrapperCodeFixProvider((ISuppressionFixProvider)fixer, diagnostics.Select(d => d.Id));
-                    fixAllState = FixAllState.Create(
-                        fixAllProviderInfo.FixAllProvider,
-                        document, fixAllProviderInfo, codeFixProvider, diagnostics,
-                        this.GetDocumentDiagnosticsAsync, this.GetProjectDiagnosticsAsync);
-                    supportedScopes = fixAllProviderInfo.SupportedScopes;
-                }
+            // If the fix provider supports fix all occurrences, then get the corresponding FixAllProviderInfo and fix all context.
+            var fixAllProviderInfo = extensionManager.PerformFunction(fixer, () => ImmutableInterlocked.GetOrAdd(ref _fixAllProviderMap, fixer, FixAllProviderInfo.Create), defaultValue: null);
 
-                var codeFix = new CodeFixCollection(
-                    fixer, span, fixes, fixAllState,
-                    supportedScopes, diagnostics.First());
-                result.Add(codeFix);
+            FixAllState fixAllState = null;
+            IEnumerable<FixAllScope> supportedScopes = null;
+            if (fixAllProviderInfo != null)
+            {
+                var codeFixProvider = (fixer as CodeFixProvider) ?? new WrapperCodeFixProvider((ISuppressionFixProvider)fixer, diagnostics.Select(d => d.Id));
+                fixAllState = FixAllState.Create(
+                    fixAllProviderInfo.FixAllProvider,
+                    document, fixAllProviderInfo, codeFixProvider, diagnostics,
+                    this.GetDocumentDiagnosticsAsync, this.GetProjectDiagnosticsAsync);
+                supportedScopes = fixAllProviderInfo.SupportedScopes;
             }
+
+            var codeFix = new CodeFixCollection(
+                fixer, span, fixes, fixAllState,
+                supportedScopes, diagnostics.First());
+            result.Add(codeFix);
         }
 
         public CodeFixProvider GetSuppressionFixer(string language, IEnumerable<string> diagnosticIds)

src/EditorFeatures/Core/Implementation/Suggestions/FixAllSuggestedAction.cs

 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
+using System;
 using System.Globalization;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
+using Microsoft.CodeAnalysis.CodeActions;
 using Microsoft.CodeAnalysis.CodeFixes;
 using Microsoft.CodeAnalysis.Editor.Host;
 using Microsoft.CodeAnalysis.Internal.Log;
@@ -23,16 +25,50 @@ internal sealed partial class FixAllSuggestedAction : SuggestedAction, ITelemetr
     {
         private readonly Diagnostic _fixedDiagnostic;
 
+        /// <summary>
+        /// The original code-action that we are a fix-all for.  i.e. _originalCodeAction
+        /// would be something like "use 'var' instead of 'int'", this suggestion action
+        /// and our <see cref="SuggestedAction.CodeAction"/> is the actual action that 
+        /// will perform the fix in the appropriate document/project/solution scope.
+        /// </summary>
+        private readonly CodeAction _originalCodeAction;
+        private readonly FixAllState _fixAllState;
+
         internal FixAllSuggestedAction(
             SuggestedActionsSourceProvider sourceProvider,
             Workspace workspace,
             ITextBuffer subjectBuffer,
             FixAllState fixAllState,
-            Diagnostic originalFixedDiagnostic)
+            Diagnostic originalFixedDiagnostic,
+            CodeAction originalCodeAction)
             : base(sourceProvider, workspace, subjectBuffer,
                    fixAllState.FixAllProvider, new FixAllCodeAction(fixAllState))
         {
             _fixedDiagnostic = originalFixedDiagnostic;
+            _originalCodeAction = originalCodeAction;
+            _fixAllState = fixAllState;
+        }
+
+        public override bool TryGetTelemetryId(out Guid telemetryId)
+        {
+            // We get the telemetry id for the original code action we are fixing,
+            // not the special 'FixAllCodeAction'.  that is the .CodeAction this
+            // SuggestedAction is pointing at.
+            var prefix = GetTelemetryPrefix(_originalCodeAction);
+            var scope = GetTelemetryScope();
+            telemetryId = new Guid(prefix, scope, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+            return true;
+        }
+
+        private short GetTelemetryScope()
+        {
+            switch (_fixAllState.Scope)
+            {
+                case FixAllScope.Document: return 1;
+                case FixAllScope.Project: return 2;
+                case FixAllScope.Solution: return 3;
+                default: return 4;
+            }
         }
 
         public string GetDiagnosticID()

src/EditorFeatures/Core/Implementation/Suggestions/SuggestedAction.cs

@@ -33,9 +33,7 @@ internal abstract partial class SuggestedAction : ForegroundThreadAffinitizedObj
         protected readonly object Provider;
         internal readonly CodeAction CodeAction;
 
-        protected ICodeActionEditHandlerService EditHandler => SourceProvider.EditHandler;
-        protected IAsynchronousOperationListener OperationListener => SourceProvider.OperationListener;
-        protected IWaitIndicator WaitIndicator => SourceProvider.WaitIndicator;
+        private ICodeActionEditHandlerService EditHandler => SourceProvider.EditHandler;
 
         internal SuggestedAction(
             SuggestedActionsSourceProvider sourceProvider,
@@ -44,7 +42,8 @@ internal abstract partial class SuggestedAction : ForegroundThreadAffinitizedObj
             object provider,
             CodeAction codeAction)
         {
-            Contract.ThrowIfTrue(provider == null);
+            Contract.ThrowIfNull(provider);
+            Contract.ThrowIfNull(codeAction);
 
             this.SourceProvider = sourceProvider;
             this.Workspace = workspace;
@@ -53,19 +52,19 @@ internal abstract partial class SuggestedAction : ForegroundThreadAffinitizedObj
             this.CodeAction = codeAction;
         }
 
-        internal virtual CodeActionPriority Priority => CodeAction?.Priority ?? CodeActionPriority.Medium;
+        internal virtual CodeActionPriority Priority => CodeAction.Priority;
 
-        public bool TryGetTelemetryId(out Guid telemetryId)
+        protected static int GetTelemetryPrefix(CodeAction codeAction)
         {
-            // TODO: this is temporary. Diagnostic team needs to figure out how to provide unique id per a fix.
-            // for now, we will use type of CodeAction, but there are some predefined code actions that are used by multiple fixes
-            // and this will not distinguish those
-
             // AssemblyQualifiedName will change across version numbers, FullName won't
-            var type = CodeAction.GetType();
+            var type = codeAction.GetType();
             type = type.IsConstructedGenericType ? type.GetGenericTypeDefinition() : type;
+            return type.FullName.GetHashCode();
+        }
 
-            telemetryId = new Guid(type.FullName.GetHashCode(), 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
+        public virtual bool TryGetTelemetryId(out Guid telemetryId)
+        {
+            telemetryId = new Guid(GetTelemetryPrefix(this.CodeAction), 0, 0, 0, 0, 0, 0, 0, 0, 0, 0);
             return true;
         }
 
@@ -96,7 +95,7 @@ public void Invoke(CancellationToken cancellationToken)
             // Create a task to do the actual async invocation of this action.
             // For testing purposes mark that we still have an outstanding async 
             // operation so that we don't try to validate things too soon.
-            var asyncToken = OperationListener.BeginAsyncOperation(GetType().Name + "." + nameof(Invoke));
+            var asyncToken = SourceProvider.OperationListener.BeginAsyncOperation(GetType().Name + "." + nameof(Invoke));
             var task = YieldThenInvokeAsync(cancellationToken);
             task.CompletesAsyncOperation(asyncToken);
         }
@@ -106,7 +105,7 @@ private async Task YieldThenInvokeAsync(CancellationToken cancellationToken)
             this.AssertIsForeground();
 
             // Always wrap whatever we're doing in a threaded wait dialog.
-            using (var context = this.WaitIndicator.StartWait(CodeAction.Title, CodeAction.Message, allowCancel: true, showProgress: true))
+            using (var context = this.SourceProvider.WaitIndicator.StartWait(CodeAction.Title, CodeAction.Message, allowCancel: true, showProgress: true))
             using (var linkedSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken, context.CancellationToken))
             {
                 // Yield the UI thread so that the light bulb can be dismissed.  This is necessary

src/EditorFeatures/Core/Implementation/Suggestions/SuggestedActionsSourceProvider.cs

@@ -341,7 +341,12 @@ private CodeRefactoring FilterOnUIThread(CodeRefactoring refactoring, Workspace
                 }
             }
 
-            private void ProcessFixCollection(Workspace workspace, IDictionary<CodeFixGroupKey, IList<SuggestedAction>> map, ArrayBuilder<CodeFixGroupKey> order, bool includeSuppressionFixes, CodeFixCollection fixCollection)
+            private void ProcessFixCollection(
+                Workspace workspace, 
+                IDictionary<CodeFixGroupKey, IList<SuggestedAction>> map, 
+                ArrayBuilder<CodeFixGroupKey> order, 
+                bool includeSuppressionFixes, 
+                CodeFixCollection fixCollection)
             {
                 var fixes = fixCollection.Fixes;
                 var fixCount = fixes.Length;
@@ -447,7 +452,8 @@ private void ProcessFixCollection(Workspace workspace, IDictionary<CodeFixGroupK
                 {
                     var fixAllStateForScope = fixAllState.WithScopeAndEquivalenceKey(scope, action.EquivalenceKey);
                     var fixAllSuggestedAction = new FixAllSuggestedAction(
-                        _owner, workspace, _subjectBuffer, fixAllStateForScope, firstDiagnostic);
+                        _owner, workspace, _subjectBuffer, fixAllStateForScope, 
+                        firstDiagnostic, action);
 
                     fixAllSuggestedActions.Add(fixAllSuggestedAction);
                 }

src/VisualStudio/Core/Def/Implementation/TableDataSource/Suppression/VisualStudioSuppressionFixService.cs

@@ -544,7 +544,7 @@ private static CodeFixProvider GetSuppressionFixer(IEnumerable<Diagnostic> diagn
                     if (documentDiagnosticsToFix.Any())
                     {
                         var diagnostics = await documentDiagnosticsToFix.ToDiagnosticsAsync(project, cancellationToken).ConfigureAwait(false);
-                        finalBuilder.Add(document, diagnostics.ToImmutableArray());
+                        finalBuilder.Add(document, diagnostics);
                     }
                 }
             }
@@ -609,7 +609,7 @@ private static CodeFixProvider GetSuppressionFixer(IEnumerable<Diagnostic> diagn
                 if (projectDiagnosticsToFix.Any())
                 {
                     var projectDiagnostics = await projectDiagnosticsToFix.ToDiagnosticsAsync(project, cancellationToken).ConfigureAwait(false);
-                    finalBuilder.Add(project, projectDiagnostics.ToImmutableArray());
+                    finalBuilder.Add(project, projectDiagnostics);
                 }
             }
 

src/Workspaces/Core/Portable/CodeFixes/CodeFix.cs

@@ -34,13 +34,7 @@ internal sealed class CodeFix
         /// in the future, if we decide to change the UI to depict the true mapping between fixes and diagnostics
         /// or if we decide to use some other heuristic to determine the <see cref="PrimaryDiagnostic"/>.
         /// </remarks>
-        internal Diagnostic PrimaryDiagnostic
-        {
-            get
-            {
-                return Diagnostics[0];
-            }
-        }
+        internal Diagnostic PrimaryDiagnostic => Diagnostics[0];
 
         internal CodeFix(Project project, CodeAction action, Diagnostic diagnostic)
         {

src/Workspaces/Core/Portable/Diagnostics/Extensions.cs

@@ -72,15 +72,15 @@ public static DiagnosticData ToDiagnosticData(this Diagnostic diagnostic, Projec
             return DiagnosticData.Create(project, diagnostic);
         }
 
-        public static async Task<IEnumerable<Diagnostic>> ToDiagnosticsAsync(this IEnumerable<DiagnosticData> diagnostics, Project project, CancellationToken cancellationToken)
+        public static async Task<ImmutableArray<Diagnostic>> ToDiagnosticsAsync(this IEnumerable<DiagnosticData> diagnostics, Project project, CancellationToken cancellationToken)
         {
-            var result = new List<Diagnostic>();
+            var result = ArrayBuilder<Diagnostic>.GetInstance();
             foreach (var diagnostic in diagnostics)
             {
                 result.Add(await diagnostic.ToDiagnosticAsync(project, cancellationToken).ConfigureAwait(false));
             }
 
-            return result;
+            return result.ToImmutableAndFree();
         }
 
         public static async Task<IList<Location>> ConvertLocationsAsync(