diff --git a/Directory.Build.props b/Directory.Build.props index 9745ad5..ec2d9aa 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -1,8 +1,8 @@ - 1.4.0 - 10.0 + 1.5.0 + 11.0 enable enable true diff --git a/ReleaseNotes.md b/ReleaseNotes.md index a6c96d1..cd28dd8 100644 --- a/ReleaseNotes.md +++ b/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. diff --git a/src/Faithlife.Analyzers/LocalFunctionEventHandler.cs b/src/Faithlife.Analyzers/LocalFunctionEventHandler.cs new file mode 100644 index 0000000..f3de153 --- /dev/null +++ b/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()) + { + 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); + + 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}" + ); +} diff --git a/tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs b/tests/Faithlife.Analyzers.Tests/LocalFunctionEventHandlerTests.cs new file mode 100644 index 0000000..28c64c3 --- /dev/null +++ b/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(); +}