Warn for local function event handlers unsubscribing before they're s…

…ubscribed.

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

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<AssignmentExpressionSyntax>())
+			{
+				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<SyntaxNode> AllChildren(SyntaxNode node)
+			{
+				foreach (var child in node.ChildNodes())
+				{
+					yield return child;
+					foreach (var grandchild in AllChildren(child))
+						yield return grandchild;
+				}
+			}
+		}
 	}
 
 	public override ImmutableArray<DiagnosticDescriptor> 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,

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) { }