Add unsafe modifier migration code fixer - #131002
Conversation
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
There was a problem hiding this comment.
Pull request overview
Adds initial “unsafe modifier migration” support to the ILLink analyzer/codefix tooling (DEBUG-only), wiring a new IL5005-style diagnostic and a code fix that normalizes unsafe modifiers based on signature/docs, plus build plumbing to enable the feature via EnableUnsafeMigration.
Changes:
- Introduces
UnsafeMigrationAnalyzer(IL5005) andUnsafeModifierMigrationCodeFixProviderto report/fix unsafe modifier normalization whenEnableUnsafeMigrationis enabled. - Adds supporting shared diagnostic IDs/categories/strings and exposes
EnableUnsafeMigrationas a compiler-visible MSBuild property. - Extends the existing analyzer/codefix test suite with targeted tests for the new diagnostic and fixer.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs | Adds verifier helpers and new tests for the unsafe modifier migration diagnostic and code fix. |
| src/tools/illink/src/ILLink.Shared/SharedStrings.resx | Adds title/message resources for the new unsafe migration diagnostic. |
| src/tools/illink/src/ILLink.Shared/DiagnosticId.cs | Adds UnsafeModifierMigration (DEBUG-only) and maps it to a new diagnostic category. |
| src/tools/illink/src/ILLink.Shared/DiagnosticCategory.cs | Adds Unsafe diagnostic category (DEBUG-only). |
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalyzer.cs | New analyzer that reports a single “file needs unsafe modifier migration” diagnostic when updates are detected. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationAnalysis.cs | New semantic analysis that computes which declarations should/shouldn’t have unsafe based on rules (docs/pointers/interop). |
| src/tools/illink/src/ILLink.RoslynAnalyzer/MSBuildPropertyOptionNames.cs | Adds EnableUnsafeMigration MSBuild property name (DEBUG-only constant). |
| src/tools/illink/src/ILLink.RoslynAnalyzer/build/Microsoft.NET.ILLink.Analyzers.props | Exposes EnableUnsafeMigration as a compiler-visible property. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeModifierMigrationCodeFixProvider.cs | New code fix provider to apply modifier normalization across a document when enabled. |
| src/tools/illink/src/ILLink.CodeFix/Resources.resx | Adds the code fix title resource string. |
| eng/liveILLink.targets | Wires EnableUnsafeMigration into LiveILLink, enabling unsafe analyzer and setting the updated-memory-safety-rules feature flag. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs:130
DiagnosticDescriptor.HelpLinkUriis a nullable string andDiagnosticDescriptors.GetDiagnosticDescriptor(...)passeshelpLinkUrithrough directly. For these new diagnosticsDiagnosticIdExtensions.GetHelpUrireturnsnull, sodescriptor.HelpLinkUriwill benullandAssert.Empty(descriptor.HelpLinkUri)will fail. Use a null check (orstring.IsNullOrEmpty) instead ofAssert.Empty.
DiagnosticDescriptor descriptor = DiagnosticDescriptors.GetDiagnosticDescriptor(diagnosticId);
Assert.Equal("Unsafe", descriptor.Category);
Assert.Empty(descriptor.HelpLinkUri);
}
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b5b020c-fb1b-459b-a49e-36b77cb62d6c
|
@333fred hopefully, I've addressed your feedback |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68ec5b5d-ac1d-42bc-b2fb-9b96ea2a0456
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 68ec5b5d-ac1d-42bc-b2fb-9b96ea2a0456
This PR adds new analyzers and codefixers (not yet shipping) that assist in migrating to the new unsafe-v2 rules ([unsafe evolution](https://github.com/dotnet/csharplang/blob/main/proposals/unsafe-evolution.md)). The goals are: * Analyzers/code-fixers should be idempotent as migrations to the new rules could be incremental. * Code-fixers should rely on the existing Roslyn analyzers as much as possible. ### New analyzers/code-fixers added in this PR: 1. [fixer] **`AddUnsafeToExternCodeFixProvider`** fixes `CS9389` (_see 'Diagnostics IDs' below_) by marking an `extern` member `unsafe` by default. Developers can replace it with `safe` after auditing the interop boundary. 2. [fixer] **`RemoveInvalidUnsafeCodeFixProvider`** fixes `CS9377` and unsafe-specific `CS0106` diagnostics by removing the compiler-reported meaningless or invalid `unsafe` modifier (for example, from a type declaration). 3. [analyzer] **`UnsafeMemberMissingSafetyDocumentationAnalyzer` (`IL5005`)** reports unsafe members without a `<safety>` XML comment. 4. [fixer] **`RemoveUndocumentedUnsafeCodeFixProvider`** fixes `IL5005` by removing an undocumented `unsafe` modifier when it is assumed to be an unsafe-v1 lexical scope rather than an intentional caller-unsafe contract. Members with pointer or function-pointer signatures keep `unsafe` for backward compatibility because such members were effectively caller-unsafe under unsafe-v1 (and in >90% cases they end up dereference their unmanaged pointers anyway). Field-like members in explicit or extended-layout types keep `unsafe`: removing it would recreate `CS9392`, and choosing `safe` requires an explicit developer audit. `AddUnsafeToFieldCodeFixProvider` remains responsible for defaulting unclassified fields to `unsafe`. 5. [analyzer] **`PointerSignatureRequiresUnsafeAnalyzer` (`IL5006`)** reports members with pointer or function-pointer signatures that are missing `unsafe`. A `<safety>` XML comment suppresses the diagnostic when the signature is intentionally safe, for example `ArgumentNullException.ThrowIfNull(void*)`. An existing explicit `safe` modifier also suppresses the diagnostic. 6. [fixer] **`AddUnsafeToPointerSignatureCodeFixProvider`** fixes `IL5006` by adding the missing `unsafe` modifier. 7. [fixer] **`AddUnsafeToFieldCodeFixProvider`** fixes `CS9392` by adding `unsafe` to instance fields, field-backed properties, and field-like events in explicit or extended-layout types. Primary-constructor parameters are intentionally not changed because C# cannot apply `unsafe` or `safe` to the parameter declaration. ### Diagnostics IDs Just for reference - `CS9389` [**Roslyn**] - An `extern` member must be explicitly marked `unsafe` or `safe`. - `CS9377` [**Roslyn**] - The `unsafe` modifier has no effect at this location under the updated memory safety rules. - `CS0106` [**Roslyn**] - The `unsafe` modifier is not valid for this item. This PR's fixer only activates when `unsafe` is the invalid modifier. - `CS9360` [**Roslyn**] - An unsafe operation may only be used in an unsafe context. - `CS9361` [**Roslyn**] - A `stackalloc` expression without an initializer inside `[SkipLocalsInit]` may only be used in an unsafe context. - `CS9362` [**Roslyn**] - A member marked `unsafe` must be used in an unsafe context. - `CS9363` [**Roslyn**] - A member with pointers in its signature must be used in an unsafe context. - `CS9376` [**Roslyn**] - An unsafe context is required when an `unsafe` constructor satisfies a `new()` constraint. - `CS9364` [**Roslyn**] - An unsafe member cannot override a safe member. - `CS9365` [**Roslyn**] - An unsafe member cannot implicitly implement a safe member. - `CS9366` [**Roslyn**] - An unsafe member cannot explicitly implement a safe member. - `CS0764` [**Roslyn**] - Both partial member declarations must be `unsafe`, or neither may be `unsafe`. - `CS9390` [**Roslyn**] - Both partial member declarations must be marked `safe`, or neither may be marked `safe`. - `CS9392` [**Roslyn**] - A field in an explicit or extended-layout type must be marked `unsafe` or `safe`. - `CS9388` [**Roslyn**] - The `safe` modifier is only valid on non-unsafe `extern` members or field-like members of explicit or extended-layout types. - `IL5005` [**This PR**] - An unsafe member has no `<safety>` XML documentation. - `IL5006` [**This PR**] - A member with a pointer or function-pointer signature is missing `unsafe`, unless a `<safety>` comment documents why it is safe (for example, when unsafe-v1 code relied on `unsafe` on the containing type). ### For follow ups - Implement a code fixer for unsafe-context diagnostics `CS9360`, `CS9361`, `CS9362`, `CS9363`, and `CS9376`. It should introduce `unsafe { /* SAFETY: Audit */ }` blocks or `unsafe(/* SAFETY: Audit */ ...)` expressions. - Synchronize intentional caller-unsafe contracts across overrides, interface implementations, and partial declarations. This includes Roslyn's unsafe-to-safe mismatch diagnostics `CS9364`, `CS9365`, and `CS9366`, partial modifier mismatches `CS0764` and `CS9390`, and the opposite migration case where an unsafe interface/base contract should be propagated to an implementation that is currently unannotated. - Figure out what to do with `LibraryImport`. Roslyn currently does not treat it as `extern`, `safe` cannot be applied to it, and its generated implementation is not compiled under the updated memory-safety rules. See dotnet/roslyn#84555. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 9b5b020c-fb1b-459b-a49e-36b77cb62d6c Copilot-Session: e39a2bb4-8a72-40d6-8a1c-d98b6c346afe Copilot-Session: 68ec5b5d-ac1d-42bc-b2fb-9b96ea2a0456
This PR adds code fixers for the **contract consistency** part of the unsafe-v2 migration tooling ([tracking issue](#131451) §3), continuing #131002. Both fixers are built entirely on diagnostics Roslyn already reports, so no new diagnostic ID is introduced. Nothing changes under unsafe-v1. ### Background Under the updated memory safety rules `unsafe` on a member is part of its public contract, so the compiler rejects a member that is `unsafe` when the member it overrides or implements is not (callers holding a base reference would never see the added obligation). It likewise requires both halves of a partial member to agree. Neither error has a fix today, and both are common during migration: marking a member `unsafe` immediately breaks its overrides, and source generators author one half of a partial. ### Changes in this PR 1. 🔧 **`SynchronizeUnsafeContractCodeFixProvider`** fixes `CS9364`, `CS9365` and `CS9366` (_see 'Diagnostics IDs' below_) with two actions, since the compiler cannot tell which side is wrong: - **Narrow the derived member**, which is correct when the `unsafe` was an unsafe-v1 lexical scope. For an `extern` member this replaces `unsafe` with `safe` rather than removing it, because an `extern` member must keep an explicit marker. - **Mark the base member `unsafe`**, which is correct when the contract was under-annotated. Only offered when that member is declared in source. 2. 🔧 **`MatchPartialSafetyModifierCodeFixProvider`** fixes `CS0764` and `CS9390` by copying the safety modifier onto the declaration that lacks it. The modifier is always propagated rather than removed: for `unsafe` that preserves the caller obligation, and for `safe` removing it would reintroduce `CS9389` on an `extern` member. Both diagnostics for partials are reported at `implementation.GetFirstLocation()` regardless of which part carries the modifier, so the fixer frequently has to edit a different document than the one the diagnostic is in. ### Notable cases handled - **`extern` overrides.** Removing `unsafe` from `public unsafe extern override object M();` only trades `CS9364` for `CS9389`, and `AddUnsafeToExternCodeFixProvider` from #131002 would then add it straight back. The fix offers `safe` instead. This shape is real, e.g. `RuntimeFieldInfo.UnsafeGetValue`. - **Partial base members.** A partial symbol's `DeclaringSyntaxReferences` only covers its own half, so annotating the base has to walk both parts or the fix trades `CS9364` for `CS0764`. Edits are batched per document so no node is re-resolved against a tree whose spans have already shifted. - **Conflicting partial contracts.** When one part is `safe` and the other `unsafe`, the compiler reports both `CS0764` and `CS9390`, and naively satisfying each would put both modifiers on one declaration (`CS9388`). No fix is offered, since resolving it requires deciding which contract is correct. - **Property accessors.** `CS9364` is reported once per accessor while `unsafe` may sit on the containing property, so the fixer walks outward to the declaration that actually carries the modifier. ### Diagnostics IDs Just for reference - `CS9364` [**Roslyn**] - An `unsafe` member cannot override a safe member. - `CS9365` [**Roslyn**] - An `unsafe` member cannot implicitly implement a safe member. - `CS9366` [**Roslyn**] - An `unsafe` member cannot explicitly implement a safe member. - `CS0764` [**Roslyn**] - Both partial member declarations must be `unsafe`, or neither may be `unsafe`. - `CS9390` [**Roslyn**] - Both partial member declarations must be marked `safe`, or neither may be marked `safe`. - `CS9388` [**Roslyn**] - The `safe` modifier is only valid on non-unsafe `extern` members or field-like members of explicit or extended-layout types. - `CS9389` [**Roslyn**] - An `extern` member must be explicitly marked `unsafe` or `safe`. ### Known limitations / follow ups - The "mark the base member `unsafe`" action is only offered when the base is in source. Roslyn filters `RequiresUnsafeAttribute` out of `ISymbol.GetAttributes()`, so a contract coming from a referenced assembly is invisible to analyzers. - The opposite migration direction, where an override or implementation is left unannotated while the base is `unsafe`, is **not** covered here. That is legal and the compiler is silent, so it needs its own analyzer (`IL5008` in the tracking issue) and is deliberately left out of this PR. - `MatchPartialSafetyModifierCodeFixProvider` skips parts that live in generated code, since there is no editable document for them. ### Testing `ILLink.RoslynAnalyzer.Tests`: 1223 passed. 12 new tests cover both narrowing and base-annotating actions across methods, properties, and implicit and explicit interface implementations, plus regressions for the `extern`, partial-base and conflicting-contract cases above. > [!NOTE] > This description and the changes in this PR were generated with GitHub Copilot. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
This PR adds new analyzers and codefixers (not yet shipping) that assist in migrating to the new unsafe-v2 rules (unsafe evolution). The goals are:
New analyzers/code-fixers added in this PR:
[fixer]
AddUnsafeToExternCodeFixProviderfixesCS9389(see 'Diagnostics IDs' below) by marking anexternmemberunsafeby default. Developers can replace it withsafeafter auditing the interop boundary.[fixer]
RemoveInvalidUnsafeCodeFixProviderfixesCS9377and unsafe-specificCS0106diagnostics by removing the compiler-reported meaningless or invalidunsafemodifier (for example, from a type declaration).[analyzer]
UnsafeMemberMissingSafetyDocumentationAnalyzer(IL5005) reports unsafe members without a<safety>XML comment.[fixer]
RemoveUndocumentedUnsafeCodeFixProviderfixesIL5005by removing an undocumentedunsafemodifier when it is assumed to be an unsafe-v1 lexical scope rather than an intentional caller-unsafe contract. Members with pointer or function-pointer signatures keepunsafefor backward compatibility because such members were effectively caller-unsafe under unsafe-v1 (and in >90% cases they end up dereference their unmanaged pointers anyway). Field-like members in explicit or extended-layout types keepunsafe: removing it would recreateCS9392, and choosingsaferequires an explicit developer audit.AddUnsafeToFieldCodeFixProviderremains responsible for defaulting unclassified fields tounsafe.[analyzer]
PointerSignatureRequiresUnsafeAnalyzer(IL5006) reports members with pointer or function-pointer signatures that are missingunsafe. A<safety>XML comment suppresses the diagnostic when the signature is intentionally safe, for exampleArgumentNullException.ThrowIfNull(void*). An existing explicitsafemodifier also suppresses the diagnostic.[fixer]
AddUnsafeToPointerSignatureCodeFixProviderfixesIL5006by adding the missingunsafemodifier.[fixer]
AddUnsafeToFieldCodeFixProviderfixesCS9392by addingunsafeto instance fields, field-backed properties, and field-like events in explicit or extended-layout types. Primary-constructor parameters are intentionally not changed because C# cannot applyunsafeorsafeto the parameter declaration.Diagnostics IDs
Just for reference
CS9389[Roslyn] - Anexternmember must be explicitly markedunsafeorsafe.CS9377[Roslyn] - Theunsafemodifier has no effect at this location under the updated memory safety rules.CS0106[Roslyn] - Theunsafemodifier is not valid for this item. This PR's fixer only activates whenunsafeis the invalid modifier.CS9360[Roslyn] - An unsafe operation may only be used in an unsafe context.CS9361[Roslyn] - Astackallocexpression without an initializer inside[SkipLocalsInit]may only be used in an unsafe context.CS9362[Roslyn] - A member markedunsafemust be used in an unsafe context.CS9363[Roslyn] - A member with pointers in its signature must be used in an unsafe context.CS9376[Roslyn] - An unsafe context is required when anunsafeconstructor satisfies anew()constraint.CS9364[Roslyn] - An unsafe member cannot override a safe member.CS9365[Roslyn] - An unsafe member cannot implicitly implement a safe member.CS9366[Roslyn] - An unsafe member cannot explicitly implement a safe member.CS0764[Roslyn] - Both partial member declarations must beunsafe, or neither may beunsafe.CS9390[Roslyn] - Both partial member declarations must be markedsafe, or neither may be markedsafe.CS9392[Roslyn] - A field in an explicit or extended-layout type must be markedunsafeorsafe.CS9388[Roslyn] - Thesafemodifier is only valid on non-unsafeexternmembers or field-like members of explicit or extended-layout types.IL5005[This PR] - An unsafe member has no<safety>XML documentation.IL5006[This PR] - A member with a pointer or function-pointer signature is missingunsafe, unless a<safety>comment documents why it is safe (for example, when unsafe-v1 code relied onunsafeon the containing type).For follow ups
Implement a code fixer for unsafe-context diagnostics
CS9360,CS9361,CS9362,CS9363, andCS9376. It should introduceunsafe { /* SAFETY: Audit */ }blocks orunsafe(/* SAFETY: Audit */ ...)expressions.Synchronize intentional caller-unsafe contracts across overrides, interface implementations, and partial declarations. This includes Roslyn's unsafe-to-safe mismatch diagnostics
CS9364,CS9365, andCS9366, partial modifier mismatchesCS0764andCS9390, and the opposite migration case where an unsafe interface/base contract should be propagated to an implementation that is currently unannotated.Figure out what to do with
LibraryImport. Roslyn currently does not treat it asextern,safecannot be applied to it, and its generated implementation is not compiled under the updated memory-safety rules. See safe keyword on LibraryImport roslyn#84555.