From 1e38314a47ec4482fcb54d35d7e4ec0f95bb88cd Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Wed, 2 Jul 2025 11:15:20 -0700 Subject: [PATCH 1/2] Add EPC37: Warn on argument validation in async methods Introduces analyzer EPC37 to detect argument validation in public async methods, which can cause exceptions to be thrown only when the returned Task is awaited. Adds documentation, analyzer implementation, tests, and diagnostic descriptor for EPC37. Updates ReadMe and analyzer release notes accordingly. --- ReadMe.md | 4 +- docs/Rules/EPC37.md | 277 +++++++++++++ ...ateArgumentsInAsyncMethodsAnalyzerTests.cs | 383 ++++++++++++++++++ .../AnalyzerReleases.Unshipped.md | 1 + ...ValidateArgumentsInAsyncMethodsAnalyzer.cs | 204 ++++++++++ .../DiagnosticDescriptors.cs | 12 + .../CSharpCodeFixVerifier`2+Test.cs | 11 +- 7 files changed, 888 insertions(+), 4 deletions(-) create mode 100644 docs/Rules/EPC37.md create mode 100644 src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzerTests.cs create mode 100644 src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs diff --git a/ReadMe.md b/ReadMe.md index fed3238..a521125 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -20,19 +20,21 @@ Add the following nuget package to you project: https://www.nuget.org/packages/E | [EPC16](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC16.md) | Awaiting a result of a null-conditional expression will cause NullReferenceException | | [EPC17](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC17.md) | Avoid async-void delegates | | [EPC18](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC18.md) | A task instance is implicitly converted to a string | -| [EPC20](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC20.md) | Avoid using default ToString implementation | | [EPC26](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC26.md) | Do not use tasks in using block | | [EPC27](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC27.md) | Avoid async void methods | | [EPC31](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC31.md) | Do not return null for Task-like types | | [EPC32](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC32.md) | TaskCompletionSource should use RunContinuationsAsynchronously | | [EPC33](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC33.md) | Do not use Thread.Sleep in async methods | | [EPC35](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC35.md) | Do not block unnecessarily in async methods | +| [EPC36](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC36.md) | Do not use async delegates with Task.Factory.StartNew and TaskCreationOptions.LongRunning | +| [EPC37](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC37.md) | Do not validate arguments in async methods | ### Generic Bugs and Code Smells | Id | Description | |---|---| | [EPC19](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC19.md) | Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks | +| [EPC20](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC20.md) | Avoid using default ToString implementation | | [EPC28](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC28.md) | Do not use ExcludeFromCodeCoverage on partial classes | | [EPC29](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC29.md) | ExcludeFromCodeCoverageAttribute should provide a message | | [EPC30](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC30.md) | Method calls itself recursively | diff --git a/docs/Rules/EPC37.md b/docs/Rules/EPC37.md new file mode 100644 index 0000000..b85e4f5 --- /dev/null +++ b/docs/Rules/EPC37.md @@ -0,0 +1,277 @@ +# EPC37 - Do not validate arguments in async methods + +This analyzer warns when public async methods validate arguments and throw exceptions. Such validation is not eager and exceptions are thrown when the task is awaited, which can lead to unexpected behavior. + +## Description + +When you validate arguments in async methods by throwing exceptions, the exceptions are not thrown immediately when the method is called. Instead, they are thrown when the returned `Task` is awaited. This behavior can be confusing and lead to bugs because: + +1. **Delayed failure**: Argument validation failures are not discovered until the task is awaited, which may happen much later in the code +2. **Inconsistent behavior**: Synchronous methods validate arguments eagerly, but async methods do not +3. **Debugging difficulty**: Stack traces may not clearly show where the invalid argument was passed + +This analyzer only reports diagnostics for public methods in public classes, as these are the API boundaries where eager validation is most important. + +## Code that triggers the analyzer + +❌ **Bad** - Argument validation in async methods: +```csharp +using System; +using System.Threading.Tasks; + +public class FileService +{ + public async Task ReadFileAsync(string fileName) + { + // ❌ EPC37: This exception won't be thrown until the task is awaited + if (fileName == null) + throw new ArgumentNullException(nameof(fileName)); + + if (string.IsNullOrWhiteSpace(fileName)) + throw new ArgumentException("File name cannot be empty", nameof(fileName)); + + return await File.ReadAllTextAsync(fileName); + } + + public async Task ProcessDataAsync(int[] data) + { + // ❌ EPC37: Using modern validation methods still has the same problem + ArgumentNullException.ThrowIfNull(data); + + foreach (var item in data) + { + await ProcessItemAsync(item); + } + } +} +``` + +## How to fix + +There are several approaches to fix this issue: + +✅ **Good** - Use wrapper method pattern: +```csharp +using System; +using System.Threading.Tasks; + +public class FileService +{ + public Task ReadFileAsync(string fileName) + { + // Validate arguments eagerly in the wrapper + if (fileName == null) + throw new ArgumentNullException(nameof(fileName)); + + if (string.IsNullOrWhiteSpace(fileName)) + throw new ArgumentException("File name cannot be empty", nameof(fileName)); + + // Call the actual async implementation + return ReadFileAsyncCore(fileName); + } + + private async Task ReadFileAsyncCore(string fileName) + { + return await File.ReadAllTextAsync(fileName); + } +} +``` +✅ **Good** - Use local functions for cleaner code: +```csharp +using System; +using System.Threading.Tasks; + +public class FileService +{ + public Task ReadFileAsync(string fileName) + { + // Validate arguments eagerly + if (fileName == null) + throw new ArgumentNullException(nameof(fileName)); + + if (string.IsNullOrWhiteSpace(fileName)) + throw new ArgumentException("File name cannot be empty", nameof(fileName)); + + // Use local async function + return ReadFileAsyncLocal(); + + async Task ReadFileAsyncLocal() + { + return await File.ReadAllTextAsync(fileName); + } + } +} +``` + +## When this rule doesn't apply + +The analyzer only reports diagnostics when: +- The method is `async` +- The method is `public` +- The containing type and all parent types are `public` +- The exception is thrown before any `await` expressions in the method +- The exception is an `ArgumentException`, `ArgumentNullException`, `ArgumentOutOfRangeException`, or inherits from `ArgumentException` + +❌ **No warning** - Private methods: +```csharp +public class FileService +{ + private async Task ReadFileAsyncInternal(string fileName) + { + // No warning: private method + if (fileName == null) throw new ArgumentNullException(nameof(fileName)); + return await File.ReadAllTextAsync(fileName); + } +} +``` + +❌ **No warning** - Non-public classes: +```csharp +internal class FileService +{ + public async Task ReadFileAsync(string fileName) + { + // No warning: internal class + if (fileName == null) throw new ArgumentNullException(nameof(fileName)); + return await File.ReadAllTextAsync(fileName); + } +} +``` + +❌ **No warning** - Synchronous methods: +```csharp +public class FileService +{ + public string ReadFile(string fileName) + { + // No warning: synchronous method validates arguments eagerly + if (fileName == null) throw new ArgumentNullException(nameof(fileName)); + return File.ReadAllText(fileName); + } +} +``` + +❌ **No warning** - Exceptions after await: +```csharp +public class FileService +{ + public async Task ProcessFileAsync(string fileName) + { + var content = await File.ReadAllTextAsync(fileName); + + // No warning: this is not argument validation, it's business logic validation + if (content.Length == 0) + throw new ArgumentException("File is empty"); + + return content.ToUpper(); + } +} +``` + +## Performance and behavior impact + +The wrapper method pattern has minimal performance overhead and provides the expected eager validation behavior: + +```csharp +// This will throw immediately +try +{ + var task = fileService.ReadFileAsync(null); // Throws ArgumentNullException here + await task; +} +catch (ArgumentNullException ex) +{ + // Exception caught immediately, not when awaiting +} + +// VS the problematic async validation: +try +{ + var task = problematicService.ReadFileAsync(null); // Returns a task + await task; // ArgumentNullException thrown here when awaited +} +catch (ArgumentNullException ex) +{ + // Exception caught later, during await +} +``` + +## Examples in context + +❌ **Bad** - API that fails unexpectedly: +```csharp +public class DataProcessor +{ + public async Task ProcessAsync(string data, CancellationToken cancellationToken) + { + // ❌ These validations won't fail until the task is awaited + ArgumentNullException.ThrowIfNull(data); + if (data.Length == 0) throw new ArgumentException("Data cannot be empty"); + + await SomeAsyncOperation(cancellationToken); + return new ProcessResult(data); + } +} + +// Usage that demonstrates the problem: +var processor = new DataProcessor(); +var tasks = new List>(); + +// All these calls succeed and return tasks, even with invalid arguments! +tasks.Add(processor.ProcessAsync(null, CancellationToken.None)); +tasks.Add(processor.ProcessAsync("", CancellationToken.None)); +tasks.Add(processor.ProcessAsync("valid", CancellationToken.None)); + +// Only when we await do we discover the validation failures +foreach (var task in tasks) +{ + try + { + var result = await task; // Failures happen here, not at call site + } + catch (ArgumentException ex) + { + Console.WriteLine($"Validation failed: {ex.Message}"); + } +} +``` + +✅ **Good** - API that fails eagerly: +```csharp +public class DataProcessor +{ + public Task ProcessAsync(string data, CancellationToken cancellationToken) + { + // ✅ Validate arguments eagerly + ArgumentNullException.ThrowIfNull(data); + if (data.Length == 0) throw new ArgumentException("Data cannot be empty"); + + return ProcessAsyncCore(data, cancellationToken); + } + + private async Task ProcessAsyncCore(string data, CancellationToken cancellationToken) + { + await SomeAsyncOperation(cancellationToken); + return new ProcessResult(data); + } +} + +// Usage with eager validation: +var processor = new DataProcessor(); +var tasks = new List>(); + +try +{ + // These calls throw immediately if arguments are invalid + tasks.Add(processor.ProcessAsync(null, CancellationToken.None)); // Throws here! +} +catch (ArgumentNullException ex) +{ + Console.WriteLine($"Invalid argument caught immediately: {ex.Message}"); +} +``` + +## See also + +- [Async/await best practices](https://docs.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming) +- [Argument validation in async methods - Stephen Cleary](https://blog.stephencleary.com/2014/12/async-oop-2-constructors.html) diff --git a/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzerTests.cs b/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzerTests.cs new file mode 100644 index 0000000..a1837bd --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers.Tests/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzerTests.cs @@ -0,0 +1,383 @@ +using System; +using NUnit.Framework; +using System.Threading.Tasks; +using Verify = ErrorProne.NET.TestHelpers.CSharpCodeFixVerifier< + ErrorProne.NET.AsyncAnalyzers.DoNotValidateArgumentsInAsyncMethodsAnalyzer, + Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>; + +namespace ErrorProne.NET.CoreAnalyzers.Tests.AsyncAnalyzers +{ + [TestFixture] + public class DoNotValidateArgumentsInAsyncMethodsAnalyzerTests + { + [Test] + public async Task WarnOnArgumentNullExceptionInPublicAsyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + if (s == null) [|throw new ArgumentNullException(nameof(s));|] + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnArgumentExceptionInPublicAsyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + if (string.IsNullOrEmpty(s)) [|throw new ArgumentException(""Value cannot be empty"", nameof(s));|] + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnArgumentException_Via_Factory_Method_InPublicAsyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + if (string.IsNullOrEmpty(s)) [|throw CreateArgumentException(nameof(s));|] + await Task.Delay(42); + } + + static ArgumentException CreateArgumentException(string paramName) + { + return new ArgumentException(""Value cannot be empty"", paramName); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnArgumentOutOfRangeExceptionInPublicAsyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(int value) + { + if (value < 0) [|throw new ArgumentOutOfRangeException(nameof(value));|] + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnArgumentNullExceptionThrowIfNull() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + [|ArgumentNullException.ThrowIfNull(s)|]; + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnNonPublicAsyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + private async Task FooAsync(string s) + { + if (s == null) throw new ArgumentNullException(nameof(s)); + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnAsyncMethodInNonPublicClass() + { + var test = @" +using System; +using System.Threading.Tasks; + +internal class Test +{ + public async Task FooAsync(string s) + { + if (s == null) throw new ArgumentNullException(nameof(s)); + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnSyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public void Foo(string s) + { + if (s == null) throw new ArgumentNullException(nameof(s)); + // Some synchronous work + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnThrowAfterAwait() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + await Task.Delay(42); + if (s == null) throw new ArgumentNullException(nameof(s)); // This is fine, not argument validation + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnNonArgumentException() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + if (s == null) throw new InvalidOperationException(); + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnCustomArgumentException() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class CustomArgumentException : ArgumentException +{ + public CustomArgumentException(string message) : base(message) { } +} + +public class Test +{ + public async Task FooAsync(string s) + { + if (s == null) [|throw new CustomArgumentException(""Invalid argument"");|] + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnThrowInLambda() + { + var test = @" +using System; +using System.Linq; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string[] items) + { + var result = items.Where(x => + { + if (x == null) throw new ArgumentNullException(nameof(x)); // This is in a lambda, not direct validation + return x.Length > 0; + }); + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnThrowInLocalFunction() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s) + { + void ValidateArgument(string arg) + { + if (arg == null) throw new ArgumentNullException(nameof(arg)); // This is in a local function + } + + ValidateArgument(s); + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnGenericAsyncMethod() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(T value) where T : class + { + if (value == null) [|throw new ArgumentNullException(nameof(value));|] + await Task.Delay(42); + return value; + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnAsyncMethodReturningCustomTask() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async ValueTask FooAsync(string s) + { + if (s == null) [|throw new ArgumentNullException(nameof(s));|] + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnMultipleValidationStatements() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Test +{ + public async Task FooAsync(string s, int value) + { + if (s == null) [|throw new ArgumentNullException(nameof(s));|] + if (value < 0) [|throw new ArgumentOutOfRangeException(nameof(value));|] + await Task.Delay(42); + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task WarnOnNestedPublicClass() + { + var test = @" +using System; +using System.Threading.Tasks; + +public class Outer +{ + public class Inner + { + public async Task FooAsync(string s) + { + if (s == null) [|throw new ArgumentNullException(nameof(s));|] + await Task.Delay(42); + } + } +} +"; + await Verify.VerifyAsync(test); + } + + [Test] + public async Task DoNotWarnOnNestedClassWithNonPublicParent() + { + var test = @" +using System; +using System.Threading.Tasks; + +internal class Outer +{ + public class Inner + { + public async Task FooAsync(string s) + { + if (s == null) throw new ArgumentNullException(nameof(s)); + await Task.Delay(42); + } + } +} +"; + await Verify.VerifyAsync(test); + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md index 1fb7d80..c166929 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -28,6 +28,7 @@ EPC33 | Async | Warning | DoNotUseThreadSleepAnalyzer EPC34 | ErrorHandling | Warning | MustUseResultAnalyzer EPC35 | Async | Warning | DoNotBlockUnnecessarilyInAsyncMethodsAnalyzer EPC36 | Async | Warning | DoNotUseAsyncDelegatesForLongRunningTasksAnalyzer +EPC37 | Async | Info | DoNotValidateArgumentsInAsyncMethodsAnalyzer ERP021 | ErrorHandling | Warning | ThrowExAnalyzer ERP022 | ErrorHandling | Warning | SwallowAllExceptionsAnalyzer ERP031 | Concurrency | Warning | ConcurrentCollectionAnalyzer diff --git a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs new file mode 100644 index 0000000..ff71f7a --- /dev/null +++ b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs @@ -0,0 +1,204 @@ +using System; +using System.Collections.Immutable; +using System.Linq; +using ErrorProne.NET.Core; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.CSharp.Syntax; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Operations; + +namespace ErrorProne.NET.AsyncAnalyzers +{ + /// + /// Analyzer that warns when async methods validate arguments and throw exceptions. + /// Such validation is not eager and exceptions are thrown when the task is awaited. + /// + [DiagnosticAnalyzer(LanguageNames.CSharp)] + public sealed class DoNotValidateArgumentsInAsyncMethodsAnalyzer : DiagnosticAnalyzer + { + private static readonly DiagnosticDescriptor Rule = DiagnosticDescriptors.EPC37; + + /// + public override ImmutableArray SupportedDiagnostics => [Rule]; + + /// + public override void Initialize(AnalysisContext context) + { + context.EnableConcurrentExecution(); + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.Analyze | GeneratedCodeAnalysisFlags.ReportDiagnostics); + context.RegisterOperationAction(AnalyzeThrow, OperationKind.Throw); + context.RegisterOperationAction(AnalyzeInvocation, OperationKind.Invocation); + } + + private static void AnalyzeThrow(OperationAnalysisContext context) + { + var throwOperation = (IThrowOperation)context.Operation; + var method = GetEnclosingMethod(throwOperation, context); + + if (method == null || !ShouldAnalyze(method)) + { + return; + } + + // Check if this is argument validation (throw in the beginning of the method) + if (IsArgumentValidationThrow(throwOperation, method)) + { + ReportDiagnostic(context, throwOperation.Syntax.GetLocation(), method.Name); + } + } + + private static void AnalyzeInvocation(OperationAnalysisContext context) + { + var invocation = (IInvocationOperation)context.Operation; + var method = GetEnclosingMethod(invocation, context); + + if (method == null || !ShouldAnalyze(method)) + { + return; + } + + // Check for ArgumentNullException.ThrowIfNull and similar methods + if (IsArgumentValidationInvocation(invocation)) + { + ReportDiagnostic(context, invocation.Syntax.GetLocation(), method.Name); + } + } + + private static IMethodSymbol? GetEnclosingMethod(IOperation operation, OperationAnalysisContext context) + { + if (operation.FindParentLocalOrLambdaSymbol() != null) + { + // We don't want to check inside local functions or lambdas + return null; + } + + return context.ContainingSymbol as IMethodSymbol; + } + + private static bool ShouldAnalyze(IMethodSymbol method) + { + // Only analyze async methods + if (!method.IsAsync) + { + return false; + } + + // Only analyze public methods in public types + if (method.DeclaredAccessibility != Accessibility.Public) + { + return false; + } + + var type = method.ContainingType; + while (type != null) + { + if (type.DeclaredAccessibility != Accessibility.Public) + { + return false; + } + type = type.ContainingType; + } + + return true; + } + + private static bool IsArgumentValidationThrow(IThrowOperation throwOperation, IMethodSymbol method) + { + // Check if the throw is in the beginning of the method (before any await) + if (!IsInMethodBeginning(throwOperation, method)) + { + return false; + } + + // Check if it's throwing argument-related exceptions + var thrownException = throwOperation.Exception; + if (thrownException is IConversionOperation conversion) + { + if (conversion.Operand is IObjectCreationOperation objectCreation) + { + // throw new ArgumentException("message", "paramName"); + return IsArgumentException(objectCreation.Type); + } + + if (conversion.Operand is IInvocationOperation invocation) + { + // throw CreateArgumentException("message", "paramName"); + return IsArgumentException(invocation.TargetMethod.ReturnType); + } + } + + return false; + } + + private static bool IsArgumentValidationInvocation(IInvocationOperation invocation) + { + var targetMethod = invocation.TargetMethod; + + // Checking for 'ThrowIf' methods + + if (!targetMethod.Name.StartsWith("ThrowIf")) + { + return false; + } + + // This is 'ThrowIfNull' or similar methods + + var typeName = targetMethod.ContainingType?.Name; + return typeName is nameof(ArgumentNullException) or nameof(ArgumentException) or nameof(ArgumentOutOfRangeException); + + // This potentially can be extended in the future by providing a list of validation methods + // inside .editorconfig to support custom validation methods. + return false; + } + + private static bool IsInMethodBeginning(IOperation operation, IMethodSymbol method) + { + // Check if there are any await operations before this operation in the method + var methodSyntax = method.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() as MethodDeclarationSyntax; + if (methodSyntax?.Body == null) + { + return false; + } + + var operationSyntax = operation.Syntax; + var awaitExpressions = methodSyntax.Body.DescendantNodes() + .OfType() + .Where(await => await.SpanStart < operationSyntax.SpanStart); + + return !awaitExpressions.Any(); + } + + private static bool IsArgumentException(ITypeSymbol? exceptionType) + { + if (exceptionType == null) + { + return false; + } + + var typeName = exceptionType.Name; + return typeName == nameof(ArgumentException) || + typeName == nameof(ArgumentNullException) || + typeName == nameof(ArgumentOutOfRangeException) || + IsInheritedFromArgumentException(exceptionType); + } + + private static bool IsInheritedFromArgumentException(ITypeSymbol exceptionType) + { + var baseType = exceptionType.BaseType; + while (baseType != null) + { + if (baseType.Name == nameof(ArgumentException)) + { + return true; + } + baseType = baseType.BaseType; + } + return false; + } + + private static void ReportDiagnostic(OperationAnalysisContext context, Location location, string methodName) + { + context.ReportDiagnostic(Diagnostic.Create(Rule, location, methodName)); + } + } +} diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index 34396fe..b243ccc 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -300,6 +300,18 @@ internal static class DiagnosticDescriptors description: "TaskCreationOptions.LongRunning is intended for long-running synchronous work. Using it with async delegates wastes thread pool threads and defeats the purpose of using async methods in this case.", helpLinkUri: GetHelpUri(nameof(EPC36))); + /// + public static readonly DiagnosticDescriptor EPC37 = new DiagnosticDescriptor( + nameof(EPC37), + title: "Do not validate arguments in async methods", + messageFormat: "Argument validation in async method '{0}' will not fail eagerly. Consider using a wrapper method or Task.FromException.", + category: AsyncCategory, + // Info by default, since it might generate quite a bit of warnings for a codebase. + defaultSeverity: DiagnosticSeverity.Info, + isEnabledByDefault: true, + description: "Argument validation in async methods does not throw exceptions eagerly. The exception is thrown when the task is awaited, which can lead to unexpected behavior.", + helpLinkUri: GetHelpUri(nameof(EPC37))); + public static string GetHelpUri(string ruleId) { return $"https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/{ruleId}.md"; diff --git a/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs b/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs index 513f91d..8d9ad01 100644 --- a/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs +++ b/src/RoslynNunitTestRunner/CSharpCodeFixVerifier`2+Test.cs @@ -12,12 +12,17 @@ public static partial class CSharpCodeFixVerifier where TAnalyzer : DiagnosticAnalyzer, new() where TCodeFix : CodeFixProvider, new() { - public static Task VerifyAsync(string code) + public static Task VerifyAsync(string code, LanguageVersion langVersion = LanguageVersion.Latest) { return new Test { - TestState = { Sources = { code } } - + TestState = + { + Sources = { code }, + }, + LanguageVersion = langVersion, + ReferenceAssemblies = ReferenceAssemblies.Net.Net80 + }.WithoutGeneratedCodeVerification().RunAsync(); } From dfb03c8c6f84386bb40f1992f17b6eca01bc45c5 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Wed, 2 Jul 2025 11:19:59 -0700 Subject: [PATCH 2/2] Refactor argument validation type check logic Replaces a single-line return statement with an explicit if block for checking exception type names. This change improves readability and prepares for potential future extensions. --- .../DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs index ff71f7a..c426c45 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/AsyncAnalyzers/DoNotValidateArgumentsInAsyncMethodsAnalyzer.cs @@ -144,7 +144,12 @@ private static bool IsArgumentValidationInvocation(IInvocationOperation invocati // This is 'ThrowIfNull' or similar methods var typeName = targetMethod.ContainingType?.Name; - return typeName is nameof(ArgumentNullException) or nameof(ArgumentException) or nameof(ArgumentOutOfRangeException); + if (typeName is nameof(ArgumentNullException) + or nameof(ArgumentException) + or nameof(ArgumentOutOfRangeException)) + { + return true; + } // This potentially can be extended in the future by providing a list of validation methods // inside .editorconfig to support custom validation methods.