diff --git a/Directory.Packages.props b/Directory.Packages.props index 70480f111..3d57c73d3 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -5,11 +5,16 @@ + + + + + - - + + diff --git a/Terminal.Gui.Analyzers.Tests/HandledEventArgsAnalyzerTests.cs b/Terminal.Gui.Analyzers.Tests/HandledEventArgsAnalyzerTests.cs new file mode 100644 index 000000000..6eeab40dd --- /dev/null +++ b/Terminal.Gui.Analyzers.Tests/HandledEventArgsAnalyzerTests.cs @@ -0,0 +1,61 @@ +using Terminal.Gui.Input; +using Terminal.Gui.Views; + +namespace Terminal.Gui.Analyzers.Tests; + +public class HandledEventArgsAnalyzerTests +{ + [Theory] + [InlineData("e")] + [InlineData ("args")] + public async Task Should_ReportDiagnostic_When_EHandledNotSet_Lambda (string paramName) + { + var originalCode = $$""" + using Terminal.Gui.Views; + + class TestClass + { + void Setup() + { + var b = new Button(); + b.Accepting += (s, {{paramName}}) => + { + // Forgot {{paramName}}.Handled = true; + }; + } + } + """; + await new ProjectBuilder () + .WithSourceCode (originalCode) + .WithAnalyzer (new HandledEventArgsAnalyzer ()) + .ValidateAsync (); + } + + [Theory] + [InlineData ("e")] + [InlineData ("args")] + public async Task Should_ReportDiagnostic_When_EHandledNotSet_Method (string paramName) + { + var originalCode = $$""" + using Terminal.Gui.Views; + using Terminal.Gui.Input; + + class TestClass + { + void Setup() + { + var b = new Button(); + b.Accepting += BOnAccepting; + } + private void BOnAccepting (object? sender, CommandEventArgs {{paramName}}) + { + + } + } + """; + await new ProjectBuilder () + .WithSourceCode (originalCode) + .WithAnalyzer (new HandledEventArgsAnalyzer ()) + .ValidateAsync (); + } +} diff --git a/Terminal.Gui.Analyzers.Tests/ProjectBuilder.cs b/Terminal.Gui.Analyzers.Tests/ProjectBuilder.cs new file mode 100644 index 000000000..e5fec924b --- /dev/null +++ b/Terminal.Gui.Analyzers.Tests/ProjectBuilder.cs @@ -0,0 +1,165 @@ +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.Text; +using Microsoft.CodeAnalysis.CodeFixes; +using Microsoft.CodeAnalysis.Diagnostics; +using System.Collections.Immutable; +using System.Collections.ObjectModel; +using System.ComponentModel; +using System.Drawing; +using Microsoft.CodeAnalysis.CodeActions; +using Terminal.Gui.ViewBase; +using Terminal.Gui.Views; +using Document = Microsoft.CodeAnalysis.Document; +using Formatter = Microsoft.CodeAnalysis.Formatting.Formatter; +using System.Reflection; +using JetBrains.Annotations; + +public sealed class ProjectBuilder +{ + private string _sourceCode; + private string _expectedFixedCode; + private DiagnosticAnalyzer _analyzer; + private CodeFixProvider _codeFix; + + public ProjectBuilder WithSourceCode (string source) + { + _sourceCode = source; + return this; + } + + public ProjectBuilder ShouldFixCodeWith (string expected) + { + _expectedFixedCode = expected; + return this; + } + + public ProjectBuilder WithAnalyzer (DiagnosticAnalyzer analyzer) + { + _analyzer = analyzer; + return this; + } + + public ProjectBuilder WithCodeFix (CodeFixProvider codeFix) + { + _codeFix = codeFix; + return this; + } + + public async Task ValidateAsync () + { + if (_sourceCode == null) + { + throw new InvalidOperationException ("Source code not set."); + } + + if (_analyzer == null) + { + throw new InvalidOperationException ("Analyzer not set."); + } + + // Parse original document + var document = CreateDocument (_sourceCode); + var compilation = await document.Project.GetCompilationAsync (); + + var diagnostics = compilation.GetDiagnostics (); + var errors = diagnostics.Where (d => d.Severity == DiagnosticSeverity.Error); + + if (errors.Any ()) + { + var errorMessages = string.Join (Environment.NewLine, errors.Select (e => e.ToString ())); + throw new Exception ("Compilation failed with errors:" + Environment.NewLine + errorMessages); + } + + // Run analyzer + var analyzerDiagnostics = await GetAnalyzerDiagnosticsAsync (compilation, _analyzer); + + Assert.NotEmpty (analyzerDiagnostics); + + if (_expectedFixedCode != null) + { + if (_codeFix == null) + { + throw new InvalidOperationException ("Expected code fix but none was set."); + } + + var fixedDocument = await ApplyCodeFixAsync (document, analyzerDiagnostics.First (), _codeFix); + + var formattedDocument = await Formatter.FormatAsync (fixedDocument); + var fixedSource = (await formattedDocument.GetTextAsync ()).ToString (); + + Assert.Equal (_expectedFixedCode, fixedSource); + } + } + + private static Document CreateDocument (string source) + { + var dd = typeof (Enumerable).GetTypeInfo ().Assembly.Location; + var coreDir = Directory.GetParent (dd) ?? throw new Exception ($"Could not find parent directory of dotnet sdk. Sdk directory was {dd}"); + + var workspace = new AdhocWorkspace (); + var projectId = ProjectId.CreateNewId (); + var documentId = DocumentId.CreateNewId (projectId); + + var references = new List () + { + MetadataReference.CreateFromFile(typeof(Button).Assembly.Location), + MetadataReference.CreateFromFile(typeof(View).Assembly.Location), + MetadataReference.CreateFromFile(typeof(System.IO.FileSystemInfo).Assembly.Location), + MetadataReference.CreateFromFile(typeof(System.Linq.Enumerable).Assembly.Location), + MetadataReference.CreateFromFile(typeof(object).Assembly.Location), + MetadataReference.CreateFromFile(typeof(MarshalByValueComponent).Assembly.Location), + MetadataReference.CreateFromFile(typeof(ObservableCollection).Assembly.Location), + + // New assemblies required by Terminal.Gui version 2 + MetadataReference.CreateFromFile(typeof(Size).Assembly.Location), + MetadataReference.CreateFromFile(typeof(CanBeNullAttribute).Assembly.Location), + + + MetadataReference.CreateFromFile(Path.Combine(coreDir.FullName, "mscorlib.dll")), + MetadataReference.CreateFromFile(Path.Combine(coreDir.FullName, "System.Runtime.dll")), + MetadataReference.CreateFromFile(Path.Combine(coreDir.FullName, "System.Collections.dll")), + MetadataReference.CreateFromFile(Path.Combine(coreDir.FullName, "System.Data.Common.dll")), + // Add more as necessary + }; + + + var projectInfo = ProjectInfo.Create ( + projectId, + VersionStamp.Create (), + "TestProject", + "TestAssembly", + LanguageNames.CSharp, + compilationOptions: new CSharpCompilationOptions (OutputKind.DynamicallyLinkedLibrary), + metadataReferences: references); + + var solution = workspace.CurrentSolution + .AddProject (projectInfo) + .AddDocument (documentId, "Test.cs", SourceText.From (source)); + + return solution.GetDocument (documentId)!; + } + + private static async Task> GetAnalyzerDiagnosticsAsync (Compilation compilation, DiagnosticAnalyzer analyzer) + { + var compilationWithAnalyzers = compilation.WithAnalyzers (ImmutableArray.Create (analyzer)); + return await compilationWithAnalyzers.GetAnalyzerDiagnosticsAsync (); + } + + private static async Task ApplyCodeFixAsync (Document document, Diagnostic diagnostic, CodeFixProvider codeFix) + { + CodeAction _codeAction = null; + var context = new CodeFixContext ((TextDocument)document, diagnostic, (action, _) => _codeAction = action, CancellationToken.None); + + await codeFix.RegisterCodeFixesAsync (context); + + if (_codeAction == null) + { + throw new InvalidOperationException ("Code fix did not register a fix."); + } + + var operations = await _codeAction.GetOperationsAsync (CancellationToken.None); + var solution = operations.OfType ().First ().ChangedSolution; + return solution.GetDocument (document.Id); + } +} diff --git a/Terminal.Gui.Analyzers.Tests/Terminal.Gui.Analyzers.Tests.csproj b/Terminal.Gui.Analyzers.Tests/Terminal.Gui.Analyzers.Tests.csproj new file mode 100644 index 000000000..a1d8f9a02 --- /dev/null +++ b/Terminal.Gui.Analyzers.Tests/Terminal.Gui.Analyzers.Tests.csproj @@ -0,0 +1,36 @@ + + + + enable + false + true + $(DefineConstants);JETBRAINS_ANNOTATIONS;CONTRACTS_FULL + portable + enable + true + true + true + + + + + + + + + + + + + + + + + + + + + PreserveNewest + + + \ No newline at end of file diff --git a/Terminal.Gui.Analyzers/AnalyzerReleases.Shipped.md b/Terminal.Gui.Analyzers/AnalyzerReleases.Shipped.md new file mode 100644 index 000000000..d00904b7a --- /dev/null +++ b/Terminal.Gui.Analyzers/AnalyzerReleases.Shipped.md @@ -0,0 +1,6 @@ +## Release 1.0.0 + +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|-------------------- diff --git a/Terminal.Gui.Analyzers/AnalyzerReleases.Unshipped.md b/Terminal.Gui.Analyzers/AnalyzerReleases.Unshipped.md new file mode 100644 index 000000000..77056a7b6 --- /dev/null +++ b/Terminal.Gui.Analyzers/AnalyzerReleases.Unshipped.md @@ -0,0 +1,5 @@ +### New Rules + +Rule ID | Category | Severity | Notes +--------|----------|----------|-------------------- +TGUI001 | Reliability | Warning | HandledEventArgsAnalyzer, [Documentation](./TGUI001.md) \ No newline at end of file diff --git a/Terminal.Gui.Analyzers/DiagnosticCategory.cs b/Terminal.Gui.Analyzers/DiagnosticCategory.cs new file mode 100644 index 000000000..dd580aa4e --- /dev/null +++ b/Terminal.Gui.Analyzers/DiagnosticCategory.cs @@ -0,0 +1,67 @@ +namespace Terminal.Gui.Analyzers; + +/// +/// Categories commonly used for diagnostic analyzers, inspired by FxCop and .NET analyzers conventions. +/// +internal enum DiagnosticCategory +{ + /// + /// Issues related to naming conventions and identifiers. + /// + Naming, + + /// + /// API design, class structure, inheritance, etc. + /// + Design, + + /// + /// How code uses APIs or language features incorrectly or suboptimally. + /// + Usage, + + /// + /// Patterns that cause poor runtime performance. + /// + Performance, + + /// + /// Vulnerabilities or insecure coding patterns. + /// + Security, + + /// + /// Code patterns that can cause bugs, crashes, or unpredictable behavior. + /// + Reliability, + + /// + /// Code readability, complexity, or future-proofing concerns. + /// + Maintainability, + + /// + /// Code patterns that may not work on all platforms or frameworks. + /// + Portability, + + /// + /// Issues with culture, localization, or globalization support. + /// + Globalization, + + /// + /// Problems when working with COM, P/Invoke, or other interop scenarios. + /// + Interoperability, + + /// + /// Issues with missing or incorrect XML doc comments. + /// + Documentation, + + /// + /// Purely stylistic issues not affecting semantics (e.g., whitespace, order). + /// + Style +} diff --git a/Terminal.Gui.Analyzers/HandledEventArgsAnalyzer.cs b/Terminal.Gui.Analyzers/HandledEventArgsAnalyzer.cs new file mode 100644 index 000000000..fa8e19bd4 --- /dev/null +++ b/Terminal.Gui.Analyzers/HandledEventArgsAnalyzer.cs @@ -0,0 +1,269 @@ +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; + +namespace Terminal.Gui.Analyzers; + +[DiagnosticAnalyzer (LanguageNames.CSharp)] +public class HandledEventArgsAnalyzer : DiagnosticAnalyzer +{ + public const string DiagnosticId = "TGUI001"; + private static readonly LocalizableString Title = "Accepting event handler should set Handled = true"; + private static readonly LocalizableString MessageFormat = "Accepting event handler does not set Handled = true"; + private static readonly LocalizableString Description = "Handlers for Accepting should mark the CommandEventArgs as handled by setting Handled = true otherwise subsequent Accepting event handlers may also fire (e.g. default buttons)."; + private static readonly string Url = "https://github.com/tznind/gui.cs/blob/analyzer-no-handled/Terminal.Gui.Analyzers/TGUI001.md"; + private const string Category = nameof(DiagnosticCategory.Reliability); + + private static readonly DiagnosticDescriptor _rule = new ( + DiagnosticId, + Title, + MessageFormat, + Category, + DiagnosticSeverity.Warning, + true, + Description, + helpLinkUri: Url); + + public override ImmutableArray SupportedDiagnostics => [_rule]; + + public override void Initialize (AnalysisContext context) + { + context.EnableConcurrentExecution (); + + // Only analyze non-generated code + context.ConfigureGeneratedCodeAnalysis (GeneratedCodeAnalysisFlags.None); + + // Register for b.Accepting += (s,e)=>{...}; + context.RegisterSyntaxNodeAction ( + AnalyzeLambdaOrAnonymous, + SyntaxKind.ParenthesizedLambdaExpression, + SyntaxKind.SimpleLambdaExpression, + SyntaxKind.AnonymousMethodExpression); + + // Register for b.Accepting += MyMethod; + context.RegisterSyntaxNodeAction ( + AnalyzeEventSubscriptionWithMethodGroup, + SyntaxKind.AddAssignmentExpression); + } + + private static void AnalyzeLambdaOrAnonymous (SyntaxNodeAnalysisContext context) + { + var lambda = (AnonymousFunctionExpressionSyntax)context.Node; + + // Check if this lambda is assigned to the Accepting event + if (!IsAssignedToAcceptingEvent (lambda.Parent, context)) + { + return; + } + + // Look for any parameter of type CommandEventArgs (regardless of name) + IParameterSymbol? eParam = GetCommandEventArgsParameter (lambda, context.SemanticModel); + + if (eParam == null) + { + return; + } + + // Analyze lambda body for e.Handled = true assignment + if (lambda.Body is BlockSyntax block) + { + bool setsHandled = block.Statements + .SelectMany (s => s.DescendantNodes ().OfType ()) + .Any (a => IsHandledAssignment (a, eParam, context)); + + if (!setsHandled) + { + var diag = Diagnostic.Create (_rule, lambda.GetLocation ()); + context.ReportDiagnostic (diag); + } + } + else if (lambda.Body is ExpressionSyntax) + { + // Expression-bodied lambdas unlikely for event handlers — skip + } + } + + /// + /// Finds the first parameter of type CommandEventArgs in any parameter list (method or lambda). + /// + /// + /// + /// + private static IParameterSymbol? GetCommandEventArgsParameter (SyntaxNode paramOwner, SemanticModel semanticModel) + { + SeparatedSyntaxList? parameters = paramOwner switch + { + AnonymousFunctionExpressionSyntax lambda => GetParameters (lambda), + MethodDeclarationSyntax method => method.ParameterList.Parameters, + _ => null + }; + + if (parameters == null || parameters.Value.Count == 0) + { + return null; + } + + foreach (ParameterSyntax param in parameters.Value) + { + IParameterSymbol? symbol = semanticModel.GetDeclaredSymbol (param); + + if (symbol != null && IsCommandEventArgsType (symbol.Type)) + { + return symbol; + } + } + + return null; + } + + private static bool IsAssignedToAcceptingEvent (SyntaxNode? node, SyntaxNodeAnalysisContext context) + { + if (node is AssignmentExpressionSyntax assignment && IsAcceptingEvent (assignment.Left, context)) + { + return true; + } + + if (node?.Parent is AssignmentExpressionSyntax parentAssignment && IsAcceptingEvent (parentAssignment.Left, context)) + { + return true; + } + + return false; + } + + private static bool IsCommandEventArgsType (ITypeSymbol? type) { return type != null && type.Name == "CommandEventArgs"; } + + private static void AnalyzeEventSubscriptionWithMethodGroup (SyntaxNodeAnalysisContext context) + { + var assignment = (AssignmentExpressionSyntax)context.Node; + + // Check event name: b.Accepting += ... + if (!IsAcceptingEvent (assignment.Left, context)) + { + return; + } + + // Right side: should be method group (IdentifierNameSyntax) + if (assignment.Right is IdentifierNameSyntax methodGroup) + { + // Resolve symbol of method group + SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo (methodGroup); + + if (symbolInfo.Symbol is IMethodSymbol methodSymbol) + { + // Find method declaration in syntax tree + ImmutableArray declRefs = methodSymbol.DeclaringSyntaxReferences; + + foreach (SyntaxReference declRef in declRefs) + { + var methodDecl = declRef.GetSyntax () as MethodDeclarationSyntax; + + if (methodDecl != null) + { + AnalyzeHandlerMethodBody (context, methodDecl, methodSymbol); + } + } + } + } + } + + private static void AnalyzeHandlerMethodBody (SyntaxNodeAnalysisContext context, MethodDeclarationSyntax methodDecl, IMethodSymbol methodSymbol) + { + // Look for any parameter of type CommandEventArgs + IParameterSymbol? eParam = GetCommandEventArgsParameter (methodDecl, context.SemanticModel); + + if (eParam == null) + { + return; + } + + // Analyze method body + if (methodDecl.Body != null) + { + bool setsHandled = methodDecl.Body.Statements + .SelectMany (s => s.DescendantNodes ().OfType ()) + .Any (a => IsHandledAssignment (a, eParam, context)); + + if (!setsHandled) + { + var diag = Diagnostic.Create (_rule, methodDecl.Identifier.GetLocation ()); + context.ReportDiagnostic (diag); + } + } + } + + private static SeparatedSyntaxList GetParameters (AnonymousFunctionExpressionSyntax lambda) + { + switch (lambda) + { + case ParenthesizedLambdaExpressionSyntax p: + return p.ParameterList.Parameters; + case SimpleLambdaExpressionSyntax s: + // Simple lambda has a single parameter, wrap it in a list + return SyntaxFactory.SeparatedList (new [] { s.Parameter }); + case AnonymousMethodExpressionSyntax a: + return a.ParameterList?.Parameters ?? default (SeparatedSyntaxList); + default: + return default (SeparatedSyntaxList); + } + } + + private static bool IsAcceptingEvent (ExpressionSyntax expr, SyntaxNodeAnalysisContext context) + { + // Check if expr is b.Accepting or similar + + // Get symbol info + SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo (expr); + ISymbol? symbol = symbolInfo.Symbol; + + if (symbol == null) + { + return false; + } + + // Accepting event symbol should be an event named "Accepting" + if (symbol.Kind == SymbolKind.Event && symbol.Name == "Accepting") + { + return true; + } + + return false; + } + + private static bool IsHandledAssignment (AssignmentExpressionSyntax assignment, IParameterSymbol eParamSymbol, SyntaxNodeAnalysisContext context) + { + // Check if left side is "e.Handled" and right side is "true" + // Left side should be MemberAccessExpression: e.Handled + + if (assignment.Left is MemberAccessExpressionSyntax memberAccess) + { + // Check that member access expression is "e.Handled" + ISymbol? exprSymbol = context.SemanticModel.GetSymbolInfo (memberAccess.Expression).Symbol; + + if (exprSymbol == null) + { + return false; + } + + if (!SymbolEqualityComparer.Default.Equals (exprSymbol, eParamSymbol)) + { + return false; + } + + if (memberAccess.Name.Identifier.Text != "Handled") + { + return false; + } + + // Check right side is true literal + if (assignment.Right is LiteralExpressionSyntax literal && literal.IsKind (SyntaxKind.TrueLiteralExpression)) + { + return true; + } + } + + return false; + } +} diff --git a/Terminal.Gui.Analyzers/TGUI001.md b/Terminal.Gui.Analyzers/TGUI001.md new file mode 100644 index 000000000..457a32180 --- /dev/null +++ b/Terminal.Gui.Analyzers/TGUI001.md @@ -0,0 +1,34 @@ +# TGUI001: Describe what your rule checks + +**Category:** Reliability +**Severity:** Warning +**Enabled by default:** Yes + +## Cause + +When registering an event handler for `Accepting`, you should set Handled to true, this prevents other subsequent Views from responding to the same input event. + +## Reason for rule + +If you do not do this then you may see unpredictable behaviour such as clicking a Button resulting in another `IsDefault` button in the View also firing. + +See: + +- https://github.com/gui-cs/Terminal.Gui/issues/3913 +- https://github.com/gui-cs/Terminal.Gui/issues/4170 + +## How to fix violations + +Set Handled to `true` in your event handler + +### Examples + +```diff +var b = new Button(); +b.Accepting += (s, e) => +{ + // Do something + ++ e.Handled = true; +}; +``` \ No newline at end of file diff --git a/Terminal.Gui.Analyzers/Terminal.Gui.Analyzers.csproj b/Terminal.Gui.Analyzers/Terminal.Gui.Analyzers.csproj new file mode 100644 index 000000000..d1390b5b8 --- /dev/null +++ b/Terminal.Gui.Analyzers/Terminal.Gui.Analyzers.csproj @@ -0,0 +1,18 @@ + + + + netstandard2.0 + false + true + + enable + + + + + + + + + + diff --git a/Terminal.Gui/Terminal.Gui.csproj b/Terminal.Gui/Terminal.Gui.csproj index 300e5a972..b78fc2e16 100644 --- a/Terminal.Gui/Terminal.Gui.csproj +++ b/Terminal.Gui/Terminal.Gui.csproj @@ -61,7 +61,11 @@ - + + + @@ -155,6 +159,16 @@ + + + + + + + + + +