Skip to content

Migrate Csc to multithreaded MSBuild execution - #84701

Merged
jaredpar merged 45 commits into
dotnet:mainfrom
jaredpar:fix-csc-task
Aug 26, 2026
Merged

Migrate Csc to multithreaded MSBuild execution#84701
jaredpar merged 45 commits into
dotnet:mainfrom
jaredpar:fix-csc-task

Conversation

@jaredpar

@jaredpar jaredpar commented Jul 30, 2026

Copy link
Copy Markdown
Member

This is an alternate approach to #84500

Microsoft Reviewers: Open in CodeFlow

JanProvaznik and others added 11 commits July 9, 2026 11:05
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…for 18.7

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 4145242b-ec7c-4ff0-bace-d006bf403feb
…host

discovery and compiler server logging.

Skip relative PATH entries instead of trying to absolutize it.
…d of

caller-owned ProcessStartInfo instances. Keep process construction consistent
across Windows and Unix
This change uses BannedSymbols to ensure we're not using problematic
APIs from the `System.Environment` type
@azure-pipelines

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

Comment thread src/Compilers/Core/Portable/InternalUtilities/CompilerOptionParseUtilities.cs Outdated
Comment thread src/Compilers/Core/MSBuildTask/ManagedCompiler.cs Outdated
@jaredpar
jaredpar changed the base branch from main to features/msbuildmt July 30, 2026 17:54
@jaredpar
jaredpar marked this pull request as ready for review July 30, 2026 18:03
@jaredpar
jaredpar requested a review from a team as a code owner July 30, 2026 18:03
Copilot AI lite review requested due to automatic review settings July 30, 2026 18:03
@jaredpar
jaredpar requested review from a team as code owners July 30, 2026 18:03
@azure-pipelines

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes broad infrastructure changes across the build (MSBuild task execution + compiler server environment capture), MSBuild workspace project loading (including file-based apps via RPC), and several related test/benchmark updates to validate multithreaded and daemonized execution scenarios.

Changes:

  • Opt Csc/Vbc into MSBuild multithreaded task execution and route task-local state through TaskEnvironment (working directory, env vars, path resolution, server startup).
  • Add RPC plumbing to support loading “file-based app” projects via MSBuild workspace/build host, including a new IFileBasedProgramService workspace service and synced FileBasedPrograms source.
  • Update rename candidate-reason handling and add/adjust LSP/unit/benchmark coverage to exercise the new concurrency model and file-based program flows.
Show a summary per file
File Description
src/Workspaces/MSBuild/Core/Rpc/RemoteProjectInstance.cs New RPC wrapper for ProjectInstance operations.
src/Workspaces/MSBuild/Core/Rpc/RemoteBuildHost.cs Adds project+instance loading overloads (virtual project support).
src/Workspaces/MSBuild/Core/MSBuild/ProjectFileExtensionRegistry.cs Recognizes file-based app entrypoints (via service).
src/Workspaces/MSBuild/Core/MSBuild/MSBuildWorkspace.cs Handles file-based apps during ApplyChanges load.
src/Workspaces/MSBuild/Core/MSBuild/MSBuildProjectLoader.cs Wires IFileBasedProgramService into loader/provider path.
src/Workspaces/MSBuild/Core/MSBuild/FileBasedProgramsProjectLoader.cs New helper to build/load file-based app projects via RPC.
src/Workspaces/MSBuild/Core/MSBuild/BuildHostProjectFileInfoProvider.cs Uses file-based loader when applicable; selects host kind.
src/Workspaces/MSBuild/Contracts/IProjectInstance.cs New RPC contract for MSBuild ProjectInstance.
src/Workspaces/MSBuild/Contracts/IBuildHost.cs Extends load APIs for virtual project + project instance.
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstanceReader.cs Uses physical file path for virtual projects.
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectInstance.cs New server-side IProjectInstance implementation.
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/ProjectFile.cs Carries PhysicalFilePath into instance reading.
src/Workspaces/MSBuild/BuildHost/MSBuild/ProjectFile/Extensions.cs Adjusts ref directive references to use physical paths.
src/Workspaces/MSBuild/BuildHost/MSBuild/Constants/MetadataNames.cs Adds metadata name for ref-directive source.
src/Workspaces/MSBuild/BuildHost/Build/ProjectBuildManager.cs Adds global properties + project instance creation support.
src/Workspaces/MSBuild/BuildHost/AbstractBuildHost.cs Plumbs global properties + instance loading through host.
src/Workspaces/CSharp/Portable/SyncedSource/README.md Marks SyncedSource as generated.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hant.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.zh-Hans.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.tr.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.ru.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.pt-BR.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.pl.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.ko.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.ja.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.it.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.fr.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.es.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.de.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/xlf/FileBasedProgramsResources.cs.xlf Updates translations/strings for FileBasedPrograms.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/Sha256Hasher.cs Adds synced hashing utility.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/ProjectLocator.cs Adds synced project discovery utility.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/MSBuildUtilities.cs Updates shared MSBuild utilities file sharing logic.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/IBuildService.cs Adds synced build-service abstraction interfaces.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/FileBasedProgramsResources.resx Adds new resource strings and locked notes.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/ExternalHelpers.cs Adds synced external helper hooks for netstandard.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/Extensions.cs Adds polyfill extensions for older targets.
src/Workspaces/CSharp/Portable/SyncedSource/FileBasedPrograms/.editorconfig Marks synced sources as generated/analyzer-suppressed.
src/Workspaces/CSharp/Portable/SyncedSource/commitid.txt Records upstream SDK commit for synced sources.
src/Workspaces/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Workspaces.csproj Embeds FileBasedPrograms resources; removes one compile item.
src/Workspaces/CSharp/Portable/FileBasedPrograms/FileBasedProgramService.cs New workspace service implementation over VirtualProjectBuilder.
src/Workspaces/CSharp/Portable/FileBasedPrograms/ExternalHelpers.cs Implements source-package hooks using Roslyn utilities.
src/Workspaces/Core/Portable/Rename/SymbolicRenameLocations.ReferenceProcessing.cs Refines candidate reason for overload-resolution failures.
src/Workspaces/Core/Portable/Rename/RenameUtilities.cs Treats overload-resolution failure similarly to member group.
src/Workspaces/Core/Portable/Microsoft.CodeAnalysis.Workspaces.csproj Links in IBuildService abstraction into Workspaces.
src/Workspaces/Core/Portable/FileBasedPrograms/IFileBasedProgramService.cs New internal service contract for file-based programs.
src/Tools/IdeCoreBenchmarks/Program.cs Ensures BenchmarkDotNet global properties don’t break graphs; returns exit code.
src/Tools/IdeCoreBenchmarks/LanguageServer/LanguageServerDaemonBenchmarks.cs New benchmarks to compare daemon vs standalone servers.
src/Tools/IdeCoreBenchmarks/LanguageServer/LanguageServerBenchmarkHost.cs New benchmark host wrapper around LSP test infra.
src/Tools/IdeCoreBenchmarks/IdeCoreBenchmarks.csproj Adds conditional LSP test project refs; excludes net472 sources.
src/Razor/src/Razor/src/Microsoft.VisualStudioCode.RazorExtension/Services/CohostConfigurationChangedService.cs Adds new VS Code setting ingestion for Razor cohost.
src/NuGet/Microsoft.CodeAnalysis.BuildClient.Package/Microsoft.CodeAnalysis.BuildClient.targets Adds shared internal utilities to build client package.
src/LanguageServer/Protocol.TestUtilities/Microsoft.CodeAnalysis.LanguageServer.Protocol.Test.Utilities.csproj Adds IVT for IdeCoreBenchmarks.
src/LanguageServer/Protocol.TestUtilities/LanguageServer/MaterializedLspWorkspace.cs New helper to materialize test workspaces on disk.
src/LanguageServer/Protocol.TestUtilities/LanguageServer/LspWorkspaceContent.cs Moves namespace + small refactors for reuse.
src/LanguageServer/Protocol.TestUtilities/LanguageServer/LspTestWorkspaces.cs Moves namespace + adds console app workspace factory.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/HostWorkspace/LanguageServerProjectSystem.cs Uses updated ProjectFileExtensionRegistry (file-based support).
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/VirtualProjectXmlProvider.cs Removes dotnet run-api-based virtual project provider.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/IFileBasedProgramServiceExtensions.cs Adds discovery cache helpers via new service.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsWorkspaceProviderFactory.cs Stops injecting removed virtual project provider.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsProjectSystem.cs Switches file-based app loading to FileBasedProgramsProjectLoader.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/FileBasedProgramsEntryPointDiscovery.cs Routes cache directory logic through new service.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer/FileBasedPrograms/CanonicalMiscellaneousFilesProjectProvider.cs Updates LoadProjectAsync call signature.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/VirtualProjectXmlProviderTests.cs Removes tests for deleted run-api provider.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerMefHost.cs Adds composition cache warming helper.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Utilities/AbstractLanguageServerHostTests.cs Adds OpenProjectsAsync helper + daemon shutdown wait.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/MiscellaneousFiles/FileBasedProgramsWorkspaceTests.cs Adds #:ref directive test coverage.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Microsoft.CodeAnalysis.LanguageServer.UnitTests.csproj Adds package ref + IVT to benchmarks.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.UnitTests/Daemon/LanguageServerProjectLoadingTests.cs Adds daemon/standalone loading coverage.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Utilities/Usings.cs Adds global using for new test utilities namespace.
src/LanguageServer/Microsoft.CodeAnalysis.LanguageServer.ProcessHost.UnitTests/Utilities/AbstractLanguageServerClientTests.cs Reuses MaterializedLspWorkspace helper.
src/Features/CSharp/Portable/SyncedSource/commitid.txt Removes old synced-source commit record.
src/Features/CSharp/Portable/Microsoft.CodeAnalysis.CSharp.Features.csproj Removes embedded resources/defines for FileBasedPrograms.
src/EditorFeatures/Test2/Rename/RenameEngineTests.CSharpConflicts.vb Adds rename regression tests for unbound/ambiguous cases.
src/EditorFeatures/Test2/Rename/InlineRenameTests.vb Updates rename flyout test to allow submit with conflicts.
src/EditorFeatures/Core/InlineRename/UI/Adornment/RenameFlyoutViewModel.cs Allows Submit when replacement text is valid (not blocked by error severity).
src/Compilers/Shared/RuntimeHostInfo.cs Adds TaskEnvironment-friendly env/path probing; skips relative PATH segments.
src/Compilers/Shared/CompilerServerLogger.cs Adds env/path delegates for task-aware logging.
src/Compilers/Shared/BuildServerConnection.cs Makes server env/process creation task-aware via env snapshots.
src/Compilers/Shared/BuildClient.cs Passes typed environment map into server build requests.
src/Compilers/Server/VBCSCompilerTests/BuildServerConnectionTests.cs Updates tests for typed env maps.
src/Compilers/CSharp/Portable/Generated/BoundNodes.xml.Generated.cs Updates BoundRefArrayAccess typing and rewriter integration.
src/Compilers/CSharp/Portable/CodeGen/Optimizer.cs Updates BoundRefArrayAccess construction.
src/Compilers/CSharp/Portable/CodeGen/EmitExpression.cs Fixes ref-array access emission path for used/unused cases.
src/Compilers/CSharp/Portable/CodeGen/EmitAddress.cs Emits address for ref-array access via ref-assignment helper.
src/Compilers/CSharp/Portable/CodeGen/CodeGenerator_HasHome.cs Adds HasHome handling for ref-array access.
src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml Makes BoundRefArrayAccess.Type non-nullable.
src/Compilers/Core/Portable/InternalUtilities/PathExtensions.cs Adds Path.IsPathFullyQualified polyfill for non-NET targets.
src/Compilers/Core/Portable/InternalUtilities/EnvironmentExtensions.cs Adds typed env snapshot + comparer/compare helpers.
src/Compilers/Core/Portable/InternalUtilities/Debug.cs Narrows banned-API suppression scope around env var read.
src/Compilers/Core/Portable/InternalUtilities/CompilerOptionParseUtilities.cs Adds delegate-based env reading (task-aware).
src/Compilers/Core/MSBuildTaskTests/VbcTests.cs Updates runtime host path expectations.
src/Compilers/Core/MSBuildTaskTests/TestUtilities/IntegrationTestBase.cs Uses new CompilerServerLogger constructor overload.
src/Compilers/Core/MSBuildTaskTests/RuntimeHostInfoTests.cs Adds task-environment PATH tests + relative PATH filtering test.
src/Compilers/Core/MSBuildTaskTests/ManagedCompilerGlobalCacheTests.cs Updates env-based feature flag plumbing.
src/Compilers/Core/MSBuildTaskTests/CscTests.cs Adds TaskEnvironment/multithreaded reference resolution tests.
src/Compilers/Core/MSBuildTask/Vbc.cs Marks Vbc as multithreaded task.
src/Compilers/Core/MSBuildTask/Utilities.cs Adds TaskEnvironment-based GetFullPathNoThrow overload.
src/Compilers/Core/MSBuildTask/Sdk/Microsoft.Build.Tasks.CodeAnalysis.Sdk.csproj Adds MT banned symbols + define for MSBuild task builds.
src/Compilers/Core/MSBuildTask/MSBuild/Microsoft.Build.Tasks.CodeAnalysis.csproj Adds MT banned symbols + define for MSBuild task builds.
src/Compilers/Core/MSBuildTask/ManagedToolTask.cs Uses TaskEnvironment for DOTNET_ROOT/LIB/working dir behavior.
src/Compilers/Core/MSBuildTask/ManagedCompiler.cs Uses TaskEnvironment env/path; updates server request + reference checks.
src/Compilers/Core/MSBuildTask/Extensions.cs Adds TaskEnvironment.GetFullPath helper.
src/Compilers/Core/MSBuildTask/Directory.Build.props Includes new Extensions.cs in task builds.
src/Compilers/Core/MSBuildTask/Csc.cs Marks Csc as multithreaded task.
src/Compilers/Core/MSBuildTask/BannedSymbols.MSBuildMT.txt Adds banned APIs list for multithreaded task safety.
eng/Version.Details.xml Bumps arcade/helix/xliff toolset versions.
eng/Version.Details.props Bumps arcade/helix/xliff props versions.
eng/validate-benchmarks.ps1 Adds IdeCoreBenchmarks runner build validation in CI.
eng/ensure-sources-synced.cs Moves synced-source location to Workspaces + includes MSBuildUtilities.
eng/common/core-templates/steps/publish-logs.yml Removes one feed token argument.
.vscode/tasks.json Adds “build Compilers.slnf with analyzers” task.
global.json Moves repo to .NET 11 preview SDK/tooling and updates Arcade/Helix versions.

Copilot's findings

  • Files reviewed: 112/118 changed files
  • Comments generated: 3

Comment thread src/Compilers/Core/MSBuildTask/ManagedCompiler.cs
Comment thread src/Compilers/Core/Portable/InternalUtilities/PathExtensions.cs
Comment thread global.json

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (6)

Previously missed (5) — in code that hasn't changed since the last review.

src/Compilers/Core/MSBuildTask/TaskEnvironmentExtensions.cs:43

  • DeleteNoThrow(string) can throw if TaskEnvironment.GetAbsolutePath(path) throws (e.g., invalid characters). That breaks the method’s no-throw intent and can surface as unexpected task failures when callers pass untrusted/metadata-derived paths. Wrap GetAbsolutePath in a try/catch as well.

This issue also appears on line 59 of the same file.

        public void DeleteNoThrow(string path) => taskEnvironment.DeleteNoThrow(taskEnvironment.GetAbsolutePath(path));

src/Compilers/Shared/StandardBuildEnvironment.cs:19

  • StandardBuildEnvironment.GetFullPath uses Path.Combine(CurrentDirectory, path), which does not guarantee a fully qualified path (e.g., Windows drive-relative paths like "C:foo" remain not-fully-qualified). This can violate callers’ assumptions (CompilerServerLogger asserts Path.IsPathFullyQualified) and change path normalization behavior. Use Path.GetFullPath with a base directory instead.
    src/Compilers/Core/MSBuildTask/MapSourceRoots.cs:85
  • This comment hard-codes a specific PR number as the removal trigger. That will become stale quickly and doesn’t describe the actual condition for removal. Prefer describing the removal condition without referencing a PR, or link to a tracking issue if one exists.
        /// <summary>
        /// This will be removed in PR https://github.com/dotnet/roslyn/pull/84421 once these changes are
        /// merged into the main branch. The TaskEnvironment.GetFullPathNoThrow method should be used instead.
        /// </summary>

src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs:15

  • File-wide #pragma warning disable RS0030 suppresses all banned-API warnings for the remainder of the file, which makes it hard to audit new/changed I/O and path usage (especially with multi-threadable task work). Prefer narrowing the suppression to the specific safe call sites (or switching those sites to TaskEnvironment-absolutized paths) so new uses are still flagged.
// https://github.com/dotnet/roslyn/issues/84893
#pragma warning disable RS0030

src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs:12

  • File-wide #pragma warning disable RS0030 suppresses all banned-API warnings for the rest of the file, which reduces the value of the banned-API guardrails for MSBuild tasks. Prefer targeted suppressions around the specific safe I/O calls (with a justification that the paths are already absolute) or update the code to absolutize before I/O where needed.
// https://github.com/dotnet/roslyn/issues/84893
#pragma warning disable RS0030

src/Compilers/Core/MSBuildTask/TaskEnvironmentExtensions.cs:59

  • FileExists(string) can throw if TaskEnvironment.GetAbsolutePath(path) throws (e.g., invalid characters). Callers like ManagedCompiler.CheckAllReferencesExistOnDisk expect invalid references to be treated as missing (MSB3104) rather than crashing the task. Consider returning false on absolutization failures.
        public bool FileExists(string path) => taskEnvironment.FileExists(taskEnvironment.GetAbsolutePath(path));
  • Files reviewed: 46/46 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Comment thread src/Compilers/Core/MSBuildTask/TaskEnvironmentExtensions.cs Outdated
#pragma warning restore RS0030 // Do not used banned APIs
}

public bool FileExists(string path) => taskEnvironment.FileExists(taskEnvironment.GetAbsolutePath(path));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly here

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes this would throw if GetAbsolutePath threw. Under the hood though that is roughly just Path.Combine(ProjectDirectory / CurrentWorkingDirectory, path). That should effectively be the cases that File.Exists(path) would also throw so this shouldn't be a behavior change (that I'm aware of).

Guessing there are likely some edge cases with malformed paths that could change exception behavior but my assumption is all of them would already be generating exceptions today.


public string CurrentDirectory => Directory.GetCurrentDirectory();
public string? GetTempPath() => Path.GetTempPath();
public string GetFullPath(string path) => Path.Combine(CurrentDirectory, path);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be using Path.GetFullPath instead of Path.Combine? I think their behavior can differ in edge cases

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal is to emulate the behavior of TaskEnvironment and that uses Path.Combine under the hood. I agree that GutFullPath is a bit ambiguous because there are, likely, subtle behavior differences. Not sure what a better name is for the API though. Can't call it GetAbsolutePath because that will conflict with the TaskEnvironment API. Thinking of calling it GetQualifiedPath.

Copilot AI review requested due to automatic review settings August 25, 2026 18:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/Compilers/Core/MSBuildTask/Vbc.cs:302

  • TaskEnvironment.GetFullPathNoThrow(...) is not string outputAssembly is redundant here: GetFullPathNoThrow returns a string and never returns null. This pattern adds noise and suggests a failure mode that can't happen. Consider simplifying the guard to just validate PdbFile and OutputAssembly.ItemSpec, then compute outputAssembly in a separate statement.
            if (string.IsNullOrEmpty(PdbFile) ||
                string.IsNullOrEmpty(OutputAssembly?.ItemSpec) ||
                TaskEnvironment.GetFullPathNoThrow(OutputAssembly!.ItemSpec) is not string outputAssembly)
            {
                return;
  • Files reviewed: 46/46 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs
@jaredpar

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 2 pipeline(s).

Copilot AI review requested due to automatic review settings August 26, 2026 02:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

src/Compilers/Core/MSBuildTask/CopyRefAssembly.cs:12

  • The file-wide #pragma warning disable RS0030 has no corresponding restore and suppresses the banned-API analyzer for the entire file, which can hide future regressions. Consider narrowing the suppression to the specific call sites that trigger RS0030, or at least adding an explicit restore once the affected code region ends.
// https://github.com/dotnet/roslyn/issues/84893
#pragma warning disable RS0030

src/Compilers/Core/MSBuildTask/Vbc.cs:303

  • TaskEnvironment.GetFullPathNoThrow(...) returns a string, so the pattern check ... is not string outputAssembly is redundant and makes the nullability/flow harder to follow (and forces a null-forgiving !). Consider capturing OutputAssembly?.ItemSpec once, validating it, then computing outputAssembly unconditionally.
            if (string.IsNullOrEmpty(PdbFile) ||
                string.IsNullOrEmpty(OutputAssembly?.ItemSpec) ||
                TaskEnvironment.GetFullPathNoThrow(OutputAssembly!.ItemSpec) is not string outputAssembly)
            {
                return;

src/Compilers/Core/MSBuildTask/GenerateMSBuildEditorConfig.cs:15

  • The file-wide #pragma warning disable RS0030 has no restore and suppresses the banned-API analyzer for the entire file. If RS0030 is only needed for a few I/O calls, it would be safer to scope the suppression tightly to those statements (or add an explicit restore after the relevant region) so new banned API usage can’t slip in unnoticed.
// https://github.com/dotnet/roslyn/issues/84893
#pragma warning disable RS0030

  • Files reviewed: 46/46 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/Compilers/Shared/IBuildEnvironment.cs
@jaredpar
jaredpar merged commit 8f0233b into dotnet:main Aug 26, 2026
25 checks passed
@jaredpar
jaredpar deleted the fix-csc-task branch August 26, 2026 18:31
@dotnet-policy-service dotnet-policy-service Bot added this to the Next milestone Aug 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area-Razor Needs API Review Needs to be reviewed by the API review council VSCode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants