Skip to content

Add polyfills for RandomNumberGenerator class and Fill(Span<byte>) method#76

Merged
Tyrrrz merged 12 commits intomasterfrom
copilot/update-pr-73-with-reviews
Feb 17, 2026
Merged

Add polyfills for RandomNumberGenerator class and Fill(Span<byte>) method#76
Tyrrrz merged 12 commits intomasterfrom
copilot/update-pr-73-with-reviews

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Feb 17, 2026

  • Create RandomNumberGenerator.cs polyfill with correct compilation guards
  • Add RandomNumberGenerator class polyfill for .NET Standard < 1.3
  • Add fast-path for empty spans
  • Add proper class naming convention
  • Add ExcludeFromCodeCoverage attribute
  • Create RandomNumberGeneratorTests.cs with proper test initialization
  • Run tests to validate implementation
  • Address all PR review feedback
    • End comment sentences with periods
    • Check directly for zeroes using Should().Contain(b => b != 0)
    • Inline buffer into span creation
    • Use new Span<>() for empty span
    • Move RandomNumberGenerator class polyfill to NetCore10 directory
    • Replace reflection-based approach with Random wrapper for netstandard < 1.3
    • Use try-finally pattern for disposal
    • Add comment explaining IDisposable cast necessity
    • Remove unnecessary comments
    • Use arrow method for GetBytes implementation
    • Reorder Dispose methods (protected virtual before public)
    • Move buffer allocation inside try block
    • Fix signature generation script to detect abstract classes
  • Run CodeQL security scan (no issues found)
  • All tests passing

Implementation Details

This PR adds polyfills to enable RandomNumberGenerator.Fill(Span<byte>) functionality across all target frameworks, including older .NET Standard versions where the RandomNumberGenerator class doesn't exist.

RandomNumberGenerator Class Polyfill (NetCore10)

For .NET Standard < 1.3, a complete polyfill of the RandomNumberGenerator abstract class is provided in PolyShim/NetCore10/RandomNumberGenerator.cs that:

  • Wraps Random for compatibility (not cryptographically secure on these platforms as System.Security.Cryptography types are not available)
  • Implements proper IDisposable pattern
  • Provides minimal functionality required for the Fill method
  • Placed in NetCore10 directory as it's a base type, not a NetCore21-specific feature

Fill Method Polyfill (NetCore21)

The Fill(Span<byte>) method polyfill in PolyShim/NetCore21/RandomNumberGenerator.cs:

  • Creates a temporary byte array and fills it using the existing GetBytes method
  • Copies the result to the target span
  • Includes empty span fast-path optimization
  • Uses try-finally with explicit IDisposable cast for .NET Framework 3.5 compatibility
  • Buffer allocation moved inside try block for proper exception handling

Signature Generation Fix

Fixed the signature generation script (List-Signatures.ps1) to include abstract as an optional class modifier in the regex pattern. This bug was preventing abstract classes from being detected, which affected both RandomNumberGenerator and OSPlatformAttribute. The Types count in Signatures.md now correctly reflects 76 types (increased from 74).

This enables modern Span-based random number generation APIs on all .NET Standard versions, even where the infrastructure didn't originally exist. Note that on .NET Standard < 1.3, the random number generation uses Random rather than cryptographically secure methods due to platform limitations.

Original prompt

can you either edit the existing PR (#73) or recreate it, incorporating the provided review comments?


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@Tyrrrz Tyrrrz added the enhancement New feature or request label Feb 17, 2026
Copilot AI and others added 4 commits February 17, 2026 14:42
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
…empty span test

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
…rtion

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Copilot AI changed the title [WIP] Incorporate review comments into PR #73 Add RandomNumberGenerator.Fill(Span<byte>) polyfill Feb 17, 2026
Copilot AI requested a review from Tyrrrz February 17, 2026 14:52
Copy link
Copy Markdown
Owner

@Tyrrrz Tyrrrz left a comment

Choose a reason for hiding this comment

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

Comment on lines +21 to +23
// Verify that not all bytes are zero (cryptographic RNG should produce non-zero data)
// Note: Theoretically, a crypto RNG could produce all zeros (probability: 2^-128),
// but this is astronomically unlikely in practice
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

End sentences with a period

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.

Done in 981b9ca

// Verify that not all bytes are zero (cryptographic RNG should produce non-zero data)
// Note: Theoretically, a crypto RNG could produce all zeros (probability: 2^-128),
// but this is astronomically unlikely in practice
buffer.Should().NotBeEquivalentTo(new byte[16]);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I think it's cleaner to check directly for zeroes

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.

Changed to span.ToArray().Should().Contain(b => b != 0) in 981b9ca

Comment on lines +14 to +15
var buffer = new byte[16];
var span = new Span<byte>(buffer);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

First expression can be inlined into the second since we don't use the buffer array separately

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.

Inlined in 981b9ca

{
// Arrange
var buffer = new byte[0];
var span = new Span<byte>(buffer);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just new Span<>() should do?

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.

Changed to new Span<byte>() in 981b9ca

@@ -0,0 +1,33 @@
#if (NETCOREAPP && !NETCOREAPP2_1_OR_GREATER) || (NETFRAMEWORK && NET40_OR_GREATER) || (NETSTANDARD && NETSTANDARD1_3_OR_GREATER && !NETSTANDARD2_1_OR_GREATER)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why the .net std 1.3 limitation? If it's because the class itself doesn't exist, we can create it.

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.

Removed the NETSTANDARD1_3 limitation and used nested #if !NETSTANDARD || NETSTANDARD1_3_OR_GREATER to conditionally include when the class exists, matching the pattern in Path.cs. In 981b9ca

return;

var buffer = new byte[data.Length];
using (var rng = RandomNumberGenerator.Create())
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why not an implicit scoped using here?

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.

Changed to try-finally with explicit Dispose() for compatibility with net35 where RandomNumberGenerator doesn't work properly with using statements. In 981b9ca

…ions, use try-finally for disposal

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Copilot AI requested a review from Tyrrrz February 17, 2026 15:11
Copy link
Copy Markdown
Owner

@Tyrrrz Tyrrrz left a comment

Choose a reason for hiding this comment

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


var buffer = new byte[data.Length];
var rng = RandomNumberGenerator.Create();
try
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Oh I meant a using statement that ends with a semicolon and doesn't use curly braces.

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.

Changed to using var scoped declaration with conditional compilation to handle net35 compatibility in 1bf1f0c

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why the net35 exclusion? It should work the same everywhere @copilot

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.

Removed the NET35 conditional by explicitly casting to IDisposable in the using statement, which works across all frameworks in 5c023b4

@Tyrrrz Tyrrrz marked this pull request as ready for review February 17, 2026 15:19
Copilot AI review requested due to automatic review settings February 17, 2026 15:19
… for net35

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
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 adds a polyfill for RandomNumberGenerator.Fill(Span<byte>) to provide compatibility with .NET Framework and older .NET Standard versions. The implementation creates a temporary byte array, fills it with random data using the existing GetBytes method, and copies the result to the target span.

Changes:

  • Added RandomNumberGenerator.Fill(Span<byte>) polyfill with proper compilation guards and platform-specific disposal patterns
  • Updated signature documentation to reflect the new member
  • Added comprehensive tests for both non-empty and empty span scenarios

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
PolyShim/Signatures.md Updated totals and added documentation entry for the new Fill method
PolyShim/NetCore21/RandomNumberGenerator.cs Implemented the Fill polyfill with empty span fast-path and NET35-compatible disposal
PolyShim.Tests/NetCore21/RandomNumberGeneratorTests.cs Added tests validating Fill behavior for both empty and non-empty spans

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copy link
Copy Markdown
Owner

@Tyrrrz Tyrrrz left a comment

Choose a reason for hiding this comment

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

@@ -0,0 +1,46 @@
#if (NETCOREAPP && !NETCOREAPP2_1_OR_GREATER) || (NETFRAMEWORK) || (NETSTANDARD && !NETSTANDARD2_1_OR_GREATER)
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

How do we get away with these boundaries if the class doesn't exist on some older targets?

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.

The nested #if !NETSTANDARD || NETSTANDARD1_3_OR_GREATER on line 10 excludes the entire class definition for netstandard < 1.3 where RandomNumberGenerator doesn't exist. For those targets, this file compiles to empty content, so the polyfill simply isn't available (which is correct since both the class and Span don't exist there).

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

We provide Span for all targets, even net std 1.0. can we just create a polyfill for the Random number generator class for net std <1.3? It can just contain the bare minimum required functionality to work with our Fill method.

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.

Added RandomNumberGenerator polyfill for netstandard < 1.3 that wraps RNGCryptoServiceProvider using reflection in 0d9a36a

@Tyrrrz Tyrrrz changed the title Add RandomNumberGenerator.Fill(Span<byte>) polyfill Add polyfill for RandomNumberGenerator.Fill(Span<byte>) Feb 17, 2026
Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Copilot AI requested a review from Tyrrrz February 17, 2026 15:32
Copy link
Copy Markdown
Owner

@Tyrrrz Tyrrrz left a comment

Choose a reason for hiding this comment

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

namespace System.Security.Cryptography;

[ExcludeFromCodeCoverage]
internal abstract class RandomNumberGenerator : IDisposable
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Move this into its own appropriate file. This class wasn't introduced in net core 2.1.

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.

Moved to PolyShim/NetCore10/RandomNumberGenerator.cs in 1207292


protected virtual void Dispose(bool disposing) { }

private sealed class RNGCryptoServiceProviderWrapper : RandomNumberGenerator
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Don't use reflection. If there's no way to recreate the functionality of RNGCryptoServiceProvider, just wrap Random

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.

Replaced with Random wrapper in 1207292

}
finally
{
((IDisposable)rng).Dispose();
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Explain in a comment why this cast is necessary

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.

Added comment explaining it's needed for .NET Framework 3.5 compatibility in 1207292

…apper, add IDisposable cast comment

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Copilot AI requested a review from Tyrrrz February 17, 2026 15:51
Copy link
Copy Markdown
Owner

@Tyrrrz Tyrrrz left a comment

Choose a reason for hiding this comment

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


public override void GetBytes(byte[] data)
{
_random.NextBytes(data);
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Use arrow method

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.

Changed to arrow method in 46d1155

GC.SuppressFinalize(this);
}

protected virtual void Dispose(bool disposing) { }
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Move this above the other Dispose

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.

Moved in 46d1155


- **Total:** 340
- **Total:** 341
- **Types:** 74
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Why did this not increase?

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.

The RandomNumberGenerator class polyfill is only active for netstandard < 1.3 (conditional compilation), so the signature generation script doesn't detect it when running on netstandard2.0 target where it generates the Signatures.md file.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That makes no sense. The script scans files as text it doesn't take conditional compilation into account. Are you sure you re-ran the build? Please investigate

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That makes no sense. The script scans files as text it doesn't take conditional compilation into account. Are you sure you re-ran the build? Please investigate

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.

You were right - the script does scan as text. The issue was the regex pattern internal\s+(?:(?:readonly|partial|static|sealed|ref)\s+)*(class|...) was missing abstract as an optional modifier. Fixed in a0ca669. The Types count now correctly shows 76 (up from 74) - it detected both RandomNumberGenerator and OSPlatformAttribute which are both abstract classes.

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.

You were right - the script does scan as text. The issue was the regex pattern was missing abstract as an optional modifier. Fixed in a0ca669. Types count increased from 74 to 76 (RandomNumberGenerator and OSPlatformAttribute).

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Copilot AI requested a review from Tyrrrz February 17, 2026 16:00
Copy link
Copy Markdown
Owner

@Tyrrrz Tyrrrz left a comment

Choose a reason for hiding this comment

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

finally
{
// Explicit cast needed for .NET Framework 3.5 where RandomNumberGenerator
// doesn't properly expose IDisposable for using statements
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

End with a period

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.

Added period in a0ca669

if (data.Length == 0)
return;

var buffer = new byte[data.Length];
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Move this inside try

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.

Moved inside try in a0ca669


- **Total:** 340
- **Total:** 341
- **Types:** 74
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That makes no sense. The script scans files as text it doesn't take conditional compilation into account. Are you sure you re-ran the build? Please investigate


- **Total:** 340
- **Total:** 341
- **Types:** 74
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

That makes no sense. The script scans files as text it doesn't take conditional compilation into account. Are you sure you re-ran the build? Please investigate

…re script to detect abstract classes

Co-authored-by: Tyrrrz <1935960+Tyrrrz@users.noreply.github.com>
Copilot AI requested a review from Tyrrrz February 17, 2026 16:11
@Tyrrrz Tyrrrz merged commit 66a278d into master Feb 17, 2026
5 checks passed
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 17, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (7585585) to head (a0ca669).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@     Coverage Diff      @@
##   master   #76   +/-   ##
============================
============================

☔ 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

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants