Merge requiresunsafe-corelib2 into feature/requires-unsafe #3209

Copilot · 2025-11-12T22:33:53Z

✅ All PR Feedback Addressed!

Latest Changes:

Added Field to RequiresUnsafeAttribute AttributeUsage targets
Added [RequiresUnsafe] to unsafe fields across multiple files
Changed TryPush method from unsafe to [RequiresUnsafe]
Added [RequiresUnsafe] to ContextCallback delegate and s_ecCallback lambda
Removed nested unsafe blocks from ComWrappers methods and added [RequiresUnsafe]
Refactored PointerArrayMarshaller.GetManagedValuesSource to return directly with comment
Refactored PinnedGCHandle.Target getter to return directly from unsafe block
Added FIXME comments to ThreadPoolWorkQueue, String.Manipulation, MemoryExtensions, and Delegate
Fixed nested unsafe blocks in Delegate.cs

The branch copilot/merge-requiresunsafe-corelib2 now contains all the work from requiresunsafe-corelib2 properly merged with feature/requires-unsafe and is ready for review.

Original prompt

Please create a new branch that merges my fork's requiresunsafe-corelib2 into feature/requires-unsafe

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

agocke

@copilot Please fix

src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs

eng/Versions.props

src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

…OWNERS and format unsafe blocks Co-authored-by: agocke <[email protected]>

Copilot · 2025-11-13T00:08:33Z

@copilot Please fix

Fixed in commit f2e66af. All PR feedback has been addressed:

Reverted eng/Versions.props and Directory.Build.props
Restored .github/CODEOWNERS
Formatted all unsafe blocks to multi-line format
Added FIXME comments to all unsafe blocks

agocke

@copilot please fix

agocke · 2025-11-12T23:54:16Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs

                    // or a U[] which is blittable to a T[] (e.g., int[] and uint[]).

                    // The array may be prepinned, so remove the high bit from the start index in the line below.
                    // The ArraySegment<T> ctor will perform bounds checking on index & length.


Please move this comment down to keep it on ArraySegment.

Done in commit 9a8c31b. Moved the comment about ArraySegment.<T> constructor bounds checking down to the line before the constructor call.

agocke · 2025-11-13T08:14:48Z

src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs

            // ! or you may introduce a security hole!
-            T[] @this = Unsafe.As<T[]>(this);
+            T[] @this;
+            // FIXME: review unsafe to confirm correct annotation


For every place in this file with the // ! Warning: "this" is an array comment, please remove the FIXME, as it is stated what the invariant is in the comment.

Done in commit 9a8c31b. Removed FIXME comments from all 7 methods in Array.CoreCLR.cs that have the "Warning: this is an array" comment explaining the invariant.

src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs

...s/System.Private.CoreLib/src/System/Runtime/CompilerServices/ConfiguredValueTaskAwaitable.cs

…, refactor Delegate.CoreCLR.cs, add clarifying comments Co-authored-by: agocke <[email protected]>

Copilot · 2025-11-13T08:26:55Z

@copilot please fix

Fixed in commit 9a8c31b. All PR feedback has been addressed:

Moved ArraySegment comment in MemoryMarshal.cs
Removed FIXME from Array.CoreCLR.cs where Warning comment exists
Refactored Delegate.CoreCLR.cs to return directly from unsafe block
Added clarifying comments to ConfiguredValueTaskAwaitable.cs

…ocks Co-authored-by: agocke <[email protected]>

agocke

@copilot Please address comments

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

agocke · 2025-11-19T18:30:33Z

src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs

            Debug.Assert(obj is MulticastDelegate, "Shouldn't have failed here since we already checked the types are the same!");
-            MulticastDelegate d = Unsafe.As<MulticastDelegate>(obj);
+            MulticastDelegate d;
+            // FIXME: review unsafe to confirm correct annotation


This looks good to me, I think we can remove the comment.

Done in commit c0df804. Removed the FIXME comment.

agocke · 2025-11-19T18:31:58Z

src/coreclr/System.Private.CoreLib/src/System/MulticastDelegate.CoreCLR.cs

            return false;
        }

        private unsafe MulticastDelegate NewMulticastDelegate(object[] invocationList, int invocationCount, bool thisIsMultiCastAlready)


You can remove unsafe from this method since we have an unsafe block.

Done in commit c0df804. Removed unsafe modifier from the method signature.

src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs

agocke · 2025-11-19T22:13:55Z

src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/ValueTaskAwaiter.cs

        /// <summary>Schedules the continuation action for this ValueTask.</summary>
        public void OnCompleted(Action continuation)
        {
            object? obj = _value._obj;


Please make the same changes here as suggested in the ConfiguredValueTaskAwaitable class

Done in commit c0df804. Made the same changes to ValueTaskAwaiter._value._obj field.

agocke · 2025-11-19T22:14:57Z

...tem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/PointerArrayMarshaller.cs

+            IntPtr[]? result;
+            unsafe
+            {
+                result = Unsafe.As<IntPtr[]>(managed);


Just return the value directly instead of saving to a local.

Done in commit c0df804. Refactored to return the value directly.

agocke · 2025-11-19T22:15:08Z

...tem.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/PointerArrayMarshaller.cs

+                IntPtr[]? result;
+                unsafe
+                {
+                    result = Unsafe.As<IntPtr[]>(_managedArray);


Also add a comment above the unsafe block in both of the instances in this file saying

// IntPtr[] and T*[] have the same representation

Done in commit c0df804. Refactored to return directly and added comment "// IntPtr[] and T*[] have the same representation" to both instances.

Done in commit c0df804. Added comment "// IntPtr[] and T*[] have the same representation" to both instances.

agocke · 2025-11-19T22:17:26Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.T.cs

+                T result;
+                unsafe
+                {
+                    result = Unsafe.As<T>(GCHandle.InternalGet(handle)!);


Just return the value and remove the result variable.

Done in commit c0df804. Refactored to return the value directly and removed the result variable.

agocke · 2025-11-19T22:17:41Z

src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.cs

+                        ManagedObjectWrapperHolder? holder;
+                        unsafe
+                        {
+                            holder = Unsafe.As<ManagedObjectWrapperHolder>(GCHandle.FromIntPtr(handle).Target);


Just return, and remove the holder variable

Done in commit c0df804. Refactored to return directly and removed the holder variable.

…urn statements, add clarifying comments Co-authored-by: agocke <[email protected]>

Copilot · 2025-11-19T22:31:22Z

@copilot Please address comments

Addressed all comments in commit c0df804.

agocke

@copilot Please address comments. Also, there are new warnings/errors from [RequiresUnsafe] additions. Please fix those by adding a new unsafe block with the comment

// FIXME: review unsafe to confirm correct annotation
``` above

agocke · 2025-11-19T23:45:34Z

src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs

-                    buffer = Unsafe.As<T[]>(b.TryPop());
+                    unsafe
+                    {
+                        buffer = Unsafe.As<T[]>(b.TryPop());


Please mark the TryPush method RequiresUnsafe instead.

agocke · 2025-11-19T23:46:23Z

...raries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/RequiresUnsafeAttribute.cs

+    /// This allows tools to understand which methods are unsafe to call when removing unreferenced
+    /// code from an application.
+    /// </remarks>
+    [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor, Inherited = false)]


Please add "field" to the valid targets

Done in commit b73ffce. Added Field to AttributeTargets.

agocke · 2025-11-19T23:46:46Z

src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs

 #pragma warning restore CA1823
-        private IntPtr m_cache;
+        // Must be a handle to a RuntimeTypeCache type
+        private unsafe IntPtr m_cache;


Please add [RequiresUnsafe] as well

Done in commit b73ffce. Added [RequiresUnsafe] to m_cache field.

agocke · 2025-11-19T23:47:34Z

src/libraries/System.Private.CoreLib/src/System/Buffers/SharedArrayPool.cs

        /// <summary>The stored array.</summary>
-        public Array? Array;
+        // Must be an array of T[] at runtime
+        public unsafe Array? Array;


Please also add [RequiresUnsafe]

Done in commit b73ffce. Added [RequiresUnsafe] to SharedArrayPoolThreadLocalArray.Array field.

agocke · 2025-11-19T23:47:55Z

src/libraries/System.Private.CoreLib/src/System/Collections/Concurrent/ConcurrentDictionary.cs

            /// <summary>The comparer to use for lookups in the tables.</summary>
-            internal readonly IEqualityComparer<TKey>? _comparer;
+            // Must be IAlternateEqualityComparer<TAlternateKey, TKey>
+            internal readonly unsafe IEqualityComparer<TKey>? _comparer;


Please add [RequiresUnsafe]

Done in commit b73ffce. Added [RequiresUnsafe] to Tables._comparer field.

agocke · 2025-11-20T00:01:05Z

src/libraries/System.Private.CoreLib/src/System/Threading/Tasks/ValueTask.cs

        /// <summary>null if representing a successful synchronous completion, otherwise a <see cref="Task"/> or a <see cref="IValueTaskSource"/>.</summary>
-        internal readonly object? _obj;
+        // Must be either a Task or IValueTaskSource
+        internal readonly unsafe object? _obj;


Please add [RequiresUnsafe]

Done in commit b73ffce. Added [RequiresUnsafe] to both _obj fields in ValueTask structs.

agocke · 2025-11-20T00:03:07Z

src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPoolWorkQueue.cs

            {
                Debug.Assert(workItem is IThreadPoolWorkItem);
+                IThreadPoolWorkItem tpWorkItem;
+                unsafe


Please add // FIXME: parent should be unsafe and specify input must be Task or IThreadPoolWorkItem

Done in commit b73ffce. Added the FIXME comment.

agocke · 2025-11-20T00:05:11Z

src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs

                    // enumerable-based equivalent of new string(char[]).

-                    IEnumerator<char> en = Unsafe.As<IEnumerator<char>>(e);
+                    IEnumerator<char> en;


Please add // FIXME: Consider if (e is IEnumerator<char> en) instead

Done in commit b73ffce. Added the FIXME comment.

agocke · 2025-11-20T00:06:44Z

src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs

                if (typeof(T) == typeof(char) && separators.Length == 0)
                {
-                    _searchValues = Unsafe.As<SearchValues<T>>(string.SearchValuesStorage.WhiteSpaceChars);
+                    unsafe


Please add // FIXME: review unsafe to confirm correct annotation to all unsafe blocks added in this file.

Done in commit b73ffce. Added "// FIXME: review unsafe to confirm correct annotation" to the unsafe block in MemoryExtensions.cs.

agocke · 2025-11-20T00:07:32Z

src/libraries/System.Private.CoreLib/src/System/Delegate.cs

            {
                int index = _index + 1;
-                if ((_current = Unsafe.As<TDelegate>(_delegate?.TryGetAt(index))) == null)
+                unsafe


Please add // FIXME: review unsafe to confirm correct annotation

Done in commit b73ffce. Added the FIXME comment and removed nested unsafe block.

…esUnsafe to fields and methods, add FIXME comments, refactor return statements Co-authored-by: agocke <[email protected]>

Copilot · 2025-11-20T00:21:38Z

@copilot Please address comments. Also, there are new warnings/errors from [RequiresUnsafe] additions. Please fix those by adding a new unsafe block with the comment

Addressed all comments in commit b73ffce. Added Field to RequiresUnsafeAttribute, added RequiresUnsafe to fields, added FIXME comments, and refactored return statements.

agocke added 4 commits October 29, 2025 01:03

Add RequiresUnsafeAttribute and start annotating corelib

f831e85

Follow onboarding instructions

41893f5

Typo

73b3389

Initial work from Claude

8aec308

Copilot AI assigned Copilot and agocke Nov 12, 2025

Copilot started work on behalf of agocke November 12, 2025 22:34 View session

Copilot AI changed the title ~~[WIP] Merge requiresunsafe-corelib2 into feature/requires-unsafe~~ Merge requiresunsafe-corelib2 into feature/requires-unsafe Nov 12, 2025

Copilot AI requested a review from agocke November 12, 2025 22:52

Copilot finished work on behalf of agocke November 12, 2025 22:52

agocke changed the base branch from docs to feature/requires-unsafe November 12, 2025 23:44

agocke force-pushed the copilot/merge-requiresunsafe-corelib2 branch from 28bb667 to 8aec308 Compare November 12, 2025 23:46

agocke reviewed Nov 12, 2025

View reviewed changes

src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs Outdated Show resolved Hide resolved

eng/Versions.props Outdated Show resolved Hide resolved

src/coreclr/System.Private.CoreLib/src/System/Array.CoreCLR.cs Show resolved Hide resolved

Copilot started work on behalf of agocke November 12, 2025 23:52 View session

Address PR feedback: revert Versions.props/Directory.Build.props/CODE…

f2e66af

…OWNERS and format unsafe blocks Co-authored-by: agocke <[email protected]>

Copilot finished work on behalf of agocke November 13, 2025 00:10

Copilot AI requested a review from agocke November 13, 2025 00:10

agocke reviewed Nov 13, 2025

View reviewed changes

Copilot started work on behalf of agocke November 13, 2025 08:19 View session

Address PR feedback: move comment, remove FIXME from Array.CoreCLR.cs…

9a8c31b

…, refactor Delegate.CoreCLR.cs, add clarifying comments Co-authored-by: agocke <[email protected]>

Copilot AI requested a review from agocke November 13, 2025 08:29

Copilot finished work on behalf of agocke November 13, 2025 08:29

Copilot started work on behalf of agocke November 14, 2025 09:21 View session

Refactor all temp variable patterns to return directly from unsafe bl…

bc538f8

…ocks Co-authored-by: agocke <[email protected]>

Copilot finished work on behalf of agocke November 14, 2025 09:30

agocke reviewed Nov 19, 2025

View reviewed changes

Copilot started work on behalf of agocke November 19, 2025 22:18 View session

Address PR comments: add unsafe to fields, remove FIXME, refactor ret…

c0df804

…urn statements, add clarifying comments Co-authored-by: agocke <[email protected]>

Copilot AI requested a review from agocke November 19, 2025 22:34

Copilot finished work on behalf of agocke November 19, 2025 22:34

agocke reviewed Nov 20, 2025

View reviewed changes

Copilot started work on behalf of agocke November 20, 2025 00:09 View session

Address PR comments: add Field to RequiresUnsafeAttribute, add Requir…

b73ffce

…esUnsafe to fields and methods, add FIXME comments, refactor return statements Co-authored-by: agocke <[email protected]>

Copilot AI requested a review from agocke November 20, 2025 00:24

Copilot finished work on behalf of agocke November 20, 2025 00:24

Merge requiresunsafe-corelib2 into feature/requires-unsafe #3209

Are you sure you want to change the base?

Merge requiresunsafe-corelib2 into feature/requires-unsafe #3209

Uh oh!

Conversation

Copilot AI commented Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Nov 13, 2025

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 13, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI commented Nov 13, 2025

Uh oh!

agocke left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 19, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI commented Nov 19, 2025

Copilot AI commented Nov 12, 2025 •

edited

Loading