EnC simplify reporting module diagnostics. (#40999)

Previously we reported diagnostics caused by the debuggee not allowing edits in the corresponding module in EnC diagnostic analyzer.
A module does not support edits if any of its instances does not support the edits. There are scenarios in which module instances may be loaded while in break mode.
E.g. if the user attaches to a secondary process while debugging a primary process, the secondary process might have a module instance loaded of the same module loaded to the primary process.
To handle these cases we hooked up debugger module load/unload events and invalidated the reported diagnostics.

This approach has a few problems:
1) EnC analyzer is now non-deterministic, as it can give different answers for the same inputs depending on the state of the debuggee
2) Turns out that the debugger does not deliver module load/unload events in all cases (e.g. when the secondary process is killed).
3) It's a bit complicated and would get even more complicated once we moved EnC to OOP/Cloud

Instead, we don't report these errors until the user triggers the "continue" action. At this point the VS and all processes being debugged are blocked until the changes analyzed and applied.
Since this situation is not very common the slight regression in interactivity is acceptable.

src/EditorFeatures/Test/EditAndContinue/EditAndContinueWorkspaceServiceTests.cs

@@ -914,18 +914,30 @@ public async Task BreakMode_FileAdded()
             var document2 = project.AddDocument("file2.cs", SourceText.From("class C2 {}"));
             workspace.ChangeSolution(document2.Project.Solution);
 
-            // update in document2:
             var diagnostics2 = await service.GetDocumentDiagnosticsAsync(document2, CancellationToken.None).ConfigureAwait(false);
-            AssertEx.Equal(new[] { "ENC2123" }, diagnostics2.Select(d => d.Id));
+            AssertEx.Equal(
+                new[] { "ENC0071: " + string.Format(FeaturesResources.Adding_a_new_file_will_prevent_the_debug_session_from_continuing) },
+                diagnostics2.Select(d => $"{d.Id}: {d.GetMessage()}"));
 
             Assert.True(await service.HasChangesAsync(sourceFilePath: null, CancellationToken.None).ConfigureAwait(false));
 
+            var (solutionStatusEmit, deltas) = await service.EmitSolutionUpdateAsync(CancellationToken.None).ConfigureAwait(false);
+            Assert.Equal(SolutionUpdateStatus.Blocked, solutionStatusEmit);
+            Assert.Empty(deltas);
+
+            AssertEx.Equal(
+                new[] { "ENC2123: " + string.Format(FeaturesResources.EditAndContinueDisallowedByProject, "Test", "*message*") },
+                _emitDiagnosticsUpdated.Single().Diagnostics.Select(d => $"{d.Id}: {d.Message}"));
+
             service.EndEditSession();
             service.EndDebuggingSession();
 
             AssertEx.Equal(new[]
             {
-                "Debugging_EncSession: SessionId=1|SessionCount=0|EmptySessionCount=1"
+                "Debugging_EncSession: SessionId=1|SessionCount=1|EmptySessionCount=0",
+                "Debugging_EncSession_EditSession: SessionId=1|EditSessionId=2|HadCompilationErrors=False|HadRudeEdits=True|HadValidChanges=False|HadValidInsignificantChanges=False|RudeEditsCount=1|EmitDeltaErrorIdCount=1",
+                "Debugging_EncSession_EditSession_EmitDeltaErrorId: SessionId=1|EditSessionId=2|ErrorId=ENC2123",
+                "Debugging_EncSession_EditSession_RudeEdit: SessionId=1|EditSessionId=2|RudeEditKind=71|RudeEditSyntaxKind=0|RudeEditBlocking=True"
             }, _telemetryLog);
         }
 
@@ -955,29 +967,13 @@ void M()
     System.Console.WriteLine(30); 
   } 
 }";
-            var expectedMessage = "ENC2123: " + string.Format(FeaturesResources.EditAndContinueDisallowedByProject, "Test", "*message*");
-
-            var expectedDiagnostics = new[]
-            {
-                "[17..19): " + expectedMessage,
-                "[66..67): " + expectedMessage,
-                "[101..101): " + expectedMessage,
-                "[136..137): " + expectedMessage,
-            };
-
-            static string inspectDiagnostic(Diagnostic d)
-                => $"{d.Location.SourceSpan}: {d.Id}: {d.GetMessage()}";
-
             using (var workspace = TestWorkspace.CreateCSharp(source1))
             {
                 var project = workspace.CurrentSolution.Projects.Single();
                 _mockCompilationOutputsService.Outputs.Add(project.Id, new MockCompilationOutputs(moduleId));
 
-                bool isEditAndContinueAvailableInvocationAllowed = true;
                 _mockDebugeeModuleMetadataProvider.IsEditAndContinueAvailable = (Guid guid, out int errorCode, out string localizedMessage) =>
                 {
-                    Assert.True(isEditAndContinueAvailableInvocationAllowed);
-
                     Assert.Equal(moduleId, guid);
                     errorCode = 123;
                     localizedMessage = "*message*";
@@ -996,20 +992,10 @@ static string inspectDiagnostic(Diagnostic d)
                 workspace.ChangeDocument(document1.Id, SourceText.From(source2));
                 var document2 = workspace.CurrentSolution.Projects.Single().Documents.Single();
 
-                isEditAndContinueAvailableInvocationAllowed = true;
+                // We do not report module diagnostics until emit.
+                // This is to make the analysis deterministic (not dependent on the current state of the debuggee).
                 var diagnostics1 = await service.GetDocumentDiagnosticsAsync(document2, CancellationToken.None).ConfigureAwait(false);
-                AssertEx.Equal(expectedDiagnostics, diagnostics1.Select(inspectDiagnostic));
-
-                // the diagnostic should be cached and we should not invoke isEditAndContinueAvailable again:
-                isEditAndContinueAvailableInvocationAllowed = false;
-                var diagnostics2 = await service.GetDocumentDiagnosticsAsync(document2, CancellationToken.None).ConfigureAwait(false);
-                AssertEx.Equal(expectedDiagnostics, diagnostics2.Select(inspectDiagnostic));
-
-                // invalidate cache:
-                service.Test_GetEditSession().ModuleInstanceLoadedOrUnloaded(moduleId);
-                isEditAndContinueAvailableInvocationAllowed = true;
-                var diagnostics3 = await service.GetDocumentDiagnosticsAsync(document2, CancellationToken.None).ConfigureAwait(false);
-                AssertEx.Equal(expectedDiagnostics, diagnostics3.Select(inspectDiagnostic));
+                AssertEx.Empty(diagnostics1);
 
                 // validate solution update status and emit:
                 Assert.True(await service.HasChangesAsync(sourceFilePath: null, CancellationToken.None).ConfigureAwait(false));
@@ -1018,8 +1004,11 @@ static string inspectDiagnostic(Diagnostic d)
                 Assert.Equal(SolutionUpdateStatus.Blocked, solutionStatusEmit);
                 Assert.Empty(deltas);
 
+                AssertEx.Equal(new[] { "ENC2123: " + string.Format(FeaturesResources.EditAndContinueDisallowedByProject, "Test", "*message*") },
+                    _emitDiagnosticsUpdated.Single().Diagnostics.Select(d => $"{d.Id}: {d.Message}"));
+
                 service.EndEditSession();
-                VerifyReanalyzeInvocation(workspace, null, ImmutableArray.Create(document2.Id), false);
+                VerifyReanalyzeInvocation(workspace, null, ImmutableArray<DocumentId>.Empty, false);
 
                 service.EndDebuggingSession();
                 VerifyReanalyzeInvocation(workspace, null, ImmutableArray<DocumentId>.Empty, false);

src/Features/Core/Portable/EditAndContinue/EditAndContinueWorkspaceService.cs

@@ -92,18 +92,6 @@ public void OnSourceFileUpdated(DocumentId documentId)
             }
         }
 
-        /// <summary>
-        /// Invoked whenever a module instance is loaded to a process being debugged.
-        /// </summary>
-        public void OnManagedModuleInstanceLoaded(Guid mvid)
-            => _editSession?.ModuleInstanceLoadedOrUnloaded(mvid);
-
-        /// <summary>
-        /// Invoked whenever a module instance is unloaded from a process being debugged.
-        /// </summary>
-        public void OnManagedModuleInstanceUnloaded(Guid mvid)
-            => _editSession?.ModuleInstanceLoadedOrUnloaded(mvid);
-
         public void StartDebuggingSession()
         {
             var previousSession = Interlocked.CompareExchange(ref _debuggingSession, new DebuggingSession(_workspace, _debugeeModuleMetadataProvider, _activeStatementProvider, _compilationOutputsProvider), null);
@@ -237,33 +225,6 @@ public async Task<ImmutableArray<Diagnostic>> GetDocumentDiagnosticsAsync(Docume
                     // is about to be updated, so that it can start initializing it for EnC update, reducing the amount of time applying
                     // the change blocks the UI when the user "continues".
                     debuggingSession.PrepareModuleForUpdate(mvid);
-
-                    // Check if EnC is allowed for all loaded modules corresponding to the project.
-                    var moduleDiagnostics = editSession.GetModuleDiagnostics(mvid, project.Name);
-
-                    if (!moduleDiagnostics.IsEmpty)
-                    {
-                        // track the document, so that we can refresh or clean diagnostics at the end of edit session:
-                        editSession.TrackDocumentWithReportedDiagnostics(document.Id);
-
-                        var newSyntaxTree = await document.GetSyntaxTreeAsync(cancellationToken).ConfigureAwait(false);
-                        Contract.ThrowIfNull(newSyntaxTree);
-
-                        var changedSpans = await GetChangedSpansAsync(oldDocument, newSyntaxTree, cancellationToken).ConfigureAwait(false);
-
-                        var diagnosticsBuilder = ArrayBuilder<Diagnostic>.GetInstance();
-                        foreach (var span in changedSpans)
-                        {
-                            var location = Location.Create(newSyntaxTree, span);
-
-                            foreach (var diagnostic in moduleDiagnostics)
-                            {
-                                diagnosticsBuilder.Add(diagnostic.ToDiagnostic(location));
-                            }
-                        }
-
-                        return diagnosticsBuilder.ToImmutableAndFree();
-                    }
                 }
 
                 if (analysis.RudeEditErrors.IsEmpty)

src/Features/Core/Portable/EditAndContinue/EditSession.cs

@@ -50,18 +50,6 @@ internal sealed class EditSession : IDisposable
             = new Dictionary<DocumentId, (Document, AsyncLazy<DocumentAnalysisResults>)>();
         private readonly object _analysesGuard = new object();
 
-        /// <summary>
-        /// Errors to be reported when a project is updated but the corresponding module does not support EnC.
-        /// 
-        /// The capability of a module to apply edits may change during edit session if the user attaches debugger to 
-        /// an additional process that doesn't support EnC (or detaches from such process). The diagnostic reflects 
-        /// the state of the module when queried for the first time. Before we actually apply an edit to the module 
-        /// we need to query again instead of just reusing the diagnostic.
-        /// </summary>
-        private readonly Dictionary<Guid, ImmutableArray<LocationlessDiagnostic>> _moduleDiagnostics
-             = new Dictionary<Guid, ImmutableArray<LocationlessDiagnostic>>();
-        private readonly object _moduleDiagnosticsGuard = new object();
-
         /// <summary>
         /// A <see cref="DocumentId"/> is added whenever <see cref="EditAndContinueDiagnosticAnalyzer"/> reports 
         /// rude edits or module diagnostics. At the end of the session we ask the diagnostic analyzer to reanalyze 
@@ -89,42 +77,18 @@ public void Dispose()
             _cancellationSource.Dispose();
         }
 
-        internal void ModuleInstanceLoadedOrUnloaded(Guid mvid)
-        {
-            // invalidate diagnostic cache for the module:
-            lock (_moduleDiagnosticsGuard)
-            {
-                _moduleDiagnostics.Remove(mvid);
-            }
-        }
-
-        public ImmutableArray<LocationlessDiagnostic> GetModuleDiagnostics(Guid mvid, string projectDisplayName)
+        /// <summary>
+        /// Errors to be reported when a project is updated but the corresponding module does not support EnC.
+        /// </summary>
+        public ImmutableArray<Diagnostic> GetModuleDiagnostics(Guid mvid, string projectDisplayName)
         {
-            ImmutableArray<LocationlessDiagnostic> result;
-            lock (_moduleDiagnosticsGuard)
-            {
-                if (_moduleDiagnostics.TryGetValue(mvid, out result))
-                {
-                    return result;
-                }
-            }
-
-            var newResult = ImmutableArray<LocationlessDiagnostic>.Empty;
-            if (!DebuggingSession.DebugeeModuleMetadataProvider.IsEditAndContinueAvailable(mvid, out var errorCode, out var localizedMessage))
+            if (DebuggingSession.DebugeeModuleMetadataProvider.IsEditAndContinueAvailable(mvid, out var errorCode, out var localizedMessage))
             {
-                var descriptor = EditAndContinueDiagnosticDescriptors.GetModuleDiagnosticDescriptor(errorCode);
-                newResult = ImmutableArray.Create(new LocationlessDiagnostic(descriptor, new[] { projectDisplayName, localizedMessage }));
-            }
-
-            lock (_moduleDiagnosticsGuard)
-            {
-                if (!_moduleDiagnostics.TryGetValue(mvid, out result))
-                {
-                    _moduleDiagnostics.Add(mvid, result = newResult);
-                }
+                return ImmutableArray<Diagnostic>.Empty;
             }
 
-            return result;
+            var descriptor = EditAndContinueDiagnosticDescriptors.GetModuleDiagnosticDescriptor(errorCode);
+            return ImmutableArray.Create(Diagnostic.Create(descriptor, Location.None, new[] { projectDisplayName, localizedMessage }));
         }
 
         private async Task<ActiveStatementsMap> GetBaseActiveStatementsAsync(CancellationToken cancellationToken)
@@ -680,11 +644,6 @@ private static async Task<ProjectChanges> GetProjectChangesAsync(ImmutableArray<
             }
         }
 
-        internal ImmutableArray<LocationlessDiagnostic> GetDebugeeStateDiagnostics()
-        {
-            return ImmutableArray<LocationlessDiagnostic>.Empty;
-        }
-
         public async Task<SolutionUpdate> EmitSolutionUpdateAsync(Solution solution, CancellationToken cancellationToken)
         {
             try
@@ -750,25 +709,25 @@ public async Task<SolutionUpdate> EmitSolutionUpdateAsync(Solution solution, Can
                         diagnostics.Add((project.Id, documentDiagnostics));
                     }
 
-                    var projectSummary = await GetProjectAnalysisSymmaryAsync(changedDocumentAnalyses, cancellationToken).ConfigureAwait(false);
-
-                    if (projectSummary != ProjectAnalysisSummary.ValidChanges)
+                    // The capability of a module to apply edits may change during edit session if the user attaches debugger to 
+                    // an additional process that doesn't support EnC (or detaches from such process). Before we apply edits 
+                    // we need to check with the debugger.
+                    var moduleDiagnostics = GetModuleDiagnostics(mvid, project.Name);
+                    if (!moduleDiagnostics.IsEmpty)
                     {
-                        Telemetry.LogProjectAnalysisSummary(projectSummary, ImmutableArray<string>.Empty);
-
-                        if (projectSummary == ProjectAnalysisSummary.CompilationErrors || projectSummary == ProjectAnalysisSummary.RudeEdits)
-                        {
-                            isBlocked = true;
-                        }
+                        diagnostics.Add((project.Id, moduleDiagnostics));
+                        isBlocked = true;
+                    }
 
-                        continue;
+                    var projectSummary = await GetProjectAnalysisSymmaryAsync(changedDocumentAnalyses, cancellationToken).ConfigureAwait(false);
+                    if (projectSummary == ProjectAnalysisSummary.CompilationErrors || projectSummary == ProjectAnalysisSummary.RudeEdits)
+                    {
+                        isBlocked = true;
                     }
 
-                    var moduleDiagnostics = GetModuleDiagnostics(mvid, project.Name);
-                    if (!moduleDiagnostics.IsEmpty)
+                    if (!moduleDiagnostics.IsEmpty || projectSummary != ProjectAnalysisSummary.ValidChanges)
                     {
                         Telemetry.LogProjectAnalysisSummary(projectSummary, moduleDiagnostics.SelectAsArray(d => d.Descriptor.Id));
-                        isBlocked = true;
                         continue;
                     }
 

src/Features/Core/Portable/EditAndContinue/IEditAndContinueWorkspaceService.cs

@@ -18,9 +18,6 @@ internal interface IEditAndContinueWorkspaceService : IWorkspaceService
         void CommitSolutionUpdate();
         void DiscardSolutionUpdate();
 
-        void OnManagedModuleInstanceLoaded(Guid mvid);
-        void OnManagedModuleInstanceUnloaded(Guid mvid);
-
         bool IsDebuggingSessionInProgress { get; }
         void OnSourceFileUpdated(DocumentId documentId);
 

src/Features/Core/Portable/EditAndContinue/LocationlessDiagnostic.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.Collections.Immutable;
-
-namespace Microsoft.CodeAnalysis.EditAndContinue
-{
-    internal readonly struct LocationlessDiagnostic
-    {
-        public readonly DiagnosticDescriptor Descriptor;
-        public readonly object[] MessageArgs;
-
-        public LocationlessDiagnostic(DiagnosticDescriptor descriptor, object[] messageArgs)
-        {
-            Descriptor = descriptor;
-            MessageArgs = messageArgs;
-        }
-
-        public Diagnostic ToDiagnostic(Location location)
-            => Diagnostic.Create(Descriptor, location, MessageArgs);
-    }
-}

src/VisualStudio/Core/Def/Implementation/EditAndContinue/VisualStudioDebugStateChangeListener.cs

@@ -24,59 +24,6 @@ internal sealed class VisualStudioDebugStateChangeListener : IDebugStateChangeLi
         // EnC service or null if EnC is disabled for the debug session.
         private IEditAndContinueWorkspaceService? _encService;
 
-        /// <summary>
-        /// Concord component. Singleton created on demand during debugging session and discarded as soon as the session ends.
-        /// </summary>
-        private sealed class DebuggerService : IDkmCustomMessageForwardReceiver, IDkmModuleInstanceLoadNotification, IDkmModuleInstanceUnloadNotification
-        {
-            private IEditAndContinueWorkspaceService? _encService;
-
-            /// <summary>
-            /// Message source id as specified in ManagedEditAndContinueService.vsdconfigxml.
-            /// </summary>
-            public static readonly Guid MessageSourceId = new Guid("730432E7-1B68-4B3A-BD6A-BD4C13E0566B");
-
-            DkmCustomMessage? IDkmCustomMessageForwardReceiver.SendLower(DkmCustomMessage customMessage)
-            {
-                // Initialize the listener before OnModuleInstanceLoad/OnModuleInstanceUnload can be triggered.
-                // These events are only called when managed debugging is being used due to RuntimeId=DkmRuntimeId.Clr filter in vsdconfigxml.
-                _encService = (IEditAndContinueWorkspaceService)customMessage.Parameter1;
-                return null;
-            }
-
-            /// <summary>
-            /// <see cref="IDkmModuleInstanceLoadNotification"/> is implemented by components that want to listen
-            /// for the ModuleInstanceLoad event. When this notification fires, the target process
-            /// will be suspended and can be examined. ModuleInstanceLoad is fired when a module is
-            /// loaded by a target process. Among other things, this event is used for symbol
-            /// providers to load symbols, and for the breakpoint manager to set breakpoints.
-            /// ModuleInstanceLoad fires for all modules, even if there are no symbols loaded.
-            /// </summary>
-            void IDkmModuleInstanceLoadNotification.OnModuleInstanceLoad(DkmModuleInstance moduleInstance, DkmWorkList workList, DkmEventDescriptorS eventDescriptor)
-            {
-                if (moduleInstance is DkmClrModuleInstance clrModuleInstance)
-                {
-                    Contract.ThrowIfNull(_encService);
-                    _encService.OnManagedModuleInstanceLoaded(clrModuleInstance.Mvid);
-                }
-            }
-
-            /// <summary>
-            /// <see cref="IDkmModuleInstanceUnloadNotification"/> is implemented by components that want to listen
-            /// for the ModuleInstanceUnload event. When this notification fires, the target process
-            /// will be suspended and can be examined. ModuleInstanceUnload is sent when the monitor
-            /// detects that a module has unloaded from within the target process.
-            /// </summary>
-            void IDkmModuleInstanceUnloadNotification.OnModuleInstanceUnload(DkmModuleInstance moduleInstance, DkmWorkList workList, DkmEventDescriptor eventDescriptor)
-            {
-                if (moduleInstance is DkmClrModuleInstance clrModuleInstance)
-                {
-                    Contract.ThrowIfNull(_encService);
-                    _encService.OnManagedModuleInstanceUnloaded(clrModuleInstance.Mvid);
-                }
-            }
-        }
-
         [ImportingConstructor]
         public VisualStudioDebugStateChangeListener(VisualStudioWorkspace workspace)
         {
@@ -94,19 +41,6 @@ public void StartDebugging(DebugSessionOptions options)
             if ((options & DebugSessionOptions.EditAndContinueDisabled) == 0)
             {
                 _encService = _workspace.Services.GetRequiredService<IEditAndContinueWorkspaceService>();
-
-                // hook up a callbacks (the call blocks until the message is processed):
-                using (DebuggerComponent.ManagedEditAndContinueService())
-                {
-                    DkmCustomMessage.Create(
-                        Connection: null,
-                        Process: null,
-                        SourceId: DebuggerService.MessageSourceId,
-                        MessageCode: 0,
-                        Parameter1: _encService,
-                        Parameter2: null).SendLower();
-                }
-
                 _encService.StartDebuggingSession();
             }
             else

src/VisualStudio/Core/Def/Implementation/EditAndContinue/VisualStudioDebuggeeModuleMetadataProvider.cs

@@ -57,7 +57,6 @@ DkmCustomMessage IDkmCustomMessageForwardReceiver.SendLower(DkmCustomMessage cus
         }
 
         private readonly DebuggeeModuleInfoCache _baselineMetadata;
-        private readonly object _moduleApplyGuard = new object();
 
         public VisualStudioDebuggeeModuleMetadataProvider()
         {

src/VisualStudio/Core/Def/ManagedEditAndContinueService.vsdconfigxml

@@ -19,22 +19,5 @@
         </InterfaceGroup>
       </Implements>
     </Class>
-    <Class Name="Microsoft.VisualStudio.LanguageServices.EditAndContinue.VisualStudioDebugStateChangeListener+DebuggerService">
-      <Implements>
-        <InterfaceGroup>
-          <Filter>
-            <SourceId RequiredValue="StateChangeListenerMessageSourceId"/>
-          </Filter>
-          <Interface Name="IDkmCustomMessageForwardReceiver"/>
-        </InterfaceGroup>
-        <InterfaceGroup CallOnlyWhenLoaded="true">
-          <Filter>
-            <RuntimeId RequiredValue="DkmRuntimeId.Clr"/>
-          </Filter>
-          <Interface Name="IDkmModuleInstanceLoadNotification"/>
-          <Interface Name="IDkmModuleInstanceUnloadNotification"/>
-        </InterfaceGroup>
-      </Implements>
-    </Class>
   </ManagedComponent>
 </Configuration>
\ No newline at end of file