Skip to content

Add unsafe contract consistency code fixes - #131454

Open
EgorBo wants to merge 2 commits into
dotnet:mainfrom
EgorBo:egorbo/unsafe-v2-contract-consistency
Open

Add unsafe contract consistency code fixes#131454
EgorBo wants to merge 2 commits into
dotnet:mainfrom
EgorBo:egorbo/unsafe-v2-contract-consistency

Conversation

@EgorBo

@EgorBo EgorBo commented Jul 28, 2026

Copy link
Copy Markdown
Member

This PR adds code fixers for the contract consistency part of the unsafe-v2 migration tooling (tracking issue §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 Add unsafe modifier migration code fixer #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.

Adds two code fixers to the unsafe-v2 migration tooling, both built on
diagnostics Roslyn already reports.

SynchronizeUnsafeContractCodeFixProvider fixes CS9364, CS9365 and CS9366,
reported when an unsafe member overrides or implements a safe member. It
offers narrowing the derived member, using safe rather than removal when the
member is extern so the fix does not trade the error for CS9389, and marking
the base member unsafe when that member is declared in source.

MatchPartialSafetyModifierCodeFixProvider fixes CS0764 and CS9390 by copying
the safety modifier onto the partial declaration that lacks it. No fix is
offered when the parts declare opposite contracts, since that would place
safe and unsafe on the same declaration.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
Copilot AI review requested due to automatic review settings July 28, 2026 08:38
@EgorBo
EgorBo requested a review from sbomer as a code owner July 28, 2026 08:38
@github-actions github-actions Bot added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Jul 28, 2026
@dotnet-policy-service dotnet-policy-service Bot added the linkable-framework Issues associated with delivering a linker friendly framework label Jul 28, 2026
@azure-pipelines

Copy link
Copy Markdown
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

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @agocke, @dotnet/illink
See info in area-owners.md if you want to be subscribed.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds new (DEBUG-only) Roslyn code fix providers to the ILLink unsafe-v2 migration tooling to resolve compiler-reported safety-contract inconsistencies in overrides/interface implementations and partial members, plus associated helper updates, resources, and tests.

Changes:

  • Add SynchronizeUnsafeContractCodeFixProvider to fix CS9364/CS9365/CS9366 by either narrowing the derived member or marking the base member unsafe when editable.
  • Add MatchPartialSafetyModifierCodeFixProvider to fix CS0764/CS9390 by propagating unsafe/safe across partial member declarations (when an editable counterpart exists).
  • Extend shared syntax/modifier helper infrastructure and add targeted xUnit coverage.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/SynchronizeUnsafeContractCodeFixTests.cs Adds tests for CS9364/5/6 fixes, including extern and partial-base scenarios.
src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/MatchPartialSafetyModifierCodeFixTests.cs Adds tests for CS0764/CS9390 partial safety-modifier propagation behavior.
src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeMigrationSyntaxHelpers.cs Exposes SafeKeywordKind to allow code fixes to reason about safe on older Roslyn builds.
src/tools/illink/src/ILLink.CodeFix/UnsafeModifierCodeFixHelpers.cs Generalizes modifier editing to support both unsafe and safe, and adds unsafesafe replacement helper.
src/tools/illink/src/ILLink.CodeFix/SynchronizeUnsafeContractCodeFixProvider.cs New code fix provider for derived/base contract mismatches (CS9364-6).
src/tools/illink/src/ILLink.CodeFix/MatchPartialSafetyModifierCodeFixProvider.cs New code fix provider for partial declaration modifier mismatches (CS0764/CS9390).
src/tools/illink/src/ILLink.CodeFix/Resources.resx Adds localized titles for the new code actions.

Comment on lines +143 to +146
private static SyntaxNode? FindPartialDeclaration(SyntaxNode node) =>
node.AncestorsAndSelf().FirstOrDefault(static ancestor => ancestor is MethodDeclarationSyntax
or PropertyDeclarationSyntax
or EventDeclarationSyntax);
FindPartialDeclaration only matched methods, properties and events with
accessors, so CS0764 and CS9390 on a partial indexer or constructor found no
declaration and no fix was offered. Match the base declaration kinds instead,
and normalize a field-like event's variable declarator to the declaration
that carries the modifier.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
Copilot AI review requested due to automatic review settings July 28, 2026 11:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

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

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants