Consolidate OS detection to OsConstants#4255
Conversation
paulmedynski
added
the
Code Health 💊
There was a problem hiding this comment.
Pull request overview
This PR centralizes runtime OS detection behind a new internal OsConstants type, replacing scattered RuntimeInformation / Environment.OSVersion / OperatingSystem.IsWindows() checks to keep platform branching consistent across the driver.
Changes:
- Added
OsConstantswith cached OS flags (IsWindows,IsLinux,IsMacOS,IsFreeBSD(NET),IsUnix). - Replaced OS checks across SqlClient core, encryption providers, registry helpers, and
SqlFileStream. - Simplified some call sites by removing
ADP.IsWindowsand other ad-hoc platform detection.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/OsConstants.cs | Introduces centralized OS detection flags used by the rest of the PR. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs | Removes ADP.IsWindows and uses OsConstants for registry availability checks. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParserStaticMethods.cs | Switches Windows-only registry/NIC logic to OsConstants.IsWindows. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/TdsParser.cs | Replaces OperatingSystem.IsWindows() with OsConstants.IsWindows in SSL handshake path. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/UserAgent.cs | Uses OsConstants for OS name selection, including FreeBSD under #if NET. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/LocalAppContextSwitches.cs | Uses OsConstants in managed/native networking switch logic. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlUtil.cs | Replaces Environment.OSVersion.Platform checks with OsConstants.IsWindows for error shaping. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionCertificateStoreProvider.cs | Uses OsConstants.IsWindows to gate LocalMachine vs CurrentUser behavior. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionCngProvider.cs | Uses OsConstants for platform guards around Windows-only CNG operations. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/SqlColumnEncryptionCspProvider.cs | Uses OsConstants for platform guards around Windows-only CSP operations. |
| src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlFileStream.cs | Replaces ADP.IsWindows with OsConstants.IsUnix for platform not supported gating. |
Comments suppressed due to low confidence (1)
src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs:442
- This check uses
OsConstants.IsUnix, but the comment says "No registry in non-Windows environments". IfIsUnixremains limited to Linux/macOS/FreeBSD, this will attempt registry access on other non-Windows platforms. Preferif (!OsConstants.IsWindows)here (or ensureIsUnixis equivalent to!IsWindows).
#if NET
if (OsConstants.IsUnix)
{
// No registry in non-Windows environments
return null;
}
| /// rather than in a throw helper. | ||
| /// </remarks> | ||
| #if NETFRAMEWORK | ||
| public const bool IsWindows = true; |
There was a problem hiding this comment.
This isn't true - TFM != OS. We may happen to only support .NET Framework when running on Windows, but we shouldn't be conflating the two.
| #endif | ||
|
|
||
| /// <summary> | ||
| /// Initializes platform detection flags by querying <see cref="RuntimeInformation"/>. |
There was a problem hiding this comment.
This impacts IL trimming, which runs at publish time. The IL trimmer needs to be able to statically see that IsWindows will always be false on Linux, otherwise it'll leave the Windows-specific code (including the native SNI, with all its dependencies on the native DLL...) in the Linux app.
I've linked against the CI artifacts and confirmed that the IL trimmer can't analyze the static constructor, so I think this rules out everything besides exposing properties which returns the value of RuntimeInformation.IsOSPlatform (as ADP.IsWindows currently does - but expanded for each OS.)
Incidentally, exposing it as a field or a property also blocks .NET Framework from removing dead code paths (which in turn can consume the inlining budget), which is why IsWindows is currently a const. I've not got a strong preference there though, perhaps the performance gains are small enough that correctness is more important here.
There was a problem hiding this comment.
Good catch — confirmed the static constructor blocks the trimmer. Reworked OsConstants in 49c3f72 to expose each flag as a property that returns RuntimeInformation.IsOSPlatform(...) directly, so the trimmer can fold the guards and drop OS-specific branches (including the Windows-only native SNI graph) when published for another platform.
I also verified the UseManagedNetworking ILLink.Substitutions.xml mechanism is independent of this change — it body-stubs the getter based on the UseManagedNetworkingOnWindows feature switch at publish time, so it never reads OsConstants.
I went with properties over const to keep correctness on .NET Framework-running-on-non-Windows; per your note, the netfx dead-code-elimination tradeoff seems acceptable in exchange for correctness.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4255 +/- ##
==========================================
- Coverage 66.02% 64.41% -1.61%
==========================================
Files 277 273 -4
Lines 42988 65787 +22799
==========================================
+ Hits 28382 42379 +13997
- Misses 14606 23408 +8802
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Create new OsConstants class with IsWindows, IsLinux, IsMacOS, IsFreeBSD, and IsUnix fields - Add comprehensive XML documentation and explain why static constructor is preferred over [ModuleInitializer] (CA2255 rule) - Replace all RuntimeInformation.IsOSPlatform() calls with OsConstants fields in: - TdsParserStaticMethods.cs - UserAgent.cs - AdapterUtil.cs (now forwards to OsConstants) - Replace Environment.OSVersion.Platform checks with OsConstants in: - SqlColumnEncryptionCertificateStoreProvider.cs (3 instances) - SqlUtil.cs (3 instances) - Replace OperatingSystem static methods with OsConstants in: - LocalAppContextSwitches.cs (uses IsUnix for clearer semantics) - TdsParser.cs - Replace all ADP.IsWindows usage with OsConstants in: - SqlFileStream.cs - SqlColumnEncryptionCngProvider.cs (4 instances) - SqlColumnEncryptionCspProvider.cs (4 instances) - Remove unused AdapterUtil.IsWindows property Benefits: - Single point of control for OS detection - Improved JIT optimization through static readonly cached flags - Clearer semantics with IsUnix flag for Unix-like platforms - Consistent API surface across the codebase
- Remove OsConstants.IsUnix to avoid non-Windows ambiguity - Update Windows-only guards in SqlFileStream, CNG, CSP, and switch logic - Keep explicit OS flags (Windows/Linux/macOS/FreeBSD)
paulmedynski
force-pushed
the
dev/paul/os-constants
branch
from
f5035e4 to
8f5e2d0
Compare
Replace the static readonly fields and static constructor in OsConstants with properties that directly return RuntimeInformation.IsOSPlatform(...). The IL trimmer cannot analyze a static constructor, so the cached-field design left OS-specific code (including Windows-only native dependencies) in apps published for other platforms. Returning the intrinsic directly lets the trimmer fold each guard to a constant and drop the dead branch. Also document why the type exists (brevity and encapsulation), the inline-guard requirement for trimming, and reconcile the stale remark about JIT branch elision. Addresses PR #4255 review feedback from @edwardneal.
On modern .NET, OperatingSystem.IsWindows()/IsLinux()/IsMacOS()/IsFreeBSD() are JIT intrinsics that get constant-folded (and are recognized by the IL trimmer), so switch the OsConstants properties to them under #if NET. .NET Framework keeps RuntimeInformation.IsOSPlatform(...) since the OperatingSystem platform-check helpers do not exist there. This addresses the automated reviewer feedback that RuntimeInformation .IsOSPlatform(...) misses the JIT constant-folding optimization on net8/net9, while preserving the trimming correctness from the prior change. Addresses PR #4255 review feedback.
Summary
This PR consolidates all OS detection logic into a single internal
OsConstantsclass, giving the codebase one consistent, terse way to ask "what OS are we on?" (OsConstants.IsWindows,IsLinux,IsMacOS,IsFreeBSD) and a single seam for any future OS-specific handling.Just as importantly, the flags are implemented so they remain statically analyzable, which is what lets the JIT and the IL trimmer eliminate OS-specific branches (and their dependencies, including Windows-only native SNI) in downstream apps.
OsConstants design
net8.0/net9.0): usesOperatingSystem.IsWindows()/IsLinux()/IsMacOS()/IsFreeBSD(), which the JIT recognizes as intrinsics and constant-folds, and which the IL trimmer also understands.net462): falls back toRuntimeInformation.IsOSPlatform(...), since theOperatingSystemplatform-check helpers don't exist there.IsFreeBSDis#if NET-only.Why not static readonly fields / a static constructor?
The original approach cached the results in
static readonlyfields populated by a static constructor. That defeats trimming: the IL trimmer can't analyze a static constructor, so it can't prove a flag is constant-falseon a given OS and conservatively keeps the OS-specific code (including the native SNI DLL graph) in apps published for other platforms. Returning the intrinsic check directly preserves both the JIT constant-folding and trimmer dead-code elimination.Usage requirement (documented in the class)
For trimming to actually drop a branch, each guard must be used inline in the same method as the OS-specific code it gates. The trimmer's constant folding is shallow, so a guard hidden behind a throw-helper traps the constant inside that helper and leaves the protected code reachable. The class XML docs include do/don't examples.
Call-site replacements (10 files)
Consolidated assorted ad-hoc OS checks onto
OsConstants:RuntimeInformation.IsOSPlatform()/OperatingSystem.IsWindows().RuntimeInformation.IsOSPlatform()and added FreeBSD detection.ADP.IsWindowsmember;OsConstantsis now the single source.ADP.IsWindows.Environment.OSVersion-based checks.All existing guards were verified to be inline with the code they protect (no throw-helper indirection), so they remain trimmer-foldable.
Benefits
OperatingSystem.Is*()intrinsics.IsWindows = true).Notes
UseManagedNetworkingIL trimming substitution (ILLink.Substitutions.xml) is independent of this change — it body-stubs the getter based on theUseManagedNetworkingOnWindowsfeature switch and never readsOsConstants.