From ed10bcc23a2890432b9420dce99976e2b1baae51 Mon Sep 17 00:00:00 2001 From: StephenCleary Date: Tue, 13 Feb 2024 14:16:58 -0500 Subject: [PATCH] Warn for local function event handlers unsubscribing before they're subscribed. --- ReleaseNotes.md | 2 +- .../LocalFunctionEventHandler.cs | 64 +++++++++++++++++-- .../LocalFunctionEventHandlerTests.cs | 37 +++++++++-- 3 files changed, 90 insertions(+), 13 deletions(-) diff --git a/ReleaseNotes.md b/ReleaseNotes.md index 2acc228..cd28dd8 100644 --- a/ReleaseNotes.md +++ b/ReleaseNotes.md @@ -11,7 +11,7 @@ or errors for existing code when the package is upgraded). ## 1.5.0 -* Add FL0019: Local functions used as event handlers (unless they are static). +* Add FL0019: Local functions used as event handlers (unless they are static, or else they are subscribed and later unsubscribed in the same method). * Add FL0020: Lambda expressions used as event handlers. ## 1.4.0 diff --git a/src/Faithlife.Analyzers/LocalFunctionEventHandler.cs b/src/Faithlife.Analyzers/LocalFunctionEventHandler.cs index 62fdd7c..f3de153 100644 --- a/src/Faithlife.Analyzers/LocalFunctionEventHandler.cs +++ b/src/Faithlife.Analyzers/LocalFunctionEventHandler.cs @@ -19,15 +19,67 @@ public override void Initialize(AnalysisContext context) private void Analyze(SyntaxNodeAnalysisContext context) { + var containingMethod = TryFindEnclosingMethod(context.Node); + if (containingMethod == null) + return; + var assignment = (AssignmentExpressionSyntax) context.Node; - if (context.SemanticModel.GetSymbolInfo(assignment.Left).Symbol is not IEventSymbol) + if (context.SemanticModel.GetSymbolInfo(assignment.Left).Symbol is not IEventSymbol eventSymbol) return; if (context.SemanticModel.GetSymbolInfo(assignment.Right).Symbol is not IMethodSymbol methodGroup) return; - if (methodGroup is { MethodKind: MethodKind.LocalFunction, IsStatic: false }) - context.ReportDiagnostic(Diagnostic.Create(s_localFunctionRule, assignment.GetLocation())); - else if (methodGroup.MethodKind == MethodKind.AnonymousFunction) + + if (methodGroup.MethodKind == MethodKind.AnonymousFunction) + { context.ReportDiagnostic(Diagnostic.Create(s_lambdaRule, assignment.GetLocation())); + return; + } + + if (methodGroup is not { MethodKind: MethodKind.LocalFunction, IsStatic: false }) + return; + + var matchingSubscription = FindMatchingSubscription(context.SemanticModel, containingMethod, assignment, eventSymbol, methodGroup); + if (matchingSubscription == null) + context.ReportDiagnostic(Diagnostic.Create(s_localFunctionRule, assignment.GetLocation())); + + static MethodDeclarationSyntax? TryFindEnclosingMethod(SyntaxNode? node) + { + while (node != null) + { + if (node is MethodDeclarationSyntax method) + return method; + node = node.Parent; + } + return null; + } + + static AssignmentExpressionSyntax? FindMatchingSubscription(SemanticModel model, MethodDeclarationSyntax method, AssignmentExpressionSyntax stop, IEventSymbol eventSymbol, IMethodSymbol methodSymbol) + { + foreach (var assignmentExpression in AllChildren(method).OfType()) + { + if (assignmentExpression == stop) + break; + if (assignmentExpression.Kind() != SyntaxKind.AddAssignmentExpression) + continue; + if (!SymbolEqualityComparer.Default.Equals(model.GetSymbolInfo(assignmentExpression.Left).Symbol, eventSymbol)) + continue; + if (!SymbolEqualityComparer.Default.Equals(model.GetSymbolInfo(assignmentExpression.Right).Symbol, methodSymbol)) + continue; + return assignmentExpression; + } + + return null; + + static IEnumerable AllChildren(SyntaxNode node) + { + foreach (var child in node.ChildNodes()) + { + yield return child; + foreach (var grandchild in AllChildren(child)) + yield return grandchild; + } + } + } } public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(s_localFunctionRule, s_lambdaRule); @@ -38,7 +90,7 @@ private void Analyze(SyntaxNodeAnalysisContext context) private static readonly DiagnosticDescriptor s_localFunctionRule = new( id: LocalFunctionDiagnosticId, title: "Local Functions as Event Handlers", - messageFormat: "Local functions should probably not be used as event handlers (unless they are static).", + messageFormat: "Local function event handler.", category: "Usage", defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, @@ -48,7 +100,7 @@ private void Analyze(SyntaxNodeAnalysisContext context) private static readonly DiagnosticDescriptor s_lambdaRule = new( id: LambdaDiagnosticId, title: "Lambda Expressions as Event Handlers", - messageFormat: "Lambda expressions may not be used as event handlers.", + messageFormat: "Lambda expression event handler.", category: "Usage", defaultSeverity: DiagnosticSeverity.Error, isEnabledByDefault: true, diff --git a/tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs b/tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs index 3683cf6..28c64c3 100644 --- a/tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs +++ b/tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs @@ -10,6 +10,8 @@ public class LocalFunctionEventHandlerTests : CodeFixVerifier [Test] public void LocalFunction() { + // This pattern is "unsubscribe the old handler; then subscribe the new handler". + // This pattern is wrong because the old and new handlers are different instances. const string program = """ using System; @@ -18,8 +20,8 @@ class Test public event EventHandler OnFrob; public void Hook() { - OnFrob += Local; OnFrob -= Local; + OnFrob += Local; void Local(object a, EventArgs b) { } OnFrob?.Invoke(this, EventArgs.Empty); } @@ -29,11 +31,34 @@ void Local(object a, EventArgs b) { } { Id = LocalFunctionEventHandler.LocalFunctionDiagnosticId, Severity = DiagnosticSeverity.Warning, - Message = "Local functions should probably not be used as event handlers (unless they are static).", - Locations = new[] { new DiagnosticResultLocation("Test0.cs", 8, 3) }, + Message = "Local function event handler.", + Locations = new[] { new DiagnosticResultLocation("Test0.cs", 7, 3) }, }); } + [Test] + public void LocalFunctionReturningUnsubscribe() + { + // This pattern is "subscribe the new handler; then return some action to later unsubscribe the handler". + // This pattern is correct because the same handler is being subscribed and unsubscribed. + const string program = + """ + using System; + class Test + { + public event EventHandler OnFrob; + public Action Hook() + { + OnFrob += Local; + void Local(object a, EventArgs b) { } + OnFrob?.Invoke(this, EventArgs.Empty); + return () => OnFrob -= Local; + } + } + """; + VerifyCSharpDiagnostic(program); + } + [Test] public void StaticLocalFunction() { @@ -45,8 +70,8 @@ class Test public event EventHandler OnFrob; public void Hook() { - OnFrob += Local; OnFrob -= Local; + OnFrob += Local; static void Local(object a, EventArgs b) { } OnFrob?.Invoke(this, EventArgs.Empty); } @@ -76,7 +101,7 @@ public void Hook() { Id = LocalFunctionEventHandler.LambdaDiagnosticId, Severity = DiagnosticSeverity.Error, - Message = "Lambda expressions may not be used as event handlers.", + Message = "Lambda expression event handler.", Locations = new[] { new DiagnosticResultLocation("Test0.cs", 8, 3) }, }); } @@ -92,8 +117,8 @@ class Test public event EventHandler OnFrob; public void Hook() { - OnFrob += Frobbed; OnFrob -= Frobbed; + OnFrob += Frobbed; OnFrob?.Invoke(this, EventArgs.Empty); } private void Frobbed(object a, EventArgs b) { }