Skip to content

Add Item 13: Use proper initialization for static class members #66

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Sep 26, 2024
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 190 additions & 0 deletions docs/rules/ECS1300.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
# ECS1300: Use Proper Initialization for Static Class Members

## Cause

Static fields initialized with complex expressions or method calls that might throw exceptions can cause runtime errors during type initialization. These exceptions occur before any static constructors or error-handling mechanisms can intercept them, potentially crashing the application.

## Rule Description

This rule identifies static fields that are initialized directly with complex expressions, method calls, or operations that could throw exceptions. Initializing static fields in this manner can lead to unhandled exceptions during the type's initialization phase, making debugging difficult and potentially causing the application to terminate unexpectedly.

By moving complex initializations into a static constructor or using `Lazy<T>`, you can control the timing of the initialization and handle any exceptions appropriately. This approach enhances the reliability and maintainability of your code.

## How to fix violations

- **Use a Static Constructor**: Move the complex initialization logic into a static constructor where you can handle exceptions and control execution flow.

```csharp
public class MyClass
{
private static readonly string ConfigValue;

static MyClass()
{
ConfigValue = LoadConfigValue();
}

private static string LoadConfigValue()
{
// Complex initialization logic
return System.IO.File.ReadAllText("config.txt");
}
}
```

- **Use `Lazy<T>` for Lazy Initialization**: Wrap the initialization logic in a `Lazy<T>` object to defer execution until the value is needed.

```csharp
public class MyClass
{
private static readonly Lazy<string> ConfigValue = new Lazy<string>(LoadConfigValue);

private static string LoadConfigValue()
{
// Complex initialization logic
return System.IO.File.ReadAllText("config.txt");
}
}
```

## Extending the Rule with Safe Methods

### Customizing Safe Methods using Configuration

The analyzer allows you to specify additional methods that should be considered safe for static initialization. You can extend the list of safe methods by using the `dotnet_diagnostic.ECS1300.safe_methods` option in an EditorConfig file.

### How to Configure Additional Safe Methods

1. Create or Update an EditorConfig File: Add or modify an `.editorconfig` file in your project directory. If you don't have one, you can create it at the root of your project.
2. Add the `safe_items` Option: Use the `dotnet_diagnostic.ECS1300.safe_items` key to specify a comma-separated list of fully qualified method and/or property names that you want to treat as safe.

```ini
[*.cs]
dotnet_diagnostic.ECS1300.safe_items = System.MySafeClass.MySafeMethod, System.AnotherClass.AnotherSafeMethod
```

>Note: Ensure that the method and property names are fully qualified, including the namespace and class name.

#### Example Configuration

Suppose you have methods and properties in your codebase that you know are safe for static initialization:

- `Contoso.Utilities.GetDefaultSettings`
- `Contoso.Constants.GetMaxValue`
- `Contoso.RecordManager.MaxValue`

You can add the fully qualified symbol to the safe list:

```ini
[*.cs]
dotnet_diagnostic.ECS1300.safe_methods = Contoso.Utilities.GetDefaultSettings, Contoso.Constants.GetMaxValue, Contoso.RecordManager.MaxValue
```

#### Effect on the Analyzer

Check notice on line 83 in docs/rules/ECS1300.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1300.md#L83

Expected: 1; Actual: 0; Below
By configuring these methods and properties as safe, the analyzer will no longer report diagnostics for static fields initialized using them:

```csharp
public class MyClass
{
// No diagnostic will be reported for this initialization
private static readonly Settings DefaultSettings = Utilities.GetDefaultSettings();
}
```

### When to Use This Option

- **Third-Party Libraries**: If you use methods from external libraries that you know are safe but are not included in the default safe methods list.
- **Custom Utility Methods**: For utility methods in your codebase that are deterministic and exception-safe.
- **Performance Considerations**: When you prefer direct initialization for performance reasons and are confident in the safety of the methods used.

### Precautions

Check notice on line 100 in docs/rules/ECS1300.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1300.md#L100

Expected: 1; Actual: 0; Below
- **Ensure Safety**: Only add methods that are guaranteed not to throw exceptions during static initialization.

Check notice on line 101 in docs/rules/ECS1300.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1300.md#L101

Lists should be surrounded by blank lines
- **Fully Qualified Names**: Use the full namespace and class names to avoid conflicts and ensure the correct methods are treated as safe.

## When to suppress warnings

You may choose to suppress this warning if:

- The static field initialization is guaranteed not to throw exceptions.
- The initialization logic is simple and does not involve method calls or expressions that could fail.
- You have thoroughly tested the initialization and are confident in its safety.

### Suppress a warning

If you just want to suppress a single violation, add preprocessor directives to your source file to disable and then re-enable the rule.

```csharp
#pragma warning disable ECS1300
// The code that's violating the rule
#pragma warning restore ECS1300
```

To disable the rule for a file, folder, or project, set its severity to none in the [configuration file](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/configuration-files).

```ini
[*.cs]
dotnet_diagnostic.ECS1300.severity = none
```

## Example of a violation

### Description

A static field is initialized using a method that reads from a file. This method call could throw an `IOException` if the file does not exist or is inaccessible.

### Code

```csharp
public class MyClass
{
private static readonly string ConfigValue = LoadConfigValue();

private static string LoadConfigValue()
{
return System.IO.File.ReadAllText("config.txt");
}
}
```

## Example of how to fix

### Description

Check notice on line 151 in docs/rules/ECS1300.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1300.md#L151

Multiple headings with the same content

```csharp
public class MyClass
{
private static readonly string ConfigValue;

static MyClass()
{
try
{
ConfigValue = LoadConfigValue();
}
catch (IOException ex)
{
// Handle exception, possibly logging or setting a default value
ConfigValue = "DefaultConfig";
}
}

private static string LoadConfigValue()
{
return System.IO.File.ReadAllText("config.txt");
}
}
```

Alternatively, use `Lazy<T>` to defer the initialization:

```csharp
public class MyClass
{
private static readonly Lazy<string> ConfigValue = new Lazy<string>(() =>
{
return System.IO.File.ReadAllText("config.txt");
});

// Access ConfigValue.Value when needed
}
```
3 changes: 2 additions & 1 deletion effectivecsharpanalyzers.sln.DotSettings
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
<wpf:ResourceDictionary xml:space="preserve" xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml" xmlns:s="clr-namespace:System;assembly=mscorlib" xmlns:ss="urn:shemas-jetbrains-com:settings-storage-xaml" xmlns:wpf="http://schemas.microsoft.com/winfx/2006/xaml/presentation">
<s:Boolean x:Key="/Default/Environment/Filtering/ExcludeCoverageFilters/=EffectiveCSharp_002EAnalyzers_002EBenchmarks_003B_002A_003B_002A_003B_002A/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Formattable/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Formattable/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/UserDictionary/Words/=stringly/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
Rule ID | Category | Severity | Notes
--------|----------|----------|-------
ECS1200 | Maintainability | Info | PreferMemberInitializerAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1200.md)
ECS1300 | Initialization | Info | StaticClassMemberInitializationAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/ce4060555cece9d43160bfbef91e30dab9f0178b/docs/rules/ECS1300.md)
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
namespace EffectiveCSharp.Analyzers.Common;

Check failure on line 1 in src/EffectiveCSharp.Analyzers/Common/AnalyzerConfigurationHelper.cs

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

src/EffectiveCSharp.Analyzers/Common/AnalyzerConfigurationHelper.cs#L1

Provide an 'AssemblyVersion' attribute for assembly 'srcassembly.dll'.

internal static class AnalyzerConfigurationHelper
{
internal static List<string> GetConfiguredSafeItems(AnalyzerOptions options, string diagnosticId = DiagnosticIds.StaticClassMemberInitialization)

Check failure on line 5 in src/EffectiveCSharp.Analyzers/Common/AnalyzerConfigurationHelper.cs

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

src/EffectiveCSharp.Analyzers/Common/AnalyzerConfigurationHelper.cs#L5

The Cyclomatic Complexity of this method is 11 which is greater than 10 authorized.
{
List<string> safeItems = [];
AnalyzerConfigOptions configOptions = options.AnalyzerConfigOptionsProvider.GlobalOptions;

if (!configOptions.TryGetValue($"dotnet_diagnostic.{diagnosticId}.safe_items", out string? items))
{
return safeItems;
}

ReadOnlySpan<char> itemsSpan = items.AsSpan();
int start = 0;

while (start < itemsSpan.Length)
{
// Skip leading whitespace
while (start < itemsSpan.Length && char.IsWhiteSpace(itemsSpan[start]))
{
start++;
}

if (start >= itemsSpan.Length)
{
break;
}

// Find the end of the current item
int end = start;
while (end < itemsSpan.Length && itemsSpan[end] != ',')
{
end++;
}

// Trim trailing whitespace
int itemEnd = end;
while (itemEnd > start && char.IsWhiteSpace(itemsSpan[itemEnd - 1]))
{
itemEnd--;
}

if (itemEnd > start)
{
// Add the trimmed item to the list
safeItems.Add(itemsSpan.Slice(start, itemEnd - start).ToString());
}

start = end + 1;
}

return safeItems;
}
}
1 change: 1 addition & 0 deletions src/EffectiveCSharp.Analyzers/Common/Categories.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,5 @@ internal static class Categories
internal static readonly string Style = nameof(Style);
internal static readonly string Usage = nameof(Usage);
internal static readonly string Globalization = nameof(Globalization);
internal static readonly string Initialization = nameof(Initialization);
}
1 change: 1 addition & 0 deletions src/EffectiveCSharp.Analyzers/Common/DiagnosticIds.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,5 @@ internal static class DiagnosticIds
internal const string BeAwareOfValueTypeCopyInReferenceTypes = "ECS0901";
internal const string UseSpanInstead = "ECS1000";
internal const string PreferDeclarationInitializersToAssignmentStatement = "ECS1200";
internal const string StaticClassMemberInitialization = "ECS1300";
}
27 changes: 24 additions & 3 deletions src/EffectiveCSharp.Analyzers/Common/SemanticModelExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ internal static class SemanticModelExtensions
/// <param name="semanticModel">The semantic model.</param>
/// <param name="left">The left.</param>
/// <param name="right">The right.</param>
/// <returns><c>true</c> if <paramref name="left"/> is the same <see cref="OperationKind"/>, has a constant value, and the values are the same; otherwise, <c>false</c>.</returns>
/// <returns>
/// <see langword="true" /> if <paramref name="left"/> is the same <see cref="OperationKind"/>,
/// has a constant value, and the values are the same; otherwise, <see langword="false" />.
/// </returns>
internal static bool AreExpressionsEquivalent(this SemanticModel semanticModel, ExpressionSyntax left, ExpressionSyntax right)
{
IOperation? leftOperation = semanticModel.GetOperation(left);
Expand Down Expand Up @@ -163,6 +166,24 @@ internal static bool IsInitializedFromMethodCall(this SemanticModel semanticMode
return semanticModel.GetOperation(right) is IInvocationOperation;
}

/// <summary>
/// Determines if the <paramref name="node" /> provided has a constant value.
/// </summary>
/// <param name="semanticModel">The semantic model.</param>
/// <param name="node">The node.</param>
/// <param name="cancellationToken">The cancellation token.</param>
/// <returns>
/// Gets the constant value from the <paramref name="semanticModel" /> and <paramref name="node" />.
/// This produces an <see cref="Optional{T}" /> value with <see cref="Optional{T}.HasValue" /> set to
/// true and with <see cref="Optional{T}.Value" /> set to the constant. If the optional has a value,
/// <see langword="true" /> is returned; otherwise <see langword="false" />.
/// </returns>
internal static bool IsCompileTimeConstant(this SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken = default)
{
Optional<object?> constantValue = semanticModel.GetConstantValue(node, cancellationToken);
return constantValue.HasValue;
}

private static bool IsDefaultValue(object? value, ITypeSymbol fieldType)
{
if (value == null)
Expand Down Expand Up @@ -211,8 +232,8 @@ private static bool IsDefaultValue(object? value, ITypeSymbol fieldType)
/// <param name="memberReferenceOperation">The member reference operation to evaluate.</param>
/// <param name="right">The <see cref="ExpressionSyntax"/> to get the containing type.</param>
/// <returns>
/// <c>true</c> if the member referenced by <paramref name="memberReferenceOperation"/> is a non-static member
/// of the containing type; otherwise, <c>false</c>.
/// <see langword="true" /> if the member referenced by <paramref name="memberReferenceOperation"/> is a non-static member
/// of the containing type; otherwise, <see langword="false" />.
/// </returns>
/// <remarks>
/// This method checks if the member being referenced belongs to the same type that contains the operation
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@
<DevelopmentDependency>true</DevelopmentDependency>
</PropertyGroup>

<ItemGroup>
<ItemGroup>
<AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">
<_Parameter1>$(MSBuildProjectName).Test</_Parameter1>
</AssemblyAttribute>
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" PrivateAssets="all" />
</ItemGroup>

Expand Down
2 changes: 1 addition & 1 deletion src/EffectiveCSharp.Analyzers/SquiggleCop.Baseline.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -752,7 +752,7 @@
- {Id: RCS1198, Title: Avoid unnecessary boxing of value type, Category: Roslynator, DefaultSeverity: Warning, IsEnabledByDefault: false, EffectiveSeverities: [None], IsEverSuppressed: true}
- {Id: RCS1199, Title: Unnecessary null check, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
- {Id: RCS1200, Title: Call 'Enumerable.ThenBy' instead of 'Enumerable.OrderBy', Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
- {Id: RCS1201, Title: Use method chaining, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
- {Id: RCS1201, Title: Use method chaining, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: true}
- {Id: RCS1202, Title: Avoid NullReferenceException, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
- {Id: RCS1203, Title: Use AttributeUsageAttribute, Category: Roslynator, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
- {Id: RCS1204, Title: Use EventArgs.Empty, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
Expand Down
Loading