Add [RequiresUnsafe] to all extern and LibraryImport (CoreLib)#125880
Add [RequiresUnsafe] to all extern and LibraryImport (CoreLib)#125880EgorBo wants to merge 5 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
There was a problem hiding this comment.
Pull request overview
Updates the ILLink RequiresUnsafe analyzer/code-fix to also cover P/Invoke-like entrypoints declared via [LibraryImport]/extern, and bulk-applies [RequiresUnsafe] annotations to CoreLib interop entrypoints while temporarily suppressing related warnings in CoreLib builds.
Changes:
- Add a new RequiresUnsafe-related diagnostic (IL5005) for methods with
LibraryImportAttributemissing[RequiresUnsafe], plus code-fix support and tests. - Annotate numerous CoreLib
[LibraryImport]declarations with[RequiresUnsafe](and add requiredusing System.Diagnostics.CodeAnalysiswhere needed). - Suppress IL5000–IL5005 warnings in
System.Private.CoreLib.csproj(Debug/Checked) pending callsite-fixer rollout.
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMethodMissingRequiresUnsafeTests.cs | Adds test coverage for the new LibraryImport/extern-related diagnostic and code fix. |
| src/tools/illink/src/ILLink.Shared/SharedStrings.resx | Adds title/message resources for the new diagnostic. |
| src/tools/illink/src/ILLink.Shared/DiagnosticId.cs | Introduces ExternLibraryImportMissingRequiresUnsafe = 5005. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs | Extends analyzer to report on [LibraryImport] methods missing [RequiresUnsafe]. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeMethodMissingRequiresUnsafeCodeFixProvider.cs | Extends code fix provider to handle the new diagnostic id. |
| src/libraries/System.Private.CoreLib/src/System/Threading/TimerQueue.Browser.cs | Adds [RequiresUnsafe] to QCall LibraryImport timer scheduling entrypoint. |
| src/libraries/System.Private.CoreLib/src/System/Threading/ThreadPool.Browser.cs | Adds [RequiresUnsafe] to QCall LibraryImport background job scheduling entrypoint. |
| src/libraries/System.Private.CoreLib/src/System/Threading/Interlocked.cs | Adds [RequiresUnsafe] to a QCall LibraryImport entrypoint. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TypeMapLazyDictionary.cs | Adds [RequiresUnsafe] to QCall LibraryImport typemap lookup entrypoints. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncHelpers.Browser.cs | Adds [RequiresUnsafe] to Browser QCall LibraryImport entrypoints and required using. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/XplatEventLogger.cs | Adds [RequiresUnsafe] to QCall LibraryImport logging entrypoints. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Threading.NativeSinks.Internal.cs | Adds [RequiresUnsafe] to multiple QCall LibraryImport native sink entrypoints and required using. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/NativeRuntimeEventSource.Exceptions.NativeSinks.Internal.cs | Adds [RequiresUnsafe] to QCall LibraryImport exception sink entrypoint and required using. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventPipe.Internal.cs | Adds [RequiresUnsafe] to QCall LibraryImport EventPipe entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Variant.cs | Adds [RequiresUnsafe] to QCall LibraryImport COM marshalling entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/TypeLoadException.CoreCLR.cs | Adds [RequiresUnsafe] to a QCall LibraryImport message helper and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Threading/Thread.CoreCLR.cs | Adds [RequiresUnsafe] to numerous QCall LibraryImport Thread entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Threading/Monitor.CoreCLR.cs | Adds [RequiresUnsafe] to a QCall LibraryImport monitor entrypoint and required using. |
| src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs | Adds [RequiresUnsafe] to many QCall LibraryImport stub-helper entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/String.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport string intern entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeType.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport reflection/interop entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs | Adds [RequiresUnsafe] to many QCall LibraryImport runtime handle helpers. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/Loader/AssemblyLoadContext.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport ALC entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/TrackerObjectManager.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport tracker entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ObjectiveCMarshal.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport ObjC marshal entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport NativeLibrary entrypoint and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs | Adds [RequiresUnsafe] to many QCall/interop LibraryImport entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Java/JavaMarshal.CoreCLR.cs | Adds [RequiresUnsafe] to a QCall LibraryImport Java marshal entrypoint. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/GCHandle.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport GCHandle entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/ComWrappers.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport ComWrappers entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/DependentHandle.cs | Adds [RequiresUnsafe] to QCall LibraryImport DependentHandle entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/ControlledExecution.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport thread abort/reset entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/VirtualDispatchHelpers.cs | Adds [RequiresUnsafe] to QCall LibraryImport virtual dispatch resolver and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/StaticsHelpers.cs | Adds [RequiresUnsafe] to a QCall LibraryImport statics helper. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs | Adds [RequiresUnsafe] to several QCall LibraryImport RuntimeHelpers entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/GenericsHelpers.cs | Adds [RequiresUnsafe] to QCall LibraryImport generic handle helper. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/TypeNameResolver.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport resolver entrypoint. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs | Adds [RequiresUnsafe] to QCall LibraryImport RuntimeModule entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs | Adds [RequiresUnsafe] to QCall LibraryImport custom attribute entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeAssembly.cs | Adds [RequiresUnsafe] to many QCall LibraryImport RuntimeAssembly entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/MethodBase.CoreCLR.cs | Adds [RequiresUnsafe] to a QCall LibraryImport current-method helper. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Metadata/MetadataUpdater.cs | Adds [RequiresUnsafe] to QCall LibraryImport metadata update support query. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/LoaderAllocator.cs | Adds [RequiresUnsafe] to QCall LibraryImport loader allocator destroy helper. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs | Adds [RequiresUnsafe] to numerous QCall LibraryImport Reflection.Emit entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeModuleBuilder.cs | Adds [RequiresUnsafe] to QCall LibraryImport module builder entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Assembly.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport Assembly entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/IO/FileLoadException.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport message helpers and required using. |
| src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | Adds [RequiresUnsafe] to many QCall LibraryImport GC entrypoints and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Exception.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport exception helpers. |
| src/coreclr/System.Private.CoreLib/src/System/Environment.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport Environment entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/Enum.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport enum helper and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackTrace.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport stack trace helper and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Diagnostics/StackFrame.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport stack frame helper and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Diagnostics/Debugger.cs | Adds [RequiresUnsafe] to QCall LibraryImport debugger helpers and required using. |
| src/coreclr/System.Private.CoreLib/src/System/Delegate.CoreCLR.cs | Adds [RequiresUnsafe] to multiple QCall LibraryImport delegate helpers and required using. |
| src/coreclr/System.Private.CoreLib/src/System/ComAwareWeakReference.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport COM weak reference helpers and required using. |
| src/coreclr/System.Private.CoreLib/src/System/CLRConfig.cs | Adds [RequiresUnsafe] to QCall LibraryImport config helper and required using. |
| src/coreclr/System.Private.CoreLib/src/System/ArgIterator.cs | Adds [RequiresUnsafe] to QCall LibraryImport ArgIterator entrypoints. |
| src/coreclr/System.Private.CoreLib/src/System/AppContext.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport handler hook. |
| src/coreclr/System.Private.CoreLib/src/Internal/VersionResilientHashCode.CoreCLR.cs | Adds [RequiresUnsafe] to QCall LibraryImport hashing helper and required using. |
| src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj | Suppresses IL5000–IL5005 in Debug/Checked builds during adoption period. |
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMethodMissingRequiresUnsafeTests.cs
Outdated
Show resolved
Hide resolved
src/tools/illink/src/ILLink.CodeFix/UnsafeMethodMissingRequiresUnsafeCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
Outdated
Show resolved
Hide resolved
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs
Outdated
Show resolved
Hide resolved
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMethodMissingRequiresUnsafeTests.cs
Outdated
Show resolved
Hide resolved
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 81 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs:1
System.Linqis imported but not used in this file. Please remove the unused using to avoid unnecessary dependencies and keep the file tidy.
src/libraries/System.Runtime.InteropServices/gen/LibraryImportGenerator/LibraryImportGenerator.cs:1- The generator now forwards
[RequiresUnsafe]onto generated stubs. Please add a source-generator unit test that asserts the generated__PInvokelocal extern method includes[global::System.Diagnostics.CodeAnalysis.RequiresUnsafe]when the user-authoredLibraryImportmethod is annotated.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs
Show resolved
Hide resolved
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj
Outdated
Show resolved
Hide resolved
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMethodMissingRequiresUnsafeAnalyzer.cs
Show resolved
Hide resolved
|
Source generators cannot currently emit LibraryImport methods as source generators cannot consume source created by other source generators (at least until dotnet/roslyn#82680 or something like it is approved). |
|
nit: we should re-consider the diagnostic code. An "IL" prefix does not seem correct for this. It's an implementation detail that it's reusing illink infrastructure. |
e567e8e to
3a517f6
Compare
src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs
Outdated
Show resolved
Hide resolved
3a517f6 to
fa8ec81
Compare
src/tools/illink/src/ILLink.CodeFix/UnsafeMethodMissingRequiresUnsafeCodeFixProvider.cs
Show resolved
Hide resolved
4c58afc to
32d502c
Compare
| foreach (AttributeData? attr in method.GetAttributes ()) { | ||
| if (attr.AttributeClass?.HasName ("System.Runtime.CompilerServices.MethodImplAttribute") == true) { | ||
| foreach (TypedConstant arg in attr.ConstructorArguments) { | ||
| if (arg.Value is int intVal && (intVal & (int)MethodImplOptions.InternalCall) != 0) | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The InternalCall exemption checks only TypedConstant.Value is int. Roslyn can surface MethodImplAttribute ctor arguments as short (legacy ctor) or as the enum type; in those cases this will fail to recognize InternalCall and will incorrectly report IL5005 on InternalCall externs. Consider normalizing the constant to an int (handling short/enum) before doing the bitmask test.
| private static readonly DiagnosticDescriptor s_pointerRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnsafeMethodMissingRequiresUnsafe, diagnosticSeverity: DiagnosticSeverity.Info); | ||
| private static readonly DiagnosticDescriptor s_externLibraryImportRule = DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.ExternMethodMissingRequiresUnsafe, diagnosticSeverity: DiagnosticSeverity.Info); | ||
|
|
||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create (s_rule); | ||
| public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [s_pointerRule, s_externLibraryImportRule]; | ||
|
|
There was a problem hiding this comment.
This change introduces a new diagnostic (IL5005) and expands the code fix to cover it, but there are no analyzer/code-fix tests covering the new extern/LibraryImport scenarios. Please add test cases similar to the existing UnsafeMethodMissingRequiresUnsafeTests to validate both diagnostics and the fixer behavior for [LibraryImport] partial methods and extern methods (including the InternalCall exemption).
|
Moved to #125894 |
This PR does 4 things:
IL5005for[LibraryImport]partial methods andexternmethods without[RequiresUnsafe]on them. (IL5004already exists for pointer-type signatures.)[RequiresUnsafe]when it's missing on[LibraryImport]/externmethods[RequiresUnsafe]to the generated__PInvokelocal extern methods.Not done:
MethodImplOptions.InternalCallis ignored with a TODO despite theextern- there are way too many safe APIs withInternalCalle.g. allMath.*APIs.[RequiresUnsafe(false)](in case if we decide to introduce that overload)?