Skip to content

Add analyzer+codefix for applying RequiresUnsafe to pointer methods#125196

Open
Copilot wants to merge 22 commits intomainfrom
copilot/duplicate-pr-content-125195
Open

Add analyzer+codefix for applying RequiresUnsafe to pointer methods#125196
Copilot wants to merge 22 commits intomainfrom
copilot/duplicate-pr-content-125195

Conversation

Copy link
Contributor

Copilot AI commented Mar 4, 2026

Description

Cherry-pick of PR #125195 (agocke/runtime). Adds a Roslyn analyzer and code fixer that warns on methods with the unsafe modifier but missing [RequiresUnsafe], and applies [RequiresUnsafe] to all unsafe methods in System.Private.CoreLib.

  • Analyzer (UnsafeMethodMissingRequiresUnsafeAnalyzer): Reports IL2900 on unsafe methods lacking [RequiresUnsafe]
  • Code fixer (UnsafeMethodMissingRequiresUnsafeCodeFixProvider): Adds [RequiresUnsafe] attribute automatically
  • Attribute application: 276 files across CoreCLR and shared CoreLib annotated with [RequiresUnsafe]
  • Merge conflict resolution:
    • MdImport.cs — kept current LibraryImport/partial signature (diverged from fork's MethodImpl/extern), added [RequiresUnsafe]
    • Sve.PlatformNotSupported.cs — resolved conflicts from SVE method signature changes (Vector<int>/Vector<uint>Vector<long>/Vector<ulong>), ensured new unsafe pointer methods have [RequiresUnsafe]
  • Corner case exclusions:
    • Reverted [RequiresUnsafe] from IntPtr.cs and UIntPtr.cs — pointers in those files do not imply RequiresUnsafe
    • Removed [RequiresUnsafe] from Add<T>(void*, int) and Subtract<T>(void*, int) in Unsafe.cs — these methods do not dereference the pointer
    • Removed [RequiresUnsafe] from Alloc and AllocZeroed in NativeMemory.cs and NativeMemory.Unix.cs — returning a pointer is not inherently unsafe, only dereferencing it would be
  • Cleanup: Reverted 43 files that had only unnecessary using System.Diagnostics.CodeAnalysis; additions without any [RequiresUnsafe] usage (leftover from code fixer iterations), and restored UTF-8 BOMs in 20 files that were inadvertently stripped by the fork's editor
  • Scope note: [RequiresUnsafe] on Add, Subtract, AddByteOffset, and SubtractByteOffset methods returning ref T in Unsafe.cs was reverted to keep this PR focused on pointer-based methods; non-pointer RequiresUnsafe changes will be grouped into a separate PR

🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

Add a new DiagnosticAnalyzer (UnsafeMethodMissingRequiresUnsafeAnalyzer)
that warns when a method, constructor, or local function has the 'unsafe'
modifier but is not annotated with [RequiresUnsafe].

Add a matching CodeFixProvider that adds the [RequiresUnsafe] attribute
to the flagged declaration.

Both are #if DEBUG guarded and enabled via the existing
EnableUnsafeAnalyzer MSBuild property.

New diagnostic: IL5004 (UnsafeMethodMissingRequiresUnsafe)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI changed the title [WIP] Add exact contents from PR 125195 Add analyzer+codefix for unsafe methods missing [RequiresUnsafe] Mar 4, 2026
@dotnet-policy-service dotnet-policy-service bot added the linkable-framework Issues associated with delivering a linker friendly framework label Mar 4, 2026
agocke added 2 commits March 5, 2026 14:44
Now based on methods with pointer types, rather than methods with .
agocke and others added 4 commits March 5, 2026 16:10
…r-content-125195

# Conflicts:
#	src/libraries/Common/src/Interop/Unix/System.Native/Interop.Futex.cs
#	src/libraries/System.Private.CoreLib/src/System/Threading/LowLevelFutex.Unix.cs
- Remove lambda/anonymous method break in RequiresUnsafeAnalyzer so
  unsafe context flows through nested lambdas (matching C# semantics)
- Add FieldDeclarationSyntax to IsInRequiresScope for unsafe field
  initializers
- Remove [RequiresUnsafe] from files compiled outside CoreLib (Common/,
  nativeaot/Runtime.Base/, Resources/) where the attribute is unavailable
- Add tests for lambda-in-unsafe-method, anonymous delegate, and field
  initializer scenarios

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the DiagnosticSeverity.Info overload so IL5004 shows as a
suggestion rather than a warning/error in builds. Update tests to
expect Info severity.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@agocke agocke force-pushed the copilot/duplicate-pr-content-125195 branch from c08c9a1 to a6490f4 Compare March 6, 2026 18:35
agocke added 3 commits March 6, 2026 10:50
…r-content-125195

# Conflicts:
#	src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs
#	src/coreclr/System.Private.CoreLib/src/System/StartupHookProvider.CoreCLR.cs
Co-authored-by: agocke <515774+agocke@users.noreply.github.com>
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@agocke agocke changed the title Add analyzer+codefix for unsafe methods missing [RequiresUnsafe] Add analyzer+codefix for applying RequiresUnsafe to pointer methods Mar 10, 2026
@jkotas
Copy link
Member

jkotas commented Mar 11, 2026

Revert changes to Unsafe.Add/Subtract

This does not look right to me.

Unsafe.Add/Subtract on byrefs need RequiresUnsafe. Otherwise, all byref dereferences would have to be unsafe - I do not think we want that.

EDIT: I see your comment above. This PR is not trying to be complete; you are leaving it for later.

@agocke
Copy link
Member

agocke commented Mar 11, 2026

@copilot please merge with main and resolve merge conflicts

…/duplicate-pr-content-125195

# Conflicts:
#	src/libraries/System.Private.CoreLib/src/System/Runtime/Intrinsics/Arm/Sve.PlatformNotSupported.cs
Copy link
Contributor Author

Copilot AI commented Mar 11, 2026

@copilot please merge with main and resolve merge conflicts

Merged with main and resolved merge conflicts in Sve.PlatformNotSupported.cs in 7c9bf4b. The conflict was caused by method signature changes (old Vector<int>/Vector<uint> overloads replaced with Vector<long>/Vector<ulong> versions). Added [RequiresUnsafe] to the new unsafe methods.

@agocke agocke marked this pull request as ready for review March 11, 2026 23:17
Copilot AI review requested due to automatic review settings March 11, 2026 23:17
Copy link
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.

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


You can also share your feedback on Copilot code review. Take the survey.

@agocke
Copy link
Member

agocke commented Mar 13, 2026

I think this is ready -- it only annotates System.Private.CoreLib so once everything goes through here we can take on the remaining libraries.


using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Diagnostics.CodeAnalysis;
Copy link
Member

Choose a reason for hiding this comment

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

@copilot This file has unnecessary whitespace only changes. Please revert the changes in this file. Also, check the rest of the PR for the same issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted StackFrameIterator.cs and found 42 additional files with the same issue (unnecessary using System.Diagnostics.CodeAnalysis; additions without any [RequiresUnsafe]). Also restored UTF-8 BOMs that were inadvertently stripped by the fork's editor in 20 files. All fixed in 06eb913.

Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
@jkotas
Copy link
Member

jkotas commented Mar 14, 2026

The annotations look fine to me.

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

Labels

area-Infrastructure-libraries linkable-framework Issues associated with delivering a linker friendly framework

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

4 participants