Skip to content

Consolidate OS detection to OsConstants#4255

Draft
paulmedynski wants to merge 2 commits intomainfrom
dev/paul/os-constants
Draft

Consolidate OS detection to OsConstants#4255
paulmedynski wants to merge 2 commits intomainfrom
dev/paul/os-constants

Conversation

@paulmedynski
Copy link
Copy Markdown
Contributor

@paulmedynski paulmedynski commented May 1, 2026

Summary

This PR consolidates all OS detection logic into a centralized class, improving JIT optimization opportunities and providing a single point of control for platform detection.

Changes

New Class

  • OsConstants.cs - New internal class with OS detection flags:
      • Detect Windows OS
      • Detect Linux OS
      • Detect macOS OS
      • Detect FreeBSD OS (NET only)

Implementation Details

  • Uses static readonly fields for JIT branch elimination optimization
  • Static constructor instead of [ModuleInitializer] to avoid CA2255 security concerns
  • Comprehensive XML documentation

Replacements (11 files)

  • TdsParserStaticMethods.cs - Replaced RuntimeInformation.IsOSPlatform()
  • UserAgent.cs - Replaced RuntimeInformation.IsOSPlatform() and added FreeBSD support
  • AdapterUtil.cs - Removed unused IsWindows property, now single source is OsConstants
  • SqlColumnEncryptionCertificateStoreProvider.cs - Replaced Environment.OSVersion checks (3 instances)
  • SqlUtil.cs - Replaced Environment.OSVersion checks (3 instances)
  • LocalAppContextSwitches.cs - Replaced OperatingSystem.IsWindows() with OsConstants.IsWindows
  • TdsParser.cs - Replaced OperatingSystem.IsWindows()
  • SqlFileStream.cs - Replaced ADP.IsWindows
  • SqlColumnEncryptionCngProvider.cs - Replaced ADP.IsWindows (4 instances)
  • SqlColumnEncryptionCspProvider.cs - Replaced ADP.IsWindows (4 instances)

Benefits

  • Single point of control for OS detection
  • Improved JIT optimization through static readonly cached flags
  • Consistent API surface across the codebase
  • Removed unnecessary indirection through ADP.IsWindows

- 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
Copilot AI review requested due to automatic review settings May 1, 2026 15:36
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board May 1, 2026
@paulmedynski paulmedynski added this to the 7.1.0-preview2 milestone May 1, 2026
@paulmedynski paulmedynski moved this from To triage to In progress in SqlClient Board May 1, 2026
@paulmedynski paulmedynski added the Code Health 💊 Issues/PRs that are targeted to source code quality improvements. label May 1, 2026
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

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 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 OsConstants with 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.IsWindows and 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". If IsUnix remains limited to Linux/macOS/FreeBSD, this will attempt registry access on other non-Windows platforms. Prefer if (!OsConstants.IsWindows) here (or ensure IsUnix is equivalent to !IsWindows).
            #if NET
            if (OsConstants.IsUnix)
            {
                // No registry in non-Windows environments
                return null;
            }

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/OsConstants.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlTypes/SqlFileStream.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Outdated
/// rather than in a throw helper.
/// </remarks>
#if NETFRAMEWORK
public const bool IsWindows = true;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This isn't strictly true - you can run .NET Framework on non-Windows.

We're conflating .NET Framework and Windows elsewhere, and we will deal with that later.

Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/Common/AdapterUtil.cs Outdated
Comment thread src/Microsoft.Data.SqlClient/src/Microsoft/Data/SqlClient/OsConstants.cs Outdated
- 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)
#endif

/// <summary>
/// Initializes platform detection flags by querying <see cref="RuntimeInformation"/>.
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.

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.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 1, 2026

Codecov Report

❌ Patch coverage is 93.10345% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 64.41%. Comparing base (7a01dbe) to head (f5035e4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...qlClient/src/Microsoft/Data/SqlClient/UserAgent.cs 75.00% 1 Missing ⚠️
...lient/src/Microsoft/Data/SqlTypes/SqlFileStream.cs 0.00% 1 Missing ⚠️
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     
Flag Coverage Δ
CI-SqlClient ?
PR-SqlClient-Project 64.41% <93.10%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

Code Health 💊 Issues/PRs that are targeted to source code quality improvements.

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

3 participants