Move pollyfills for !NET targets to src/libraries/Common/src/Polyfills/#126287
Move pollyfills for !NET targets to src/libraries/Common/src/Polyfills/#126287
Conversation
…e IsFinite - Revert OperatingSystem.Is* polyfills (keep #if NET guards) - Replace more 0x7FFFFFC7 constants with Array.MaxLength via polyfill (FrozenHashTable 2nd occurrence, CborWriter, JsonDocument.MetadataDb) - Inline JsonHelpers.IsFinite into call sites (double.IsFinite/float.IsFinite) - Inline Base2ExponentialHistogramAggregator.IsFinite into call sites Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split into IReadOnlySetPolyfill.cs and BitOperationsPolyfill.cs. Both use #if !NET guards since the types don't exist on downlevel. Update both src and test csproj references. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Split into BitConverterPolyfills.cs and GuidPolyfills.cs for reuse across libraries. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @dotnet/area-meta |
There was a problem hiding this comment.
Pull request overview
Moves downlevel polyfills into src/libraries/Common/src/Polyfills/ and updates multiple libraries to call the “modern” APIs unconditionally (e.g., Stopwatch.GetElapsedTime, double.IsFinite, Array.MaxLength), relying on polyfills for non-.NETCoreApp targets to reduce #if usage.
Changes:
- Replaced many
#if NET/#elsecode paths with unified API calls and added corresponding polyfills via$(CommonPath)Polyfills\*.cs. - Removed library-local polyfill helpers (e.g.,
JsonHelpers.IsFinite,System.Collections.Immutable’sSystem/Polyfills.cs) in favor of shared Common polyfills. - Updated project files to link in the required polyfill sources for non-
.NETCoreApptarget frameworks.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Threading.RateLimiting/src/System/Threading/RateLimiting/RateLimiterHelper.cs | Uses Stopwatch.GetElapsedTime unconditionally (polyfilled for downlevel). |
| src/libraries/System.Threading.RateLimiting/src/System.Threading.RateLimiting.csproj | Links StopwatchPolyfills.cs for non-.NETCoreApp builds. |
| src/libraries/System.Text.Json/src/System/Text/Json/Writer/JsonWriterHelper.cs | Switches finite checks to double.IsFinite / float.IsFinite. |
| src/libraries/System.Text.Json/src/System/Text/Json/Reader/Utf8JsonReader.TryGet.cs | Switches finite checks to double.IsFinite / float.IsFinite. |
| src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs | Removes local IsFinite helpers and BitArray.HasAllSet polyfill. |
| src/libraries/System.Text.Json/src/System/Text/Json/Document/JsonDocument.MetadataDb.cs | Uses Array.MaxLength instead of hardcoded constant. |
| src/libraries/System.Text.Json/src/System.Text.Json.csproj | Links Double/Single/BitArray/Array polyfills for non-.NETCoreApp. |
| src/libraries/System.Speech/src/System.Speech.csproj | Links StreamPolyfills.cs for non-.NETCoreApp. |
| src/libraries/System.Speech/src/Internal/Synthesis/EngineSite.cs | Replaces manual read loop with Stream.ReadExactly. |
| src/libraries/System.Speech/src/Internal/Synthesis/AudioBase.cs | Replaces manual read loop with Stream.ReadExactly. |
| src/libraries/System.Reflection.Metadata/src/System.Reflection.Metadata.csproj | Replaces in-project polyfills with linked Common BitConverter/Guid polyfills. |
| src/libraries/System.IO.Hashing/src/System/IO/Hashing/NonCryptographicHashAlgorithm.cs | Uses CopyToAsync(destination, cancellationToken) overload unconditionally (polyfilled downlevel). |
| src/libraries/System.IO.Hashing/src/System.IO.Hashing.csproj | Links StreamPolyfills.cs for non-.NETCoreApp. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Writer/CborWriter.cs | Uses Array.MaxLength unconditionally. |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/Writer/CborWriter.Tag.cs | Uses DateTimeOffset.TotalOffsetMinutes unconditionally (polyfilled downlevel). |
| src/libraries/System.Formats.Cbor/src/System/Formats/Cbor/CborConformanceLevel.cs | Uses HashCode.AddBytes unconditionally (polyfilled downlevel). |
| src/libraries/System.Formats.Cbor/src/System.Formats.Cbor.csproj | Links DateTimeOffset/HashCode/Array polyfills for non-.NETCoreApp. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/Metrics/Base2ExponentialHistogramAggregator.cs | Uses double.IsFinite directly; removes local helper. |
| src/libraries/System.Diagnostics.DiagnosticSource/src/System.Diagnostics.DiagnosticSource.csproj | Links DoublePolyfills.cs for non-.NETCoreApp. |
| src/libraries/System.Collections.Immutable/tests/System.Collections.Immutable.Tests.csproj | Replaces product-local polyfills include with linked Common polyfills in tests. |
| src/libraries/System.Collections.Immutable/src/System/Polyfills.cs | Removes library-local polyfills file. |
| src/libraries/System.Collections.Immutable/src/System/Collections/Frozen/FrozenHashTable.cs | Uses Array.MaxLength unconditionally. |
| src/libraries/System.Collections.Immutable/src/System.Collections.Immutable.csproj | Removes System\Polyfills.cs; links Common polyfills for non-.NETCoreApp. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/SimpleConsoleFormatter.cs | Simplifies conditional IsAndroidOrAppleMobile expression body. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/Microsoft.Extensions.Logging.Console.csproj | Links TextWriter/Encoding polyfills for non-.NETCoreApp. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/JsonConsoleFormatter.cs | Removes unsafe GetChars fallback in favor of polyfilled overloads; simplifies char handling. |
| src/libraries/Microsoft.Extensions.Logging.Console/src/AnsiParsingLogConsole.cs | Uses TextWriter.Write(ReadOnlySpan<char>) unconditionally (polyfilled downlevel). |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs | Uses Environment.ProcessId unconditionally (polyfilled downlevel). |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/Microsoft.Extensions.Hosting.Systemd.csproj | Links EnvironmentPolyfills.cs for non-.NETCoreApp. |
| src/libraries/Common/src/Polyfills/TextWriterPolyfills.cs | Adds downlevel TextWriter.Write(ReadOnlySpan<char>). |
| src/libraries/Common/src/Polyfills/StreamPolyfills.cs | Adds downlevel Stream.ReadExactly(byte[]) and CopyToAsync(Stream, CancellationToken). |
| src/libraries/Common/src/Polyfills/StopwatchPolyfills.cs | Adds downlevel Stopwatch.GetElapsedTime overloads. |
| src/libraries/Common/src/Polyfills/SinglePolyfills.cs | Adds downlevel float.IsFinite. |
| src/libraries/Common/src/Polyfills/DoublePolyfills.cs | Adds downlevel double.IsFinite. |
| src/libraries/Common/src/Polyfills/BitArrayPolyfills.cs | Adds downlevel BitArray.HasAllSet(). |
| src/libraries/Common/src/Polyfills/ArrayPolyfills.cs | Adds downlevel Array.MaxLength. |
| src/libraries/Common/src/Polyfills/DateTimeOffsetPolyfills.cs | Adds downlevel DateTimeOffset.TotalOffsetMinutes. |
| src/libraries/Common/src/Polyfills/HashCodePolyfills.cs | Adds downlevel HashCode.AddBytes(ReadOnlySpan<byte>) as an extension method. |
| src/libraries/Common/src/Polyfills/EnvironmentPolyfills.cs | Adds downlevel Environment.ProcessId. |
| src/libraries/Common/src/Polyfills/GuidPolyfills.cs | Adds downlevel Guid.TryWriteBytes(Span<byte>). |
| src/libraries/Common/src/Polyfills/BitConverterPolyfills.cs | Adds downlevel BitConverter.SingleToUInt32Bits / DoubleToUInt64Bits. |
| src/libraries/Common/src/Polyfills/IReadOnlySetPolyfill.cs | Adds downlevel IReadOnlySet<T> (guarded by #if !NET). |
| src/libraries/Common/src/Polyfills/BitOperationsPolyfill.cs | Adds downlevel System.Numerics.BitOperations.RotateLeft (guarded by #if !NET). |
| src/libraries/Common/src/Polyfills/CollectionsPolyfills.cs | Adds downlevel KeyValuePair.Deconstruct extension method. |
| src/libraries/Common/src/Polyfills/DictionaryPolyfills.cs | Adds downlevel TryAdd / TryDequeue polyfills (new shared source). |
This comment has been minimized.
This comment has been minimized.
Leverage polyfills already in Common/src/Polyfills/ to remove #if NET blocks across additional libraries: - ArrayPolyfills: LengthBuckets.cs, ArrayInfo.cs (Nrbf) - IReadOnlySetPolyfill: ImmutableSortedSet, ImmutableHashSet, FrozenSet - StreamPolyfills: SoundPlayer.cs, XmlBuffer.cs (Syndication) - DateTimeOffsetPolyfills: CacheEntry.cs, Rss20FeedFormatter, Atom10FeedFormatter - HashCodePolyfills: CoseHeaderValue.cs - EnvironmentPolyfills: Win32.cs (Hosting.WindowsServices), SystemdHelpers.cs cleanup Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- EnvironmentPolyfills: dispose Process instance to avoid handle leak - DoublePolyfills/SinglePolyfills: use same bit-manipulation impl as the real Double.IsFinite/Single.IsFinite in CoreLib - IReadOnlySetPolyfill: add comment explaining why #if !NET is needed - StreamPolyfills: add comment explaining the 81_920 buffer size constant Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
New polyfill: RuntimeHelpersPolyfills.cs provides: - TryEnsureSufficientExecutionStack (wraps EnsureSufficientExecutionStack) - GetUninitializedObject (delegates to FormatterServices) Cleaned up in System.Text.Json: - StackHelper.cs: remove #if NET for TryEnsureSufficientExecutionStack - FSharpUnionConverter.cs: remove #if NET for GetUninitializedObject and dead conditional using Skipped (not polyfillable with extension() blocks): - JsonArray.cs: List.AddRange(ReadOnlySpan) and uint.TryFormat - CharConverter.cs: new ReadOnlySpan<char>(in value) constructor - JsonCamelCaseNamingPolicy.cs: string.Create (shared with generators) - JsonReaderHelper.Unescaping.cs: Utf8.IsValid (different error handling) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
...libraries/Microsoft.Extensions.Caching.Memory/src/Microsoft.Extensions.Caching.Memory.csproj
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Console/src/JsonConsoleFormatter.cs
Outdated
Show resolved
Hide resolved
...braries/Microsoft.Extensions.Logging.Console/src/Microsoft.Extensions.Logging.Console.csproj
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/StackHelper.cs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
- Fix stray carriage return in Caching.Memory csproj - JsonConsoleFormatter: use stackalloc span for char writing instead of ToString() to avoid allocation regression (copilot review) - Delete StackHelper.cs, inline RuntimeHelpers.TryEnsureSufficientExecutionStack call directly in JsonElement.cs (jkotas) - Revert IReadOnlySet polyfill: restore #if NET guards on FrozenSet, ImmutableHashSet, ImmutableSortedSet; keep IReadOnlySet<T> definition in library-local Polyfills.cs (jkotas concern about subtle bugs) - Rename BitOperationsPolyfill.cs -> BitOperationsPolyfills.cs for consistent plural naming across all polyfill files - Remove dead StreamPolyfills ref from System.Windows.Extensions (only targets NETCoreApp, polyfill would never compile) - Fix EncodingPolyfills Link path for directory structure consistency Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
- Fully revert all changes to SimpleConsoleFormatter.cs - JsonConsoleFormatter: use [charValue] collection literal instead of stackalloc + manual assignment Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
The Libraries_NET481 CI leg fails because extension() blocks on primitive types may not resolve correctly under the VS MSBuild toolchain. Restore the original #if NET / #else pattern for Base2ExponentialHistogramAggregator.IsFinite. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Jan Kotas <jkotas@microsoft.com>
…into common-pollyfills
This comment has been minimized.
This comment has been minimized.
- StopwatchPolyfills.cs: add missing 'using System' for TimeSpan - RuntimeHelpersPolyfills.cs: add missing 'using System' for Type - CollectionsPolyfills.cs: remove unused 'using System.ComponentModel' - Delete DictionaryPolyfills.cs: unreferenced dead code, duplicates test DictionaryExtensions.cs, Queue polyfill misplaced (jkotas) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Would it make more sense to delete src\libraries\Common\tests\System\Collections\DictionaryExtensions.cs and move the polyfill implementation into the Polyfill directory? |
What's the problem with this? Why is use of polyfills conflicting with VS MSBuild? |
Sure, will do. This PR doesn't move all of them; there are still tons of in-place polyfills we can move there (some of them are quite complex) - definitely too many for a single PR to handle. My main motivation is to be able to move unsafe code under the rug and out of libraries due to the lack of safe APIs in old targets |
This comment has been minimized.
This comment has been minimized.
…lls.cs Per jkotas feedback: move the TryAdd polyfill from Common/tests/ to Common/src/Polyfills/ so it lives alongside other polyfills. Update all 5 test projects that referenced the old file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
…lyfill Convert all src/libraries/Common/src/Polyfills/*.cs to file-scoped namespace declarations. Restore DoublePolyfills usage in Base2ExponentialHistogramAggregator (revert the revert). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🤖 Copilot Code Review — PR #126287Note This review was generated by GitHub Copilot. Holistic AssessmentMotivation: Justified. Duplicate Approach: Sound. Moving polyfills to Summary: ✅ LGTM. The refactoring is mechanically correct, all source-project csproj conditions are properly guarded ( Detailed Findings✅ Correctness — csproj conditional includes are all correctVerified every source-project polyfill include. All are inside Test projects ( ✅ Correctness — Code transformations are mechanically faithfulSpot-checked all major replacements:
✅ Correctness — Unsafe code properly enabled
✅ Dead code cleanup
💡 Missing trailing newlines on all 16 new polyfill filesAll files in
|
This allows us to pretend new APIs exist for .NET Standard 2.0/NETFX/etc targets and avoid
#if NET elsemess.