Improves tracking of pending solution updates

[tmat]

Set new property ManagedHotReloadUpdates.HasPendingUpdates, which instructs the debugger that CommitUpdatesAsync or DiscardUpdatesAsync has to be called after GetUpdatesAsync.
Previously, the debugger inferred it from other properties but such implementation is brittle as it duplicates logic that can diverge.

Add EditAndContinueDiagnosticLevel that can be set via Microsoft_CodeAnalysis_EditAndContinue_DiagnosticLevel environment variable. When enabled we inspect source-generated files for changes even if there is no other change in the compilation. This is useful for diagnosing issues with misbehaving (non-deterministic) source generators.

Microsoft Reviewers: Open in CodeFlow

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[tmat]

@DustinCampbell

[Copilot]

Pull request overview

This PR updates the Edit-and-Continue / Hot Reload pipeline to more explicitly track whether a solution update batch is pending (requiring a subsequent commit/discard), and adds a diagnostic mode intended to help diagnose non-deterministic source generators by inspecting source-generated document changes even when there are no other project/compilation changes.

Changes:

• Plumbs a new SolutionAction through EmitSolutionUpdateResults and uses it to set ManagedHotReloadUpdates.HasPendingUpdates so the debugger can reliably know when it must call CommitUpdatesAsync/DiscardUpdatesAsync.
• Adds EditAndContinueDiagnosticLevel (configurable via Microsoft_CodeAnalysis_EditAndContinue_DiagnosticLevel) and uses it to control when source-generated document diffs are computed/logged.
• Updates and extends tests and test utilities to accommodate the new result fields and improve symbol edit diagnostics.

Reviewed changes

Copilot reviewed 20 out of 20 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
src/Features/TestUtilities/EditAndContinue/SemanticEditDescription.cs	Adjusts semantic edit symbol retrieval to normalize partial symbols to implementation parts.
src/Features/TestUtilities/EditAndContinue/EditAndContinueTestVerifier.cs	Improves semantic edit verification diagnostics and symbol inspection formatting.
src/Features/Test/EditAndContinue/RemoteEditAndContinueServiceTests.cs	Updates test result construction to include SolutionAction.
src/Features/Test/EditAndContinue/EmitSolutionUpdateResultsTests.cs	Updates tests for new EmitSolutionUpdateResults.Data.SolutionAction member.
src/Features/Test/EditAndContinue/EditSessionActiveStatementsTests.cs	Updates session construction to pass EditAndContinueDiagnosticLevel.
src/Features/Test/EditAndContinue/EditAndContinueWorkspaceServiceTests.cs	Updates project-diff tests to pass diagnostic level into GetProjectDifferencesAsync.
src/Features/ExternalAccess/HotReload/Api/HotReloadService.cs	Adjusts Updates.Status property declaration (init-only).
src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs	Adds a regression test for a partial-member delete/insert scenario.
src/Features/Core/Portable/EditAndContinue/SolutionAction.cs	Introduces an enum describing what action was taken/required for the solution snapshot.
src/Features/Core/Portable/EditAndContinue/EmitSolutionUpdateResults.cs	Adds SolutionAction to results and dehydration payload; initializes it in helper/empty cases.
src/Features/Core/Portable/EditAndContinue/EditSession.cs	Adds diagnostic-level-controlled source-generated diff behavior and logging; tweaks exception logging.
src/Features/Core/Portable/EditAndContinue/EditAndContinueService.cs	Reads diagnostic level from env var and threads it into DebuggingSession.
src/Features/Core/Portable/EditAndContinue/EditAndContinueDiagnosticLevel.cs	Introduces diagnostic-level enum used to gate additional validation/diff work.
src/Features/Core/Portable/EditAndContinue/DebuggingSession.cs	Computes SolutionAction from module-update status and returns it to callers.
src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs	Adds HasPendingUpdates to the contract and changes contract shape to init properties.
src/EditorFeatures/Test/EditAndContinue/EditorManagedHotReloadLanguageServiceTests.cs	Refactors test setup into a reusable context and adds coverage for solution-action handling.
src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageServiceImpl.cs	Switches pending/committed tracking to use SolutionAction and returns HasPendingUpdates; adds test accessor.
src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageService.cs	Exposes implementation internally for tests.
src/EditorFeatures/Core/EditAndContinue/Contracts/ContractWrappers.cs	Updates wrapper construction to match new init-property shapes and pass HasPendingUpdates.
eng/Packages.props	Updates Microsoft.VisualStudio.Debugger.Contracts package version.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (7)

src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs:17

• ManagedHotReloadUpdates is a DataContract used for RPC/serialization, but its members no longer specify explicit [DataMember(Name = ...)] values. Other types in Microsoft.CodeAnalysis.Contracts.EditAndContinue consistently use explicit camelCase names (e.g. "moduleName", "projectInstanceId"), so leaving these defaulted to Updates/Diagnostics/etc risks breaking the wire contract and versioning between components. Please restore explicit Name values for all members (including the newly added flag).

    [DataMember]
    public ImmutableArray<ManagedHotReloadUpdate> Updates { get; init; }

    [DataMember]
    public ImmutableArray<ManagedHotReloadDiagnostic> Diagnostics { get; init; }

src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs:26

• PR description calls out a new ManagedHotReloadUpdates.HasPendingChanges property, but the implementation adds HasPendingUpdates. If the debugger contract really expects HasPendingChanges, this will be a breaking mismatch; otherwise, please update the PR description to match the shipped API name to avoid confusion for debugger consumers.

    [DataMember]
    public bool HasPendingUpdates { get; init; }

src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageServiceImpl.cs:60

• CommittedSolution now raises SolutionCommitted from the property setter, but the event invocation is not protected by FatalError.ReportAndCatch(...). Previously, event invocation in CommitUpdatesAsync was wrapped to avoid a subscriber exception taking down Hot Reload. With the current setter, a throwing subscriber can escape (and can also cause double-raising when CommitUpdatesAsync still invokes the event explicitly). Consider adding the same guarded invocation pattern to the setter (and then removing the explicit invocation in CommitUpdatesAsync).

            if (value != null)
            {
                SolutionCommitted?.Invoke(value);
            }
        }

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

• Typo in the SolutionAction property doc comment: "Action taked on the solution" should be "Action taken on the solution".

    /// <summary>
    /// Action taked on the solution.
    /// </summary>
    public required SolutionAction SolutionAction { get; init; }

src/Features/Core/Portable/EditAndContinue/EditAndContinueDiagnosticLevel.cs:17

• Grammar in the Debug value doc comment: "should only be used to when diagnosing" should be "should only be used when diagnosing".

    /// Adds extra validation that is normally not performed due to its impact on performance
    /// and should only be used to when diagnosing issues with EnC.
    /// </summary>

src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageServiceImpl.cs:187

• With CommittedSolution now raising SolutionCommitted from its setter, the explicit SolutionCommitted?.Invoke(committedSolution) call in CommitUpdatesAsync will cause duplicate notifications (and uses different exception-handling semantics). After moving guarded invocation into the setter, this explicit invocation should be removed to ensure the event fires exactly once per commit.

        {
        }

        CommittedSolution = committedSolution;

src/Features/Core/Portable/EditAndContinue/EditAndContinueService.cs:49

• DiagnosticLevel parses a raw byte and casts it to EditAndContinueDiagnosticLevel without validating the value is defined. Any non-zero value (including typos like 2) will enable diagnostic-mode behavior. Consider parsing/validating against the enum to keep behavior predictable and to allow named values (e.g. Debug).

    private static EditAndContinueDiagnosticLevel DiagnosticLevel
        => byte.TryParse(Environment.GetEnvironmentVariable("Microsoft_CodeAnalysis_EditAndContinue_DiagnosticLevel"), out var level)
            ? (EditAndContinueDiagnosticLevel)level
            : EditAndContinueDiagnosticLevel.None;

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Suppressed comments (8)

src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs:17

• Data contract members in this folder consistently specify explicit camelCase DataMember names (e.g., "moduleName", "projectsToRestart"). Leaving ManagedHotReloadUpdates without Name changes the serialized field names (e.g., "Updates" instead of "updates"), which can break wire compatibility with existing debugger/service consumers.

    [DataMember]
    public ImmutableArray<ManagedHotReloadUpdate> Updates { get; init; }

    [DataMember]
    public ImmutableArray<ManagedHotReloadDiagnostic> Diagnostics { get; init; }

src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs:26

• PR description mentions a new property ManagedHotReloadUpdates.HasPendingChanges, but the implementation introduces HasPendingUpdates. If the intent is to align with the debugger contract/property name in the description, this mismatch should be resolved (either rename the property everywhere, or update the PR description/contract expectations).

    [DataMember]
    public bool HasPendingUpdates { get; init; }

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

• XML doc comment typo: "Action taked" should be "Action taken".

    /// <summary>
    /// Action taked on the solution.
    /// </summary>
    public required SolutionAction SolutionAction { get; init; }

src/Features/Core/Portable/EditAndContinue/EditAndContinueDiagnosticLevel.cs:17

• Grammar in the enum doc comment: "should only be used to when diagnosing" reads incorrectly.

    /// <summary>
    /// Adds extra validation that is normally not performed due to its impact on performance
    /// and should only be used to when diagnosing issues with EnC.
    /// </summary>

src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs:9367

• Trailing whitespace after editScripts: is visible here and will violate Roslyn's no-trailing-whitespace rule.

            editScripts: 

src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageServiceImpl.cs:193

• CommitUpdatesAsync invokes SolutionCommitted explicitly and then assigns CommittedSolution, whose setter also invokes SolutionCommitted. This results in the event firing twice for a single commit.

        {
        }

        CommittedSolution = committedSolution;

src/Features/Core/Portable/EditAndContinue/EditAndContinueService.cs:49

• The environment variable parsing allows any byte value and casts it to EditAndContinueDiagnosticLevel, including undefined values. It would be safer to validate the parsed value (or map only known values) to avoid accidentally enabling unsupported modes.

    private static EditAndContinueDiagnosticLevel DiagnosticLevel
        => byte.TryParse(Environment.GetEnvironmentVariable("Microsoft_CodeAnalysis_EditAndContinue_DiagnosticLevel"), out var level)
            ? (EditAndContinueDiagnosticLevel)level
            : EditAndContinueDiagnosticLevel.None;

src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs:9365

• Test name suffix "2" is ambiguous; consider a descriptive name that captures what differs from PartialMember_DeleteInsert_AddFieldInitializer (e.g., involving partial method + constructor update).

    public void PartialMember_DeleteInsert_AddFieldInitializer2()

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageServiceImpl.cs:60

• CommittedSolution setter invokes SolutionCommitted directly, which (1) can fire during StartSessionAsync (initial baseline assignment) and (2) can allow exceptions from handlers to escape. This event appears intended to signal committed updates, so it should only fire after the initial baseline is already set, and should be wrapped in FatalError.ReportAndCatch like other SolutionCommitted forwarding paths.

    private Solution? CommittedSolution
    {
        get;
        set
        {
            field = value;

            if (value != null)
            {
                SolutionCommitted?.Invoke(value);
            }
        }

src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs:26

• This contracts namespace consistently specifies camelCase JSON field names via DataMember(Name = ...). Dropping the Name arguments here will change the serialized field names (e.g. Updates instead of updates) and risks breaking contract compatibility. Please restore explicit Name values and add one for the new HasPendingUpdates field.

    [DataMember]
    public ImmutableArray<ManagedHotReloadUpdate> Updates { get; init; }

    [DataMember]
    public ImmutableArray<ManagedHotReloadDiagnostic> Diagnostics { get; init; }

    [DataMember]
    public ImmutableArray<ProjectInstanceId> ProjectsToRebuild { get; init; }

    [DataMember]
    public ImmutableArray<ProjectInstanceId> ProjectsToRestart { get; init; }

    [DataMember]
    public bool HasPendingUpdates { get; init; }

src/Features/Core/Portable/EditAndContinue/EditAndContinueService.cs:49

• DiagnosticLevel currently accepts any byte value and casts it to EditAndContinueDiagnosticLevel, which means invalid values (e.g. 255) will silently enable the expensive diagnostic path. Please validate the parsed value against the defined enum values and fall back to None when invalid.

    private static EditAndContinueDiagnosticLevel DiagnosticLevel
        => byte.TryParse(Environment.GetEnvironmentVariable("Microsoft_CodeAnalysis_EditAndContinue_DiagnosticLevel"), out var level)
            ? (EditAndContinueDiagnosticLevel)level
            : EditAndContinueDiagnosticLevel.None;

src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs:9365

• The new test name ends with a numeric suffix (...AddFieldInitializer2), which makes it hard to understand what scenario differs from the existing ...AddFieldInitializer test. Please rename it to reflect the specific additional behavior being validated (e.g. involving the partial method implementation/body change).

    public void PartialMember_DeleteInsert_AddFieldInitializer2()

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/Features/Core/Portable/Contracts/EditAndContinue/ManagedHotReloadUpdates.cs:17

• [DataMember] attributes here omit explicit Name=... values, unlike the rest of the EditAndContinue contracts in this folder (which consistently use camelCase names). This changes the serialized field names (e.g. "Updates" vs "updates"), which can break RPC/version tolerance for the contract. Please restore explicit DataMember(Name=...) for existing members and assign a name for the new HasPendingUpdates member as well.

    [DataMember]
    public ImmutableArray<ManagedHotReloadUpdate> Updates { get; init; }

    [DataMember]
    public ImmutableArray<ManagedHotReloadDiagnostic> Diagnostics { get; init; }

src/EditorFeatures/Core/EditAndContinue/ManagedHotReloadLanguageServiceImpl.cs:59

• CommittedSolution setter now invokes SolutionCommitted without any exception handling. Since GetUpdatesAsync doesn’t wrap its body in a FatalError catch, an exception from an event handler would escape into the debugger/RPC layer and could destabilize the session. This used to be guarded in CommitUpdatesAsync; please keep the invocation guarded (and consistent with XamlEditAndContinueSolutionProvider).

            field = value;

            if (value != null)
            {
                SolutionCommitted?.Invoke(value);
            }

src/Features/Core/Portable/EditAndContinue/EditAndContinueService.cs:49

• DiagnosticLevel casts an arbitrary byte from an environment variable to EditAndContinueDiagnosticLevel without validating that the value is defined. Unexpected values (e.g. 2/255) will silently enable an undefined mode. Consider validating with Enum.IsDefined and defaulting to None for unknown values.

    private static EditAndContinueDiagnosticLevel DiagnosticLevel
        => byte.TryParse(Environment.GetEnvironmentVariable("Microsoft_CodeAnalysis_EditAndContinue_DiagnosticLevel"), out var level)
            ? (EditAndContinueDiagnosticLevel)level
            : EditAndContinueDiagnosticLevel.None;

src/Features/CSharpTest/EditAndContinue/TopLevelEditingTests.cs:9365

• Test name "PartialMember_DeleteInsert_AddFieldInitializer2" is not descriptive (the trailing "2" doesn’t indicate what scenario differs from the existing AddFieldInitializer test). Please rename to reflect the distinguishing condition (e.g. that this involves a partial method and constructor interaction).

    [Fact]
    public void PartialMember_DeleteInsert_AddFieldInitializer2()
        => EditAndContinueValidation.VerifySemantics(

[DustinCampbell]

I noticed the same thing as Copilot -- there's no longer a try/catch to report exceptions. Did that turn out to be unnecessary?

[tmat]

That was actually a good catch (pun intended ;).

I ended up moving the catch here: https://github.com/dotnet/roslyn/pull/84726/changes#diff-1b2bd8e67dfecf6bcecc504f88921d6e1031d5993291c5d063d384ebaafbda19R47 since that's where we actually call external code we don't control.