Add unsafe context migration code fixes#131337
Open
EgorBo wants to merge 4 commits into
Open
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86742a9-ab20-4209-83f8-d7074a99b968
|
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. |
Contributor
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the ILLink Roslyn analyzer/code-fix test infrastructure with new (DEBUG-only) “unsafe-v2” migration code fixers: one that introduces a minimal unsafe context for compiler-reported unsafe usages, and another that propagates intentional unsafe contracts across partials/overrides/interface implementations. It also removes the prior RequiresUnsafeCodeFixProvider and its tests/resources.
Changes:
- Add
AddUnsafeContextCodeFixProviderand a comprehensive test suite covering many syntax positions and compiler diagnostics. - Add
SynchronizeUnsafeContractCodeFixProvider, shared contract helpers, and tests for contract propagation scenarios. - Remove the legacy
RequiresUnsafeCodeFixProviderand its associated tests, replacing the user-facing resource strings accordingly.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeMigrationTestHelpers.cs | Minor formatting update in shared test setup for unsafe-v2 scenarios. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs | Adds new tests validating unsafe contract propagation behavior. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs | Removes tests for the deprecated legacy fixer. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/AddUnsafeContextCodeFixTests.cs | Adds new tests validating unsafe-context insertion across many code shapes. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs | Adds GetSafeModifier helper for safe→unsafe token replacement scenarios. |
| src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeContractHelpers.cs | Introduces shared symbol/syntax helpers for unsafe contract propagation logic. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs | Adds SetUnsafeModifierAsync, adjusts insertion ordering for partial, and exposes WithModifiers. |
| src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs | New code fix provider that computes a contract closure and applies unsafe propagation edits. |
| src/tools/illink/src/ILLink.CodeFix/Resources.resx | Replaces removed fixer title and adds titles for the new code fixers. |
| src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs | Removes the deprecated legacy fixer implementation. |
| src/tools/illink/src/ILLink.CodeFix/ILLink.CodeFixProvider.csproj | Links in the new shared UnsafeContractHelpers.cs for code fix usage. |
| src/tools/illink/src/ILLink.CodeFix/AddUnsafeContext.cs | New code fix provider implementing minimal unsafe statement/expression introduction logic. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a86742a9-ab20-4209-83f8-d7074a99b968
Roslyn does not treat an unsafe expression as an unsafe context for property and indexer accessors or for method group conversions, because those are bound by the enclosing binder. The fixer used to wrap such operations anyway, which left the diagnostic in place and nested another unsafe expression on every later pass. It now writes an explicit cast inside the unsafe expression for those operations, declines the fix when no cast can express it (ref-returning accessors, unspeakable types), and never re-wraps an expression that is already inside an unsafe expression. Assignment targets, increments, and deconstruction right sides are handled as statements instead. Modifiers that the fixers add now come with a <safety>TODO: Audit</safety> stub. Without it IL5005 removed the modifier that had just been added, so a second migration pass undid the first one. Contract propagation no longer marks sibling overrides and implementations: a safe member may override or implement a caller-unsafe one, so widening them only grew the audit surface. Replacing an explicit safe modifier is offered under its own title because it discards a deliberate audit. Also merges a new unsafe region with adjacent generated regions, registers the fixes for every diagnostic in the context, and hardens top level statement replacement, statement list access, and the identifier fallback used when expanding a statement range. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c3f7fe4-85f4-40f3-9ccc-e6113369014b
The unsafe expression template is now parsed with the parse options of the document being fixed instead of a fixed set, so the generated syntax always matches the language mode the project compiles with, and the fix is declined when that mode cannot express an unsafe expression. The template also carries the syntax kind used to detect an enclosing unsafe expression, which removes the static probe that guessed it. Generated safety documentation reuses the line ending the file already uses. Member declarations carry their indentation but not the preceding line break, so the previous fallback emitted a carriage return into files that use line feeds only. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1c3f7fe4-85f4-40f3-9ccc-e6113369014b
EgorBo
marked this pull request as ready for review
July 25, 2026 09:38
|
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. |
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.
Follow-up to #131002
This PR adds the remaining code fixers (along with #131245) that assist in migrating to the new unsafe-v2 rules (unsafe evolution). The goals are:
New code fixers added in this PR:
[fixer]
AddUnsafeContextCodeFixProviderfixesCS9360,CS9361,CS9362,CS9363, andCS9376by introducing an unsafe context around the compiler-reported operation.The fixer prefers an
unsafe { /* ... */ }statement containing a// SAFETY: Auditcomment. When a statement would change scope, lifetime, control flow, or otherwise be invalid, it uses anunsafe(/* SAFETY: Audit */ expression)expression instead.Local declarations are split when that preserves semantics. In particular, stackalloc-to-span declarations are rewritten with a
scopedforward declaration:The fixer also handles constructor initializers, using aliases, expression-bodied members, catch filters, async/iterator restrictions, directives, top-level statements, scoped refs, implicit conversions, and generated disposal/enumeration operations. It declines to offer a fix when no semantics-preserving automated transformation is available.
[fixer]
SynchronizeUnsafeContractCodeFixProviderfixes Roslyn's unsafe-to-safe contract mismatch diagnosticsCS9364,CS9365, andCS9366, along with partial modifier mismatchesCS0764andCS9390.By the time this fixer runs, removable caller-unsafe modifiers have already been handled, so surviving unsafe contracts are propagated through source base members, interface declarations, overrides, implementations, and partial declarations. A pure
safepartial mismatch defaults tounsafe, sincesafeis not currently valid on the non-extern partial declaration.The fixer does not add a new diagnostic for the opposite direction when the compiler accepts a safe implementation of an unsafe contract.
The old (added long ago by Andy)
RequiresUnsafeCodeFixProvideris removed. It only handledCS9362, could change caller contracts by marking parent members unsafe, and did not preserve scope and lifetime reliably.Diagnostics IDs
Just for reference:
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.Once this and #131245 are in, we should be able to perform a migration.