Fix a couple of issues with committing and diagnostic reporting

src/EditorFeatures/Core/EditAndContinue/EditAndContinueLanguageService.cs

@@ -385,8 +385,10 @@ public async ValueTask<ManagedHotReloadUpdates> GetUpdatesAsync(ImmutableArray<s
 
         switch (result.ModuleUpdates.Status)
         {
-            case ModuleUpdateStatus.Ready:
-                // We have updates to be applied. The debugger will call Commit/Discard on the solution
+            case ModuleUpdateStatus.Ready when result.ProjectsToRebuild.IsEmpty:
+                // We have updates to be applied and no rude edits.
+                //
+                // The debugger will call Commit/Discard on the solution
                 // based on whether the updates will be applied successfully or not.
                 _pendingUpdatedDesignTimeSolution = designTimeSolution;
                 break;

src/EditorFeatures/Test/EditAndContinue/EditAndContinueLanguageServiceTests.cs

@@ -14,6 +14,7 @@
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.BrokeredServices;
 using Microsoft.CodeAnalysis.BrokeredServices.UnitTests;
+using Microsoft.CodeAnalysis.Contracts.EditAndContinue;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.EditAndContinue;
 using Microsoft.CodeAnalysis.EditAndContinue.UnitTests;
@@ -243,6 +244,70 @@ await localWorkspace.ChangeSolutionAsync(localWorkspace.CurrentSolution
             $"Error CS0001: {document.FilePath}(0, 1, 0, 2): Syntax error",
         ], updates.Diagnostics.Select(Inspect));
 
+        var moduleId = Guid.NewGuid();
+        var methodId = new ManagedModuleMethodId(token: 0x06000001, version: 2);
+
+        mockEncService.EmitSolutionUpdateImpl = (solution, _, _) =>
+        {
+            var syntaxTree = solution.GetRequiredDocument(documentId).GetSyntaxTreeSynchronously(CancellationToken.None)!;
+
+            return new()
+            {
+                Solution = solution,
+                ModuleUpdates = new ModuleUpdates(
+                    ModuleUpdateStatus.Ready,
+                    [
+                        new ManagedHotReloadUpdate(
+                            moduleId,
+                            "module.dll",
+                            project.Id,
+                            ilDelta: [1],
+                            metadataDelta: [2],
+                            pdbDelta: [3],
+                            updatedTypes: [0x02000001],
+                            requiredCapabilities: ["Baseline"],
+                            updatedMethods: [0x06000002],
+                            sequencePoints: [new SequencePointUpdates("file.cs", [new SourceLineUpdate(1, 2)])],
+                            activeStatements: [new ManagedActiveStatementUpdate(methodId, ilOffset: 1, new(1, 2, 3, 4))],
+                            exceptionRegions: [new ManagedExceptionRegionUpdate(methodId, delta: 1, new(10, 20, 30, 40))])
+                    ]),
+                Diagnostics = [],
+                SyntaxError = null,
+                ProjectsToRebuild = [],
+                ProjectsToRestart = ImmutableDictionary<ProjectId, ImmutableArray<ProjectId>>.Empty,
+            };
+        };
+
+        updates = await localService.GetUpdatesAsync(runningProjects: [project.FilePath], CancellationToken.None);
+
+        Assert.Equal(++observedDiagnosticVersion, diagnosticRefresher.GlobalStateVersion);
+
+        var update = updates.Updates.Single();
+        Assert.Equal(moduleId, update.Module);
+        Assert.Equal("module.dll", update.ModuleName);
+        AssertEx.SequenceEqual([(byte)1], update.ILDelta);
+        AssertEx.SequenceEqual([(byte)2], update.MetadataDelta);
+        AssertEx.SequenceEqual([(byte)3], update.PdbDelta);
+        AssertEx.SequenceEqual([0x02000001], update.UpdatedTypes);
+        AssertEx.SequenceEqual(["Baseline"], update.RequiredCapabilities);
+        AssertEx.SequenceEqual([0x06000002], update.UpdatedMethods);
+
+        var sequencePoint = update.SequencePoints.Single();
+        Assert.Equal("file.cs", sequencePoint.FileName);
+        AssertEx.SequenceEqual(["1->2"], sequencePoint.LineUpdates.Select(u => $"{u.OldLine}->{u.NewLine}"));
+
+        var activeStatement = update.ActiveStatements.Single();
+        Assert.Equal(0x06000001, activeStatement.Method.Token);
+        Assert.Equal(2, activeStatement.Method.Version);
+        Assert.Equal(1, activeStatement.ILOffset);
+        Assert.Equal(new(1, 2, 3, 4), activeStatement.NewSpan);
+
+        var exceptionRegion = update.ExceptionRegions.Single();
+        Assert.Equal(0x06000001, exceptionRegion.Method.Token);
+        Assert.Equal(2, exceptionRegion.Method.Version);
+        Assert.Equal(1, exceptionRegion.Delta);
+        Assert.Equal(new(10, 20, 30, 40), exceptionRegion.NewSpan);
+
         Assert.True(sessionState.IsSessionActive);
 
         if (commitChanges)

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

@@ -943,7 +943,7 @@ void UpdateChangedDocumentsStaleness(bool isStale)
 
                     if (mvid == Guid.Empty)
                     {
-                        Log.Write($"Changes not applied to {newProject.Name} '{newProject.FilePath}': project not built");
+                        Log.Write($"Changes not applied to {newProject.GetLogDisplay()}: project not built");
                         UpdateChangedDocumentsStaleness(isStale: true);
                         continue;
                     }
@@ -970,7 +970,7 @@ void UpdateChangedDocumentsStaleness(bool isStale)
                     {
                         // The project is considered stale as long as it has at least one document that is out-of-sync.
                         // Treat the project the same as if it hasn't been built. We won't produce delta for it until it gets rebuilt.
-                        Log.Write($"Changes not applied to {newProject.Name} '{newProject.FilePath}': binaries not up-to-date");
+                        Log.Write($"Changes not applied to {newProject.GetLogDisplay()}: binaries not up-to-date");
 
                         projectsToStale.Add(newProject.Id);
                         UpdateChangedDocumentsStaleness(isStale: true);
@@ -999,7 +999,7 @@ void UpdateChangedDocumentsStaleness(bool isStale)
                     }
 
                     var projectSummary = GetProjectAnalysisSummary(changedDocumentAnalyses);
-                    Log.Write($"Project summary for {newProject.Name} '{newProject.FilePath}': {projectSummary}");
+                    Log.Write($"Project summary for {newProject.GetLogDisplay()}: {projectSummary}");
 
                     if (projectSummary is ProjectAnalysisSummary.NoChanges or ProjectAnalysisSummary.ValidInsignificantChanges)
                     {
@@ -1050,7 +1050,7 @@ void UpdateChangedDocumentsStaleness(bool isStale)
                         continue;
                     }
 
-                    Log.Write($"Emitting update of {newProject.Name} '{newProject.FilePath}': project not built");
+                    Log.Write($"Emitting update of {newProject.GetLogDisplay()}");
 
                     var newCompilation = await newProject.GetCompilationAsync(cancellationToken).ConfigureAwait(false);
 

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

@@ -432,7 +432,7 @@ public ImmutableArray<Diagnostic> GetPersistentDiagnostics()
             {
                 if (!diagnostic.IsEncDiagnostic())
                 {
-                    result.AddRange(diagnostics);
+                    result.Add(diagnostic);
                 }
             }
         }

src/Features/Core/Portable/EditAndContinue/Utilities/Extensions.cs

@@ -74,7 +74,7 @@ void LogReason(string message)
 
     public static string GetLogDisplay(this Project project)
         => project.FilePath != null
-            ? $"'{project.FilePath}' ('{project.State.NameAndFlavor.flavor}')"
+            ? $"'{project.FilePath}'" + (project.State.NameAndFlavor.flavor is { } flavor ? $" ('{flavor}')" : "")
             : $"'{project.Name}' ('{project.Id.DebugName}'";
 
     public static bool SupportsEditAndContinue(this TextDocumentState textDocumentState)

src/Features/Core/Portable/ExternalAccess/Watch/Api/WatchHotReloadService.cs

@@ -209,14 +209,14 @@ public async Task<Updates2> GetUpdatesAsync(Solution solution, ImmutableDictiona
             static e => new EditAndContinue.RunningProjectInfo()
             {
                 RestartWhenChangesHaveNoEffect = e.Value.RestartWhenChangesHaveNoEffect,
-                AllowPartialUpdate = true
+                AllowPartialUpdate = RequireCommit
             });
 
         var results = await _encService.EmitSolutionUpdateAsync(sessionId, solution, runningProjectsImpl, s_solutionActiveStatementSpanProvider, cancellationToken).ConfigureAwait(false);
 
         // If the changes fail to apply dotnet-watch fails.
         // We don't support discarding the changes and letting the user retry.
-        if (!RequireCommit && results.ModuleUpdates.Status is ModuleUpdateStatus.Ready)
+        if (!RequireCommit && results.ModuleUpdates.Status is ModuleUpdateStatus.Ready && results.ProjectsToRebuild.IsEmpty)
         {
             _encService.CommitSolutionUpdate(sessionId);
         }

src/Features/Test/EditAndContinue/WatchHotReloadServiceTests.cs

@@ -5,14 +5,14 @@
 #if NET
 
 using System;
+using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.IO;
 using System.Linq;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.CSharp;
-using Microsoft.CodeAnalysis.CSharp.Test.Utilities;
 using Microsoft.CodeAnalysis.ExternalAccess.Watch.Api;
 using Microsoft.CodeAnalysis.Shared.Extensions;
 using Microsoft.CodeAnalysis.Test.Utilities;
@@ -27,6 +27,16 @@ namespace Microsoft.CodeAnalysis.EditAndContinue.UnitTests;
 [UseExportProvider]
 public sealed class WatchHotReloadServiceTests : EditAndContinueWorkspaceTestBase
 {
+    private static Task<SourceText> GetCommittedDocumentTextAsync(WatchHotReloadService service, DocumentId documentId)
+        => ((EditAndContinueService)service.GetTestAccessor().EncService)
+           .GetTestAccessor()
+           .GetActiveDebuggingSessions()
+           .Single()
+           .LastCommittedSolution
+           .GetRequiredProject(documentId.ProjectId)
+           .GetRequiredDocument(documentId)
+           .GetTextAsync();
+
     [Theory]
     [CombinatorialData]
     public async Task Test(bool requireCommit)
@@ -41,7 +51,7 @@ public async Task Test(bool requireCommit)
         var source3 = "class C { void M() { System.Console.WriteLine(2); /*3*/} }";
         var source4 = "class C { void M<T>() { System.Console.WriteLine(2); } }";
         var source5 = "class C { void M() { System.Console.WriteLine(2)/* missing semicolon */ }";
-        var source6 = "class C { void M() { Unknown(); } }";
+        var source6 = "class C { void M() { Unknown(); } static C() { int x = 1; } }";
 
         var dir = Temp.CreateDirectory();
         var sourceFileA = dir.CreateFile("A.cs").WriteAllText(source1, Encoding.UTF8);
@@ -79,6 +89,7 @@ public async Task Test(bool requireCommit)
 
         var result = await hotReload.GetUpdatesAsync(solution, runningProjects: ImmutableDictionary<ProjectId, WatchHotReloadService.RunningProjectInfo>.Empty, CancellationToken.None);
         Assert.Empty(result.CompilationDiagnostics);
+        Assert.Empty(result.RudeEdits);
         Assert.Equal(1, result.ProjectUpdates.Length);
         AssertEx.Equal([0x02000002], result.ProjectUpdates[0].UpdatedTypes);
 
@@ -87,36 +98,32 @@ public async Task Test(bool requireCommit)
             hotReload.CommitUpdate();
         }
 
+        var updatedText = await GetCommittedDocumentTextAsync(hotReload, documentIdA);
+        Assert.Equal(source2, updatedText.ToString());
+
         // Insignificant change:
         solution = solution.WithDocumentText(documentIdA, CreateText(source3));
 
         result = await hotReload.GetUpdatesAsync(solution, runningProjects: ImmutableDictionary<ProjectId, WatchHotReloadService.RunningProjectInfo>.Empty, CancellationToken.None);
         Assert.Empty(result.CompilationDiagnostics);
-        Assert.Empty(result.CompilationDiagnostics);
+        Assert.Empty(result.RudeEdits);
         Assert.Empty(result.ProjectUpdates);
         Assert.Equal(WatchHotReloadService.Status.NoChangesToApply, result.Status);
 
-        var updatedText = await ((EditAndContinueService)hotReload.GetTestAccessor().EncService)
-            .GetTestAccessor()
-            .GetActiveDebuggingSessions()
-            .Single()
-            .LastCommittedSolution
-            .GetRequiredProject(documentIdA.ProjectId)
-            .GetRequiredDocument(documentIdA)
-            .GetTextAsync();
-
+        updatedText = await GetCommittedDocumentTextAsync(hotReload, documentIdA);
         Assert.Equal(source3, updatedText.ToString());
 
         // Rude edit:
         solution = solution.WithDocumentText(documentIdA, CreateText(source4));
 
         var runningProjects = ImmutableDictionary<ProjectId, WatchHotReloadService.RunningProjectInfo>.Empty
-            .Add(projectId, new WatchHotReloadService.RunningProjectInfo() { RestartWhenChangesHaveNoEffect = false });
+            .Add(projectId, new WatchHotReloadService.RunningProjectInfo() { RestartWhenChangesHaveNoEffect = true });
 
         result = await hotReload.GetUpdatesAsync(solution, runningProjects, CancellationToken.None);
+        Assert.Empty(result.CompilationDiagnostics);
         AssertEx.Equal(
-            ["P: ENC0110: " + string.Format(FeaturesResources.Changing_the_signature_of_0_requires_restarting_the_application_because_it_is_not_supported_by_the_runtime, FeaturesResources.method)],
-            result.RudeEdits.SelectMany(re => re.diagnostics.Select(d => $"{re.project.DebugName}: {d.Id}: {d.GetMessage()}")));
+            [$"P: {sourceFileA.Path}: (0,17)-(0,18): Error ENC0110: {string.Format(FeaturesResources.Changing_the_signature_of_0_requires_restarting_the_application_because_it_is_not_supported_by_the_runtime, FeaturesResources.method)}"],
+            InspectDiagnostics(result.RudeEdits));
         Assert.Empty(result.ProjectUpdates);
         AssertEx.SetEqual(["P"], result.ProjectsToRestart.Select(p => solution.GetRequiredProject(p.Key).Name));
         AssertEx.SetEqual(["P"], result.ProjectsToRebuild.Select(p => solution.GetRequiredProject(p).Name));
@@ -128,24 +135,40 @@ public async Task Test(bool requireCommit)
             hotReload.DiscardUpdate();
         }
 
+        updatedText = await GetCommittedDocumentTextAsync(hotReload, documentIdA);
+        Assert.Equal(source3, updatedText.ToString());
+
         // Syntax error:
         solution = solution.WithDocumentText(documentIdA, CreateText(source5));
 
         result = await hotReload.GetUpdatesAsync(solution, runningProjects, CancellationToken.None);
         AssertEx.Equal(
-            ["CS1002: " + CSharpResources.ERR_SemicolonExpected],
-            result.CompilationDiagnostics.Select(d => $"{d.Id}: {d.GetMessage()}"));
+            [$"{sourceFileA.Path}: (0,72)-(0,73): Error CS1002: {CSharpResources.ERR_SemicolonExpected}"],
+            InspectDiagnostics(result.CompilationDiagnostics));
         Assert.Empty(result.ProjectUpdates);
         Assert.Empty(result.ProjectsToRestart);
         Assert.Empty(result.ProjectsToRebuild);
 
-        // Semantic error:
+        updatedText = await GetCommittedDocumentTextAsync(hotReload, documentIdA);
+        Assert.Equal(source3, updatedText.ToString());
+
+        // Semantic diagnostics and no-effect edit:
         solution = solution.WithDocumentText(documentIdA, CreateText(source6));
 
         result = await hotReload.GetUpdatesAsync(solution, runningProjects, CancellationToken.None);
         AssertEx.Equal(
-            ["CS0103: " + string.Format(CSharpResources.ERR_NameNotInContext, "Unknown")],
-            result.CompilationDiagnostics.Select(d => $"{d.Id}: {d.GetMessage()}"));
+        [
+            $"{sourceFileA.Path}: (0,21)-(0,28): Error CS0103: {string.Format(CSharpResources.ERR_NameNotInContext, "Unknown")}",
+            $"{sourceFileA.Path}: (0,51)-(0,52): Warning CS0219: {string.Format(CSharpResources.WRN_UnreferencedVarAssg, "x")}",
+        ], InspectDiagnostics(result.CompilationDiagnostics));
+
+        // TODO: https://github.com/dotnet/roslyn/issues/79017
+        //AssertEx.Equal(
+        //[
+        //    $"P: {sourceFileA.Path}: (0,34)-(0,44): Warning ENC0118: {string.Format(FeaturesResources.Changing_0_might_not_have_any_effect_until_the_application_is_restarted, FeaturesResources.static_constructor)}",
+        //], InspectDiagnostics(result.RudeEdits));
+        AssertEx.Empty(result.RudeEdits);
+
         Assert.Empty(result.ProjectUpdates);
         Assert.Empty(result.ProjectsToRestart);
         Assert.Empty(result.ProjectsToRebuild);

src/Features/TestUtilities/EditAndContinue/EditAndContinueWorkspaceTestBase.cs

@@ -267,6 +267,9 @@ internal static string InspectDiagnostic(DiagnosticData diagnostic)
     internal static IEnumerable<string> InspectDiagnostics(ImmutableArray<ProjectDiagnostics> actual)
         => actual.SelectMany(pd => pd.Diagnostics.Select(d => $"{pd.ProjectId.DebugName}: {InspectDiagnostic(d)}"));
 
+    internal static IEnumerable<string> InspectDiagnostics(ImmutableArray<(ProjectId project, ImmutableArray<Diagnostic> diagnostics)> diagnostics)
+        => diagnostics.SelectMany(pd => pd.diagnostics.Select(d => $"{pd.project.DebugName}: {InspectDiagnostic(d)}"));
+
     internal static string InspectDiagnostic(Diagnostic actual)
         => $"{Inspect(actual.Location)}: {actual.Severity} {actual.Id}: {actual.GetMessage()}";