Add FL0019 and FL0020. (#89)

Directory.Build.props

@@ -1,8 +1,8 @@
 <Project>
 
   <PropertyGroup>
-    <VersionPrefix>1.4.0</VersionPrefix>
-    <LangVersion>10.0</LangVersion>
+    <VersionPrefix>1.5.0</VersionPrefix>
+    <LangVersion>11.0</LangVersion>
     <Nullable>enable</Nullable>
     <ImplicitUsings>enable</ImplicitUsings>
     <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

ReleaseNotes.md

@@ -9,6 +9,11 @@ Prefix the description of the change with `[major]`, `[minor]`, or `[patch]` in
 New analyzers are considered "minor" changes (even though adding a new analyzer is likely to generate warnings
 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, or else they are subscribed and later unsubscribed in the same method).
+* Add FL0020: Lambda expressions used as event handlers.
+
 ## 1.4.0
 
 * Add FL0017: Do not switch on a constant value.

src/Faithlife.Analyzers/LocalFunctionEventHandler.cs

@@ -0,0 +1,109 @@
+using System.Collections.Immutable;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+using Microsoft.CodeAnalysis.Diagnostics;
+
+namespace Faithlife.Analyzers;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public class LocalFunctionEventHandler : DiagnosticAnalyzer
+{
+	public override void Initialize(AnalysisContext context)
+	{
+		context.EnableConcurrentExecution();
+		context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+
+		context.RegisterSyntaxNodeAction(Analyze, SyntaxKind.SubtractAssignmentExpression);
+	}
+
+	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 eventSymbol)
+			return;
+		if (context.SemanticModel.GetSymbolInfo(assignment.Right).Symbol is not IMethodSymbol methodGroup)
+			return;
+
+		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);
+
+	public const string LocalFunctionDiagnosticId = "FL0019";
+	public const string LambdaDiagnosticId = "FL0020";
+
+	private static readonly DiagnosticDescriptor s_localFunctionRule = new(
+		id: LocalFunctionDiagnosticId,
+		title: "Local Functions as Event Handlers",
+		messageFormat: "Local function event handler.",
+		category: "Usage",
+		defaultSeverity: DiagnosticSeverity.Warning,
+		isEnabledByDefault: true,
+		helpLinkUri: $"https://github.com/Faithlife/FaithlifeAnalyzers/wiki/{LocalFunctionDiagnosticId}"
+	);
+
+	private static readonly DiagnosticDescriptor s_lambdaRule = new(
+		id: LambdaDiagnosticId,
+		title: "Lambda Expressions as Event Handlers",
+		messageFormat: "Lambda expression event handler.",
+		category: "Usage",
+		defaultSeverity: DiagnosticSeverity.Error,
+		isEnabledByDefault: true,
+		helpLinkUri: $"https://github.com/Faithlife/FaithlifeAnalyzers/wiki/{LambdaDiagnosticId}"
+	);
+}

tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs

@@ -0,0 +1,131 @@
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+using NUnit.Framework;
+
+namespace Faithlife.Analyzers.Tests;
+
+[TestFixture]
+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;
+			class Test
+			{
+				public event EventHandler OnFrob;
+				public void Hook()
+				{
+					OnFrob -= Local;
+					OnFrob += Local;
+					void Local(object a, EventArgs b) { }
+					OnFrob?.Invoke(this, EventArgs.Empty);
+				}
+			}
+			""";
+		VerifyCSharpDiagnostic(program, new DiagnosticResult
+		{
+			Id = LocalFunctionEventHandler.LocalFunctionDiagnosticId,
+			Severity = DiagnosticSeverity.Warning,
+			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()
+	{
+		const string program =
+			"""
+			using System;
+			class Test
+			{
+				public event EventHandler OnFrob;
+				public void Hook()
+				{
+					OnFrob -= Local;
+					OnFrob += Local;
+					static void Local(object a, EventArgs b) { }
+					OnFrob?.Invoke(this, EventArgs.Empty);
+				}
+			}
+			""";
+		VerifyCSharpDiagnostic(program);
+	}
+
+	[Test]
+	public void Lambda()
+	{
+		const string program =
+			"""
+			using System;
+			class Test
+			{
+				public event EventHandler OnFrob;
+				public void Hook()
+				{
+					OnFrob += (_, __) => { };
+					OnFrob -= (_, __) => { };
+					OnFrob?.Invoke(this, EventArgs.Empty);
+				}
+			}
+			""";
+		VerifyCSharpDiagnostic(program, new DiagnosticResult
+		{
+			Id = LocalFunctionEventHandler.LambdaDiagnosticId,
+			Severity = DiagnosticSeverity.Error,
+			Message = "Lambda expression event handler.",
+			Locations = new[] { new DiagnosticResultLocation("Test0.cs", 8, 3) },
+		});
+	}
+
+	[Test]
+	public void NonStaticInstanceMethod()
+	{
+		const string program =
+			"""
+			using System;
+			class Test
+			{
+				public event EventHandler OnFrob;
+				public void Hook()
+				{
+					OnFrob -= Frobbed;
+					OnFrob += Frobbed;
+					OnFrob?.Invoke(this, EventArgs.Empty);
+				}
+				private void Frobbed(object a, EventArgs b) { }
+			}
+			""";
+		VerifyCSharpDiagnostic(program);
+	}
+
+	protected override DiagnosticAnalyzer GetCSharpDiagnosticAnalyzer() => new LocalFunctionEventHandler();
+}