Skip to content

[WIP] Fix nullability mismatch for environment overrides#13814

Closed
Copilot wants to merge 1 commit into
mainfrom
copilot/fix-nullability-mismatch-environment-overrides
Closed

[WIP] Fix nullability mismatch for environment overrides#13814
Copilot wants to merge 1 commit into
mainfrom
copilot/fix-nullability-mismatch-environment-overrides

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 19, 2026

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.


This section details on the original issue you should resolve

<issue_title>Nullability mismatch: NodeLaunchData.EnvironmentOverrides should be IDictionary<string, string?>?</issue_title>
<issue_description>> Body of this issue authored by GitHub Copilot at @rainersigwald's request.

Problem

DotnetHostEnvironmentHelper.CreateDotnetRootEnvironmentOverrides() returns IDictionary<string, string?>?. The null values in the dictionary are intentional sentinels meaning "remove this variable from the child process environment" — they're consumed by DotnetHostEnvironmentHelper.ApplyEnvironmentOverrides, which calls environment.Remove(key) when the value is null. This is how the helper clears the architecture-specific overrides (e.g. DOTNET_ROOT_X64) that would otherwise take precedence over DOTNET_ROOT.

But NodeLaunchData.EnvironmentOverrides (INodeLauncher.cs:28) is declared as IDictionary<string, string>? — i.e., the dictionary itself can be null but its values cannot. The two existing call sites both have to paper over the mismatch:

Both call sites are correct in practice (because the consumer chain ultimately routes through ApplyEnvironmentOverrides, which honours the null sentinels), but the type signature lies about what the API contract actually is.

Proposed fix

Change NodeLaunchData.EnvironmentOverrides — and any consumer types it flows through (INodeLauncher.Start, NodeLauncher.Start, Process start-info wiring on the receiving side, etc.) — to IDictionary<string, string?>? so the null-as-sentinel semantics are encoded in the type system. Then drop the ! in MSBuildClient.cs and the implicit reliance on #nullable disable in NodeProviderOutOfProc.cs.

This will surface any consumer that's currently mishandling null values (e.g., enumerating with KeyValuePair<string, string> instead of KeyValuePair<string, string?> and skipping the Remove path).

Acceptance

  • NodeLaunchData.EnvironmentOverrides is typed IDictionary<string, string?>?.
  • Neither call site uses ! or #nullable disable to assign to it.
  • All consumers either set or remove the variable based on whether the value is null, matching ApplyEnvironmentOverrides.
  • No behavior change at runtime; this is a type-system tightening only.

Context

Surfaced while reviewing #13716 (the DOTNET_CLI_USE_MSBUILD_SERVER=true server-launch fix). That PR added a temporary block comment in MSBuildClient.cs describing the suppression; this issue tracks doing it properly so the comment can be removed.
</issue_description>

Comments on the Issue (you are @copilot in this section)

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Hello @copilot, I noticed that you’re changing an .swr file or any file under src/Package/MSBuild.VSSetup.. Please make sure to validate this change by an experimental VS insertion. This is accomplished by pushing to an exp/* branch, which requires write permissions to this repo.

Copilot stopped work on behalf of OvesN due to an error May 19, 2026 13:00
Copilot AI requested a review from OvesN May 19, 2026 13:00
@JanProvaznik
Copy link
Copy Markdown
Member

this agent session was killed before producing output

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Nullability mismatch: NodeLaunchData.EnvironmentOverrides should be IDictionary<string, string?>?

3 participants