Fix reporting of EnC (and other) diagnostics (dotnet#11447)

* Fix reporting of EnC diagnostics

* Keep original overloads on DocumentDiagnosticAnalyzer for compat with TS and F#

src/Compilers/Core/CodeAnalysisTest/CodeAnalysisTest.csproj

@@ -28,6 +28,7 @@
     <Compile Include="Collections\BoxesTest.cs" />
     <Compile Include="Collections\ByteSequenceComparerTests.cs" />
     <Compile Include="CryptoBlobParserTests.cs" />
+    <Compile Include="Diagnostics\CompilationWithAnalyzersTests.cs" />
     <Compile Include="Diagnostics\ErrorLoggerTests.cs" />
     <Compile Include="Diagnostics\AnalysisContextInfoTests.cs" />
     <Compile Include="Diagnostics\BoxingOperationAnalyzer.cs" />

src/Compilers/Core/CodeAnalysisTest/Diagnostics/CompilationWithAnalyzersTests.cs

@@ -0,0 +1,49 @@
+// 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.Collections.Generic;
+using System.Collections.Immutable;
+using System.Linq;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Roslyn.Test.Utilities;
+using Roslyn.Utilities;
+using Xunit;
+
+namespace Microsoft.CodeAnalysis.UnitTests.Diagnostics
+{
+    using SimpleDiagnostic = Diagnostic.SimpleDiagnostic;
+
+    public class CompilationWithAnalyzersTests : TestBase
+    {
+        [Fact]
+        public void GetEffectiveDiagnostics_Errors()
+        {
+            var c = CSharpCompilation.Create("c");
+            var ds = new[] { default(Diagnostic) };
+
+            Assert.Throws<ArgumentNullException>(() => CompilationWithAnalyzers.GetEffectiveDiagnostics(default(ImmutableArray<Diagnostic>), c));
+            Assert.Throws<ArgumentNullException>(() => CompilationWithAnalyzers.GetEffectiveDiagnostics(default(IEnumerable<Diagnostic>), c));
+            Assert.Throws<ArgumentNullException>(() => CompilationWithAnalyzers.GetEffectiveDiagnostics(ds, null));
+        }
+
+        [Fact]
+        public void GetEffectiveDiagnostics()
+        {
+            var c = CSharpCompilation.Create("c", options: new CSharpCompilationOptions(OutputKind.DynamicallyLinkedLibrary).
+                WithSpecificDiagnosticOptions(
+                    new[] { KeyValuePair.Create($"CS{(int)ErrorCode.WRN_AlwaysNull:D4}", ReportDiagnostic.Suppress) }));
+
+            var d1 = SimpleDiagnostic.Create(MessageProvider.Instance, (int)ErrorCode.WRN_AlignmentMagnitude);
+            var d2 = SimpleDiagnostic.Create(MessageProvider.Instance, (int)ErrorCode.WRN_AlwaysNull);
+            var ds = new[] { default(Diagnostic), d1, d2 };
+
+            var filtered = CompilationWithAnalyzers.GetEffectiveDiagnostics(ds, c);
+
+            // overwrite the original value to test eagerness:
+            ds[1] = default(Diagnostic);
+
+            AssertEx.Equal(new[] { d1 }, filtered);
+        }
+    }
+}

src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

@@ -1010,8 +1010,18 @@ private static void FreeEventQueue(AsyncQueue<CompilationEvent> eventQueue, Obje
         /// 4) Pragma directives for the given <paramref name="compilation"/>.
         /// </summary>
         public static IEnumerable<Diagnostic> GetEffectiveDiagnostics(IEnumerable<Diagnostic> diagnostics, Compilation compilation)
+            => GetEffectiveDiagnostics(diagnostics.AsImmutableOrNull(), compilation);
+
+        /// <summary>
+        /// Given a set of compiler or <see cref="DiagnosticAnalyzer"/> generated <paramref name="diagnostics"/>, returns the effective diagnostics after applying the below filters:
+        /// 1) <see cref="CompilationOptions.SpecificDiagnosticOptions"/> specified for the given <paramref name="compilation"/>.
+        /// 2) <see cref="CompilationOptions.GeneralDiagnosticOption"/> specified for the given <paramref name="compilation"/>.
+        /// 3) Diagnostic suppression through applied <see cref="System.Diagnostics.CodeAnalysis.SuppressMessageAttribute"/>.
+        /// 4) Pragma directives for the given <paramref name="compilation"/>.
+        /// </summary>
+        public static IEnumerable<Diagnostic> GetEffectiveDiagnostics(ImmutableArray<Diagnostic> diagnostics, Compilation compilation)
         {
-            if (diagnostics == null)
+            if (diagnostics.IsDefault)
             {
                 throw new ArgumentNullException(nameof(diagnostics));
             }
@@ -1021,16 +1031,20 @@ public static IEnumerable<Diagnostic> GetEffectiveDiagnostics(IEnumerable<Diagno
                 throw new ArgumentNullException(nameof(compilation));
             }
 
+            return GetEffectiveDiagnosticsImpl(diagnostics, compilation);
+        }
+
+        private static IEnumerable<Diagnostic> GetEffectiveDiagnosticsImpl(ImmutableArray<Diagnostic> diagnostics, Compilation compilation)
+        {
             var suppressMessageState = new SuppressMessageAttributeState(compilation);
-            foreach (var diagnostic in diagnostics.ToImmutableArray())
+            foreach (var diagnostic in diagnostics)
             {
                 if (diagnostic != null)
                 {
                     var effectiveDiagnostic = compilation.Options.FilterDiagnostic(diagnostic);
                     if (effectiveDiagnostic != null)
                     {
-                        effectiveDiagnostic = suppressMessageState.ApplySourceSuppressions(effectiveDiagnostic);
-                        yield return effectiveDiagnostic;
+                        yield return suppressMessageState.ApplySourceSuppressions(effectiveDiagnostic);
                     }
                 }
             }

src/Compilers/Core/Portable/PublicAPI.Unshipped.txt

@@ -738,6 +738,7 @@ override Microsoft.CodeAnalysis.Semantics.OperationWalker.VisitVariableDeclarati
 override Microsoft.CodeAnalysis.Semantics.OperationWalker.VisitWhileUntilLoopStatement(Microsoft.CodeAnalysis.Semantics.IWhileUntilLoopStatement operation) -> void
 override Microsoft.CodeAnalysis.Semantics.OperationWalker.VisitWithStatement(Microsoft.CodeAnalysis.Semantics.IWithStatement operation) -> void
 override Microsoft.CodeAnalysis.Semantics.OperationWalker.VisitYieldBreakStatement(Microsoft.CodeAnalysis.Semantics.IReturnStatement operation) -> void
+static Microsoft.CodeAnalysis.Diagnostics.CompilationWithAnalyzers.GetEffectiveDiagnostics(System.Collections.Immutable.ImmutableArray<Microsoft.CodeAnalysis.Diagnostic> diagnostics, Microsoft.CodeAnalysis.Compilation compilation) -> System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.Diagnostic>
 static Microsoft.CodeAnalysis.Semantics.OperationExtensions.Descendants(this Microsoft.CodeAnalysis.IOperation operation) -> System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.IOperation>
 static Microsoft.CodeAnalysis.Semantics.OperationExtensions.DescendantsAndSelf(this Microsoft.CodeAnalysis.IOperation operation) -> System.Collections.Generic.IEnumerable<Microsoft.CodeAnalysis.IOperation>
 static Microsoft.CodeAnalysis.Semantics.OperationExtensions.GetRootOperation(this Microsoft.CodeAnalysis.ISymbol symbol, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.IOperation

src/EditorFeatures/Core/Implementation/EditAndContinue/EditAndContinueDiagnosticUpdateSource.cs

@@ -12,20 +12,30 @@
 
 namespace Microsoft.CodeAnalysis.Editor.Implementation.EditAndContinue
 {
+    internal sealed class EncErrorId : BuildToolId.Base<DebuggingSession, object>
+    {
+        public EncErrorId(DebuggingSession session, object errorId)
+            : base(session, errorId)
+        {
+        }
+
+        public override string BuildTool => PredefinedBuildTools.EnC;
+    }
+
     [Export(typeof(EditAndContinueDiagnosticUpdateSource))]
     [Shared]
     internal sealed class EditAndContinueDiagnosticUpdateSource : IDiagnosticUpdateSource
     {
-        internal static object DebuggerErrorId = new object();
-        internal static object EmitErrorId = new object();
+        internal static readonly object DebuggerErrorId = new object();
+        internal static readonly object EmitErrorId = new object();
 
         [ImportingConstructor]
         public EditAndContinueDiagnosticUpdateSource(IDiagnosticUpdateSourceRegistrationService registrationService)
         {
             registrationService.Register(this);
         }
 
-        public bool SupportGetDiagnostics { get { return false; } }
+        public bool SupportGetDiagnostics => false;
 
         public event EventHandler<DiagnosticsUpdatedArgs> DiagnosticsUpdated;
 
@@ -34,100 +44,94 @@ public EditAndContinueDiagnosticUpdateSource(IDiagnosticUpdateSourceRegistration
             return ImmutableArray<DiagnosticData>.Empty;
         }
 
-        public void ClearDiagnostics(DebuggingSession session, Workspace workspace, object kind, ProjectId projectId, ImmutableArray<DocumentId> documentIds)
+        public void ClearDiagnostics(EncErrorId errorId, Solution solution, ProjectId projectId, ImmutableArray<DocumentId> documentIds)
         {
-            if (documentIds.IsDefault)
-            {
-                return;
-            }
+            // clear project diagnostics:
+            ClearDiagnostics(errorId, solution, projectId, null);
 
-            foreach (var documentId in documentIds)
+            // clear document diagnostics:
+            foreach (var documentIdOpt in documentIds)
             {
-                ClearDiagnostics(session, workspace, kind, projectId, documentId);
+                ClearDiagnostics(errorId, solution, projectId, documentIdOpt);
             }
         }
 
-        public void ClearDiagnostics(DebuggingSession session, Workspace workspace, object errorId, ProjectId projectId, DocumentId documentId)
+        public void ClearDiagnostics(EncErrorId errorId, Solution solution, ProjectId projectId, DocumentId documentIdOpt)
         {
-            RaiseDiagnosticsUpdated(MakeRemovedArgs(session, workspace, errorId, projectId, documentId));
+            DiagnosticsUpdated?.Invoke(this, DiagnosticsUpdatedArgs.DiagnosticsRemoved(
+                errorId,
+                solution.Workspace,
+                solution: solution,
+                projectId: projectId,
+                documentId: documentIdOpt));
         }
 
-        public ImmutableArray<DocumentId> ReportDiagnostics(DebuggingSession session, object errorId, ProjectId projectId, Solution solution, IEnumerable<Diagnostic> diagnostics)
+        public ImmutableArray<DocumentId> ReportDiagnostics(object errorId, Solution solution, ProjectId projectId, IEnumerable<Diagnostic> diagnostics)
         {
-            var argsByDocument = ImmutableArray.CreateRange(
-                from diagnostic in diagnostics
-                let document = solution.GetDocument(diagnostic.Location.SourceTree, projectId)
-                where document != null
-                let item = MakeDiagnosticData(projectId, document, solution, diagnostic)
-                group item by document.Id into itemsByDocumentId
-                select MakeCreatedArgs(session, errorId, solution.Workspace, solution, projectId, itemsByDocumentId.Key, ImmutableArray.CreateRange(itemsByDocumentId)));
-
-            foreach (var args in argsByDocument)
-            {
-                RaiseDiagnosticsUpdated(args);
-            }
+            Debug.Assert(errorId != null);
+            Debug.Assert(solution != null);
+            Debug.Assert(projectId != null);
 
-            return argsByDocument.SelectAsArray(args => args.DocumentId);
-        }
+            var updateEvent = DiagnosticsUpdated;
+            var documentIds = ArrayBuilder<DocumentId>.GetInstance();
+            var documentDiagnosticData = ArrayBuilder<DiagnosticData>.GetInstance();
+            var projectDiagnosticData = ArrayBuilder<DiagnosticData>.GetInstance();
+            var project = solution.GetProject(projectId);
 
-        private static DiagnosticData MakeDiagnosticData(ProjectId projectId, Document document, Solution solution, Diagnostic d)
-        {
-            if (document != null)
+            foreach (var diagnostic in diagnostics)
             {
-                return DiagnosticData.Create(document, d);
+                var documentOpt = solution.GetDocument(diagnostic.Location.SourceTree, projectId);
+
+                if (documentOpt != null)
+                {
+                    if (updateEvent != null)
+                    {
+                        documentDiagnosticData.Add(DiagnosticData.Create(documentOpt, diagnostic));
+                    }
+
+                    documentIds.Add(documentOpt.Id);
+                }
+                else if (updateEvent != null)
+                {
+                    projectDiagnosticData.Add(DiagnosticData.Create(project, diagnostic));
+                }
             }
-            else
+
+            foreach (var documentDiagnostics in documentDiagnosticData.ToDictionary(data => data.DocumentId))
             {
-                var project = solution.GetProject(projectId);
-                Debug.Assert(project != null);
-                return DiagnosticData.Create(project, d);
+                updateEvent(this, DiagnosticsUpdatedArgs.DiagnosticsCreated(
+                    errorId,
+                    solution.Workspace,
+                    solution,
+                    projectId,
+                    documentId: documentDiagnostics.Key,
+                    diagnostics: documentDiagnostics.Value));
             }
-        }
 
-        private DiagnosticsUpdatedArgs MakeCreatedArgs(
-            DebuggingSession session, Workspace workspace, object errorId, ProjectId projectId, DocumentId documentId, ImmutableArray<DiagnosticData> items)
-        {
-            return MakeCreatedArgs(session, errorId, workspace, solution: null, projectId: projectId, documentId: documentId, items: items);
-        }
-
-        private DiagnosticsUpdatedArgs MakeRemovedArgs(
-            DebuggingSession session, Workspace workspace, object errorId, ProjectId projectId, DocumentId documentId)
-        {
-            return MakeRemovedArgs(session, errorId, workspace, solution: null, projectId: projectId, documentId: documentId);
-        }
+            if (projectDiagnosticData.Count > 0)
+            {
+                updateEvent(this, DiagnosticsUpdatedArgs.DiagnosticsCreated(
+                    errorId,
+                    solution.Workspace,
+                    solution,
+                    projectId,
+                    documentId: null,
+                    diagnostics: projectDiagnosticData.ToImmutable()));
+            }
 
-        private DiagnosticsUpdatedArgs MakeCreatedArgs(
-            DebuggingSession session, object errorId, Workspace workspace, Solution solution, ProjectId projectId, DocumentId documentId, ImmutableArray<DiagnosticData> items)
-        {
-            return DiagnosticsUpdatedArgs.DiagnosticsCreated(
-                CreateId(session, errorId), workspace, solution, projectId, documentId, items);
+            documentDiagnosticData.Free();
+            projectDiagnosticData.Free();
+            return documentIds.ToImmutableAndFree();
         }
 
-        private DiagnosticsUpdatedArgs MakeRemovedArgs(
-            DebuggingSession session, object errorId, Workspace workspace, Solution solution, ProjectId projectId, DocumentId documentId)
+        internal ImmutableArray<DocumentId> ReportDiagnostics(DebuggingSession session, object errorId, ProjectId projectId, Solution solution, IEnumerable<Diagnostic> diagnostics)
         {
-            return DiagnosticsUpdatedArgs.DiagnosticsRemoved(
-                CreateId(session, errorId), workspace, solution, projectId, documentId);
+            return ReportDiagnostics(new EncErrorId(session, errorId), solution, projectId, diagnostics);
         }
 
-        private static EnCId CreateId(DebuggingSession session, object errorId) => new EnCId(session, errorId);
-
-        private void RaiseDiagnosticsUpdated(DiagnosticsUpdatedArgs args)
+        internal void ClearDiagnostics(DebuggingSession session, Workspace workspace, object errorId, ProjectId projectId, ImmutableArray<DocumentId> documentIds)
         {
-            this.DiagnosticsUpdated?.Invoke(this, args);
-        }
-
-        private class EnCId : BuildToolId.Base<DebuggingSession, object>
-        {
-            public EnCId(DebuggingSession session, object errorId) :
-                base(session, errorId)
-            {
-            }
-
-            public override string BuildTool
-            {
-                get { return PredefinedBuildTools.EnC; }
-            }
+            ClearDiagnostics(new EncErrorId(session, errorId), workspace.CurrentSolution, projectId, documentIds);
         }
     }
 }

src/EditorFeatures/Test/Workspaces/NoCompilationDocumentDiagnosticAnalyzer.cs

@@ -6,6 +6,7 @@
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Text;
+using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces
 {
@@ -17,16 +18,15 @@ internal class NoCompilationDocumentDiagnosticAnalyzer : DocumentDiagnosticAnaly
 
         public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Descriptor);
 
-        public override Task AnalyzeSemanticsAsync(Document document, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
+        public override Task<ImmutableArray<Diagnostic>> AnalyzeSemanticsAsync(Document document, CancellationToken cancellationToken)
         {
-            return Task.FromResult(true);
+            return SpecializedTasks.EmptyImmutableArray<Diagnostic>();
         }
 
-        public override Task AnalyzeSyntaxAsync(Document document, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
+        public override Task<ImmutableArray<Diagnostic>> AnalyzeSyntaxAsync(Document document, CancellationToken cancellationToken)
         {
-            addDiagnostic(Diagnostic.Create(Descriptor,
-                Location.Create(document.FilePath, default(TextSpan), default(LinePositionSpan))));
-            return Task.FromResult(true);
+            return Task.FromResult(ImmutableArray.Create(
+                Diagnostic.Create(Descriptor, Location.Create(document.FilePath, default(TextSpan), default(LinePositionSpan)))));
         }
     }
 }

src/Features/Core/Portable/Diagnostics/Analyzers/IDocumentDiagnosticAnalyzer.cs

@@ -1,8 +1,10 @@
 // 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.Collections.Immutable;
 using System.Threading;
 using System.Threading.Tasks;
+using Roslyn.Utilities;
 
 namespace Microsoft.CodeAnalysis.Diagnostics
 {
@@ -12,8 +14,33 @@ namespace Microsoft.CodeAnalysis.Diagnostics
     internal abstract class DocumentDiagnosticAnalyzer : DiagnosticAnalyzer
     {
         // REVIEW: why DocumentDiagnosticAnalyzer doesn't have span based analysis?
-        public abstract Task AnalyzeSyntaxAsync(Document document, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken);
-        public abstract Task AnalyzeSemanticsAsync(Document document, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken);
+        // TODO: Make abstract once TypeScript and F# move over to the overloads above
+        public async virtual Task<ImmutableArray<Diagnostic>> AnalyzeSyntaxAsync(Document document, CancellationToken cancellationToken)
+        {
+            var builder = ArrayBuilder<Diagnostic>.GetInstance();
+            await AnalyzeSyntaxAsync(document, builder.Add, cancellationToken).ConfigureAwait(false);
+            return builder.ToImmutableAndFree();
+        }
+
+        // TODO: Make abstract once TypeScript and F# move over to the overloads above
+        public async virtual Task<ImmutableArray<Diagnostic>> AnalyzeSemanticsAsync(Document document, CancellationToken cancellationToken)
+        {
+            var builder = ArrayBuilder<Diagnostic>.GetInstance();
+            await AnalyzeSemanticsAsync(document, builder.Add, cancellationToken).ConfigureAwait(false);
+            return builder.ToImmutableAndFree();
+        }
+
+        // TODO: Remove once TypeScript and F# move over to the overloads above
+        public virtual Task AnalyzeSyntaxAsync(Document document, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
+        {
+            throw ExceptionUtilities.Unreachable;
+        }
+
+        // TODO: Remove once TypeScript and F# move over to the overloads above
+        public virtual Task AnalyzeSemanticsAsync(Document document, Action<Diagnostic> addDiagnostic, CancellationToken cancellationToken)
+        {
+            throw ExceptionUtilities.Unreachable;
+        }
 
         /// <summary>
         /// it is not allowed one to implement both DocumentDiagnosticAnalzyer and DiagnosticAnalyzer