Audit CoreLib for members that stay safe under unsafe-v2 - #131719
Open
EgorBo wants to merge 8 commits into
Open
Conversation
Running the migration workflow over System.Private.CoreLib surfaced two cases where a fixer reported no work to do even though the migration was incomplete. AddUnsafeToLibraryImportCodeFixProvider only declared IL5007, the diagnostic the migration analyzer reports before an assembly opts into the updated memory safety rules. Once the assembly has opted in, IL5007 stands down and the generator reports SYSLIB1064 for the same methods, which left workflow step 7 with nothing to fix. Declare both. RemoveInvalidUnsafeCodeFixProvider assumed the declaration the diagnostic points at is the one carrying the modifier. A partial type is reported once for the merged symbol, at a declaration that need not be the part holding 'unsafe', so the fix declined those and 13 CS9377 were left behind in CoreLib (for example the diagnostic is reported on Path.cs while the modifier is on Path.Windows.cs). Walk the other parts and remove the modifier wherever it is in source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
The extern (CS9389) and field-like (CS9392) fixers always chose 'unsafe', because the compiler requires an explicit contract there but cannot tell which one is correct. That is the right conservative default for an unaudited declaration, but it also means an audited one has no way to record its answer ahead of the migration: the fixer overwrites the intent with 'unsafe' and every caller then needs an unsafe context. Members that are safe are common at the boundary the two diagnostics cover. Math and MathF are 46 InternalCall externs that compute a result from their argument values and dereference nothing, and explicit-layout types often overlap fields that cannot be used to type-pun. Use the same <safety> element the analyzers already read as that record. A declaration carrying one gets 'safe' rather than 'unsafe', so an audit written in source survives the migration. Both flavors share an equivalence key so one fix-all pass applies the contract each declaration asks for, instead of settling on whichever it saw first. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
…y rules Under the updated memory safety rules, `unsafe` on a member becomes a caller-unsafe contract: every caller has to open an `unsafe` block. The migration fixers apply that contract conservatively, to every `extern`, every instance field in an explicit or extended layout, and every signature naming a pointer. That default is right for an unaudited declaration, but applied wholesale it would make a large number of members caller-unsafe that cannot violate memory safety at all, and push `unsafe` blocks out into their callers for no benefit. Audit those declarations and record the result in source with the `<safety>` element the migration analyzers already read, so the audit lands before the switch is flipped rather than being reconstructed afterwards from a red build. Annotated 242 declarations, among them: - The Math and MathF InternalCall entry points, which compute a result from their argument values and dereference nothing. - Pointer conversions that never dereference: the IntPtr and UIntPtr pointer constructors, conversion operators and ToPointer, plus Unsafe.AsPointer, Unsafe.Add and Unsafe.Subtract. Producing a pointer value is not itself unsafe; only dereferencing it is, and that already requires an unsafe context. - ArgumentNullException.ThrowIfNull(void*), which only compares the pointer against null. - Explicit-layout unions whose members are all unmanaged, such as Decimal.DecCalc, ComVariant, PropertyValue and the ComTypes DESCUNIONs. None of them overlaps a managed reference, so none can forge one. - Marshaller shape methods that only allocate, store or free, and the String FCall constructors over managed arrays and spans. Nine members additionally drop an `unsafe` modifier that existed only to name a pointer type under the old rules and would otherwise silently widen into a caller contract. Members that dereference caller memory, hand out a reference over unmanaged memory, or expose runtime-internal structures such as MethodTable are deliberately left alone, as are explicit-layout unions overlapping a managed reference. Where the judgement was close the declaration was left unannotated: a missing annotation costs an unnecessary `unsafe`, a wrong one costs a memory-safety hole. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
The previous change recorded the audit as <safety> documentation, which is what the analyzers read but not what the compiler reads. Where the updated memory safety rules require an explicit contract, on extern members and on field-like members in explicit or extended layouts, the compiler needs the modifier itself, so state it. The modifiers were produced by running the CS9389 and CS9392 fixers over CoreLib with the updated rules enabled, which emits 'safe' exactly where a <safety> element justifies it. 186 declarations are covered, among them the Math and MathF entry points and the all-unmanaged explicit layouts such as Decimal.DecCalc and PropertyValue. 'safe' parses regardless of the memory-safety opt-in, so this builds unchanged today and settles the contract before the switch is flipped. Declarations that need a pointer in their signature but no explicit contract keep documentation only, because the compiler pinned by this repo still rejects 'safe' there (CS9388). SA1206 is suppressed for CoreLib. StyleCop does not know the 'safe' contextual keyword and asks for it ahead of the accessibility modifier, which is not how it orders 'unsafe'. The suppression can go once StyleCop handles the keyword. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
dotnet-policy-service
Bot
added
the
linkable-framework
Contributor
|
Tagging subscribers to this area: @dotnet/interop-contrib |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the unsafe-v2 migration tooling (ILLink analyzer/code fixes) and audits CoreLib declarations to reduce unnecessary caller-unsafe requirements by introducing <safety> documentation and applying safe modifiers where appropriate.
Changes:
- Add shared syntax helpers to detect
<safety>XML documentation and use it to drive whether code fixes applyunsafevssafe. - Extend unsafe-migration code fixes to handle additional diagnostics and partial-type edge cases (and add CoreLib-only SA1206 suppression for
safemodifier ordering). - Annotate many CoreLib/Interop members and explicit-layout fields with
<safety>and/orsafeto record and apply audited safety contracts.
Reviewed changes
Copilot reviewed 50 out of 50 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs | Adds shared <safety> documentation detection helper. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzerHelpers.cs | Reuses shared <safety> detection helper. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs | Code-fix helper now chooses safe for documented declarations and shares fix-all equivalence key. |
| src/tools/illink/src/ILLink.CodeFix/Resources.resx | Adds resource string for “Mark documented member 'safe'”. |
| src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs | Improves invalid-unsafe removal, including partial-type parts handling. |
| src/tools/illink/src/ILLink.CodeFix/AddUnsafeToLibraryImportCodeFixProvider.cs | Adds SYSLIB1064 to fixable diagnostics (alongside IL5007). |
| src/libraries/System.Private.CoreLib/src/System/UIntPtr.cs | Adds <safety> docs and removes/adjusts unsafe where appropriate for pointer conversions. |
| src/libraries/System.Private.CoreLib/src/System/Threading/PortableThreadPool.cs | Applies safe to explicit-layout fields with <safety> justification. |
| src/libraries/System.Private.CoreLib/src/System/Threading/LowLevelLifoSemaphore.cs | Applies safe to explicit-layout padded field with <safety> justification. |
| src/libraries/System.Private.CoreLib/src/System/String.cs | Marks selected FCall constructors as safe with <safety> docs. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/TypeMapLazyDictionary.cs | Adds <safety> docs for pointer-holding fields/properties that don’t dereference. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Swift/SwiftTypes.cs | Adds <safety> docs to pointer-wrapper structs. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/Utf8StringMarshaller.cs | Adds <safety> doc for returning stored unmanaged pointer. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/SpanMarshaller.cs | Adds <safety> docs for pointer-null-check allocation/free patterns. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ReadOnlySpanMarshaller.cs | Adds <safety> docs for returning/storing pointers without dereference. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/PointerArrayMarshaller.cs | Adds <safety> docs for pointer-array reinterpretation and free patterns. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ComVariant.cs | Adds <safety> docs and applies safe to explicit-layout union fields. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/ArrayMarshaller.cs | Adds <safety> doc for null-check-only pointer parameter. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshalling/AnsiStringMarshaller.cs | Adds <safety> docs for free / returning stored pointer. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Java/StronglyConnectedComponent.cs | Adds <safety> doc for pointer field access semantics. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/Java/MarkCrossReferencesArgs.cs | Adds <safety> docs for pointer fields in interop args struct. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComTypes/ITypeInfo.cs | Applies safe + <safety> docs to explicit-layout unions. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/ComTypes/ITypeComp.cs | Applies safe + <safety> docs to explicit-layout union fields. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/Unsafe.cs | Adds <safety> docs for pointer-producing/pointer-arithmetic APIs. |
| src/libraries/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncStateMachineDispatcher.cs | Applies safe + <safety> docs to explicit-layout fields. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Pointer.cs | Adds <safety> doc for Unbox returning stored pointer value. |
| src/libraries/System.Private.CoreLib/src/System/IntPtr.cs | Adds <safety> docs and removes/adjusts unsafe where appropriate for pointer conversions. |
| src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/TraceLogging/PropertyValue.cs | Applies safe + <safety> docs across scalar explicit-layout union fields. |
| src/libraries/System.Private.CoreLib/src/System/Decimal.DecCalc.cs | Applies safe + <safety> docs to explicit-layout integer unions/buffers. |
| src/libraries/System.Private.CoreLib/src/System/Buffers/MemoryHandle.cs | Adds <safety> docs for pointer storage/return. |
| src/libraries/System.Private.CoreLib/src/System/ArgumentNullException.cs | Adds <safety> docs and removes member-unsafe where only null-check occurs. |
| src/libraries/System.Private.CoreLib/src/Internal/Padding.cs | Applies safe + <safety> docs for explicit-layout reference field. |
| src/libraries/Common/src/System/Number.NumberBuffer.cs | Adds <safety> doc for pointer conversion without dereference. |
| src/libraries/Common/src/Interop/Windows/NtDll/Interop.RtlNtStatusToDosError.cs | Marks P/Invoke as safe with <safety> justification. |
| src/libraries/Common/src/Interop/Windows/NtDll/Interop.IO_STATUS_BLOCK.cs | Applies safe + <safety> docs to explicit-layout integer union fields. |
| src/libraries/Common/src/Interop/Windows/Kernel32/Interop.GetCurrentThreadId.cs | Marks P/Invoke as safe with <safety> justification. |
| src/libraries/Common/src/Interop/Windows/Interop.OBJECT_ATTRIBUTES.cs | Adds <safety> docs describing pointer-field non-dereference semantics. |
| src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj | Suppresses SA1206 for safe modifier ordering in CoreLib. |
| src/coreclr/System.Private.CoreLib/src/System/StubHelpers.cs | Marks selected FCall/QCall helpers as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/JitInfo.CoreCLR.cs | Marks FCall getters as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/InteropServices/Marshal.CoreCLR.cs | Marks selected FCall APIs as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/GCSettings.CoreCLR.cs | Marks GCSettings FCalls as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Runtime/CompilerServices/RuntimeHelpers.CoreCLR.cs | Marks selected FCalls as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/RuntimeCustomAttributeData.cs | Applies safe + <safety> docs to explicit-layout integer union fields. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/AssemblyName.CoreCLR.cs | Adds <safety> docs for pointer fields in internal interop struct. |
| src/coreclr/System.Private.CoreLib/src/System/MathF.CoreCLR.cs | Marks many MathF FCall entry points as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Math.CoreCLR.cs | Marks many Math FCall entry points as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs | Marks selected GC QCalls/FCalls as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/System/Environment.CoreCLR.cs | Marks selected Environment QCalls/FCalls as safe with <safety> docs. |
| src/coreclr/System.Private.CoreLib/src/Internal/Runtime/InteropServices/ComActivationContextInternal.cs | Adds <safety> docs for pointer fields in internal interop struct. |
Suppressed comments (1)
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:142
- RemoveUnsafeModifierFromPartsAsync removes the 'unsafe' modifier from the first BaseTypeDeclarationSyntax with an unsafe modifier in each document, without verifying it's the same partial type symbol that triggered the diagnostic. If a file contains multiple 'unsafe partial' types, this can remove the modifier from an unrelated type.
Reverts the <safety> documentation and safe modifiers under InteropServices/Marshalling, leaving those declarations to the migration proper. The remaining annotations are unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (7)
src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs:115
_Collecthas a new<safety>audit comment but is still declared without asafemodifier. For consistency with other audited LibraryImport/QCall members (e.g., GetTotalMemory) and to make the contract explicit under unsafe-v2, consider marking this declarationsafeas well.
/// <safety>QCall that triggers a collection from scalar generation and mode arguments; it accesses no caller-supplied memory.</safety>
[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "GCInterface_Collect")]
private static partial void _Collect(int generation, int mode, [MarshalAs(UnmanagedType.U1)] bool lowMemoryPressure);
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs:60
- RegisterAddUnsafeCodeFixAsync now conditionally applies
safe(and a different displayed title) when a declaration contains<safety>XML documentation. There doesn't appear to be a test covering this documented-member path (including Fix All behavior with the shared equivalence key). Adding one would help ensure audited members don't accidentally become caller-unsafe during migration.
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:116 - The comment says generated parts can't be edited, but the code currently uses Solution.GetDocumentId(part.SyntaxTree), which may still yield a DocumentId for non-editable / source-generated trees. This can lead the fixer to attempt edits it can't apply. Use Solution.GetDocument(part.SyntaxTree) (null for generated parts) to restrict edits to real source documents, consistent with MatchPartialSafetyModifierCodeFixProvider.
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:149 - RemoveUnsafeModifierFromPartsAsync removes the unsafe modifier from the first BaseTypeDeclarationSyntax in each document that has an unsafe modifier. If the document contains multiple types (or nested types) with unsafe modifiers, this can remove the modifier from an unrelated declaration. Consider carrying the exact SyntaxReference span(s) for the symbol's parts and re-resolving those nodes in the current syntax root (similar to MatchPartialSafetyModifierCodeFixProvider.GetDeclarationToEditAsync) so the edit targets the intended partial type declaration.
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:78 - The new partial-type path (walking DeclaringSyntaxReferences to find the part that actually has the unsafe modifier) isn't covered by tests; existing RemoveInvalidUnsafeCodeFixTests only cover single-declaration cases. Adding a test where CS9377 is reported on one partial declaration while the unsafe modifier is on another file would help prevent regressions in this new behavior.
src/coreclr/System.Private.CoreLib/src/System/GC.CoreCLR.cs:92 _StartNoGCRegionnow has a<safety>audit comment but is still declared without asafemodifier, unlike_EndNoGCRegionand other audited QCall/LibraryImport members in this file. If this method is intended to be audited safe, it should be markedsafeso the contract is explicit and tooling doesn't continue to treat it as needing annotation.
This issue also appears on line 113 of the same file.
/// <safety>QCall that starts a no-GC region from scalar size and flag arguments and returns a status code; it accesses no caller-supplied memory.</safety>
[LibraryImport(RuntimeHelpers.QCall, EntryPoint = "GCInterface_StartNoGCRegion")]
internal static partial int _StartNoGCRegion(long totalSize, [MarshalAs(UnmanagedType.Bool)] bool lohSizeKnown, long lohSize, [MarshalAs(UnmanagedType.Bool)] bool disallowFullBlockingGC);
src/coreclr/System.Private.CoreLib/System.Private.CoreLib.csproj:48
- Suppressing SA1206 at the project level is broader than necessary and can hide unrelated SA1206 regressions across all of CoreLib. If possible, consider narrowing the suppression (e.g.,
#pragma warning disable SA1206only aroundsafeusages, or a more targeted suppression mechanism) so SA1206 continues to catch genuine keyword-order issues elsewhere.
<!-- SA1206 does not know the 'safe' contextual keyword and asks for it ahead of the accessibility
modifier. Suppress it until StyleCop orders 'safe' the way it orders 'unsafe'. -->
<NoWarn>$(NoWarn);SA1206</NoWarn>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:149
- RemoveUnsafeModifierFromPartsAsync removes the first
BaseTypeDeclarationSyntaxwith anunsafemodifier in each document, without verifying it’s the partial type that corresponds to the reported symbol. If the document contains multiple types (or multiple unsafe partials), this can removeunsafefrom the wrong type and/or leave the originally-diagnostic declaration unchanged, which defeats the purpose of the fix and may introduce new errors.
Consider tracking the exact declarations to edit (e.g., collect the SyntaxReference/TextSpan for each declaring syntax that actually has the unsafe modifier) and then FindNode(span) in the corresponding document root to remove the modifier from that specific declaration (potentially multiple per document).
Says that the conversion is from a managed pointer to an unmanaged one, the terms ECMA-335 uses, and that the danger belongs to the dereference rather than to this call. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:149
- RemoveUnsafeModifierFromPartsAsync removes the first
unsafe-modified type it finds in each document, but GetPartsWithUnsafeModifierAsync only records DocumentIds (not the specific partial declaration). If a document contains multipleunsafetypes, the fix can remove the modifier from the wrong one and leave the intended one unchanged. For example,src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cscontains multipleunsafe partial structdeclarations (RuntimeTypeHandle at line 18 and RuntimeMethodHandle at line 985), so a diagnostic for the latter could cause the fixer to edit the former.
Consider tracking the exact declaration to edit per part (e.g., return (DocumentId, TextSpan) or SyntaxReference for each declaring syntax reference that has the unsafe modifier), then in RemoveUnsafeModifierFromPartsAsync locate that node by span and remove unsafe from that specific declaration.
The suppression was on src/coreclr/System.Private.CoreLib, but most of the annotated declarations live in the shared sources under src/libraries/System.Private.CoreLib/src, which Mono and NativeAOT compile into their own CoreLib, and in src/libraries/Common/src, which a further ten or so assemblies pull in. Every one of those builds hit the rule. The problem is not specific to a project: StyleCop does not know the 'safe' contextual keyword at all. Turn the rule off in the src configuration and note why, rather than spraying the suppression across each project that happens to compile an annotated file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 78ab2fd-adea-41cf-a67f-c0b823438e07
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated no new comments.
Suppressed comments (5)
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:142
- RemoveUnsafeModifierFromPartsAsync looks like it can remove the first
unsafetype declaration in each document, not necessarily the specific partial declaration for the symbol that triggered the diagnostic. If the document contains any otherunsafetype earlier in the syntax tree (or multiple partial declarations in the same file), this fixer could removeunsafefrom the wrong type and still leave the original diagnostic unresolved.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs:60 - RegisterAddUnsafeCodeFixAsync now conditionally adds
safe(instead ofunsafe) when a declaration has a<safety>doc element. This is a significant behavior change for multiple code-fix providers (including the LibraryImport fixer) and should have targeted tests to ensure the<safety>element is detected in the intended cases and that the fix addssaferather thanunsafewhen requested.
src/tools/illink/src/ILLink.CodeFix/AddUnsafeToLibraryImportCodeFixProvider.cs:37 - FixableDiagnosticIds now includes SYSLIB1064 in addition to IL5007, but the existing AddUnsafeToLibraryImport code-fix tests only exercise the IL5007 (pre-opt-in) path. Adding at least one test case for the updated-memory-safety-rules configuration (where the generator reports SYSLIB1064) would help ensure the fixer remains effective after opt-in.
src/tools/illink/src/ILLink.CodeFix/RemoveInvalidUnsafeCodeFixProvider.cs:80 - The new partial-type path (finding the unsafe modifier on a different part and applying a solution-wide fix) adds new behavior that isn't covered by existing code-fix tests. It would be good to add a multi-document test where the diagnostic is reported on one partial declaration but the
unsafemodifier is on another file/part, and verify the fixer updates the other part (and only that part).
This issue also appears on line 138 of the same file.
eng/CodeAnalysis.src.globalconfig:1376
- eng/CodeAnalysis.src.globalconfig is a global analyzer config (no path scoping). Disabling SA1206 here turns off keyword-ordering checks for all
src/**code, which is a pretty broad suppression for a StyleCop limitation affecting only the newsafemodifier. Can this be scoped more narrowly (e.g., to the directories/files that actually usesafe) so SA1206 continues to catch genuine ordering issues elsewhere?
# SA1206: Keyword ordering
# Disabled: StyleCop does not know the 'safe' contextual keyword introduced by the updated
# memory safety rules and asks for it ahead of the accessibility modifier, which is not how
# it orders 'unsafe'. Re-enable once StyleCop handles the keyword.
dotnet_diagnostic.SA1206.severity = none
27 tasks
jkoritzinsky
reviewed
Aug 3, 2026
| } | ||
|
|
||
| // Terminates this process with the given exit code. | ||
| /// <safety>QCall that passes the integer exit code to the runtime to terminate the process; it accesses no caller-supplied memory.</safety> |
Member
There was a problem hiding this comment.
Are we going to require every LibraryImport to have a safety comment, even ones with trivial signatures?
Member
Author
There was a problem hiding this comment.
Are we going to require every LibraryImport to have a safety comment, even ones with trivial signatures?
It is not really required, but for consistency it's nice to have an explanation why something needed an explicit caller-safe or caller-unsafe keyword for every memeber. Although, we probably won't add it to all functions with unmanaged pointrs in the sig..
But all LibraryImport (just like all extern and just like all fields of Extended/Explicit layout structs) have to have an explicit safe or unsafe keyword on them.
| # Disabled: StyleCop does not know the 'safe' contextual keyword introduced by the updated | ||
| # memory safety rules and asks for it ahead of the accessibility modifier, which is not how | ||
| # it orders 'unsafe'. Re-enable once StyleCop handles the keyword. | ||
| dotnet_diagnostic.SA1206.severity = none |
Member
There was a problem hiding this comment.
Is this one even relevant anymore? Wasn't it entirely replaced by IDE0036, which is more powerful and stays up to date with new language keywords?
Member
Author
There was a problem hiding this comment.
I don't know, but it does still kick in. I've got an impression that stylecop is not actively maintained?
Member
There was a problem hiding this comment.
I'm not sure if it is maintained anymore either, its not had an update in over 8 months.
Certainly many of the rules have been replaced by in-box ones as well, so it might be worth considering what's adding value still (and what can be removed or disabled).
| /// <safety>Overlaps only Byte8; both views are non-reference integers (int and long), so reinterpreting one as the other cannot forge a managed reference or read out of bounds.</safety> | ||
| [FieldOffset(0)] | ||
| public int Byte4; | ||
| public safe int Byte4; |
Member
There was a problem hiding this comment.
Not necessarily important for this PR, but is this one where longer term it'd be better to just have a long field and then extract the lower 32-bits out of it? That should already be optimized/handled by the JIT.
| /// <safety>Runtime FCall that updates the GC latency mode from an enum argument and returns a status enum; it accesses no caller-supplied memory.</safety> | ||
| [MethodImpl(MethodImplOptions.InternalCall)] | ||
| private static extern SetLatencyModeStatus SetGCLatencyMode(GCLatencyMode newLatencyMode); | ||
| private static safe extern SetLatencyModeStatus SetGCLatencyMode(GCLatencyMode newLatencyMode); |
Member
There was a problem hiding this comment.
Is this one safe if an arbitrary GCLatencyMode value is passed in?
Member
Author
There was a problem hiding this comment.
I don't see any validation so hard to say, I assume we should add one (and keep it safe).
Member
There was a problem hiding this comment.
There is validation in the safe public API:
This is the unsafe FCall that accepts valid value only.
Member
There was a problem hiding this comment.
So this one should presumably be unsafe then, since its on the caller to ensure newLatencyMode is in range?
Are there any other FCALLs we've annotated where the responsibility of "correct input" is only validated by the public entry point and so the FCALL itself should remain unsafe?
| /// <safety>The Scalar union overlaps only unmanaged value types and no managed reference, so reading or writing this field cannot forge a managed reference or access memory out of bounds.</safety> | ||
| [FieldOffset(0)] | ||
| public decimal AsDecimal; | ||
| public safe decimal AsDecimal; |
Member
There was a problem hiding this comment.
So this one in particular is not safe. Decimal has what are considered "illegal bit patterns" and we have in the past had issues when such bit patterns are encountered. While we should be handling them well today, that isn't a strict guarantee.
tannergooding
approved these changes
Aug 3, 2026
tannergooding
left a comment
tannergooding
left a comment
Member
There was a problem hiding this comment.
Overall LGTM. One case where decimal is being bitcast and that isn't strictly safe; then a few places where we might be able to remove unions (explicit layout) trivially, but those aren't important for this PR.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Preparing System.Private.CoreLib for the unsafe-v2 switch.
Normally, it is expected to audit members for safety after/during migration (automatic tools aren't allowed/can't justify
safeplacement so it has to be done by a human/LLM), but the corelib contains so much unsafe that this PR aims to reduce the noise beforehand. This PR adds<safety>andsafe(where it's explicitly required: fields of explict/extended layout structs andextern).The migration tools won't automatically/conservatively slap
unsafeon methods with this comment.Analyzers / code fixers
AddUnsafeToLibraryImportCodeFixProvideronly declaredIL5007, which stands down once an assembly opts in — the generator reportsSYSLIB1064for the same methods from then on, so the fix had nothing to do. Declare both.RemoveInvalidUnsafeCodeFixProviderassumed the reported declaration is the one holding the modifier. A partial type is reported once for the merged symbol, so 13CS9377were left behind in CoreLib (reported onPath.cs, modifier onPath.Windows.cs). Walk the other parts.CS9389/CS9392fixers now emitsafeinstead ofunsafewhen the declaration carries a<safety>element, so an audit written in source survives the migration. Both flavors share an equivalence key so one fix-all pass applies the contract each declaration asks for.StyleCop
SA1206 does not know the
safecontextual keyword and wants it ahead of the accessibility modifier, which is not how it ordersunsafe. Suppressed for CoreLib only, with a comment; it can go once StyleCop handles the keyword.