Skip to content

Commit e6c109f

Browse files
EgorBoCopilot
andcommitted
Fix BlockNeedsWrap / ExpressionBodyNeedsWrap to only skip on the fixer's own output shape
Previously BlockNeedsWrap used body.DescendantNodes() to look for an existing 'unsafe' block anywhere in the body, which incorrectly suppressed wrapping when: - the outer body contained other top-level unsafe operations alongside a manual 'unsafe { }' block, or - a nested local function / lambda happened to contain its own 'unsafe' block. In either case, removing the method-level 'unsafe' modifier without wrapping the outer body produces code that no longer compiles. Tighten the skip condition so we only treat the body as already-wrapped when it's literally '{ unsafe { ... } }' — the shape the fixer itself emits — preserving idempotence without false positives. The same descendant-scan was also in ExpressionBodyNeedsWrap, where it could only ever match nested lambdas (since an unsafe statement can't be the body of an arrow expression directly); simplified to always wrap when an expression body is present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 595e1b5 commit e6c109f

2 files changed

Lines changed: 85 additions & 5 deletions

File tree

src/tools/illink/src/ILLink.RoslynAnalyzer/UnsafeV2MigrationAnalyzer.cs

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -182,13 +182,19 @@ public static bool MemberNeedsBodyWrap(SyntaxNode member)
182182
};
183183
}
184184

185-
// True iff a block body has statements worth wrapping (non-empty, no existing
186-
// unsafe block, no conditional-compilation directives).
185+
// True iff a block body has statements worth wrapping (non-empty, no conditional-
186+
// compilation directives, and not already in the fixer's own idempotent output
187+
// shape — i.e. `{ unsafe { ... } }`).
187188
public static bool BlockNeedsWrap(BlockSyntax? body)
188189
{
189190
if (body is null || body.Statements.Count == 0)
190191
return false;
191-
if (body.DescendantNodes().OfType<UnsafeStatementSyntax>().Any())
192+
// Skip ONLY when the body's *direct* shape is the one this fixer emits — a
193+
// single top-level `unsafe { ... }` statement. Checking DescendantNodes here
194+
// would be wrong: a nested unsafe block inside a local function or lambda
195+
// does not relieve the outer body from needing its own unsafe context for
196+
// any sibling unsafe operations.
197+
if (body.Statements.Count == 1 && body.Statements[0] is UnsafeStatementSyntax)
192198
return false;
193199
// Skip wrapping when the body contains conditional-compilation directives.
194200
// Every structural / textual wrap we tried produced different output per TFM,
@@ -209,9 +215,12 @@ private static bool IsConditionalCompilationDirective(SyntaxTrivia trivia) =>
209215
|| trivia.IsKind(SyntaxKind.ElseDirectiveTrivia)
210216
|| trivia.IsKind(SyntaxKind.EndIfDirectiveTrivia);
211217

212-
// True iff an `=> expr` body is worth wrapping (has no unsafe statement already).
218+
// True iff an `=> expr` body should be wrapped. An expression body can never
219+
// itself BE an `unsafe { ... }` block (unsafe is a statement, not an expression),
220+
// so we always wrap when one is present. Nested unsafe blocks inside lambdas /
221+
// local functions don't suppress wrapping for the same reason as BlockNeedsWrap.
213222
public static bool ExpressionBodyNeedsWrap(ArrowExpressionClauseSyntax? expr) =>
214-
expr is not null && !expr.DescendantNodesAndSelf().OfType<UnsafeStatementSyntax>().Any();
223+
expr is not null;
215224

216225
// True iff a property/indexer body (expression or accessor list) needs wrapping,
217226
// skipping trivial field-forwarders.

src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeV2MigrationCodeFixTests.cs

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,77 @@ public void M()
352352
await VerifyCodeFix(source, fixedSource);
353353
}
354354

355+
[Fact]
356+
public async Task UnsafeMethodWithExistingUnsafeBlockPlusOtherStatements_StillWraps()
357+
{
358+
// Regression: previously BlockNeedsWrap used DescendantNodes() to look for an
359+
// existing unsafe block anywhere in the body, which would suppress wrapping
360+
// even when the body had top-level statements (`*p = 0` here) that were
361+
// relying on the method-level unsafe context. The fix only skips wrapping
362+
// when the body is *exactly* `{ unsafe { ... } }`.
363+
var source = """
364+
public class C
365+
{
366+
public {|IL5006:unsafe|} void M(int* p)
367+
{
368+
unsafe { System.Console.WriteLine(); }
369+
*p = 0;
370+
}
371+
}
372+
""";
373+
var fixedSource = """
374+
public class C
375+
{
376+
public unsafe void M(int* p)
377+
{
378+
unsafe
379+
{
380+
// SAFETY-TODO: Audit this unsafe usage
381+
unsafe { System.Console.WriteLine(); }
382+
*p = 0;
383+
}
384+
}
385+
}
386+
""";
387+
await VerifyCodeFix(source, fixedSource);
388+
}
389+
390+
[Fact]
391+
public async Task UnsafeMethodWithUnsafeBlockNestedInLocalFunction_StillWraps()
392+
{
393+
// Regression: a nested `unsafe { }` inside a local function does not relieve
394+
// the outer method body from needing its own unsafe context (the spec says
395+
// 'unsafe' on a member is NOT applied to nested local functions). Previously
396+
// we'd skip wrapping because the descendant scan found the nested block.
397+
var source = """
398+
public class C
399+
{
400+
public {|IL5006:unsafe|} void M(int* p)
401+
{
402+
void Inner() { unsafe { System.Console.WriteLine(); } }
403+
Inner();
404+
*p = 0;
405+
}
406+
}
407+
""";
408+
var fixedSource = """
409+
public class C
410+
{
411+
public unsafe void M(int* p)
412+
{
413+
unsafe
414+
{
415+
// SAFETY-TODO: Audit this unsafe usage
416+
void Inner() { unsafe { System.Console.WriteLine(); } }
417+
Inner();
418+
*p = 0;
419+
}
420+
}
421+
}
422+
""";
423+
await VerifyCodeFix(source, fixedSource);
424+
}
425+
355426
[Fact]
356427
public async Task UnsafeIteratorMethod_WrapsBody_DeveloperFixesFallout()
357428
{

0 commit comments

Comments
 (0)