From c7b56f5d105619828a947073a772d12cff240065 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Mon, 30 Jun 2025 22:52:13 -0700 Subject: [PATCH 1/2] Add docs for all the analyzers --- ReadMe.md | 346 +++--------------- docs/Rules/EPC11.md | 57 +++ docs/Rules/EPC12.md | 41 +++ docs/Rules/EPC13.md | 64 ++++ docs/Rules/EPC14.md | 36 ++ docs/Rules/EPC15.md | 46 +++ docs/Rules/EPC16.md | 64 ++++ docs/Rules/EPC17.md | 61 +++ docs/Rules/EPC18.md | 51 +++ docs/Rules/EPC19.md | 95 +++++ docs/Rules/EPC20.md | 78 ++++ docs/Rules/EPC23.md | 68 ++++ docs/Rules/EPC24.md | 81 ++++ docs/Rules/EPC25.md | 96 +++++ docs/Rules/EPC26.md | 80 ++++ docs/Rules/EPC27.md | 102 ++++++ docs/Rules/EPC28.md | 124 +++++++ docs/Rules/EPC29.md | 93 +++++ docs/Rules/EPC30.md | 109 ++++++ docs/Rules/EPC31.md | 116 ++++++ docs/Rules/EPC32.md | 124 +++++++ docs/Rules/EPC33.md | 106 ++++++ docs/Rules/EPC34.md | 169 +++++++++ docs/Rules/ERP021.md | 166 +++++++++ docs/Rules/ERP022.md | 214 +++++++++++ docs/Rules/ERP031.md | 253 +++++++++++++ docs/Rules/ERP041.md | 211 +++++++++++ docs/Rules/ERP042.md | 263 +++++++++++++ .../AnalyzerReleases.Unshipped.md | 38 +- .../DiagnosticDescriptors.cs | 248 +++++++------ 30 files changed, 3182 insertions(+), 418 deletions(-) create mode 100644 docs/Rules/EPC11.md create mode 100644 docs/Rules/EPC12.md create mode 100644 docs/Rules/EPC13.md create mode 100644 docs/Rules/EPC14.md create mode 100644 docs/Rules/EPC15.md create mode 100644 docs/Rules/EPC16.md create mode 100644 docs/Rules/EPC17.md create mode 100644 docs/Rules/EPC18.md create mode 100644 docs/Rules/EPC19.md create mode 100644 docs/Rules/EPC20.md create mode 100644 docs/Rules/EPC23.md create mode 100644 docs/Rules/EPC24.md create mode 100644 docs/Rules/EPC25.md create mode 100644 docs/Rules/EPC26.md create mode 100644 docs/Rules/EPC27.md create mode 100644 docs/Rules/EPC28.md create mode 100644 docs/Rules/EPC29.md create mode 100644 docs/Rules/EPC30.md create mode 100644 docs/Rules/EPC31.md create mode 100644 docs/Rules/EPC32.md create mode 100644 docs/Rules/EPC33.md create mode 100644 docs/Rules/EPC34.md create mode 100644 docs/Rules/ERP021.md create mode 100644 docs/Rules/ERP022.md create mode 100644 docs/Rules/ERP031.md create mode 100644 docs/Rules/ERP041.md create mode 100644 docs/Rules/ERP042.md diff --git a/ReadMe.md b/ReadMe.md index d2fbe18..46d61d2 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -1,304 +1,64 @@ # Error Prone .NET -[![Build Status](https://seteplia.visualstudio.com/ErrorProne.NET/_apis/build/status/SergeyTeplyakov.ErrorProne.NET?label=build)](https://seteplia.visualstudio.com/ErrorProne.NET/_build/latest?definitionId=1) - ErrorProne.NET is a set of Roslyn-based analyzers that will help you to write correct code. The idea is similar to Google's [error-prone](https://github.com/google/error-prone) but instead of Java, the analyzers are focusing on correctness (and, maybe, performance) of C# programs. -Currently, there are two types of analyzers that split into two projects that ended up in two separate nuget packages: -* ErrorProne.CoreAnalyzers - the analyzers covering the most common cases that may occur in almost any project, like error handling or correctness of some widely used API. -* ErrorProne.StructAnalyzers - the analyzers focusing on potential performance problem when dealing with structs in C#. - -## ErrorProne.CoreAnalyzers -The "core" analyzers are the analyzers that will be useful in almost any .NET project by focusing on the correctness aspects and not on performance or low memory footprint. - -### Core Analyzers - -#### Unobserved Result Analysis -Some projects are heavily rely on a set of special result type and instead of exception handling patterns are using `Result` or `Possible` families of types. -In this case, it is very important for the callers of operations that return a result to observe it: - -```csharp -public class Result -{ - public bool Success = false; -} - -public static Result ProcessRequest() => null; - -// Result of type 'Result' should be observed -ProcessRequest(); -//~~~~~~~~~~~~ -``` - -The analyzers emit a diagnostic for every method that return a type with `Result` in its name as well as for some other well-known types that should be observed in vast majority of cases: -```csharp -Stream s = null; -// The result of type 'Task' should be observed -s.FlushAsync(); -//~~~~~~~~~~ +## Installation -// The result of type 'Exception' should be observed -getException(); -//~~~~~~~~~~ -Exception getException() => null; -``` +Add the following nuget package to you project: https://www.nuget.org/packages/ErrorProne.NET.CoreAnalyzers/ -### Suspicious equality implementation -```csharp -public class FooBar : IEquatable -{ - private string _s; +## Rules - // Suspicious equality implementation: parameter 'other' is never used - public bool Equals(FooBar other) - // ~~~~~ - { - return _s == "42"; - } -} +## Rules -public class Baz : IEquatable -{ - private string _s; - - // Suspicious equality implementation: no instance members are used - public bool Equals(Baz other) - // ~~~~~~ - { - return other != null; - } -} -``` - -### Exception handling analyzers - -Correct exception handling is a very complex topic, but one case that is very common requires special attention. A generic handler that handles `System.Exception` or `System.AggregateException` should "observe" the whole exception instance and not only the `Message` property. The `Message` property is quite important, but in some cases it can be quite meaningless. For instance, the `Message` property for `TargetInvocationException`, `AggregateException`, `TypeLoadExcpetion` and some others doesn't provide anything useful in it's message and the most useful information is stored in `InnerException`. - -To avoid this anti-pattern, the analyzer will warn you if the code only traces the `Exception` property without "looking inside": - -**EPC12 - Suspicious exception handling: only '' is observed in the catch block.** - -```csharp -try -{ - Console.WriteLine(); -} -catch (Exception e) -{ - // Suspicious exception handling: only e.Message is observed in exception block - Console.WriteLine(e.Message); - // ~~~~~~~ -} -``` -**ERP022 - swallows an unobserved exception.** -```csharp -try -{ - Console.WriteLine(); -} -catch (Exception e) -{ - // Exit point 'return' swallows an unobserved exception - return; -// ~~~~~~ -} -``` -**ERP021 - incorrect exception propagation. Use 'throw;' instead** -```csharp -try -{ - Console.WriteLine(); -} -catch (Exception e) -{ - // Incorrect exception propagation: use 'throw' instead - throw e; - // ~ -} -``` ### Async Analyzers -```csharp -Stream sample = null; -// Awaiting the result of a null-conditional expression may cause NullReferenceException. -await sample?.FlushAsync(); -``` - -#### Configuring `ConfigureAwait` behavior - -The strictness of whether to use `ConfigureAwait` everywhere is very much depends on the project and the layer of the project. It is very important for all the library code to always use `ConfigureAwait(false)` to avoid potential issues like deadlocks. On the other hand, some other parts of the system maybe used only in service layer and the team may decide not to litter the code with redudnat `ConfigureAwait(false)` calls. But regardless of what a team decides to do - whether to call `ConfigureAwait` or not, its very important to enforce (if possible) the consistency of the code. - -ErrorProne.NET allows a developer to "annotate" the assembly with an attribute and the anlyzers will enforce the desired behavior: - -```csharp -[assembly: UseConfigureAwaitFalse()] -public class UseConfigureAwaitFalseAttribute : System.Attribute { } - -public static async Task CopyTo(Stream source, Stream destination) -{ - // ConfigureAwait(false) must be used - await source.CopyToAsync(destination); - // ~~~~~~~~~~~~~~~~~~ -} -``` - -```csharp -[assembly: DoNotUseConfigureAwaitFalse()] -public class UseConfigureAwaitFalseAttribute : System.Attribute { } -// ConfigureAwait(false) call is redundant -await source.CopyToAsync(destination).ConfigureAwait(false); -// ~~~~~~~~~~~~~~~~~~~~~ -``` - -In both cases, the analyzers will look for a special attribute used for a given assembly and will emit diagnostics based on the required usage of `ConfigureAwait`. In the second case, the `ConfigureAwait(false)` will be grayed-out in the IDE and the IDE will suggest a fixer to remove the redundant call. - -Please note, that you don't have to reference any ErrorProne.NET assembly in order to use this feature. You can just declare the two attributes by yourself and the analyzer will use duck-typing approach to detect that the right attributes were used. - -## Struct Analyzers - -Value types are very important in high performance scenarios, but they have its own limitations and hidden aspects that can cause incorrect behavior or performance degradations. - -### Do not use default contructors for structs - -In high-performant code it is quite common to use structs as an optimization tool. And in some cases, the default constructor for structs can not establish the invariants required for the correct behavior. Such structs can be marked with a special attribute (`DoNotUseDefaultConstruction`) and any attempt to create the struct marked with this attribute using `new` or `default` will trigger a diagnostic: - - -```csharp -[System.AttributeUsage(System.AttributeTargets.Struct)] -public class DoNotUseDefaultConstructionAttribute : System.Attribute { } - -[DoNotUseDefaultConstruction] -public readonly struct TaskSourceSlim -{ - private readonly TaskCompletionSource _tcs; - public TaskSourceSlim(TaskCompletionSource tcs) => _tcs = tcs; - // Other members -} - -// Do not use default construction for struct 'TaskSourceSlim' marked with 'DoNotUseDefaultConstruction' attribute -var tss = new TaskSourceSlim(); -// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -// The same warning. -TaskSourceSlim tss2 = default; -// ~~~~~~~ - -// The same warning. -var tss3 = Create>(); -// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ - -var a = new TaskSourceSlim[5]; // OK - -// The same warning! ImmutableArray will throw NRE -var ia = new ImmutableArray(); -// ~~~~~~~~~~~~~~~~~~~~~ - -public static T Create() where T : new() => default; -``` - -And you can't embed a struct marked with this attribute into another struct, unless the other struct is marked with `DoNotUseDefaultConstruction` attribute as well: - -```csharp -public readonly struct S2 -{ - // Do not embed struct 'TaskSourceSlim' marked with 'DoNotUseDefaultConstruction' attribute into another struct - private readonly TaskSourceSlim _tss; - // ~~~~~~~~~~~~~~~~~~~~~~~~~~~ -} -``` - -### A hashtable unfriendly type is used as a key in a dictionary - -Every type that is used as the key in a dictionary must implement `Equals` and `GetHashCode`. By default the CLR provides the default implementations for `Equals` and `GetHashCode` that follows "value semantics", i.e. the two instances of a struct are equals when all the fields are equals. But unfortunately, the calling a default `Equal` or `GetHashCode` methods causes boxing allocation and may be implemented using reflection, that can be extremely slow. - -```csharp -public struct MyStruct -{ - private readonly long _x; - private readonly long _y; - public void FooBar() { } - // Struct 'MyStruct' with the default Equals and HashCode implementation - // is used as a key in a hash table. - private static Dictionary Table; - // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -} -``` - -The same issue may occur when `MyStruct` instance is embedded into another struct that provide custom implementations for `Equals` and `GetHashCode`. And there is an analyzer that warns in this case as well: - -```csharp - -``` - -### A struct can be made readonly -Marking a struct readonly can be beneficial in terms of design, because it allows conveying the intent more clearly, and also readonly structs can be more performant by avoiding defensive copies in readonly contexts (like when passed by `in`, when stored in readonly field, `ref readonly` variables etc): - -```csharp -// Struct 'MyStruct' can be made readonly -public struct MyStruct -// ~~~~~~~~ -{ - private readonly long _x; - private readonly long _y; - public void FooBar() { } -} -``` - -All the analyzers below will trigger a diagnostics only for "large" structs, i.e. with the structs larger than 16 bytes. This is done to avoid too many warnings when there is potential performance issues because the copy of a small struct will be very much negligible. - -### Non-readonly struct is passed as in-parameter -It doesn't make sense to pass non-readonly and non-poco structs as `in` parameter, because almost every access to the argument will cause a defensive copy that will eliminate all the benefits of "passing by reference": - -```csharp -// Non-readonly struct 'MyStruct' is passed as in-parameter 'ms' -public static void InParameter(in MyStruct ms) -// ~~~~~~~~~~~~~~ -{ -} - -// Non-readonly struct 'MyStruct' returned by readonly reference -public static ref readonly MyStruct Return() => throw null; -// ~~~~~~~~~~~~~~~~~~~~~ -``` - -### Defensive copy analyzers -C# 7.3 introduced features that help passing or returning struct by "readonly" reference. The features are very helpful for readonly structs, but for non-readonly members of non-readonly structs that can decrease performance by causing a lots of redundant copies. - -```csharp -public static void HiddenCopy(in MyStruct ms) -{ - // Some analyzers are triggered only for "large" structs to avoid extra noise - // Hidden copy copy - - // Expression 'FooBar' causes a hidden copy of a non-readonly struct 'MyStruct' - ms.FooBar(); - // ~~~~~~ - - ref readonly MyStruct local = ref ms; - - // Hidden copy as well - local.FooBar(); - // ~~~~~~ - - // Hidden copy as well - _staticStruct.FooBar(); - // ~~~~~~ -} - -private static readonly MyStruct _staticStruct; -``` - -# Contributions -Are highly appreciated. You may send a pull request with various fixes or you can suggest some interesting rule that can prevent from some nasty bugs in your app! - -# HowTos -Q: How to generate stable nuget packages that can be added to a local nuget feed? -A: msbuild /p:PublicRelease=true - -Q: How to add a newly generate nuget package into a local nuget feed? -A: nuget add $package -source c:\localPackages - -# Roadmap - -TBD +| Id | Description | +|---|---| +| [EPC14](docs/Rules/EPC14.md) | ConfigureAwait(false) call is redundant | +| [EPC15](docs/Rules/EPC15.md) | ConfigureAwait(false) must be used | +| [EPC16](docs/Rules/EPC16.md) | Awaiting a result of a null-conditional expression will cause NullReferenceException | +| [EPC17](docs/Rules/EPC17.md) | Avoid async-void delegates | +| [EPC18](docs/Rules/EPC18.md) | A task instance is implicitly converted to a string | +| [EPC20](docs/Rules/EPC20.md) | Avoid using default ToString implementation | +| [EPC26](docs/Rules/EPC26.md) | Do not use tasks in using block | +| [EPC27](docs/Rules/EPC27.md) | Avoid async void methods | +| [EPC31](docs/Rules/EPC31.md) | Do not return null for Task-like types | +| [EPC32](docs/Rules/EPC32.md) | TaskCompletionSource should use RunContinuationsAsynchronously | +| [EPC33](docs/Rules/EPC33.md) | Do not use Thread.Sleep in async methods | + +### Generic Bugs and Code Smells + +| Id | Description | +|---|---| +| [EPC19](docs/Rules/EPC19.md) | Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks | +| [EPC28](docs/Rules/EPC28.md) | Do not use ExcludeFromCodeCoverage on partial classes | +| [EPC29](docs/Rules/EPC29.md) | ExcludeFromCodeCoverageAttribute should provide a message | +| [EPC30](docs/Rules/EPC30.md) | Method calls itself recursively | +| [ERP041](docs/Rules/ERP041.md) | EventSource class should be sealed | +| [ERP042](docs/Rules/ERP042.md) | EventSource implementation is not correct | + +### Concurrency + +| Id | Description | +|---|---| +| [ERP031](docs/Rules/ERP031.md) | The API is not thread-safe | + +### Error Handling Issues + +| Id | Description | +|---|---| +| [EPC11](docs/Rules/EPC11.md) | Suspicious equality implementation | +| [EPC12](docs/Rules/EPC12.md) | Suspicious exception handling: only the 'Message' property is observed in the catch block | +| [EPC13](docs/Rules/EPC13.md) | Suspiciously unobserved result | +| [EPC34](docs/Rules/EPC34.md) | Method return value marked with MustUseResultAttribute must be used | +| [ERP021](docs/Rules/ERP021.md) | Incorrect exception propagation | +| [ERP022](docs/Rules/ERP022.md) | Unobserved exception in a generic exception handler | + +### Performance + +| Id | Description | +|---|---| +| [EPC23](docs/Rules/EPC23.md) | Avoid using Enumerable.Contains on HashSet | +| [EPC24](docs/Rules/EPC24.md) | A hash table "unfriendly" type is used as the key in a hash table | +| [EPC25](docs/Rules/EPC25.md) | Avoid using default Equals or HashCode implementation from structs | \ No newline at end of file diff --git a/docs/Rules/EPC11.md b/docs/Rules/EPC11.md new file mode 100644 index 0000000..88c217e --- /dev/null +++ b/docs/Rules/EPC11.md @@ -0,0 +1,57 @@ +# EPC11 - Suspicious equality implementation + +This analyzer detects suspicious implementations of `Equals` methods that may not be working as intended. + +## Description + +The analyzer warns when an `Equals` method implementation appears suspicious, typically when the method doesn't use any instance members or compares against other instances properly. + +## Code that triggers the analyzer + +```csharp +public class MyClass +{ + public override bool Equals(object obj) + { + // Suspicious: only using static members or not using 'this' instance + return SomeStaticProperty == 42; + } +} +``` + +```csharp +public class Person +{ + public string Name { get; set; } + + public override bool Equals(object obj) + { + // Suspicious: parameter 'obj' is never used + return this.Name == "test"; + } +} +``` + +## How to fix + +Implement `Equals` method properly by: + +1. Using the parameter that's passed in +2. Checking instance members against the other object's members +3. Following the standard equality pattern + +```csharp +public class Person +{ + public string Name { get; set; } + + public override bool Equals(object obj) + { + if (obj is Person other) + { + return this.Name == other.Name; + } + return false; + } +} +``` diff --git a/docs/Rules/EPC12.md b/docs/Rules/EPC12.md new file mode 100644 index 0000000..8c763bf --- /dev/null +++ b/docs/Rules/EPC12.md @@ -0,0 +1,41 @@ +# EPC12 - Suspicious exception handling: only the 'Message' property is observed in the catch block + +This analyzer detects catch blocks where only the `Message` property of an exception is used, which often indicates incomplete exception handling. + +## Description + +The analyzer warns when a catch block only accesses the `Message` property of an exception without observing the exception object itself or its other important properties like `InnerException`. This is suspicious because the `Message` property often contains generic information, while the actual useful data is in the exception object or its inner exceptions. + +## Code that triggers the analyzer + +```csharp +try +{ + // Some risky operation + SomeMethod(); +} +catch (Exception ex) +{ + // Only using ex.Message - this triggers the warning + Console.WriteLine(ex.Message); +} +``` + + +## How to fix + +Use the full exception object or access additional properties like `InnerException`: + +```csharp +try +{ + SomeMethod(); +} +catch (Exception ex) +{ + // Log the full exception object + Console.WriteLine(ex.ToString()); + // Or access the exception object directly + LogException(ex); +} +``` diff --git a/docs/Rules/EPC13.md b/docs/Rules/EPC13.md new file mode 100644 index 0000000..3b77e3b --- /dev/null +++ b/docs/Rules/EPC13.md @@ -0,0 +1,64 @@ +# EPC13 - Suspiciously unobserved result + +This analyzer detects when method return values that should typically be observed are being ignored. + +## Description + +The analyzer warns when the result of a method call is not being used, particularly for methods that return important values that should be observed. This helps catch cases where developers might have forgotten to use the return value of a method. + +## Code that triggers the analyzer + +```csharp +public class Example +{ + public void ProcessData() + { + // Return value is ignored - this triggers the warning + GetImportantValue(); + + // String methods return new strings but result is ignored + "hello".ToUpper(); + + // Collection methods that return new collections + list.Where(x => x > 0); + } + + public string GetImportantValue() + { + return "important data"; + } +} +``` + +## How to fix + +Observe (use) the return values: + +```csharp +public class Example +{ + public void ProcessData() + { + // Store the result in a variable + var importantValue = GetImportantValue(); + + // Use the result directly + var upperText = "hello".ToUpper(); + Console.WriteLine(upperText); + + // Chain operations or store result + var filteredList = list.Where(x => x > 0).ToList(); + } + + public string GetImportantValue() + { + return "important data"; + } +} +``` + +If you intentionally want to ignore the result, you can use discard: + +```csharp +_ = GetImportantValue(); // Explicitly ignore the result +``` diff --git a/docs/Rules/EPC14.md b/docs/Rules/EPC14.md new file mode 100644 index 0000000..3e70d68 --- /dev/null +++ b/docs/Rules/EPC14.md @@ -0,0 +1,36 @@ +# EPC14 - ConfigureAwait(false) call is redundant + +This analyzer detects redundant calls to `ConfigureAwait(false)` when the assembly is configured not to require them. + +## Description + +The analyzer provides information (not a warning) when `ConfigureAwait(false)` calls are redundant because the assembly has been configured to not capture the synchronization context automatically. This helps clean up unnecessary code. + +## Code that triggers the analyzer + +```csharp +public async Task ProcessAsync() +{ + // ConfigureAwait(false) is redundant when assembly is configured for it + await SomeAsyncMethod().ConfigureAwait(false); + + // This is also redundant + var result = await GetDataAsync().ConfigureAwait(false); +} +``` + +## How to fix + +Remove the redundant `ConfigureAwait(false)` calls: + +```csharp +public async Task ProcessAsync() +{ + // ConfigureAwait(false) behavior is already configured at assembly level + await SomeAsyncMethod(); + + var result = await GetDataAsync(); +} +``` + +Note: This rule only applies when your assembly is configured to not capture synchronization context by default. The configuration is typically done through project settings or attributes. diff --git a/docs/Rules/EPC15.md b/docs/Rules/EPC15.md new file mode 100644 index 0000000..ed1a7e2 --- /dev/null +++ b/docs/Rules/EPC15.md @@ -0,0 +1,46 @@ +# EPC15 - ConfigureAwait(false) must be used + +This analyzer detects missing `ConfigureAwait(false)` calls when the assembly is configured to require them. + +## Description + +The analyzer warns when `ConfigureAwait(false)` should be used but is missing. This typically applies to library code where you want to avoid deadlocks by not capturing the synchronization context. + +## Code that triggers the analyzer + +```csharp +public async Task ProcessAsync() +{ + // Missing ConfigureAwait(false) - this triggers the warning + await SomeAsyncMethod(); + + // Also missing ConfigureAwait(false) + var result = await GetDataAsync(); +} +``` + +## How to fix + +Add `ConfigureAwait(false)` to all await expressions: + +```csharp +public async Task ProcessAsync() +{ + // Add ConfigureAwait(false) to avoid capturing sync context + await SomeAsyncMethod().ConfigureAwait(false); + + var result = await GetDataAsync().ConfigureAwait(false); +} +``` + +## When to use ConfigureAwait(false) + +Use `ConfigureAwait(false)` in: +- Library code that doesn't need to return to the original synchronization context +- Background processing code +- Code that doesn't interact with UI elements + +Don't use `ConfigureAwait(false)` in: +- UI event handlers +- Controller actions that need to return to the original context +- Code that accesses UI elements after the await diff --git a/docs/Rules/EPC16.md b/docs/Rules/EPC16.md new file mode 100644 index 0000000..f947400 --- /dev/null +++ b/docs/Rules/EPC16.md @@ -0,0 +1,64 @@ +# EPC16 - Awaiting a result of a null-conditional expression will cause NullReferenceException + +This analyzer detects dangerous patterns where awaiting the result of a null-conditional operator can cause a `NullReferenceException`. + +## Description + +The analyzer warns when code awaits the result of a null-conditional expression. When the left-hand side of the null-conditional operator is null, the expression returns null, and awaiting null will throw a `NullReferenceException`. + +## Code that triggers the analyzer + +```csharp +public async Task ProcessAsync() +{ + SomeService service = GetService(); // might return null + + // Dangerous: if service is null, this returns null, and awaiting null throws NRE + await service?.ProcessAsync(); +} +``` + +```csharp +public async Task GetDataAsync() +{ + var client = GetHttpClient(); // might return null + + // This will throw NRE if client is null + return await client?.GetStringAsync("http://example.com"); +} +``` + +## How to fix + +Check for null before awaiting, or use proper null handling: + +```csharp +public async Task ProcessAsync() +{ + SomeService service = GetService(); + + // Option 1: Check for null first + if (service != null) + { + await service.ProcessAsync(); + } + + // Option 2: Use null-coalescing with Task.CompletedTask + await (service?.ProcessAsync() ?? Task.CompletedTask); +} +``` + +```csharp +public async Task GetDataAsync() +{ + var client = GetHttpClient(); + + // Check for null before using + if (client == null) + { + return null; // or throw, or return default value + } + + return await client.GetStringAsync("http://example.com"); +} +``` diff --git a/docs/Rules/EPC17.md b/docs/Rules/EPC17.md new file mode 100644 index 0000000..a028a58 --- /dev/null +++ b/docs/Rules/EPC17.md @@ -0,0 +1,61 @@ +# EPC17 - Avoid async-void delegates + +This analyzer detects the use of async void delegates, which can cause application crashes due to unhandled exceptions. + +## Description + +The analyzer warns against using async void delegates. Unlike async void methods (which should only be used for event handlers), async void delegates are particularly dangerous because exceptions thrown in them cannot be caught and will crash the application. + +## Code that triggers the analyzer + +```csharp +public void SetupHandlers() +{ + // Dangerous: async void delegate + SomeEvent += async () => + { + await SomeAsyncMethod(); + // If this throws, it will crash the app + }; + + // Another dangerous pattern + Task.Run(async () => + { + await ProcessAsync(); + // Exceptions here are unhandled + }); +} +``` + +```csharp +public void RegisterCallback() +{ + // Async void delegate in callback + RegisterCallback(async () => + { + await DoWorkAsync(); + }); +} +``` + +## How to fix + +Use regular void-return method and make a blocking call to async method. + +```csharp +public void SetupHandlers() +{ + // Safe: async Task delegate + SomeEvent += (sender, e) => + { + try + { + SomeAsyncMethod().GetAwaiter().GetResult(); + } + catch (Exception ex) + { + // Handle exception properly + LogError(ex); + } + };} +``` \ No newline at end of file diff --git a/docs/Rules/EPC18.md b/docs/Rules/EPC18.md new file mode 100644 index 0000000..309c19e --- /dev/null +++ b/docs/Rules/EPC18.md @@ -0,0 +1,51 @@ +# EPC18 - A task instance is implicitly converted to a string + +This analyzer detects when a `Task` instance is implicitly converted to a string, which often indicates a missing `await` keyword. + +## Description + +The analyzer warns when a `Task` object is being implicitly converted to a string. This usually happens when developers forget to await a task and try to use it directly in string operations. The resulting string will be the Task's type name rather than the actual result. + +## Code that triggers the analyzer + +```csharp +public async Task GetDataAsync() +{ + return "Hello World"; +} + +public void ProcessData() +{ + // Missing await - Task converted to string + string result = GetDataAsync(); // This will be "System.Threading.Tasks.Task`1[System.String]" + + // In string interpolation + Console.WriteLine($"Result: {GetDataAsync()}"); // Prints task type, not result + + // In concatenation + string message = "Data: " + GetDataAsync(); // Concatenates with task type +} +``` + +## How to fix + +Add the `await` keyword to get the actual result: + +```csharp +public async Task GetDataAsync() +{ + return "Hello World"; +} + +public async Task ProcessData() +{ + // Properly await the task + string result = await GetDataAsync(); // Now gets "Hello World" + + // In string interpolation + Console.WriteLine($"Result: {await GetDataAsync()}"); // Prints actual result + + // In concatenation + string message = "Data: " + await GetDataAsync(); // Concatenates with actual result +} +``` \ No newline at end of file diff --git a/docs/Rules/EPC19.md b/docs/Rules/EPC19.md new file mode 100644 index 0000000..f97f087 --- /dev/null +++ b/docs/Rules/EPC19.md @@ -0,0 +1,95 @@ +# EPC19 - Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks + +This analyzer detects when `CancellationTokenRegistration` objects are not being properly disposed, which can lead to memory leaks. + +## Description + +The analyzer warns when `CancellationToken.Register()` calls create `CancellationTokenRegistration` objects that are not being stored and disposed. If the cancellation token outlives the object that registered the callback, the registration can cause a memory leak. + +## Code that triggers the analyzer + +```csharp +public class MyService +{ + public void StartOperation(CancellationToken cancellationToken) + { + // Registration is not stored - potential memory leak + cancellationToken.Register(() => + { + Console.WriteLine("Operation cancelled"); + }); + } +} +``` + +```csharp +public async Task ProcessAsync(CancellationToken token) +{ + // Registration result is ignored + token.Register(OnCancellation); + + await DoWorkAsync(); +} + +private void OnCancellation() +{ + // Cleanup logic +} +``` + +## How to fix + +Store the registration and dispose it when no longer needed: + +```csharp +public class MyService : IDisposable +{ + private CancellationTokenRegistration _registration; + + public void StartOperation(CancellationToken cancellationToken) + { + // Store the registration + _registration = cancellationToken.Register(() => + { + Console.WriteLine("Operation cancelled"); + }); + } + + public void Dispose() + { + // Dispose the registration to prevent memory leaks + _registration.Dispose(); + } +} +``` + +```csharp +public async Task ProcessAsync(CancellationToken token) +{ + // Store registration and dispose in finally block + var registration = token.Register(OnCancellation); + try + { + await DoWorkAsync(); + } + finally + { + registration.Dispose(); + } +} + +// Or use using statement +public async Task ProcessAsync(CancellationToken token) +{ + using var registration = token.Register(OnCancellation); + await DoWorkAsync(); + // registration automatically disposed here +} +``` + +## When this matters + +This is especially important when: +- The cancellation token has a longer lifetime than the registering object +- You're registering callbacks in frequently called methods +- The registered callbacks capture references to large objects diff --git a/docs/Rules/EPC20.md b/docs/Rules/EPC20.md new file mode 100644 index 0000000..d0d0149 --- /dev/null +++ b/docs/Rules/EPC20.md @@ -0,0 +1,78 @@ +# EPC20 - Avoid using default ToString implementation + +This analyzer detects when the default `ToString()` implementation is being used, which rarely provides meaningful output. + +## Description + +The analyzer warns when the default `ToString()` implementation is used. The default implementation simply returns the type name, which is rarely what you want when converting an object to string. This often indicates that a custom `ToString()` implementation should be provided. + +## Code that triggers the analyzer + +```csharp +public class Person +{ + public string Name { get; set; } + public int Age { get; set; } + // No custom ToString() implementation +} + +public void Example() +{ + var person = new Person { Name = "John", Age = 30 }; + + // Using default ToString() - outputs "Person" instead of useful info + Console.WriteLine(person.ToString()); + + // Implicit ToString() call + string personString = person; + + // In string interpolation + Console.WriteLine($"Person: {person}"); +} +``` + +## How to fix + +Implement a meaningful `ToString()` method: + +```csharp +public class Person +{ + public string Name { get; set; } + public int Age { get; set; } + + // Custom ToString() implementation + public override string ToString() + { + return $"Person(Name: {Name}, Age: {Age})"; + } +} + +public void Example() +{ + var person = new Person { Name = "John", Age = 30 }; + + // Now outputs meaningful information: "Person(Name: John, Age: 30)" + Console.WriteLine(person.ToString()); + + string personString = person; // Also uses custom ToString() + + Console.WriteLine($"Person: {person}"); // Shows useful data +} +``` + +Alternative approaches: + +```csharp +// Use string interpolation directly instead of ToString() +Console.WriteLine($"Name: {person.Name}, Age: {person.Age}"); + +// Use a dedicated formatting method +public string ToDisplayString() +{ + return $"{Name} ({Age} years old)"; +} + +// Use record types which provide automatic ToString() +public record Person(string Name, int Age); +``` diff --git a/docs/Rules/EPC23.md b/docs/Rules/EPC23.md new file mode 100644 index 0000000..2d5a125 --- /dev/null +++ b/docs/Rules/EPC23.md @@ -0,0 +1,68 @@ +# EPC23 - Avoid using Enumerable.Contains on HashSet + +This analyzer detects inefficient use of `Enumerable.Contains` on `HashSet` collections where the instance `Contains` method would be more efficient. + +## Description + +The analyzer warns when `Enumerable.Contains` is used on HashSet or other set collections. This results in a linear O(n) search instead of the O(1) hash-based lookup that the HashSet's instance `Contains` method provides. + +## Code that triggers the analyzer + +```csharp +using System.Linq; + +public void Example() +{ + var hashSet = new HashSet { "apple", "banana", "cherry" }; + + // Inefficient: uses Enumerable.Contains (O(n) linear search) + bool found = hashSet.Contains("apple", StringComparer.OrdinalIgnoreCase); + + // Also inefficient when using Enumerable.Contains explicitly + bool found2 = Enumerable.Contains(hashSet, "banana"); +} +``` + +## How to fix + +Use the HashSet's instance `Contains` method: + +```csharp +public void Example() +{ + var hashSet = new HashSet { "apple", "banana", "cherry" }; + + // Efficient: uses HashSet.Contains (O(1) hash lookup) + bool found = hashSet.Contains("apple"); + + // If you need custom comparison, create HashSet with comparer + var caseInsensitiveSet = new HashSet(StringComparer.OrdinalIgnoreCase) + { + "apple", "banana", "cherry" + }; + bool found2 = caseInsensitiveSet.Contains("APPLE"); // O(1) with custom comparer +} +``` + +Or if you need the comparer for a specific search: + +```csharp +public void Example() +{ + var hashSet = new HashSet { "apple", "banana", "cherry" }; + + // Convert to appropriate collection for the comparer you need + var lookup = hashSet.ToLookup(x => x, StringComparer.OrdinalIgnoreCase); + bool found = lookup.Contains("APPLE"); + + // Or use a Dictionary if you need case-insensitive lookups frequently + var dict = hashSet.ToDictionary(x => x, x => true, StringComparer.OrdinalIgnoreCase); + bool found2 = dict.ContainsKey("APPLE"); +} +``` + +## Performance Impact + +- `Enumerable.Contains`: O(n) - scans through all elements +- `HashSet.Contains`: O(1) - direct hash-based lookup +- For large collections, this can be a significant performance difference diff --git a/docs/Rules/EPC24.md b/docs/Rules/EPC24.md new file mode 100644 index 0000000..be4e5fe --- /dev/null +++ b/docs/Rules/EPC24.md @@ -0,0 +1,81 @@ +# EPC24 - A hash table "unfriendly" type is used as the key in a hash table + +This analyzer detects when structs with default implementations of `Equals` and `GetHashCode` are used as keys in hash tables, which can cause severe performance issues. + +## Description + +The analyzer warns when a struct that relies on the default ValueType implementation of `Equals` or `GetHashCode` is used as a key in hash-based collections like `Dictionary`, `HashSet`, etc. The default implementations use reflection and are very slow. + +## Code that triggers the analyzer + +```csharp +public struct Point +{ + public int X { get; set; } + public int Y { get; set; } + // No custom Equals or GetHashCode implementation +} + +public void Example() +{ + // Using struct with default Equals/GetHashCode as dictionary key + var pointData = new Dictionary(); + + var point = new Point { X = 1, Y = 2 }; + pointData[point] = "First point"; // Performance issue! + + // HashSet also affected + var pointSet = new HashSet(); + pointSet.Add(point); // Performance issue! +} +``` + +## How to fix + +Implement custom `Equals` and `GetHashCode` methods: + +```csharp +public struct Point : IEquatable +{ + public int X { get; set; } + public int Y { get; set; } + + // Custom Equals implementation + public bool Equals(Point other) + { + return X == other.X && Y == other.Y; + } + + public override bool Equals(object obj) + { + return obj is Point other && Equals(other); + } + + // Custom GetHashCode implementation + public override int GetHashCode() + { + return HashCode.Combine(X, Y); + } +} +``` + +Or use a record struct (C# 10+): + +```csharp +// Record structs automatically implement Equals and GetHashCode +public record struct Point(int X, int Y); + +public void Example() +{ + var pointData = new Dictionary(); + var point = new Point(1, 2); + pointData[point] = "First point"; // Now efficient! +} +``` + +## Performance Impact + +The default ValueType implementations: +- Use reflection to compare all fields +- Are significantly slower than custom implementations +- Can cause major performance bottlenecks in hash-based collections diff --git a/docs/Rules/EPC25.md b/docs/Rules/EPC25.md new file mode 100644 index 0000000..7a6bce6 --- /dev/null +++ b/docs/Rules/EPC25.md @@ -0,0 +1,96 @@ +# EPC25 - Avoid using default Equals or HashCode implementation from structs + +This analyzer detects when the default `ValueType.Equals` or `ValueType.GetHashCode` implementations are being used directly, which can cause performance issues. + +## Description + +The analyzer warns when code directly calls the default `Equals` or `GetHashCode` implementations from `ValueType`. These implementations use reflection and are significantly slower than custom implementations. + +## Code that triggers the analyzer + +```csharp +public struct MyStruct +{ + public int Value1 { get; set; } + public int Value2 { get; set; } + // No custom Equals or GetHashCode +} + +public void Example() +{ + var struct1 = new MyStruct { Value1 = 1, Value2 = 2 }; + var struct2 = new MyStruct { Value1 = 1, Value2 = 2 }; + + // Using default ValueType.Equals - slow! + bool areEqual = struct1.Equals(struct2); + + // Using default ValueType.GetHashCode - slow! + int hash = struct1.GetHashCode(); + + // In collections this causes performance issues + var dictionary = new Dictionary(); + dictionary[struct1] = "value"; // Triggers slow hash operations +} +``` + +## How to fix + +Implement custom `Equals` and `GetHashCode`: + +```csharp +public struct MyStruct : IEquatable +{ + public int Value1 { get; set; } + public int Value2 { get; set; } + + public bool Equals(MyStruct other) + { + return Value1 == other.Value1 && Value2 == other.Value2; + } + + public override bool Equals(object obj) + { + return obj is MyStruct other && Equals(other); + } + + public override int GetHashCode() + { + return HashCode.Combine(Value1, Value2); + } + + // Optional: implement operators for consistency + public static bool operator ==(MyStruct left, MyStruct right) + { + return left.Equals(right); + } + + public static bool operator !=(MyStruct left, MyStruct right) + { + return !left.Equals(right); + } +} +``` + +Or use record struct (C# 10+): + +```csharp +// Record structs automatically provide efficient implementations +public record struct MyStruct(int Value1, int Value2); + +public void Example() +{ + var struct1 = new MyStruct(1, 2); + var struct2 = new MyStruct(1, 2); + + // Now uses efficient generated implementations + bool areEqual = struct1.Equals(struct2); + int hash = struct1.GetHashCode(); +} +``` + +## Performance considerations + +- Default `ValueType.Equals`: Uses reflection to compare all fields +- Default `ValueType.GetHashCode`: Uses reflection and can cause hash collisions +- Custom implementations: Direct field access, much faster +- Record structs: Compiler-generated efficient implementations diff --git a/docs/Rules/EPC26.md b/docs/Rules/EPC26.md new file mode 100644 index 0000000..b8847fb --- /dev/null +++ b/docs/Rules/EPC26.md @@ -0,0 +1,80 @@ +# EPC26 - Do not use tasks in using block + +This analyzer detects when `Task` objects are used in `using` statements, which is problematic because tasks should not be explicitly disposed. + +## Description + +The analyzer warns when Task objects are used in `using` statements or `using` declarations. While Task implements `IDisposable`, calling `Dispose()` on a Task is almost never the right thing to do and can cause issues. + +## Code that triggers the analyzer + +```csharp +public async Task Example() +{ + // Bad: using statement with Task + using (var task = SomeAsyncMethod()) + { + await task; + } // Task.Dispose() called here - problematic! + + // Also bad: using declaration + using var task2 = GetTaskAsync(); + await task2; // task2.Dispose() called at end of scope +} + +public async Task AnotherExample() +{ + // Bad: Task in using block + using var delayTask = Task.Delay(1000); + await delayTask; +} +``` + +## How to fix + +Simply remove the `using` statement and use the task directly: + +```csharp +public async Task Example() +{ + // Good: just await the task directly + await SomeAsyncMethod(); + + // Or store in variable if needed + var task = GetTaskAsync(); + await task; + // No explicit disposal needed +} + +public async Task AnotherExample() +{ + // Good: use Task.Delay directly + await Task.Delay(1000); +} +``` + +If you need to handle task completion or cancellation: + +```csharp +public async Task ExampleWithCancellation() +{ + using var cts = new CancellationTokenSource(); + + // Good: dispose the CancellationTokenSource, not the Task + var task = SomeAsyncMethod(cts.Token); + + try + { + await task; + } + catch (OperationCanceledException) + { + // Handle cancellation + } + // CancellationTokenSource gets disposed, not the Task +} +``` + +## Why tasks shouldn't be disposed + +- Task objects don't own managed resources and should not be explicitly disposed diff --git a/docs/Rules/EPC27.md b/docs/Rules/EPC27.md new file mode 100644 index 0000000..d8e33db --- /dev/null +++ b/docs/Rules/EPC27.md @@ -0,0 +1,102 @@ +# EPC27 - Avoid async void methods + +This analyzer detects `async void` methods, which should be avoided except for event handlers. + +## Description + +The analyzer warns when methods are declared as `async void`. These methods are dangerous because exceptions thrown in them cannot be caught by the caller and will crash the application. They should only be used for event handlers. + +## Code that triggers the analyzer + +```csharp +public class Example +{ + // Bad: async void method + public async void ProcessData() + { + await SomeAsyncOperation(); + // If this throws, it will crash the app + } + + // Also bad: async void in other contexts + private async void Helper() + { + await DoWorkAsync(); + } +} +``` + +## How to fix + +Use `async Task` instead: + +```csharp +public class Example +{ + // Good: async Task method + public async Task ProcessDataAsync() + { + await SomeAsyncOperation(); + // Exceptions can be caught by caller + } + + // Good: async Task for helper methods + private async Task HelperAsync() + { + await DoWorkAsync(); + } + + // Usage example + public async Task CallerAsync() + { + try + { + await ProcessDataAsync(); + } + catch (Exception ex) + { + // Can catch exceptions properly + LogError(ex); + } + } +} +``` + +The only acceptable use of `async void` is for event handlers: + +```csharp +public partial class MainWindow : Window +{ + public MainWindow() + { + InitializeComponent(); + } + + // Acceptable: event handler must be async void + private async void Button_Click(object sender, RoutedEventArgs e) + { + try + { + await ProcessClickAsync(); + } + catch (Exception ex) + { + // Always wrap event handlers in try-catch + MessageBox.Show($"Error: {ex.Message}"); + } + } + + private async Task ProcessClickAsync() + { + // Actual work in async Task method + await SomeAsyncOperation(); + } +} +``` + +## Best practices + +1. Use `async Task` for all async methods except event handlers +2. Always wrap async void event handlers in try-catch blocks +3. Prefer extracting logic from event handlers into async Task methods +4. Use `async Task` when the method needs to return a value diff --git a/docs/Rules/EPC28.md b/docs/Rules/EPC28.md new file mode 100644 index 0000000..293644c --- /dev/null +++ b/docs/Rules/EPC28.md @@ -0,0 +1,124 @@ +# EPC28 - Do not use ExcludeFromCodeCoverage on partial classes + +This analyzer detects when `ExcludeFromCodeCoverageAttribute` is applied to partial classes, which can lead to inconsistent code coverage results. + +## Description + +The analyzer warns when `ExcludeFromCodeCoverageAttribute` is applied to partial classes. This can cause inconsistent behavior because code coverage tools might not handle partial classes with this attribute correctly, leading to unreliable coverage reports. + +## Code that triggers the analyzer + +```csharp +using System.Diagnostics.CodeAnalysis; + +// Bad: ExcludeFromCodeCoverage on partial class +[ExcludeFromCodeCoverage] +public partial class MyPartialClass +{ + public void Method1() + { + // This might or might not be excluded from coverage + } +} + +// Another part of the same class +public partial class MyPartialClass +{ + public void Method2() + { + // Coverage behavior is inconsistent + } +} +``` + +```csharp +// Also problematic: one part has the attribute +[ExcludeFromCodeCoverage] +public partial class DataClass +{ + // Part 1 +} + +public partial class DataClass +{ + // Part 2 - inconsistent coverage behavior +} +``` + +## How to fix + +Apply the attribute to specific members instead of the partial class: + +```csharp +public partial class MyPartialClass +{ + [ExcludeFromCodeCoverage] + public void Method1() + { + // Explicitly excluded from coverage + } + + public void Method2() + { + // Included in coverage + } +} + +public partial class MyPartialClass +{ + [ExcludeFromCodeCoverage] + public void Method3() + { + // Explicitly excluded from coverage + } +} +``` + +Or if the entire class should be excluded, use a non-partial class: + +```csharp +// If you need to exclude the entire class, make it non-partial +[ExcludeFromCodeCoverage] +public class MyClass +{ + public void Method1() + { + // Consistently excluded from coverage + } + + public void Method2() + { + // Also excluded + } +} +``` + +Or extract the parts that need to be excluded: + +```csharp +// Keep main class without attribute +public partial class MyPartialClass +{ + public void ImportantMethod() + { + // Included in coverage + } +} + +// Separate class for excluded code +[ExcludeFromCodeCoverage] +public class MyGeneratedClass +{ + public void GeneratedMethod() + { + // Excluded from coverage + } +} +``` + +## Why this matters + +- Code coverage tools may inconsistently handle partial classes with this attribute +- Some tools might exclude all parts, others might exclude none +- Can lead to unreliable coverage metrics +- Makes it harder to track actual test coverage diff --git a/docs/Rules/EPC29.md b/docs/Rules/EPC29.md new file mode 100644 index 0000000..8109ace --- /dev/null +++ b/docs/Rules/EPC29.md @@ -0,0 +1,93 @@ +# EPC29 - ExcludeFromCodeCoverageAttribute should provide a message + +This analyzer detects when `ExcludeFromCodeCoverageAttribute` is used without providing a justification message explaining why the code is excluded from coverage. + +## Description + +The analyzer warns when `ExcludeFromCodeCoverageAttribute` is applied without providing a `Justification` message. Including a justification helps other developers understand why certain code is excluded from coverage analysis. + +## Code that triggers the analyzer + +```csharp +using System.Diagnostics.CodeAnalysis; + +public class Example +{ + // Bad: No justification provided + [ExcludeFromCodeCoverage] + public void SomeMethod() + { + // Why is this excluded? Unclear to other developers + } + + // Also bad: Empty attribute + [ExcludeFromCodeCoverage()] + public class HelperClass + { + // No explanation for exclusion + } +} +``` + +## How to fix + +Provide a clear justification message: + +```csharp +using System.Diagnostics.CodeAnalysis; + +public class Example +{ + // Good: Clear justification provided + [ExcludeFromCodeCoverage(Justification = "This method is only used for debugging and doesn't need test coverage")] + public void DebugHelper() + { + // Debug-only code + } + + [ExcludeFromCodeCoverage(Justification = "Generated code that doesn't require testing")] + public class AutoGeneratedClass + { + // Auto-generated methods + } + + [ExcludeFromCodeCoverage(Justification = "Platform-specific code covered by integration tests")] + public void PlatformSpecificMethod() + { + // Platform-specific implementation + } + + [ExcludeFromCodeCoverage(Justification = "Simple property with no logic to test")] + public string SimpleProperty { get; set; } +} +``` + +## Common justifications + +```csharp +// Generated code +[ExcludeFromCodeCoverage(Justification = "Auto-generated by code generator")] + +// Defensive programming +[ExcludeFromCodeCoverage(Justification = "Defensive null check that should never be hit")] + +// Platform-specific code +[ExcludeFromCodeCoverage(Justification = "Platform-specific code tested through integration tests")] + +// Debug/development code +[ExcludeFromCodeCoverage(Justification = "Development helper method not used in production")] + +// Exception handling +[ExcludeFromCodeCoverage(Justification = "Exception case difficult to reproduce in unit tests")] + +// Legacy code +[ExcludeFromCodeCoverage(Justification = "Legacy code scheduled for removal in next version")] +``` + +## Benefits of providing justification + +- Documents the reason for exclusion +- Helps during code reviews +- Makes it easier to evaluate whether exclusion is still appropriate +- Improves code maintainability +- Helps new team members understand the codebase diff --git a/docs/Rules/EPC30.md b/docs/Rules/EPC30.md new file mode 100644 index 0000000..4732b43 --- /dev/null +++ b/docs/Rules/EPC30.md @@ -0,0 +1,109 @@ +# EPC30 - Method calls itself recursively + +This analyzer detects when a method calls itself recursively, which can lead to stack overflow exceptions if not properly controlled. + +## Description + +The analyzer warns when a method calls itself recursively. While recursion is sometimes necessary and appropriate, accidental recursion can cause stack overflow errors and infinite loops. + +## Code that triggers the analyzer + +```csharp +public class Example +{ + // Suspicious: might be accidental recursion + public void ProcessData() + { + // Some processing... + ProcessData(); // Calls itself without obvious base case + } + +``` + +## How to fix + +Add proper base cases and ensure recursion terminates: + +```csharp +public class Example +{ + // Good: proper recursive method with base case + public int Factorial(int n) + { + if (n <= 1) // Base case + return 1; + return n * Factorial(n - 1); // Recursive case with progress toward base case + } + + // Good: recursive tree traversal with termination condition + public void TraverseTree(TreeNode node) + { + if (node == null) // Base case + return; + + ProcessNode(node); + + // Recursive calls on smaller problems + TraverseTree(node.Left); + TraverseTree(node.Right); + } + + // Fix the property + private int _value; + public int Value + { + get { return _value; } // Return the backing field, not the property + set { _value = value; } + } +} +``` + +Convert to iterative approach when appropriate: + +```csharp +public class Example +{ + // Convert recursive to iterative to avoid stack overflow for large inputs + public int FactorialIterative(int n) + { + int result = 1; + for (int i = 2; i <= n; i++) + { + result *= i; + } + return result; + } + + // Iterative tree traversal using stack + public void TraverseTreeIterative(TreeNode root) + { + if (root == null) return; + + var stack = new Stack(); + stack.Push(root); + + while (stack.Count > 0) + { + var node = stack.Pop(); + ProcessNode(node); + + if (node.Right != null) stack.Push(node.Right); + if (node.Left != null) stack.Push(node.Left); + } + } +} +``` + +## When recursion is appropriate + +- Tree or graph traversal +- Mathematical functions (factorial, Fibonacci) +- Divide-and-conquer algorithms +- When the recursive solution is clearer than iterative + +## When to avoid recursion + +- Large datasets that might cause stack overflow +- When iterative solution is simpler +- Performance-critical code (recursion has overhead) +- When stack depth is unpredictable diff --git a/docs/Rules/EPC31.md b/docs/Rules/EPC31.md new file mode 100644 index 0000000..6636f00 --- /dev/null +++ b/docs/Rules/EPC31.md @@ -0,0 +1,116 @@ +# EPC31 - Do not return null for Task-like types + +This analyzer detects when methods return null for Task-like types, which can cause `NullReferenceException` when the task is awaited. + +## Description + +The analyzer warns when methods with Task-like return types (Task, Task, ValueTask, etc.) return null. Awaiting a null task will throw a `NullReferenceException`, which is unexpected behavior for async methods. + +## Code that triggers the analyzer + +```csharp +public class Example +{ + // Bad: returning null Task + public Task ProcessAsync() + { + if (someCondition) + return null; // This will cause NRE when awaited + + return DoWorkAsync(); + } + + // Bad: returning null Task + public Task GetDataAsync() + { + if (noData) + return null; // NRE when awaited + + return FetchDataAsync(); + } + + // Bad: conditional return with null + public Task? MaybeProcessAsync() + { + return condition ? ProcessDataAsync() : null; + } +} +``` + +## How to fix + +Return appropriate completed tasks instead of null: + +```csharp +public class Example +{ + // Good: return completed task instead of null + public Task ProcessAsync() + { + if (someCondition) + return Task.CompletedTask; // Safe to await + + return DoWorkAsync(); + } + + // Good: return completed task with result + public Task GetDataAsync() + { + if (noData) + return Task.FromResult(null); // Or return default value + + return FetchDataAsync(); + } + + // Good: return appropriate default values + public Task GetCountAsync() + { + if (isEmpty) + return Task.FromResult(0); // Return default value + + return CalculateCountAsync(); + } +} +``` + +For methods that might legitimately not have work to do: + +```csharp +public class Example +{ + // Use Task.CompletedTask for void-like async methods + public Task ProcessIfNeededAsync() + { + if (!needsProcessing) + return Task.CompletedTask; + + return ActualProcessingAsync(); + } + + // Use Task.FromResult for methods with return values + public Task GetCachedOrFetchAsync(string key) + { + var cached = cache.Get(key); + if (cached != null) + return Task.FromResult(cached); + + return FetchFromServerAsync(key); + } + + // For ValueTask, use default value + public ValueTask GetValueAsync() + { + if (hasValue) + return new ValueTask(value); + + return new ValueTask(FetchValueAsync()); + } +} +``` + +## Why null tasks are problematic + +- Awaiting null throws `NullReferenceException` +- Breaks the async/await pattern expectations +- Makes error handling inconsistent +- Can cause unexpected application crashes diff --git a/docs/Rules/EPC32.md b/docs/Rules/EPC32.md new file mode 100644 index 0000000..7acaa66 --- /dev/null +++ b/docs/Rules/EPC32.md @@ -0,0 +1,124 @@ +# EPC32 - TaskCompletionSource should use RunContinuationsAsynchronously + +This analyzer detects when `TaskCompletionSource` is created without the `TaskCreationOptions.RunContinuationsAsynchronously` flag, which can lead to deadlocks. + +## Description + +The analyzer warns when `TaskCompletionSource` instances are created without using `TaskCreationOptions.RunContinuationsAsynchronously`. Without this flag, continuations run synchronously on the thread that completes the task, which can cause deadlocks and unexpected blocking behavior. + +## Code that triggers the analyzer + +```csharp +public class Example +{ + // Bad: TaskCompletionSource without RunContinuationsAsynchronously + private TaskCompletionSource tcs = new TaskCompletionSource(); + + public Task GetResultAsync() + { + return tcs.Task; + } + + public void CompleteTask(string result) + { + // Continuations will run synchronously on this thread + tcs.SetResult(result); // Potential deadlock risk + } +} +``` + +```csharp +public class Worker +{ + // Also problematic: creating without the flag + public Task ProcessAsync() + { + var completionSource = new TaskCompletionSource(); + + ThreadPool.QueueUserWorkItem(_ => + { + var result = DoWork(); + completionSource.SetResult(result); // Continuations block this thread + }); + + return completionSource.Task; + } +} +``` + +## How to fix + +Always use `TaskCreationOptions.RunContinuationsAsynchronously`: + +```csharp +public class Example +{ + // Good: TaskCompletionSource with RunContinuationsAsynchronously + private TaskCompletionSource tcs = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + public Task GetResultAsync() + { + return tcs.Task; + } + + public void CompleteTask(string result) + { + // Continuations will run asynchronously + tcs.SetResult(result); // No deadlock risk + } +} +``` + +```csharp +public class Worker +{ + // Good: creating with the flag + public Task ProcessAsync() + { + var completionSource = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + ThreadPool.QueueUserWorkItem(_ => + { + var result = DoWork(); + completionSource.SetResult(result); // Safe + }); + + return completionSource.Task; + } +} +``` + +For generic TaskCompletionSource: + +```csharp +public class GenericExample +{ + // Good: with RunContinuationsAsynchronously + private TaskCompletionSource CreateCompletionSource() + { + return new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + } + + // Alternative: create a helper method + private static TaskCompletionSource CreateTcs() + { + return new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + } +} +``` + +## Why this flag is important + +Without `RunContinuationsAsynchronously`: +- Continuations run synchronously on the completing thread +- Can block the completing thread unexpectedly +- May cause deadlocks in certain scenarios +- Can lead to poor performance in high-concurrency situations + +With `RunContinuationsAsynchronously`: +- Continuations are queued to run asynchronously +- Completing thread is not blocked +- Reduces deadlock risk +- Better scalability and performance diff --git a/docs/Rules/EPC33.md b/docs/Rules/EPC33.md new file mode 100644 index 0000000..edb8dc3 --- /dev/null +++ b/docs/Rules/EPC33.md @@ -0,0 +1,106 @@ +# EPC33 - Do not use Thread.Sleep in async methods + +This analyzer detects the use of `Thread.Sleep` in async methods, which blocks the thread and defeats the purpose of async programming. + +## Description + +The analyzer warns when `Thread.Sleep` is used in async methods. This blocks the current thread, which is contrary to the non-blocking nature of async/await patterns. In async methods, you should use `Task.Delay` with await instead. + +## Code that triggers the analyzer + +```csharp +public async Task ProcessAsync() +{ + // Bad: Thread.Sleep blocks the thread + Thread.Sleep(1000); // Blocks for 1 second + + await DoWorkAsync(); + + // Also bad: Thread.Sleep with TimeSpan + Thread.Sleep(TimeSpan.FromSeconds(2)); +} + +public async void HandleEventAsync() +{ + // Bad: blocking in async event handler + Thread.Sleep(500); + await UpdateUIAsync(); +} + +public async Task GetDataAsync() +{ + // Bad: mixed blocking and async code + Thread.Sleep(100); // Simulate delay - wrong way + return await FetchDataAsync(); +} +``` + +## How to fix + +Use `Task.Delay` with await instead: + +```csharp +public async Task ProcessAsync() +{ + // Good: Task.Delay doesn't block the thread + await Task.Delay(1000); // Non-blocking delay + + await DoWorkAsync(); + + // Good: Task.Delay with TimeSpan + await Task.Delay(TimeSpan.FromSeconds(2)); +} + +public async void HandleEventAsync() +{ + // Good: non-blocking delay in async event handler + await Task.Delay(500); + await UpdateUIAsync(); +} + +public async Task GetDataAsync() +{ + // Good: all async operations + await Task.Delay(100); // Non-blocking simulation + return await FetchDataAsync(); +} +``` + +For cancellation support: + +```csharp +public async Task ProcessAsync(CancellationToken cancellationToken = default) +{ + // Good: Task.Delay with cancellation support + await Task.Delay(1000, cancellationToken); + + await DoWorkAsync(cancellationToken); +} + +public async Task LongRunningProcessAsync(CancellationToken cancellationToken) +{ + for (int i = 0; i < 100; i++) + { + await ProcessItemAsync(i); + + // Good: cancellable delay between iterations + await Task.Delay(100, cancellationToken); + } +} +``` + +## Why Thread.Sleep is problematic in async methods + +- Blocks the current thread, preventing it from handling other work +- Defeats the scalability benefits of async/await +- Can cause thread pool starvation +- Doesn't respect cancellation tokens +- May cause deadlocks in certain contexts (like ASP.NET) + +## Benefits of Task.Delay + +- Non-blocking - thread can handle other work +- Supports cancellation via CancellationToken +- Integrates properly with async/await patterns +- Better resource utilization +- Proper behavior in async contexts diff --git a/docs/Rules/EPC34.md b/docs/Rules/EPC34.md new file mode 100644 index 0000000..714fc3f --- /dev/null +++ b/docs/Rules/EPC34.md @@ -0,0 +1,169 @@ +# EPC34 - Method return value marked with MustUseResultAttribute must be used + +This analyzer detects when methods marked with `MustUseResultAttribute` have their return values ignored, which indicates the result should always be observed. + +## Description + +The analyzer warns when the return value of a method decorated with `MustUseResultAttribute` is not used. This attribute indicates that the method's return value contains important information that should always be observed by the caller. + +## Code that triggers the analyzer + +```csharp +[System.AttributeUsage(System.AttributeTargets.Method)] +public class MustUseResultAttribute : System.Attribute { } + +public class ValidationService +{ + [MustUseResult] + public bool ValidateInput(string input) + { + return !string.IsNullOrEmpty(input) && input.Length > 3; + } + + [MustUseResult] + public ValidationResult CheckData(object data) + { + return new ValidationResult(data != null, "Data cannot be null"); + } +} + +public class Example +{ + public void ProcessData(string input, object data) + { + var validator = new ValidationService(); + + // Bad: ignoring return value of method marked with MustUseResult + validator.ValidateInput(input); + + // Also bad: not using the validation result + validator.CheckData(data); + + // Proceeding without checking validation results - dangerous! + DoSomethingWithData(input, data); + } +} +``` + +## How to fix + +Always observe and use the return values: + +```csharp +public class Example +{ + public void ProcessData(string input, object data) + { + var validator = new ValidationService(); + + // Good: checking the validation result + bool isValid = validator.ValidateInput(input); + if (!isValid) + { + throw new ArgumentException("Invalid input"); + } + + // Good: using the validation result + var result = validator.CheckData(data); + if (!result.IsValid) + { + throw new ArgumentException(result.ErrorMessage); + } + + // Now safe to proceed + DoSomethingWithData(input, data); + } +} +``` + +In conditional checks: + +```csharp +public class Example +{ + public void ProcessDataConditionally(string input) + { + var validator = new ValidationService(); + + // Good: using result in conditional + if (validator.ValidateInput(input)) + { + ProcessValidInput(input); + } + else + { + HandleInvalidInput(input); + } + } + + public void ProcessWithEarlyReturn(object data) + { + var validator = new ValidationService(); + + // Good: using result for early return + var result = validator.CheckData(data); + if (!result.IsValid) + return; // Early exit on validation failure + + ProcessValidData(data); + } +} +``` + +If you intentionally want to ignore the result (rare cases): + +```csharp +public class Example +{ + public void ProcessData(string input) + { + var validator = new ValidationService(); + + // Explicitly ignore result with discard (use with caution) + _ = validator.ValidateInput(input); + + // Better: add comment explaining why result is ignored + _ = validator.ValidateInput(input); // Validation is optional in this context + } +} +``` + +For async methods with MustUseResult: + +```csharp +public class AsyncValidator +{ + [MustUseResult] + public async Task ValidateAsync(string input) + { + await Task.Delay(100); // Simulate async validation + return !string.IsNullOrEmpty(input); + } +} + +public class Example +{ + public async Task ProcessAsync(string input) + { + var validator = new AsyncValidator(); + + // Good: awaiting and using the result + bool isValid = await validator.ValidateAsync(input); + if (!isValid) + { + throw new ArgumentException("Validation failed"); + } + + await ProcessValidInputAsync(input); + } +} +``` + +## When to use MustUseResultAttribute + +Apply this attribute to methods where: +- Return value indicates success/failure +- Return value contains error information +- Ignoring the result could lead to bugs +- The method's primary purpose is to return information +- Return value affects subsequent program behavior diff --git a/docs/Rules/ERP021.md b/docs/Rules/ERP021.md new file mode 100644 index 0000000..1e802d2 --- /dev/null +++ b/docs/Rules/ERP021.md @@ -0,0 +1,166 @@ +# ERP021 - Incorrect exception propagation + +This analyzer detects incorrect exception propagation patterns where the original stack trace is lost. + +## Description + +The analyzer warns when exceptions are re-thrown incorrectly using `throw ex;` instead of `throw;`. Using `throw ex;` resets the stack trace, losing valuable debugging information about where the exception originally occurred. + +## Code that triggers the analyzer + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (Exception ex) + { + LogError(ex); + + // Bad: throws the exception object, losing original stack trace + throw ex; + } +} + +public async Task ProcessAsync() +{ + try + { + await RiskyOperationAsync(); + } + catch (InvalidOperationException ex) + { + // Bad: re-throwing with lost stack trace + throw ex; + } +} +``` + +## How to fix + +Use `throw;` without the exception variable to preserve the stack trace: + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (Exception ex) + { + LogError(ex); + + // Good: preserves original stack trace + throw; + } +} + +public async Task ProcessAsync() +{ + try + { + await RiskyOperationAsync(); + } + catch (InvalidOperationException ex) + { + LogError(ex); + + // Good: original stack trace preserved + throw; + } +} +``` + +When you need to wrap the exception: + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (Exception ex) + { + // Good: wrap exception while preserving original as inner exception + throw new ProcessingException("Failed to process data", ex); + } +} + +public void ProcessWithContext() +{ + try + { + RiskyOperation(); + } + catch (Exception ex) + { + // Good: add context while preserving original exception + throw new InvalidOperationException($"Processing failed for item {itemId}", ex); + } +} +``` + +When you need to modify the exception: + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (ArgumentException ex) + { + // If you must throw the same exception type with modifications + var newEx = new ArgumentException($"Processing context: {ex.Message}", ex.ParamName, ex); + throw newEx; + } +} +``` + +Pattern for conditional re-throwing: + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (Exception ex) + { + if (ShouldHandleException(ex)) + { + HandleException(ex); + return; + } + + // Re-throw without losing stack trace + throw; + } +} +``` + +## Why stack trace preservation matters + +- **Debugging**: Original stack trace shows where the error actually occurred +- **Troubleshooting**: Easier to identify root cause of issues +- **Logging**: Better error logs with complete call stack information +- **Development**: Faster bug fixing with accurate error locations + +## Exception patterns to follow + +```csharp +// ✓ Good patterns +throw; // Preserve stack trace +throw new CustomException("msg", ex); // Wrap with inner exception +throw new CustomException("msg") { Data = ex.Data }; // Transfer data + +// ✗ Bad patterns +throw ex; // Loses stack trace +throw new Exception(ex.Message); // Loses original exception +``` diff --git a/docs/Rules/ERP022.md b/docs/Rules/ERP022.md new file mode 100644 index 0000000..11a1b18 --- /dev/null +++ b/docs/Rules/ERP022.md @@ -0,0 +1,214 @@ +# ERP022 - Unobserved exception in a generic exception handler + +This analyzer detects generic exception handlers that swallow exceptions without properly observing or handling them. + +## Description + +The analyzer warns when catch blocks handle exceptions in a generic way (like `catch (Exception)` or bare `catch`) but don't properly observe the exception or provide adequate handling. This can hide important errors and make debugging difficult. + +## Code that triggers the analyzer + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (Exception) + { + // Bad: exception is completely swallowed + return; + } +} + +public void ProcessItems() +{ + try + { + foreach (var item in items) + { + ProcessItem(item); + } + } + catch + { + // Bad: bare catch that swallows everything + // No way to know what went wrong + } +} + +public bool TryProcess() +{ + try + { + DoComplexOperation(); + return true; + } + catch (Exception) + { + // Bad: swallowing exception and returning false + // Caller has no idea what failed + return false; + } +} +``` + +## How to fix + +Properly observe and handle exceptions: + +```csharp +public void ProcessData() +{ + try + { + RiskyOperation(); + } + catch (Exception ex) + { + // Good: log the exception before handling + _logger.LogError(ex, "Failed to process data"); + + // Decide whether to re-throw, return, or handle differently + throw; // or handle appropriately + } +} + +public void ProcessItems() +{ + try + { + foreach (var item in items) + { + ProcessItem(item); + } + } + catch (Exception ex) + { + // Good: observe the exception and provide context + _logger.LogError(ex, "Failed to process items"); + + // Handle gracefully or re-throw as appropriate + throw new ProcessingException("Batch processing failed", ex); + } +} + +public bool TryProcess() +{ + try + { + DoComplexOperation(); + return true; + } + catch (Exception ex) + { + // Good: log the exception even in Try methods + _logger.LogWarning(ex, "Process operation failed"); + return false; + } +} +``` + +Better patterns for different scenarios: + +```csharp +// Pattern 1: Try methods with proper error handling +public bool TryProcessWithDiagnostics(out string error) +{ + error = null; + try + { + DoComplexOperation(); + return true; + } + catch (Exception ex) + { + error = ex.Message; + _logger.LogWarning(ex, "Process operation failed"); + return false; + } +} + +// Pattern 2: Specific exception handling with fallback +public void ProcessWithFallback() +{ + try + { + RiskyOperation(); + } + catch (HttpRequestException ex) + { + _logger.LogWarning(ex, "Network request failed, using cache"); + UseCache(); + } + catch (Exception ex) + { + _logger.LogError(ex, "Unexpected error in processing"); + throw; // Re-throw unexpected exceptions + } +} + +// Pattern 3: Graceful degradation with proper logging +public async Task GetDataWithFallback() +{ + try + { + return await GetFromPrimarySource(); + } + catch (Exception ex) + { + _logger.LogWarning(ex, "Primary source failed, trying fallback"); + + try + { + return await GetFromFallbackSource(); + } + catch (Exception fallbackEx) + { + _logger.LogError(fallbackEx, "Both primary and fallback sources failed"); + throw new DataUnavailableException("All data sources failed", ex); + } + } +} +``` + +Handling different exception types appropriately: + +```csharp +public void ProcessFile(string path) +{ + try + { + var content = File.ReadAllText(path); + ProcessContent(content); + } + catch (FileNotFoundException ex) + { + // Expected exception - log and handle gracefully + _logger.LogInformation("File not found: {Path}", path); + UseDefaultContent(); + } + catch (UnauthorizedAccessException ex) + { + // Security issue - log and throw + _logger.LogError(ex, "Access denied reading file: {Path}", path); + throw; + } + catch (Exception ex) + { + // Unexpected exception - log with full details and throw + _logger.LogError(ex, "Unexpected error processing file: {Path}", path); + throw; + } +} +``` + +## Guidelines for exception handling + +1. **Always observe exceptions**: Log or examine the exception before deciding how to handle it +2. **Provide context**: Include relevant information in logs and wrapped exceptions +3. **Handle specifically**: Catch specific exception types when possible +4. **Don't swallow silently**: Unhandled exceptions should be logged at minimum +5. **Consider the caller**: Think about what information the calling code needs +6. **Use appropriate log levels**: Error for unexpected exceptions, Warning for handled issues diff --git a/docs/Rules/ERP031.md b/docs/Rules/ERP031.md new file mode 100644 index 0000000..ef37ef7 --- /dev/null +++ b/docs/Rules/ERP031.md @@ -0,0 +1,253 @@ +# ERP031 - The API is not thread-safe + +This analyzer detects usage of APIs that are not thread-safe in multi-threaded contexts. + +## Description + +The analyzer warns when APIs that are known to be not thread-safe are used in ways that could lead to race conditions or other thread-safety issues. This helps prevent subtle bugs that can occur in multi-threaded applications. + +## Code that triggers the analyzer + +```csharp +public class Example +{ + private List _items = new List(); // Not thread-safe + + public void AddItem(string item) + { + // Bad: List is not thread-safe for concurrent modifications + _items.Add(item); // Race condition possible + } + + public void ProcessItems() + { + // Bad: Iteration while other threads might modify + foreach (var item in _items) // Potential exception + { + ProcessItem(item); + } + } +} + +public class SharedCounter +{ + private int _count = 0; + + public void Increment() + { + // Bad: Non-atomic read-modify-write operation + _count++; // Race condition + } + + public int GetCount() + { + return _count; // May return inconsistent value + } +} +``` + +## How to fix + +Use thread-safe alternatives or add proper synchronization: + +```csharp +public class ThreadSafeExample +{ + // Option 1: Use thread-safe collection + private ConcurrentBag _items = new ConcurrentBag(); + + public void AddItem(string item) + { + // Good: ConcurrentBag is thread-safe + _items.Add(item); + } + + public void ProcessItems() + { + // Good: ConcurrentBag supports safe enumeration + foreach (var item in _items) + { + ProcessItem(item); + } + } +} + +public class ThreadSafeCounter +{ + private int _count = 0; + + public void Increment() + { + // Good: Atomic increment operation + Interlocked.Increment(ref _count); + } + + public int GetCount() + { + // Good: Atomic read + return Interlocked.Read(ref _count); + } +} +``` + +Using locks for synchronization: + +```csharp +public class SynchronizedExample +{ + private readonly List _items = new List(); + private readonly object _lock = new object(); + + public void AddItem(string item) + { + lock (_lock) + { + _items.Add(item); + } + } + + public void ProcessItems() + { + List itemsCopy; + + // Create a copy under lock to avoid holding lock during processing + lock (_lock) + { + itemsCopy = new List(_items); + } + + // Process outside of lock + foreach (var item in itemsCopy) + { + ProcessItem(item); + } + } + + public int Count + { + get + { + lock (_lock) + { + return _items.Count; + } + } + } +} +``` + +Using reader-writer locks for better performance: + +```csharp +public class ReaderWriterExample +{ + private readonly List _items = new List(); + private readonly ReaderWriterLockSlim _lock = new ReaderWriterLockSlim(); + + public void AddItem(string item) + { + _lock.EnterWriteLock(); + try + { + _items.Add(item); + } + finally + { + _lock.ExitWriteLock(); + } + } + + public IEnumerable GetItems() + { + _lock.EnterReadLock(); + try + { + return _items.ToList(); // Return copy + } + finally + { + _lock.ExitReadLock(); + } + } + + public void Dispose() + { + _lock?.Dispose(); + } +} +``` + +Thread-safe patterns for different scenarios: + +```csharp +// Pattern 1: Lazy initialization +public class LazyInitExample +{ + private static readonly Lazy _resource = + new Lazy(() => new ExpensiveResource()); + + public static ExpensiveResource Resource => _resource.Value; +} + +// Pattern 2: Thread-safe singleton +public sealed class ThreadSafeSingleton +{ + private static readonly object _lock = new object(); + private static ThreadSafeSingleton _instance; + + public static ThreadSafeSingleton Instance + { + get + { + if (_instance == null) + { + lock (_lock) + { + if (_instance == null) + _instance = new ThreadSafeSingleton(); + } + } + return _instance; + } + } +} + +// Pattern 3: Using concurrent collections +public class ConcurrentDataStore +{ + private readonly ConcurrentDictionary _data = + new ConcurrentDictionary(); + + public void Store(string key, object value) + { + _data.AddOrUpdate(key, value, (k, v) => value); + } + + public T Get(string key) + { + return _data.TryGetValue(key, out var value) ? (T)value : default(T); + } +} +``` + +## Thread-safe alternatives + +| Non-thread-safe | Thread-safe alternative | +|-----------------|------------------------| +| `List` | `ConcurrentBag`, `ImmutableList` | +| `Dictionary` | `ConcurrentDictionary` | +| `Queue` | `ConcurrentQueue` | +| `Stack` | `ConcurrentStack` | +| `int++` | `Interlocked.Increment(ref int)` | +| `StringBuilder` | Use local instances or synchronize | +| `Random` | `ThreadLocal` or `Random.Shared` (.NET 6+) | + +## Best practices + +1. **Prefer immutable data structures** when possible +2. **Use concurrent collections** for shared mutable data +3. **Minimize shared state** between threads +4. **Use atomic operations** for simple shared variables +5. **Lock at the smallest scope** necessary +6. **Avoid nested locks** to prevent deadlocks +7. **Consider using async patterns** instead of blocking synchronization diff --git a/docs/Rules/ERP041.md b/docs/Rules/ERP041.md new file mode 100644 index 0000000..5fd18ae --- /dev/null +++ b/docs/Rules/ERP041.md @@ -0,0 +1,211 @@ +# ERP041 - EventSource class should be sealed + +This analyzer detects EventSource classes that are not sealed, which can cause runtime errors and unexpected behavior. + +## Description + +The analyzer warns when EventSource classes are not declared as `sealed`. EventSource classes should be sealed to ensure proper ETW (Event Tracing for Windows) functionality and to prevent inheritance issues that can cause runtime errors. + +## Code that triggers the analyzer + +```csharp +using System.Diagnostics.Tracing; + +// Bad: EventSource class is not sealed +[EventSource(Name = "MyCompany-MyProduct-MyEventSource")] +public class MyEventSource : EventSource +{ + public static readonly MyEventSource Log = new MyEventSource(); + + [Event(1, Level = EventLevel.Informational)] + public void ApplicationStarted(string applicationName) + { + WriteEvent(1, applicationName); + } +} + +// Also bad: inheriting from unsealed EventSource +public class ExtendedEventSource : MyEventSource +{ + [Event(2, Level = EventLevel.Warning)] + public void CustomEvent(string message) + { + WriteEvent(2, message); + } +} +``` + +## How to fix + +Make EventSource classes sealed: + +```csharp +using System.Diagnostics.Tracing; + +// Good: EventSource class is sealed +[EventSource(Name = "MyCompany-MyProduct-MyEventSource")] +public sealed class MyEventSource : EventSource +{ + public static readonly MyEventSource Log = new MyEventSource(); + + private MyEventSource() { } // Private constructor for singleton pattern + + [Event(1, Level = EventLevel.Informational)] + public void ApplicationStarted(string applicationName) + { + WriteEvent(1, applicationName); + } + + [Event(2, Level = EventLevel.Warning)] + public void WarningOccurred(string message) + { + WriteEvent(2, message); + } + + [Event(3, Level = EventLevel.Error)] + public void ErrorOccurred(string error, string details) + { + WriteEvent(3, error, details); + } +} +``` + +Proper EventSource pattern with all recommendations: + +```csharp +using System.Diagnostics.Tracing; + +[EventSource(Name = "MyCompany-MyProduct-Logging")] +public sealed class ApplicationEventSource : EventSource +{ + // Singleton instance + public static readonly ApplicationEventSource Log = new ApplicationEventSource(); + + // Private constructor to enforce singleton pattern + private ApplicationEventSource() { } + + // Event methods with proper attributes + [Event(1, + Level = EventLevel.Informational, + Message = "Application started: {0}", + Keywords = Keywords.Startup)] + public void ApplicationStarted(string applicationName) + { + if (IsEnabled()) + { + WriteEvent(1, applicationName); + } + } + + [Event(2, + Level = EventLevel.Warning, + Message = "Warning in {0}: {1}", + Keywords = Keywords.General)] + public void Warning(string component, string message) + { + if (IsEnabled(EventLevel.Warning, Keywords.General)) + { + WriteEvent(2, component, message); + } + } + + [Event(3, + Level = EventLevel.Error, + Message = "Error in {0}: {1}", + Keywords = Keywords.General)] + public void Error(string component, string error) + { + if (IsEnabled(EventLevel.Error, Keywords.General)) + { + WriteEvent(3, component, error); + } + } + + // Event keywords for categorization + public static class Keywords + { + public const EventKeywords Startup = (EventKeywords)1; + public const EventKeywords General = (EventKeywords)2; + public const EventKeywords Performance = (EventKeywords)4; + } +} +``` + +Usage example: + +```csharp +public class MyApplication +{ + public void Start() + { + // Using the sealed EventSource + ApplicationEventSource.Log.ApplicationStarted("MyApp v1.0"); + + try + { + DoWork(); + } + catch (Exception ex) + { + ApplicationEventSource.Log.Error("Application", ex.Message); + throw; + } + } + + private void DoWork() + { + ApplicationEventSource.Log.Warning("Worker", "Processing large dataset"); + // Work implementation + } +} +``` + +If you need multiple event sources: + +```csharp +// Create separate sealed EventSource classes for different components +[EventSource(Name = "MyCompany-Authentication")] +public sealed class AuthEventSource : EventSource +{ + public static readonly AuthEventSource Log = new AuthEventSource(); + private AuthEventSource() { } + + [Event(1, Level = EventLevel.Informational)] + public void UserLoggedIn(string userId) + { + WriteEvent(1, userId); + } +} + +[EventSource(Name = "MyCompany-DataAccess")] +public sealed class DataEventSource : EventSource +{ + public static readonly DataEventSource Log = new DataEventSource(); + private DataEventSource() { } + + [Event(1, Level = EventLevel.Informational)] + public void QueryExecuted(string query, long durationMs) + { + WriteEvent(1, query, durationMs); + } +} +``` + +## Why EventSource classes should be sealed + +1. **ETW Requirements**: EventSource uses reflection and code generation that can break with inheritance +2. **Runtime Errors**: Derived EventSource classes can cause runtime exceptions +3. **Performance**: Sealed classes allow better optimization +4. **Design Intent**: EventSource classes are designed to be final implementations +5. **Tool Support**: ETW tools and analyzers expect sealed EventSource classes + +## Best practices for EventSource + +- Always make EventSource classes sealed +- Use singleton pattern with static readonly instance +- Use private constructors +- Check `IsEnabled()` before expensive operations +- Use proper event IDs (unique within the EventSource) +- Provide meaningful event names and messages +- Use keywords for event categorization +- Follow ETW parameter type restrictions diff --git a/docs/Rules/ERP042.md b/docs/Rules/ERP042.md new file mode 100644 index 0000000..f58c6ce --- /dev/null +++ b/docs/Rules/ERP042.md @@ -0,0 +1,263 @@ +# ERP042 - EventSource implementation is not correct + +This analyzer detects various issues with EventSource implementations that can cause runtime errors or incorrect behavior. + +## Description + +The analyzer warns about incorrect EventSource implementations, including issues with event IDs, parameter mismatches, incorrect WriteEvent calls, and other problems that can cause ETW (Event Tracing for Windows) to fail at runtime. + +## Code that triggers the analyzer + +```csharp +using System.Diagnostics.Tracing; + +[EventSource(Name = "MyEventSource")] +public sealed class ProblematicEventSource : EventSource +{ + public static readonly ProblematicEventSource Log = new ProblematicEventSource(); + + // Bad: Duplicate event IDs + [Event(1)] + public void Event1() => WriteEvent(1); + + [Event(1)] // Same ID as above! + public void AnotherEvent() => WriteEvent(1); + + // Bad: Event ID mismatch + [Event(2)] + public void Event2(string message) => WriteEvent(3, message); // Wrong ID! + + // Bad: Parameter count mismatch + [Event(3)] + public void Event3(string msg, int count) => WriteEvent(3, msg); // Missing parameter! + + // Bad: Parameter order mismatch + [Event(4)] + public void Event4(string first, int second) => WriteEvent(4, second, first); // Wrong order! + + // Bad: Unsupported parameter type + [Event(5)] + public void Event5(DateTime timestamp) => WriteEvent(5, timestamp); // DateTime not supported! + + // Bad: Missing parameter name in WriteEventCore + [Event(6)] + public void Event6(string message, int value) + { + WriteEventCore(6, 1, new EventData[] + { + new EventData { DataPointer = /* message pointer */, Size = message.Length } + // Missing second parameter! + }); + } +} +``` + +## How to fix + +Implement EventSource correctly: + +```csharp +using System.Diagnostics.Tracing; + +[EventSource(Name = "MyCompany-MyProduct-EventSource")] +public sealed class CorrectEventSource : EventSource +{ + public static readonly CorrectEventSource Log = new CorrectEventSource(); + private CorrectEventSource() { } + + // Good: Unique event IDs + [Event(1, Level = EventLevel.Informational)] + public void ApplicationStarted(string applicationName, string version) + { + if (IsEnabled()) + { + WriteEvent(1, applicationName, version); // Matching ID and parameters + } + } + + // Good: Correct event ID and parameter mapping + [Event(2, Level = EventLevel.Warning)] + public void WarningOccurred(string component, string message, int errorCode) + { + if (IsEnabled(EventLevel.Warning)) + { + WriteEvent(2, component, message, errorCode); // All parameters included + } + } + + // Good: Supported parameter types only + [Event(3, Level = EventLevel.Error)] + public void ErrorOccurred(string operation, int errorCode, long timestamp) + { + if (IsEnabled(EventLevel.Error)) + { + WriteEvent(3, operation, errorCode, timestamp); + } + } + + // Good: Proper WriteEventCore usage + [Event(4, Level = EventLevel.Verbose)] + public void DetailedEvent(string category, string details) + { + if (IsEnabled(EventLevel.Verbose)) + { + unsafe + { + fixed (char* categoryPtr = category) + fixed (char* detailsPtr = details) + { + WriteEventCore(4, 2, new EventData[] + { + new EventData + { + DataPointer = (IntPtr)categoryPtr, + Size = (category.Length + 1) * sizeof(char) + }, + new EventData + { + DataPointer = (IntPtr)detailsPtr, + Size = (details.Length + 1) * sizeof(char) + } + }); + } + } + } + } + + // Good: Using supported types and proper enum conversion + [Event(5, Level = EventLevel.Informational)] + public void StatusChanged(string component, int newStatus) // int instead of enum + { + if (IsEnabled()) + { + WriteEvent(5, component, newStatus); + } + } +} +``` + +Handling complex types correctly: + +```csharp +[EventSource(Name = "MyCompany-ComplexEvents")] +public sealed class ComplexEventSource : EventSource +{ + public static readonly ComplexEventSource Log = new ComplexEventSource(); + private ComplexEventSource() { } + + // Convert DateTime to supported type + [Event(1)] + public void TimestampEvent(string operation, DateTime timestamp) + { + if (IsEnabled()) + { + // Convert DateTime to long (ticks) which is supported + WriteEvent(1, operation, timestamp.Ticks); + } + } + + // Handle Guid by converting to string + [Event(2)] + public void CorrelationEvent(string operation, Guid correlationId) + { + if (IsEnabled()) + { + WriteEvent(2, operation, correlationId.ToString()); + } + } + + // Handle custom objects by extracting properties + [Event(3)] + public void UserActionEvent(string userId, string action, UserInfo userInfo) + { + if (IsEnabled()) + { + // Extract supported properties from complex object + WriteEvent(3, userId, action, userInfo.Name, userInfo.Age); + } + } + + // Overload for better usability while maintaining correctness + public void UserActionEvent(string userId, string action, string userName, int userAge) + { + WriteEvent(3, userId, action, userName, userAge); + } +} + +public class UserInfo +{ + public string Name { get; set; } + public int Age { get; set; } +} +``` + +Handling WriteEventWithRelatedActivityId correctly: + +```csharp +[EventSource(Name = "MyCompany-ActivityEvents")] +public sealed class ActivityEventSource : EventSource +{ + public static readonly ActivityEventSource Log = new ActivityEventSource(); + private ActivityEventSource() { } + + // Good: First parameter must be Guid and named "relatedActivityId" + [Event(1)] + public void StartActivityWithRelated(Guid relatedActivityId, string activityName, string details) + { + if (IsEnabled()) + { + WriteEventWithRelatedActivityId(1, relatedActivityId, activityName, details); + } + } + + // Good: Using proper parameter order and types + [Event(2)] + public void CompleteActivityWithRelated(Guid relatedActivityId, string activityName, long durationMs, bool success) + { + if (IsEnabled()) + { + WriteEventWithRelatedActivityId(2, relatedActivityId, activityName, durationMs, success); + } + } +} +``` + +## Common EventSource rules + +1. **Event IDs must be unique** within an EventSource +2. **Event ID in attribute must match WriteEvent call** +3. **Parameter count must match** between method signature and WriteEvent +4. **Parameter order must match** between method signature and WriteEvent +5. **Use only supported parameter types**: + - Primitive types: `bool`, `byte`, `short`, `int`, `long`, `float`, `double` + - `string`, `char` + - `IntPtr`, `UIntPtr` + - `Guid` (in some contexts) +6. **For WriteEventWithRelatedActivityId**: First parameter must be `Guid` named "relatedActivityId" +7. **Always check IsEnabled()** before expensive operations +8. **Use IsEnabled(level, keywords)** for conditional logging + +## Supported parameter types + +```csharp +// ✓ Supported types +public void GoodEvent( + bool flag, + byte b, short s, int i, long l, + float f, double d, + string text, char c, + IntPtr ptr, UIntPtr uptr) +{ + WriteEvent(1, flag, b, s, i, l, f, d, text, c, ptr, uptr); +} + +// ✗ Unsupported types (convert before logging) +public void BadEvent( + DateTime dt, // Convert to .Ticks (long) + Guid guid, // Convert to .ToString() + MyEnum enumVal, // Convert to (int)enumVal + MyClass obj) // Extract individual properties +{ + // Don't do this - will cause runtime errors +} +``` diff --git a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md index 8fd3f9e..5f9fb14 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md +++ b/src/ErrorProne.NET.CoreAnalyzers/AnalyzerReleases.Unshipped.md @@ -4,30 +4,30 @@ ### New Rules Rule ID | Category | Severity | Notes --------|----------|----------|------- -EPC11 | CodeSmell | Warning | SuspiciousEqualsMethodAnalyzer -EPC12 | CodeSmell | Warning | SuspiciousExceptionHandlingAnalyzer -EPC13 | CodeSmell | Warning | UnobservedResultAnalyzer -EPC14 | CodeSmell | Info | RemoveConfigureAwaitAnalyzer -EPC15 | CodeSmell | Warning | AddConfigureAwaitAnalyzer -EPC16 | CodeSmell | Warning | NullConditionalOperatorAnalyzer -EPC17 | CodeSmell | Warning | AsyncVoidLambdaAnalyzer -EPC18 | CodeSmell | Warning | TaskInstanceToStringConversionAnalyzer +EPC11 | ErrorHandling | Warning | SuspiciousEqualsMethodAnalyzer +EPC12 | ErrorHandling | Warning | SuspiciousExceptionHandlingAnalyzer +EPC13 | ErrorHandling | Warning | UnobservedResultAnalyzer +EPC14 | Async | Info | RedundantConfigureAwaitFalseAnalyzer +EPC15 | Async | Warning | ConfigureAwaitRequiredAnalyzer +EPC16 | Async | Warning | NullConditionalOperatorAnalyzer +EPC17 | Async | Warning | AsyncVoidDelegateAnalyzer +EPC18 | Async | Warning | TaskInstanceToStringConversionAnalyzer EPC19 | CodeSmell | Warning | CancellationTokenRegistrationAnalyzer -EPC20 | CodeSmell | Warning | DefaultToStringImplementationUsageAnalyzer +EPC20 | Async | Warning | DefaultToStringImplementationUsageAnalyzer EPC23 | Performance | Warning | HashSetContainsAnalyzer EPC24 | Performance | Warning | HashTableIncompatibilityAnalyzer EPC25 | Performance | Warning | DefaultEqualsOrHashCodeUsageAnalyzer -EPC26 | CodeSmell | Warning | TaskInUsingBlockAnalyzer -EPC27 | CodeSmell | Warning | AsyncVoidMethodAnalyzer +EPC26 | Async | Warning | TaskInUsingBlockAnalyzer +EPC27 | Async | Warning | AsyncVoidMethodAnalyzer EPC28 | CodeSmell | Warning | ExcludeFromCodeCoverageOnPartialClassAnalyzer -EPC29 | CodeSmell | Warning | ExcludeFromCodeCoverageMessageAnalyzer: Warn when ExcludeFromCodeCoverageAttribute is used without a message argument. -EPC30 | CodeSmell | Warning | RecursiveCallAnalyzer: Warns when a method calls itself recursively (conditionally or unconditionally). -EPC31 | CodeSmell | Warning | DoNotReturnNullForTaskLikeAnalyzer -EPC32 | CodeSmell | Warning | TaskCompletionSourceRunContinuationsAnalyzer -EPC33 | CodeSmell | Warning | DoNotUseThreadSleepAnalyzer -EPC34 | CodeSmell | Warning | MustUseResultAnalyzer -ERP021 | CodeSmell | Warning | ThrowExAnalyzer -ERP022 | CodeSmell | Warning | SwallowAllExceptionsAnalyzer +EPC29 | CodeSmell | Warning | ExcludeFromCodeCoverageMessageAnalyzer +EPC30 | CodeSmell | Warning | RecursiveCallAnalyzer +EPC31 | Async | Warning | DoNotReturnNullForTaskLikeAnalyzer +EPC32 | Async | Warning | TaskCompletionSourceRunContinuationsAnalyzer +EPC33 | Async | Warning | DoNotUseThreadSleepAnalyzer +EPC34 | ErrorHandling | Warning | MustUseResultAnalyzer +ERP021 | ErrorHandling | Warning | ThrowExAnalyzer +ERP022 | ErrorHandling | Warning | SwallowAllExceptionsAnalyzer ERP031 | Concurrency | Warning | ConcurrentCollectionAnalyzer ERP041 | CodeSmell | Warning | EventSourceSealedAnalyzer ERP042 | CodeSmell | Warning | EventSourceAnalyzer diff --git a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs index 16b98cd..b2ca473 100644 --- a/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs +++ b/src/ErrorProne.NET.CoreAnalyzers/DiagnosticDescriptors.cs @@ -9,116 +9,133 @@ internal static class DiagnosticDescriptors private const string CodeSmellCategory = "CodeSmell"; private const string PerformanceCategory = "Performance"; private const string ConcurrencyCategory = "Concurrency"; - private static readonly string[] UnnecessaryTag = new [] { WellKnownDiagnosticTags.Unnecessary }; + + private const string AsyncCategory = "Async"; + + private const string ErrorHandlingCategory = "ErrorHandling"; + private static readonly string[] UnnecessaryTag = new[] { WellKnownDiagnosticTags.Unnecessary }; /// internal static readonly DiagnosticDescriptor EPC11 = new DiagnosticDescriptor( - nameof(EPC11), - title: "Suspicious equality implementation", - messageFormat: "Suspicious equality implementation: {0}", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Equals method that does not use any instance members or other instance is suspicious."); + nameof(EPC11), + title: "Suspicious equality implementation", + messageFormat: "Suspicious equality implementation: {0}", + ErrorHandlingCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Equals method that does not use any instance members or other instance is suspicious.", + helpLinkUri: GetHelpUri(nameof(EPC11))); /// public static readonly DiagnosticDescriptor EPC12 = new DiagnosticDescriptor( nameof(EPC12), title: "Suspicious exception handling: only the 'Message' property is observed in the catch block", messageFormat: "Suspicious exception handling: only '{0}.Message' is observed in the catch block", - category: CodeSmellCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, + category: ErrorHandlingCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, description: "In many cases the 'Message' property contains irrelevant information and actual data is kept in inner exceptions.", + helpLinkUri: GetHelpUri(nameof(EPC12)), // The diagnostics fades parts of the code. customTags: UnnecessaryTag); /// internal static readonly DiagnosticDescriptor EPC13 = new DiagnosticDescriptor( - nameof(EPC13), - title: "Suspiciously unobserved result", - messageFormat: "The result of type '{0}' should be observed", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Return values of some methods should always be observed."); + nameof(EPC13), + title: "Suspiciously unobserved result", + messageFormat: "The result of type '{0}' should be observed", + ErrorHandlingCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Return values of some methods should always be observed.", + helpLinkUri: GetHelpUri(nameof(EPC13))); /// internal static readonly DiagnosticDescriptor EPC14 = new DiagnosticDescriptor( - nameof(EPC14), - title: "ConfigureAwait(false) call is redundant", - messageFormat: "ConfigureAwait(false) call is redundant", - CodeSmellCategory, + nameof(EPC14), + title: "ConfigureAwait(false) call is redundant", + messageFormat: "ConfigureAwait(false) call is redundant", + AsyncCategory, // Using Info to fade away the call, not warn on it. - DiagnosticSeverity.Info, isEnabledByDefault: true, + DiagnosticSeverity.Info, isEnabledByDefault: true, description: "The assembly is configured not to use .ConfigureAwait(false).", + helpLinkUri: GetHelpUri(nameof(EPC14)), // The diagnostics fades parts of the code. customTags: UnnecessaryTag); /// internal static readonly DiagnosticDescriptor EPC15 = new DiagnosticDescriptor( - nameof(EPC15), - title: "ConfigureAwait(false) must be used", - messageFormat: "ConfigureAwait(false) must be used", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "The assembly is configured to use .ConfigureAwait(false)."); + nameof(EPC15), + title: "ConfigureAwait(false) must be used", + messageFormat: "ConfigureAwait(false) must be used", + AsyncCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "The assembly is configured to use .ConfigureAwait(false).", + helpLinkUri: GetHelpUri(nameof(EPC15))); /// internal static readonly DiagnosticDescriptor EPC16 = new DiagnosticDescriptor( - nameof(EPC16), - title: "Awaiting a result of a null-conditional expression will cause NullReferenceException", - messageFormat: "Awaiting a result of a null-conditional expression will cause NullReferenceException", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Null-conditional operator returns null when 'lhs' is null, causing NRE when a task is awaited."); + nameof(EPC16), + title: "Awaiting a result of a null-conditional expression will cause NullReferenceException", + messageFormat: "Awaiting a result of a null-conditional expression will cause NullReferenceException", + AsyncCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Null-conditional operator returns null when 'lhs' is null, causing NRE when a task is awaited.", + helpLinkUri: GetHelpUri(nameof(EPC16))); /// internal static readonly DiagnosticDescriptor EPC17 = new DiagnosticDescriptor( - nameof(EPC17), - title: "Avoid async-void delegates", - messageFormat: "Avoid async-void delegates", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Async-void delegates can cause application to crash."); + nameof(EPC17), + title: "Avoid async-void delegates", + messageFormat: "Avoid async-void delegates", + AsyncCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Async-void delegates can cause application to crash.", + helpLinkUri: GetHelpUri(nameof(EPC17))); /// public static readonly DiagnosticDescriptor EPC18 = new DiagnosticDescriptor( - nameof(EPC18), - title: "A task instance is implicitly converted to a string", - messageFormat: "A task instance is implicitly converted to a string", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "An implicit conversion of a task instance to a string potentially indicates an lack of 'await'."); + nameof(EPC18), + title: "A task instance is implicitly converted to a string", + messageFormat: "A task instance is implicitly converted to a string", + AsyncCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "An implicit conversion of a task instance to a string potentially indicates an lack of 'await'.", + helpLinkUri: GetHelpUri(nameof(EPC18))); /// public static readonly DiagnosticDescriptor EPC19 = new DiagnosticDescriptor( - nameof(EPC19), - title: "Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks", - messageFormat: "Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Failure to dispose 'CancellationTokenRegistration' may cause a memory leak if obtained from a non-local token."); - + nameof(EPC19), + title: "Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks", + messageFormat: "Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks", + CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Failure to dispose 'CancellationTokenRegistration' may cause a memory leak if obtained from a non-local token.", + helpLinkUri: GetHelpUri(nameof(EPC19))); + /// public static readonly DiagnosticDescriptor EPC20 = new DiagnosticDescriptor( - nameof(EPC20), - title: "Avoid using default ToString implementation", - messageFormat: "A default ToString implementation is used for type {0}", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "A default ToString implementation is rarely gives the result you need."); + nameof(EPC20), + title: "Avoid using default ToString implementation", + messageFormat: "A default ToString implementation is used for type {0}", + AsyncCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "A default ToString implementation is rarely gives the result you need.", + helpLinkUri: GetHelpUri(nameof(EPC20))); /// internal static readonly DiagnosticDescriptor ERP021 = new DiagnosticDescriptor( - nameof(ERP021), - title: "Incorrect exception propagation", - messageFormat: "Incorrect exception propagation. Use 'throw;' instead.", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Incorrect exception propagation alters the stack trace of a thrown exception."); + nameof(ERP021), + title: "Incorrect exception propagation", + messageFormat: "Incorrect exception propagation. Use 'throw;' instead.", + ErrorHandlingCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Incorrect exception propagation alters the stack trace of a thrown exception.", + helpLinkUri: GetHelpUri(nameof(ERP021))); /// internal static readonly DiagnosticDescriptor ERP022 = new DiagnosticDescriptor( - nameof(ERP022), - title: "Unobserved exception in a generic exception handler", - messageFormat: "An exit point '{0}' swallows an unobserved exception", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "A generic catch block swallows an exception that was not observed."); - + nameof(ERP022), + title: "Unobserved exception in a generic exception handler", + messageFormat: "An exit point '{0}' swallows an unobserved exception", + ErrorHandlingCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "A generic catch block swallows an exception that was not observed.", + helpLinkUri: GetHelpUri(nameof(ERP022))); + /// internal static readonly DiagnosticDescriptor EPC23 = new DiagnosticDescriptor( - nameof(EPC23), - title: "Avoid using Enumerable.Contains on HashSet", - messageFormat: "Linear search via Enumerable.Contains is used instead of an instance Contains method", - PerformanceCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Enumerable.Contains is less efficient since it scans all the entries in the hashset and allocates an iterator."); + nameof(EPC23), + title: "Avoid using Enumerable.Contains on HashSet", + messageFormat: "Linear search via Enumerable.Contains is used instead of an instance Contains method", + PerformanceCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Enumerable.Contains is less efficient since it scans all the entries in the hashset and allocates an iterator.", + helpLinkUri: GetHelpUri(nameof(EPC23))); /// public static readonly DiagnosticDescriptor EPC24 = new DiagnosticDescriptor( @@ -126,7 +143,8 @@ internal static class DiagnosticDescriptors "A hash table \"unfriendly\" type is used as the key in a hash table", "A struct '{0}' with a default {1} implementation is used as a key in a hash table", category: PerformanceCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "The default implementation of 'Equals' and 'GetHashCode' for structs is inefficient and could cause severe performance issues."); + description: "The default implementation of 'Equals' and 'GetHashCode' for structs is inefficient and could cause severe performance issues.", + helpLinkUri: GetHelpUri(nameof(EPC24))); /// public static readonly DiagnosticDescriptor EPC25 = @@ -134,25 +152,28 @@ internal static class DiagnosticDescriptors title: "Avoid using default Equals or HashCode implementation from structs", messageFormat: "The default 'ValueType.{0}' is used in {1}", category: PerformanceCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "The default implementation of 'Equals' and 'GetHashCode' for structs is inefficient and could cause severe performance issues."); - + description: "The default implementation of 'Equals' and 'GetHashCode' for structs is inefficient and could cause severe performance issues.", + helpLinkUri: GetHelpUri(nameof(EPC25))); + /// public static readonly DiagnosticDescriptor EPC26 = new DiagnosticDescriptor(nameof(EPC26), title: "Do not use tasks in using block", messageFormat: "A Task was used in using block", - category: CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Task implements IDisposable but should not be ever disposed explicitly."); + category: AsyncCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "Task implements IDisposable but should not be ever disposed explicitly.", + helpLinkUri: GetHelpUri(nameof(EPC26))); /// public static readonly DiagnosticDescriptor EPC27 = new DiagnosticDescriptor( nameof(EPC27), title: "Avoid async void methods", messageFormat: "Method '{0}' is 'async void'. Use 'async Task' instead.", - category: CodeSmellCategory, + category: AsyncCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Async void methods are dangerous and should be avoided except for event handlers."); + description: "Async void methods are dangerous and should be avoided except for event handlers.", + helpLinkUri: GetHelpUri(nameof(EPC27))); /// public static readonly DiagnosticDescriptor EPC28 = new DiagnosticDescriptor( @@ -162,7 +183,8 @@ internal static class DiagnosticDescriptors category: CodeSmellCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Applying ExcludeFromCodeCoverageAttribute to a partial class can lead to inconsistent code coverage results."); + description: "Applying ExcludeFromCodeCoverageAttribute to a partial class can lead to inconsistent code coverage results.", + helpLinkUri: GetHelpUri(nameof(EPC28))); /// public static readonly DiagnosticDescriptor EPC29 = new DiagnosticDescriptor( @@ -172,7 +194,8 @@ internal static class DiagnosticDescriptors category: CodeSmellCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Always provide a message when using ExcludeFromCodeCoverageAttribute to document the reason for exclusion."); + description: "Always provide a message when using ExcludeFromCodeCoverageAttribute to document the reason for exclusion.", + helpLinkUri: GetHelpUri(nameof(EPC29))); /// public static readonly DiagnosticDescriptor EPC30 = new DiagnosticDescriptor( @@ -182,70 +205,83 @@ internal static class DiagnosticDescriptors category: CodeSmellCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Detects when a method calls itself recursively, either conditionally or unconditionally."); + description: "Detects when a method calls itself recursively, either conditionally or unconditionally.", + helpLinkUri: GetHelpUri(nameof(EPC30))); /// public static readonly DiagnosticDescriptor EPC31 = new DiagnosticDescriptor( nameof(EPC31), title: "Do not return null for Task-like types", messageFormat: "Do not return null for Task-like type from method '{0}'", - category: CodeSmellCategory, + category: AsyncCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Returning null for a Task-like type may lead to NullReferenceException when the task is awaited. Return Task.CompletedTask instead."); - - /// - public static readonly DiagnosticDescriptor ERP031 = new DiagnosticDescriptor( - nameof(ERP031), - title: "The API is not thread-safe", - messageFormat: "The API is not thread-safe.{0}", - ConcurrencyCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "The API is not thread safe and can cause runtime failures."); - - /// - public static readonly DiagnosticDescriptor ERP041 = new DiagnosticDescriptor( - nameof(ERP041), - title: "EventSource class should be sealed", - messageFormat: "{0}: {1}", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "The event source implementation must follow special rules to avoid hitting runtime errors."); - - /// - public static readonly DiagnosticDescriptor ERP042 = new DiagnosticDescriptor( - nameof(ERP042), - title: "EventSource implementation is not correct", - messageFormat: "{0}: {1}", - CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "The event source implementation must follow special rules to avoid hitting runtime errors."); + description: "Returning null for a Task-like type may lead to NullReferenceException when the task is awaited. Return Task.CompletedTask instead.", + helpLinkUri: GetHelpUri(nameof(EPC31))); /// public static readonly DiagnosticDescriptor EPC32 = new DiagnosticDescriptor( nameof(EPC32), title: "TaskCompletionSource should use RunContinuationsAsynchronously", messageFormat: "TaskCompletionSource instance should be created with TaskCreationOptions.RunContinuationsAsynchronously", - category: CodeSmellCategory, + category: AsyncCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Always use TaskCreationOptions.RunContinuationsAsynchronously when creating TaskCompletionSource to avoid potential deadlocks."); + description: "Always use TaskCreationOptions.RunContinuationsAsynchronously when creating TaskCompletionSource to avoid potential deadlocks.", + helpLinkUri: GetHelpUri(nameof(EPC32))); /// public static readonly DiagnosticDescriptor EPC33 = new DiagnosticDescriptor( nameof(EPC33), title: "Do not use Thread.Sleep in async methods", messageFormat: "Thread.Sleep should not be used in async methods. Use 'await Task.Delay()' instead.", - category: CodeSmellCategory, + category: AsyncCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Thread.Sleep blocks the thread and defeats the purpose of async programming. Use Task.Delay with await instead."); + description: "Thread.Sleep blocks the thread and defeats the purpose of async programming. Use Task.Delay with await instead.", + helpLinkUri: GetHelpUri(nameof(EPC33))); /// public static readonly DiagnosticDescriptor EPC34 = new DiagnosticDescriptor( nameof(EPC34), title: "Method return value marked with MustUseResultAttribute must be used", messageFormat: "The return value of method '{0}' marked with MustUseResultAttribute must be used", - category: CodeSmellCategory, + category: ErrorHandlingCategory, defaultSeverity: DiagnosticSeverity.Warning, isEnabledByDefault: true, - description: "Methods marked with MustUseResultAttribute return values that should always be observed and used."); + description: "Methods marked with MustUseResultAttribute return values that should always be observed and used.", + helpLinkUri: GetHelpUri(nameof(EPC34))); + + /// + public static readonly DiagnosticDescriptor ERP031 = new DiagnosticDescriptor( + nameof(ERP031), + title: "The API is not thread-safe", + messageFormat: "The API is not thread-safe.{0}", + ConcurrencyCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "The API is not thread safe and can cause runtime failures.", + helpLinkUri: GetHelpUri(nameof(ERP031))); + + /// + public static readonly DiagnosticDescriptor ERP041 = new DiagnosticDescriptor( + nameof(ERP041), + title: "EventSource class should be sealed", + messageFormat: "{0}: {1}", + CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "The event source implementation must follow special rules to avoid hitting runtime errors.", + helpLinkUri: GetHelpUri(nameof(ERP041))); + + /// + public static readonly DiagnosticDescriptor ERP042 = new DiagnosticDescriptor( + nameof(ERP042), + title: "EventSource implementation is not correct", + messageFormat: "{0}: {1}", + CodeSmellCategory, DiagnosticSeverity.Warning, isEnabledByDefault: true, + description: "The event source implementation must follow special rules to avoid hitting runtime errors.", + helpLinkUri: GetHelpUri(nameof(ERP042))); + + public static string GetHelpUri(string ruleId) + { + return $"https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/{ruleId}.md"; + } } } \ No newline at end of file From d76c5cbb9b7812d5744aaea3da7edba6b902ad26 Mon Sep 17 00:00:00 2001 From: Sergey Teplyakov Date: Mon, 30 Jun 2025 23:40:14 -0700 Subject: [PATCH 2/2] Update links in ReadMe.md to point to the correct documentation for analyzers --- ReadMe.md | 54 +++++++++++++++++++++++++++--------------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/ReadMe.md b/ReadMe.md index 46d61d2..0993cf6 100644 --- a/ReadMe.md +++ b/ReadMe.md @@ -15,50 +15,50 @@ Add the following nuget package to you project: https://www.nuget.org/packages/E | Id | Description | |---|---| -| [EPC14](docs/Rules/EPC14.md) | ConfigureAwait(false) call is redundant | -| [EPC15](docs/Rules/EPC15.md) | ConfigureAwait(false) must be used | -| [EPC16](docs/Rules/EPC16.md) | Awaiting a result of a null-conditional expression will cause NullReferenceException | -| [EPC17](docs/Rules/EPC17.md) | Avoid async-void delegates | -| [EPC18](docs/Rules/EPC18.md) | A task instance is implicitly converted to a string | -| [EPC20](docs/Rules/EPC20.md) | Avoid using default ToString implementation | -| [EPC26](docs/Rules/EPC26.md) | Do not use tasks in using block | -| [EPC27](docs/Rules/EPC27.md) | Avoid async void methods | -| [EPC31](docs/Rules/EPC31.md) | Do not return null for Task-like types | -| [EPC32](docs/Rules/EPC32.md) | TaskCompletionSource should use RunContinuationsAsynchronously | -| [EPC33](docs/Rules/EPC33.md) | Do not use Thread.Sleep in async methods | +| [EPC14](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC14.md) | ConfigureAwait(false) call is redundant | +| [EPC15](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC15.md) | ConfigureAwait(false) must be used | +| [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 | ### Generic Bugs and Code Smells | Id | Description | |---|---| -| [EPC19](docs/Rules/EPC19.md) | Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks | -| [EPC28](docs/Rules/EPC28.md) | Do not use ExcludeFromCodeCoverage on partial classes | -| [EPC29](docs/Rules/EPC29.md) | ExcludeFromCodeCoverageAttribute should provide a message | -| [EPC30](docs/Rules/EPC30.md) | Method calls itself recursively | -| [ERP041](docs/Rules/ERP041.md) | EventSource class should be sealed | -| [ERP042](docs/Rules/ERP042.md) | EventSource implementation is not correct | +| [EPC19](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC19.md) | Observe and Dispose a 'CancellationTokenRegistration' to avoid memory leaks | +| [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 | +| [ERP041](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP041.md) | EventSource class should be sealed | +| [ERP042](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP042.md) | EventSource implementation is not correct | ### Concurrency | Id | Description | |---|---| -| [ERP031](docs/Rules/ERP031.md) | The API is not thread-safe | +| [ERP031](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP031.md) | The API is not thread-safe | ### Error Handling Issues | Id | Description | |---|---| -| [EPC11](docs/Rules/EPC11.md) | Suspicious equality implementation | -| [EPC12](docs/Rules/EPC12.md) | Suspicious exception handling: only the 'Message' property is observed in the catch block | -| [EPC13](docs/Rules/EPC13.md) | Suspiciously unobserved result | -| [EPC34](docs/Rules/EPC34.md) | Method return value marked with MustUseResultAttribute must be used | -| [ERP021](docs/Rules/ERP021.md) | Incorrect exception propagation | -| [ERP022](docs/Rules/ERP022.md) | Unobserved exception in a generic exception handler | +| [EPC11](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC11.md) | Suspicious equality implementation | +| [EPC12](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC12.md) | Suspicious exception handling: only the 'Message' property is observed in the catch block | +| [EPC13](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC13.md) | Suspiciously unobserved result | +| [EPC34](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC34.md) | Method return value marked with MustUseResultAttribute must be used | +| [ERP021](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP021.md) | Incorrect exception propagation | +| [ERP022](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/ERP022.md) | Unobserved exception in a generic exception handler | ### Performance | Id | Description | |---|---| -| [EPC23](docs/Rules/EPC23.md) | Avoid using Enumerable.Contains on HashSet | -| [EPC24](docs/Rules/EPC24.md) | A hash table "unfriendly" type is used as the key in a hash table | -| [EPC25](docs/Rules/EPC25.md) | Avoid using default Equals or HashCode implementation from structs | \ No newline at end of file +| [EPC23](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC23.md) | Avoid using Enumerable.Contains on HashSet | +| [EPC24](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC24.md) | A hash table "unfriendly" type is used as the key in a hash table | +| [EPC25](https://github.com/SergeyTeplyakov/ErrorProne.NET/tree/master/docs/Rules/EPC25.md) | Avoid using default Equals or HashCode implementation from structs | \ No newline at end of file