Add IntroduceUnsafeContextCodeFixProvider for unsafe-v2 call sites - #131581
Add IntroduceUnsafeContextCodeFixProvider for unsafe-v2 call sites#131581EgorBo wants to merge 4 commits into
Conversation
Replaces RequiresUnsafeCodeFixProvider with IntroduceUnsafeContextCodeFixProvider,
which fixes CS9360, CS9361, CS9362, CS9363 and CS9376 by giving the use site the
unsafe context the updated memory safety rules require.
The old fixer offered "add unsafe to the parent method", which is not a fix under
the new rules: the modifier declares an obligation on the member's own callers and
no longer opens an unsafe context in its body.
The new fixer picks one of four shapes, preferring a block wherever a block cannot
change the meaning of the surrounding code:
* unsafe { statement }
* T x; unsafe { x = init; } so a local stays visible, adding scoped when the
initializer is a stackalloc
* unsafe(expression) where a block is not valid syntax or would shorten a scope
* one unsafe block around the whole body when fixing every use site at once and
a single body would otherwise need three or more separate regions
Scope preservation is decided by SemanticModel.AnalyzeDataFlow, so out variables
and pattern designations are handled the same way as local declarations. A block
is declined for await (CS4004), yield (CS9238), preprocessor directives, and the
positions that have no statement to wrap. The expression form is declined where
the parser would read a leading unsafe( as a declaration, where a statement
expression is required (CS0201), and where it would leave a property accessor
call outside the region.
Everything is non-shipping (#if DEBUG) and disabled by default.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
Two corrections found by running the fixer over System.Text.Json. The SAFETY marker is not this fixer's obligation. Introducing the region and justifying it are separate concerns, and IL5009 already reports an undocumented region and stubs the comment out through its own fix. Splitting a ref struct declaration cannot preserve its ref-safety scope. That scope comes from the initializer, and re-declaring the local without one has no spelling that reproduces it: leaving 'scoped' off lets the local escape to the caller, and adding 'scoped' narrows it to the enclosing block, which is narrower than the current method that 'stackalloc' implies. Six sites in System.Text.Json turned into CS8352 because of this. The expression form leaves the declaration alone and keeps the inference exactly as it was, so ref struct locals now use it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
|
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. |
|
Tagging subscribers to this area: @agocke, @dotnet/illink |
There was a problem hiding this comment.
Pull request overview
This PR replaces the previous RequiresUnsafeCodeFixProvider approach with a new IntroduceUnsafeContextCodeFixProvider implementation that introduces unsafe contexts at call sites (statement/block, split-declaration, expression form, or body-wide), and adds a custom FixAll provider to plan/coalesce edits across a document.
Changes:
- Add
IntroduceUnsafeContextCodeFixProviderplusIntroduceUnsafeContextFixAllProvider, backed by a planner/rewriter pipeline (UnsafeContextPlanner/UnsafeContextRewriter/UnsafeContextFix). - Add a comprehensive new unit test suite for the new fixer and remove the legacy
RequiresUnsafeCodeFix*tests/provider. - Update
ILLink.CodeFixresource strings to reflect the new code action titles.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/RequiresUnsafeCodeFixTests.cs | Removed legacy tests for the replaced code fix provider. |
| src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/IntroduceUnsafeContextCodeFixTests.cs | Added new end-to-end tests covering block/expression/body-wide shapes and decline cases. |
| src/tools/illink/src/ILLink.CodeFix/RequiresUnsafeCodeFixProvider.cs | Removed the replaced legacy code fix provider. |
| src/tools/illink/src/ILLink.CodeFix/IntroduceUnsafeContextCodeFixProvider.cs | New code fix provider for CS9360/1/2/3/9376 use-site unsafe contexts, with optional body-wide alternative. |
| src/tools/illink/src/ILLink.CodeFix/IntroduceUnsafeContextFixAllProvider.cs | Custom FixAll provider that fixes per-document in a single pass and supports consolidation behavior. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeContextPlanner.cs | New planning logic to choose statement vs split-declaration vs expression vs (optionally) body-wide wrapping. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeContextRewriter.cs | New application layer that performs the planned edits via DocumentEditor. |
| src/tools/illink/src/ILLink.CodeFix/UnsafeContextFix.cs | New small data model for planned unsafe-context edits. |
| src/tools/illink/src/ILLink.CodeFix/Resources.resx | Updated/added resource strings for the new code action titles. |
The helper for the cases the fixer cannot fix compared the fixed source against the input, which a code action producing no text change would also satisfy, and which the harness can skip entirely when the two are equal. It now builds the document itself, checks that the source still reports a use site and nothing else, and counts the actions the provider registers. Verified by having the provider register a no-op action: the old form passed, the new one fails all five cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
A statement such as 'Helper(span, value, out int bytesWritten);' whose 'out' variable is read afterwards could not be fixed at all: a block would scope the variable inside it, splitting only handles local declarations, and 'unsafe(...)' is not a statement expression. Give such variables a declaration of their own ahead of the statement, which leaves the scope exactly as it was since an 'out' variable is already scoped as though it were declared there, and frees the block to move. This is tried last, after the expression form, because it is the only shape that adds a declaration to the source. Over System.Text.Json this takes the residue from 16 use sites down to 2, both of which are the separate, already covered case of an expression-bodied void member. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 06c64005-a797-4517-9227-d1706eb5bca5
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/tools/illink/src/ILLink.CodeFix/UnsafeContextPlanner.cs:385
coversAPlaceonly triggers whencore.Span == diagnosticSpan. If the compiler reports CS9362 on just the property name token (a fragment insidereceiver.Property), this will be false and the fixer may emitunsafe(receiver.Property), which does not establish an unsafe context for the accessor call and will leave the diagnostic in place. The check should treat spans contained within the property access as a "place" too.
bool coversAPlace = Unwrap(expression) is { } core
&& core.Span == diagnosticSpan
&& semanticModel.GetSymbolInfo(core).Symbol is IPropertySymbol;
|
@jjonescz @333fred cc @agocke This one is probably the most complex ones - wrap statements/expressions with The most complex part is the fact we want to prefer If you have some better idea how to handle it - let me know, I plan to just polish it on the fly when we'll be converting the entire BCL into unsafe-v2. Once we collect all the nuances, we can rebuild it (or iteratively improve). |
This PR implements the unsafe contexts at call sites part of the unsafe-v2 migration tooling (tracking issue §2), continuing #131002, #131245, #131454 and #131484. Everything here is non-shipping (
#if DEBUG) and disabled by default.Background
This is the highest volume part of a migration to the updated memory safety rules: every consumer of a newly caller-unsafe API breaks. The compiler asks for an unsafe context at the use site with
CS9360,CS9361,CS9362,CS9363andCS9376, and there is no mechanical way to answer it in bulk today.RequiresUnsafeCodeFixProviderwas a first attempt and is replaced here. Its primary code action was "addunsafeto the parent method", which is not a fix under the new rules — the modifier declares an obligation on the member's own callers and no longer opens an unsafe context in its body:Changes in this PR
🔧
IntroduceUnsafeContextCodeFixProviderpicks one of five shapes, preferring a block wherever a block cannot change the meaning of the surrounding code:unsafe { statement }— the default. One region covers every use site in the statement.T x; unsafe { x = init; }— for a local declaration, so the local stays visible to the statements that follow it.unsafe(expression)— where a block is not valid syntax or would shorten a scope.T x; unsafe { M(out x); }— for anoutvariable that is read after the statement, so it too stays visible. Anoutvariable is already scoped as though it were declared just before the statement, so writing that declaration out changes nothing but frees the block to move. Tried last, since it is the only shape that adds a declaration to the source.unsafeblock around the whole body — when fixing every use site at once and a single body would otherwise need three or more separate regions.SpanHelpersand friends read far worse as a block per statement than as one region.Justifying the region is deliberately not part of this.
IL5009from #131484 reports an undocumentedunsaferegion and stubs the comment out through its own fix, so stamping a marker here would only duplicate it.When a block is not usable
A block is only ever declined for a reason the language forces, all of which are pinned by tests:
awaitunder the region (CS4004), including inside a nested lambdaint x = Read() + await t;yieldbelonging to the enclosing iterator (CS9238)yield return Read();ref int r = ref values[Read()];: base(...),catchfilters#if/#elsearound the statementScope preservation is decided by
SemanticModel.AnalyzeDataFlow, soout varand pattern designations are handled the same way as local declarations rather than by enumerating syntax shapes.Notable cases handled
unsafe(cannot begin a statement. The parser reads it as a local function declaration whose return type is a tuple, sounsafe(Act());does not even parse. This never costs a fix, because a statement that starts with an expression declares nothing and can always take the block form instead.unsafe(...)around a property access does not establish the context. The expression form preserves the place, which is what makesunsafe(*p) = 5work, but it also means the accessor call is attributed to the code outside the region:The fixer therefore keeps walking outward until the access is consumed inside the region, seeing through parentheses and the null-forgiving operator, and declines rather than emitting something that does not compile. This looks like a compiler bug worth reporting separately.
unsafe(...)is not a statement expression. An expression body belonging to a member or lambda with nothing to return needs one, sovoid M() => Read();cannot be fixed without converting it to a block body, whateverReadreturns.A ref struct declaration is never split. Its ref-safety scope comes from the initializer, and re-declaring the local without one has no spelling that reproduces it: leaving
scopedoff lets the local escape to the caller, and addingscopednarrows it to the enclosing block, which is narrower than the current method thatstackallocimplies. The expression form leaves the declaration alone and keeps the inference exactly as it was.Fix all
IntroduceUnsafeContextFixAllProviderplans a whole document in one pass rather than merging independently computed edits, which is what lets one region cover several use sites and lets a dense body collapse into a single region. Choosing "wrap the containing body" and then fixing all occurrences wraps every body; choosing the per-use-site fix applies the threshold.Validated on System.Text.Json
Beyond the unit tests, the fixer was run over a real library through
dotnet format, withupdated-memory-safety-rulesenabled and the code fix referenced as an analyzer:72 files, +860/-335, converging in one pass. Afterwards the only errors left in the project are the 16 pre-existing
CS9364s, which belong to #131454, and 2CS9362s on an expression-bodiedvoidmember, the one shape neither form covers. No new errors.The run paid for itself: it found two real bugs, both fixed here and covered by tests. The
scopedsplit above produced sixCS8352s, andRemoveUndocumentedUnsafe-style marker stamping turned out to belong toIL5009rather than here.Two findings from that exercise are worth recording:
CS9364makes the compiler skip method-body binding, sodotnet buildreported 16 errors and zero use-site errors while the workspace saw around 580. Migration has to fix contracts before the use-site work is even visible.dotnet formathas to be pointed at the.slnx. For a project workspace it analyzes every project sharing the csproj path, which includes the TFM-less outer build; that one has noCompiletarget, so it loads with no references and failsAllReferencedProjectsLoadedAsync. A solution workspace drops that filter.Diagnostics fixed
CS9360CS9361stackallocwithout an initializer insideSkipLocalsInitCS9362unsafeCS9363CS9376new()constraint satisfied by an unsafe constructorKnown limitations / follow ups
voidmember gets no fix: there is no statement to wrap, andunsafe(...)is not one of the expression forms such a body may consist of. Rewriting the body into a block would cover it. Two such sites remain in System.Text.Json.Testing
ILLink.RoslynAnalyzer.Tests: 1227 passed, 9 skipped. 56 new tests covering each shape, every reason a block is declined, both fix-all granularities, and a regression test for each of the issues code review and the System.Text.Json run turned up.Note
This description and the changes in this PR were generated with GitHub Copilot.