Skip to content

Commit 0edb9e2

Browse files
EgorBoCopilot
EgorBo
and
Copilot
committed
Address PR #128304 review feedback: arrow-body support, event tests, polish
Comments addressed: * IntroduceUnsafeBlockCodeFixProvider now handles expression-bodied members. Diagnostics in 'int M() => UnsafeCall();', 'int P => UnsafeCall();', etc. previously offered no fix because FindContainingStatement returned null. The fixer now also walks for an enclosing ArrowExpressionClauseSyntax and, when found, rewrites the member to a block body with 'unsafe { /* SAFETY-TODO */ return expr; }' (or 'expr;' for void/Task/set/init/add/remove/ctor/dtor). Properties/indexers with arrow bodies are converted to explicit 'get { ... }' accessors. * Added EventFieldDeclaration / EventDeclaration tests for IL5006 -- the analyzer has registered for those syntax kinds but no test covered them. * Removed misleading 'fall back to wrap-as-is' comment on the ForwardDeclare defensive path -- the implementation actually bails out unchanged (wrap-as-is would not be safe because the local escapes past the wrap point, which is why we picked ForwardDeclare). Comment updated to match behavior. * Removed unused 'using System;' from RemoveUnsafeModifierCodeFixTests.cs. All 51 UnsafeEvolution tests pass (was 46; +2 event tests, +3 arrow-body tests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 8123991 commit 0edb9e2

4 files changed

Lines changed: 286 additions & 4 deletions

File tree

‎src/tools/illink/src/ILLink.CodeFix/UnsafeEvolution/IntroduceUnsafeBlockCodeFixProvider.cs‎

Lines changed: 141 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,25 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
8484

8585
// Lambdas / anonymous functions do not inherit unsafe context from an enclosing
8686
// member, so we cannot fix a diagnostic that lives in one by wrapping an outer
87-
// statement. Bail out if we cross such a boundary before finding any statement.
87+
// statement.
8888
var containingStatement = FindContainingStatement(node);
8989
if (containingStatement is null)
90+
{
91+
// Expression-bodied members (e.g. 'int M() => Helper();') have no enclosing
92+
// statement. If we can rewrite the arrow body into a block body, offer that fix.
93+
var arrow = FindContainingArrowBody(node);
94+
if (arrow is null)
95+
return;
96+
97+
var semanticModelForArrow = await document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false);
98+
context.RegisterCodeFix(
99+
CodeAction.Create(
100+
title: WrapStatementTitle,
101+
createChangedDocument: ct => WrapArrowBodyAsync(document, arrow, semanticModelForArrow, ct),
102+
equivalenceKey: WrapStatementTitle),
103+
diagnostic);
90104
return;
105+
}
91106

92107
// Skip statements whose tokens enclose a preprocessor directive (e.g. an argument
93108
// list with #if/#else/#endif between commas). Wrapping such a statement would
@@ -149,6 +164,24 @@ public override async Task RegisterCodeFixesAsync(CodeFixContext context)
149164
return null;
150165
}
151166

167+
/// <summary>
168+
/// Finds the smallest enclosing <see cref="ArrowExpressionClauseSyntax"/> (the body
169+
/// of an expression-bodied member or accessor), stopping at lambda / anonymous
170+
/// function boundaries: an expression-bodied lambda is not a member body we can
171+
/// rewrite.
172+
/// </summary>
173+
private static ArrowExpressionClauseSyntax? FindContainingArrowBody(SyntaxNode node)
174+
{
175+
for (var n = node; n is not null; n = n.Parent)
176+
{
177+
if (n is ArrowExpressionClauseSyntax arrow)
178+
return arrow;
179+
if (n is AnonymousFunctionExpressionSyntax)
180+
return null;
181+
}
182+
return null;
183+
}
184+
152185
/// <summary>
153186
/// Finds the smallest method-like declaration that contains the diagnostic, stopping
154187
/// at lambda boundaries (unsafe context does not flow into lambdas, so wrapping the
@@ -356,6 +389,107 @@ private static async Task<Document> WrapMemberBodyAsync(Document document, Block
356389
return document.WithSyntaxRoot(root.ReplaceNode(body, newBody));
357390
}
358391

392+
// ---- Wrapping an expression-bodied member ----
393+
394+
private static async Task<Document> WrapArrowBodyAsync(
395+
Document document, ArrowExpressionClauseSyntax arrow, SemanticModel? semanticModel, CancellationToken ct)
396+
{
397+
var root = await document.GetSyntaxRootAsync(ct).ConfigureAwait(false);
398+
if (root is null || arrow.Parent is not { } member)
399+
return document;
400+
401+
bool requiresReturn = ArrowBodyRequiresReturn(member, semanticModel);
402+
403+
// Build 'return <expr>;' or '<expr>;' depending on the member's effective return type.
404+
// Preserve the original expression's trivia inside the new statement so any inline
405+
// comments authored on the arrow expression survive.
406+
StatementSyntax inner = requiresReturn
407+
? SyntaxFactory.ReturnStatement(arrow.Expression.WithoutTrivia())
408+
: SyntaxFactory.ExpressionStatement(arrow.Expression.WithoutTrivia());
409+
var unsafeBlock = BuildUnsafeBlock([inner]);
410+
var newBody = SyntaxFactory.Block(unsafeBlock).WithAdditionalAnnotations(Formatter.Annotation);
411+
412+
// Replace the member's '=> expr;' with '{ unsafe { ... } }'.
413+
var newMember = ReplaceArrowWithBlock(member, newBody);
414+
if (newMember is null)
415+
return document;
416+
417+
return document.WithSyntaxRoot(root.ReplaceNode(member, newMember));
418+
}
419+
420+
/// <summary>
421+
/// True if rewriting <paramref name="memberWithArrowBody"/>'s arrow body into a block
422+
/// body requires a <c>return</c> statement around the original expression. False when
423+
/// the member produces no value (void method/local-function, set/init/add/remove
424+
/// accessor, constructor/destructor, or an async method whose only result is the Task).
425+
/// </summary>
426+
private static bool ArrowBodyRequiresReturn(SyntaxNode memberWithArrowBody, SemanticModel? semanticModel)
427+
{
428+
switch (memberWithArrowBody)
429+
{
430+
case MethodDeclarationSyntax m:
431+
return !IsVoidLikeMethod(m.ReturnType, m.Modifiers, semanticModel, m);
432+
case LocalFunctionStatementSyntax lf:
433+
return !IsVoidLikeMethod(lf.ReturnType, lf.Modifiers, semanticModel, lf);
434+
case OperatorDeclarationSyntax:
435+
case ConversionOperatorDeclarationSyntax:
436+
case PropertyDeclarationSyntax:
437+
case IndexerDeclarationSyntax:
438+
return true;
439+
case AccessorDeclarationSyntax a:
440+
return a.Keyword.IsKind(SyntaxKind.GetKeyword);
441+
case ConstructorDeclarationSyntax:
442+
case DestructorDeclarationSyntax:
443+
return false;
444+
default:
445+
// Unknown member kind - rewriting could produce broken syntax; assume no return.
446+
return false;
447+
}
448+
}
449+
450+
private static bool IsVoidLikeMethod(TypeSyntax returnType, SyntaxTokenList modifiers, SemanticModel? semanticModel, SyntaxNode declaration)
451+
{
452+
if (returnType is PredefinedTypeSyntax p && p.Keyword.IsKind(SyntaxKind.VoidKeyword))
453+
return true;
454+
455+
// 'async Task' / 'async ValueTask' bodies do not return a value; the await/return
456+
// expression is just an expression statement when in block form.
457+
if (modifiers.Any(SyntaxKind.AsyncKeyword) && semanticModel is not null
458+
&& semanticModel.GetDeclaredSymbol(declaration) is IMethodSymbol m
459+
&& m.ReturnType is INamedTypeSymbol rt
460+
&& rt.TypeArguments.Length == 0
461+
&& rt.Name is "Task" or "ValueTask")
462+
{
463+
return true;
464+
}
465+
466+
return false;
467+
}
468+
469+
private static SyntaxNode? ReplaceArrowWithBlock(SyntaxNode member, BlockSyntax newBody) => member switch
470+
{
471+
MethodDeclarationSyntax m => m.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
472+
LocalFunctionStatementSyntax lf => lf.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
473+
OperatorDeclarationSyntax op => op.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
474+
ConversionOperatorDeclarationSyntax co => co.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
475+
ConstructorDeclarationSyntax c => c.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
476+
DestructorDeclarationSyntax d => d.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
477+
AccessorDeclarationSyntax a => a.WithExpressionBody(null).WithBody(newBody).WithSemicolonToken(default),
478+
// For arrow-bodied property/indexer ('int P => expr;'), turn it into a block with a
479+
// single get accessor whose body is the new unsafe block.
480+
PropertyDeclarationSyntax prop => prop
481+
.WithExpressionBody(null)
482+
.WithSemicolonToken(default)
483+
.WithAccessorList(SyntaxFactory.AccessorList(SyntaxFactory.SingletonList(
484+
SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration).WithBody(newBody)))),
485+
IndexerDeclarationSyntax idx => idx
486+
.WithExpressionBody(null)
487+
.WithSemicolonToken(default)
488+
.WithAccessorList(SyntaxFactory.AccessorList(SyntaxFactory.SingletonList(
489+
SyntaxFactory.AccessorDeclaration(SyntaxKind.GetAccessorDeclaration).WithBody(newBody)))),
490+
_ => null,
491+
};
492+
359493
// ---- Wrapping a single statement ----
360494

361495
private static async Task<Document> WrapSingleStatementAsync(
@@ -378,8 +512,12 @@ private static async Task<Document> WrapSingleStatementAsync(
378512
var rewrite = TryRewriteAsForwardDeclaration((LocalDeclarationStatementSyntax)statement, semanticModel);
379513
if (rewrite is null)
380514
{
381-
// Should not happen - ChooseFixStrategy already verified the rewrite is possible.
382-
// Fall back to wrap-as-is to avoid producing an empty fix.
515+
// Defensive: ChooseFixStrategy already verified the rewrite is possible by
516+
// calling TryRewriteAsForwardDeclaration, so this branch should be unreachable.
517+
// If it is reached (e.g. a future semantic-model change makes the rewrite fail
518+
// here but not at strategy-decision time), bail out unchanged rather than
519+
// producing a broken fix; wrap-as-is would not be safe because the local is
520+
// referenced after the wrap point (that is why we picked ForwardDeclare).
383521
return document;
384522
}
385523

‎src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/IntroduceUnsafeBlockCodeFixTests.cs‎

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -637,6 +637,116 @@ await RunAsync(source, source, [
637637
.WithArguments("C.M1()"),
638638
]);
639639
}
640+
641+
// ---- Expression-bodied members: arrow body -> block body with unsafe wrap ----
642+
643+
[Fact]
644+
public async Task ExpressionBodiedMethod_NonVoid_IsRewrittenToBlockWithReturn()
645+
{
646+
string source = """
647+
public class C
648+
{
649+
public static unsafe int M1() => 0;
650+
651+
public int M2() => M1();
652+
}
653+
""";
654+
655+
string fixedSource = """
656+
public class C
657+
{
658+
public static unsafe int M1() => 0;
659+
660+
public int M2()
661+
{
662+
unsafe
663+
{
664+
// SAFETY-TODO: Audit
665+
return M1();
666+
}
667+
}
668+
}
669+
""";
670+
671+
await RunAsync(source, fixedSource, [
672+
DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation)
673+
.WithSpan(5, 24, 5, 28)
674+
.WithArguments("C.M1()"),
675+
]);
676+
}
677+
678+
[Fact]
679+
public async Task ExpressionBodiedMethod_Void_IsRewrittenToBlockWithExpressionStatement()
680+
{
681+
string source = """
682+
public class C
683+
{
684+
public static unsafe void M1() { }
685+
686+
public void M2() => M1();
687+
}
688+
""";
689+
690+
string fixedSource = """
691+
public class C
692+
{
693+
public static unsafe void M1() { }
694+
695+
public void M2()
696+
{
697+
unsafe
698+
{
699+
// SAFETY-TODO: Audit
700+
M1();
701+
}
702+
}
703+
}
704+
""";
705+
706+
await RunAsync(source, fixedSource, [
707+
DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation)
708+
.WithSpan(5, 25, 5, 29)
709+
.WithArguments("C.M1()"),
710+
]);
711+
}
712+
713+
[Fact]
714+
public async Task ExpressionBodiedProperty_IsRewrittenToGetAccessorWithUnsafeBlock()
715+
{
716+
string source = """
717+
public class C
718+
{
719+
public static unsafe int M1() => 0;
720+
721+
public int P => M1();
722+
}
723+
""";
724+
725+
string fixedSource = """
726+
public class C
727+
{
728+
public static unsafe int M1() => 0;
729+
730+
public int P
731+
{
732+
get
733+
{
734+
unsafe
735+
{
736+
// SAFETY-TODO: Audit
737+
return M1();
738+
}
739+
}
740+
}
741+
}
742+
""";
743+
744+
await RunAsync(source, fixedSource, [
745+
DiagnosticResult.CompilerError(UnsafeEvolutionDescriptors.UnsafeMemberOperation)
746+
.WithSpan(5, 21, 5, 25)
747+
.WithArguments("C.M1()"),
748+
]);
749+
}
640750
}
641751
}
642752
#endif

‎src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/RemoveUnsafeModifierCodeFixTests.cs‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
33

44
#if DEBUG
5-
using System;
65
using System.Threading.Tasks;
76
using ILLink.CodeFix.UnsafeEvolution;
87
using Microsoft.CodeAnalysis;

‎src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/UnsafeEvolution/UnsafeEvolutionAnalyzerTests.cs‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -301,6 +301,41 @@ await RunAsync(source,
301301
.WithSpan(5, 9, 5, 15)
302302
.WithArguments("Local"));
303303
}
304+
305+
[Fact]
306+
public async Task IL5006_FiresOn_EventFieldWithoutPointer()
307+
{
308+
string source = """
309+
using System;
310+
public class C
311+
{
312+
public unsafe event Action E;
313+
}
314+
""";
315+
316+
await RunAsync(source,
317+
VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier)
318+
.WithSpan(4, 12, 4, 18)
319+
.WithArguments("E"),
320+
DiagnosticResult.CompilerWarning("CS0067").WithSpan(4, 32, 4, 33).WithArguments("C.E"));
321+
}
322+
323+
[Fact]
324+
public async Task IL5006_FiresOn_EventWithExplicitAccessorsWithoutPointer()
325+
{
326+
string source = """
327+
using System;
328+
public class C
329+
{
330+
public unsafe event Action E { add { } remove { } }
331+
}
332+
""";
333+
334+
await RunAsync(source,
335+
VerifyCS.Diagnostic(UnsafeEvolutionDescriptors.UnnecessaryUnsafeModifier)
336+
.WithSpan(4, 12, 4, 18)
337+
.WithArguments("E"));
338+
}
304339
}
305340
}
306341
#endif

0 commit comments

Comments
 (0)