Skip to content

Commit 9144e7c

Browse files
authored
Add Item 13: Use proper initialization for static class members (#66)
This pull request introduced a new Roslyn analyzer to enforce best practices for static member initializers and assignments in C#. The analyzer identifies cases where a member variable is static and complex, indicating it should move to the type's static constructor. ## Changes - Analyzer `StaticClassMemberInitializationAnalyzer`: - Detects `static` member variable assignment and initialization - Detects member variable assignment from methods, suggesting complex cases move to the type's static constructor - Code fix provider: None - The initialization logic of applications can be complex and a code fix provider can be equally complex to capture all cases of remediation. This implementation only shows areas of potential issue. - Tests: Added comprehensive unit tests for the new static class member initialization analyzer to ensure proper functionality and coverage. Includes benchmarking tests to evaluate the performance of the static member initialization analyzer. Closed #56
1 parent 97d2ddc commit 9144e7c

16 files changed

+1753
-8
lines changed

Diff for: docs/rules/ECS1300.md

+191
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,191 @@
1+
# ECS1300: Use Proper Initialization for Static Class Members
2+
3+
## Cause
4+
5+
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.
6+
7+
## Rule Description
8+
9+
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.
10+
11+
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.
12+
13+
## How to fix violations
14+
15+
- **Use a Static Constructor**: Move the complex initialization logic into a static constructor where you can handle exceptions and control execution flow.
16+
17+
```csharp
18+
public class MyClass
19+
{
20+
private static readonly string ConfigValue;
21+
22+
static MyClass()
23+
{
24+
ConfigValue = LoadConfigValue();
25+
}
26+
27+
private static string LoadConfigValue()
28+
{
29+
// Complex initialization logic
30+
return System.IO.File.ReadAllText("config.txt");
31+
}
32+
}
33+
```
34+
35+
- **Use `Lazy<T>` for Lazy Initialization**: Wrap the initialization logic in a `Lazy<T>` object to defer execution until the value is needed.
36+
37+
```csharp
38+
public class MyClass
39+
{
40+
private static readonly Lazy<string> ConfigValue = new Lazy<string>(LoadConfigValue);
41+
42+
private static string LoadConfigValue()
43+
{
44+
// Complex initialization logic
45+
return System.IO.File.ReadAllText("config.txt");
46+
}
47+
}
48+
```
49+
50+
## Extending the Rule with Safe Methods
51+
52+
### Customizing Safe Methods using Configuration
53+
54+
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.
55+
56+
### How to Configure Additional Safe Methods
57+
58+
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.
59+
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.
60+
61+
```ini
62+
[*.cs]
63+
dotnet_diagnostic.ECS1300.safe_items = System.MySafeClass.MySafeMethod, System.AnotherClass.AnotherSafeMethod
64+
```
65+
66+
>Note: Ensure that the method and property names are fully qualified, including the namespace and class name.
67+
68+
#### Example Configuration
69+
70+
Suppose you have methods and properties in your codebase that you know are safe for static initialization:
71+
72+
- `Contoso.Utilities.GetDefaultSettings`
73+
- `Contoso.Constants.GetMaxValue`
74+
- `Contoso.RecordManager.MaxValue`
75+
76+
You can add the fully qualified symbol to the safe list:
77+
78+
```ini
79+
[*.cs]
80+
dotnet_diagnostic.ECS1300.safe_methods = Contoso.Utilities.GetDefaultSettings, Contoso.Constants.GetMaxValue, Contoso.RecordManager.MaxValue
81+
```
82+
83+
#### Effect on the Analyzer
84+
By configuring these methods and properties as safe, the analyzer will no longer report diagnostics for static fields initialized using them:
85+
86+
```csharp
87+
public class MyClass
88+
{
89+
// No diagnostic will be reported for this initialization
90+
private static readonly Settings DefaultSettings = Utilities.GetDefaultSettings();
91+
}
92+
```
93+
94+
### When to Use This Option
95+
96+
- **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.
97+
- **Custom Utility Methods**: For utility methods in your codebase that are deterministic and exception-safe.
98+
- **Performance Considerations**: When you prefer direct initialization for performance reasons and are confident in the safety of the methods used.
99+
100+
### Precautions
101+
102+
- **Ensure Safety**: Only add methods that are guaranteed not to throw exceptions during static initialization.
103+
- **Fully Qualified Names**: Use the full namespace and class names to avoid conflicts and ensure the correct methods are treated as safe.
104+
105+
## When to suppress warnings
106+
107+
You may choose to suppress this warning if:
108+
109+
- The static field initialization is guaranteed not to throw exceptions.
110+
- The initialization logic is simple and does not involve method calls or expressions that could fail.
111+
- You have thoroughly tested the initialization and are confident in its safety.
112+
113+
### Suppress a warning
114+
115+
If you just want to suppress a single violation, add preprocessor directives to your source file to disable and then re-enable the rule.
116+
117+
```csharp
118+
#pragma warning disable ECS1300
119+
// The code that's violating the rule
120+
#pragma warning restore ECS1300
121+
```
122+
123+
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).
124+
125+
```ini
126+
[*.cs]
127+
dotnet_diagnostic.ECS1300.severity = none
128+
```
129+
130+
## Example of a violation
131+
132+
### Description
133+
134+
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.
135+
136+
### Code
137+
138+
```csharp
139+
public class MyClass
140+
{
141+
private static readonly string ConfigValue = LoadConfigValue();
142+
143+
private static string LoadConfigValue()
144+
{
145+
return System.IO.File.ReadAllText("config.txt");
146+
}
147+
}
148+
```
149+
150+
## Example of how to fix
151+
152+
### Description
153+
154+
```csharp
155+
public class MyClass
156+
{
157+
private static readonly string ConfigValue;
158+
159+
static MyClass()
160+
{
161+
try
162+
{
163+
ConfigValue = LoadConfigValue();
164+
}
165+
catch (IOException ex)
166+
{
167+
// Handle exception, possibly logging or setting a default value
168+
ConfigValue = "DefaultConfig";
169+
}
170+
}
171+
172+
private static string LoadConfigValue()
173+
{
174+
return System.IO.File.ReadAllText("config.txt");
175+
}
176+
}
177+
```
178+
179+
Alternatively, use `Lazy<T>` to defer the initialization:
180+
181+
```csharp
182+
public class MyClass
183+
{
184+
private static readonly Lazy<string> ConfigValue = new Lazy<string>(() =>
185+
{
186+
return System.IO.File.ReadAllText("config.txt");
187+
});
188+
189+
// Access ConfigValue.Value when needed
190+
}
191+
```

Diff for: effectivecsharpanalyzers.sln.DotSettings

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
11
<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">
22
<s:Boolean x:Key="/Default/Environment/Filtering/ExcludeCoverageFilters/=EffectiveCSharp_002EAnalyzers_002EBenchmarks_003B_002A_003B_002A_003B_002A/@EntryIndexedValue">True</s:Boolean>
3-
<s:Boolean x:Key="/Default/UserDictionary/Words/=Formattable/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
3+
<s:Boolean x:Key="/Default/UserDictionary/Words/=Formattable/@EntryIndexedValue">True</s:Boolean>
4+
<s:Boolean x:Key="/Default/UserDictionary/Words/=stringly/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>

Diff for: src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md

+1
Original file line numberDiff line numberDiff line change
@@ -6,3 +6,4 @@
66
Rule ID | Category | Severity | Notes
77
--------|----------|----------|-------
88
ECS1200 | Maintainability | Info | PreferMemberInitializerAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1200.md)
9+
ECS1300 | Initialization | Info | StaticClassMemberInitializationAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/ce4060555cece9d43160bfbef91e30dab9f0178b/docs/rules/ECS1300.md)
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
namespace EffectiveCSharp.Analyzers.Common;
2+
3+
internal static class AnalyzerConfigurationHelper
4+
{
5+
internal static List<string> GetConfiguredSafeItems(AnalyzerOptions options, string diagnosticId = DiagnosticIds.StaticClassMemberInitialization)
6+
{
7+
List<string> safeItems = [];
8+
AnalyzerConfigOptions configOptions = options.AnalyzerConfigOptionsProvider.GlobalOptions;
9+
10+
if (!configOptions.TryGetValue($"dotnet_diagnostic.{diagnosticId}.safe_items", out string? items))
11+
{
12+
return safeItems;
13+
}
14+
15+
ReadOnlySpan<char> itemsSpan = items.AsSpan();
16+
int start = 0;
17+
18+
while (start < itemsSpan.Length)
19+
{
20+
// Skip leading whitespace
21+
while (start < itemsSpan.Length && char.IsWhiteSpace(itemsSpan[start]))
22+
{
23+
start++;
24+
}
25+
26+
if (start >= itemsSpan.Length)
27+
{
28+
break;
29+
}
30+
31+
// Find the end of the current item
32+
int end = start;
33+
while (end < itemsSpan.Length && itemsSpan[end] != ',')
34+
{
35+
end++;
36+
}
37+
38+
// Trim trailing whitespace
39+
int itemEnd = end;
40+
while (itemEnd > start && char.IsWhiteSpace(itemsSpan[itemEnd - 1]))
41+
{
42+
itemEnd--;
43+
}
44+
45+
if (itemEnd > start)
46+
{
47+
// Add the trimmed item to the list
48+
safeItems.Add(itemsSpan.Slice(start, itemEnd - start).ToString());
49+
}
50+
51+
start = end + 1;
52+
}
53+
54+
return safeItems;
55+
}
56+
}

Diff for: src/EffectiveCSharp.Analyzers/Common/Categories.cs

+1
Original file line numberDiff line numberDiff line change
@@ -7,4 +7,5 @@ internal static class Categories
77
internal static readonly string Style = nameof(Style);
88
internal static readonly string Usage = nameof(Usage);
99
internal static readonly string Globalization = nameof(Globalization);
10+
internal static readonly string Initialization = nameof(Initialization);
1011
}

Diff for: src/EffectiveCSharp.Analyzers/Common/DiagnosticIds.cs

+1
Original file line numberDiff line numberDiff line change
@@ -16,4 +16,5 @@ internal static class DiagnosticIds
1616
internal const string BeAwareOfValueTypeCopyInReferenceTypes = "ECS0901";
1717
internal const string UseSpanInstead = "ECS1000";
1818
internal const string PreferDeclarationInitializersToAssignmentStatement = "ECS1200";
19+
internal const string StaticClassMemberInitialization = "ECS1300";
1920
}

Diff for: src/EffectiveCSharp.Analyzers/Common/SemanticModelExtensions.cs

+24-3
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,10 @@ internal static class SemanticModelExtensions
88
/// <param name="semanticModel">The semantic model.</param>
99
/// <param name="left">The left.</param>
1010
/// <param name="right">The right.</param>
11-
/// <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>
11+
/// <returns>
12+
/// <see langword="true" /> if <paramref name="left"/> is the same <see cref="OperationKind"/>,
13+
/// has a constant value, and the values are the same; otherwise, <see langword="false" />.
14+
/// </returns>
1215
internal static bool AreExpressionsEquivalent(this SemanticModel semanticModel, ExpressionSyntax left, ExpressionSyntax right)
1316
{
1417
IOperation? leftOperation = semanticModel.GetOperation(left);
@@ -163,6 +166,24 @@ internal static bool IsInitializedFromMethodCall(this SemanticModel semanticMode
163166
return semanticModel.GetOperation(right) is IInvocationOperation;
164167
}
165168

169+
/// <summary>
170+
/// Determines if the <paramref name="node" /> provided has a constant value.
171+
/// </summary>
172+
/// <param name="semanticModel">The semantic model.</param>
173+
/// <param name="node">The node.</param>
174+
/// <param name="cancellationToken">The cancellation token.</param>
175+
/// <returns>
176+
/// Gets the constant value from the <paramref name="semanticModel" /> and <paramref name="node" />.
177+
/// This produces an <see cref="Optional{T}" /> value with <see cref="Optional{T}.HasValue" /> set to
178+
/// true and with <see cref="Optional{T}.Value" /> set to the constant. If the optional has a value,
179+
/// <see langword="true" /> is returned; otherwise <see langword="false" />.
180+
/// </returns>
181+
internal static bool IsCompileTimeConstant(this SemanticModel semanticModel, SyntaxNode node, CancellationToken cancellationToken = default)
182+
{
183+
Optional<object?> constantValue = semanticModel.GetConstantValue(node, cancellationToken);
184+
return constantValue.HasValue;
185+
}
186+
166187
private static bool IsDefaultValue(object? value, ITypeSymbol fieldType)
167188
{
168189
if (value == null)
@@ -211,8 +232,8 @@ private static bool IsDefaultValue(object? value, ITypeSymbol fieldType)
211232
/// <param name="memberReferenceOperation">The member reference operation to evaluate.</param>
212233
/// <param name="right">The <see cref="ExpressionSyntax"/> to get the containing type.</param>
213234
/// <returns>
214-
/// <c>true</c> if the member referenced by <paramref name="memberReferenceOperation"/> is a non-static member
215-
/// of the containing type; otherwise, <c>false</c>.
235+
/// <see langword="true" /> if the member referenced by <paramref name="memberReferenceOperation"/> is a non-static member
236+
/// of the containing type; otherwise, <see langword="false" />.
216237
/// </returns>
217238
/// <remarks>
218239
/// This method checks if the member being referenced belongs to the same type that contains the operation

Diff for: src/EffectiveCSharp.Analyzers/EffectiveCSharp.Analyzers.csproj

+7-1
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,13 @@
2424
<DevelopmentDependency>true</DevelopmentDependency>
2525
</PropertyGroup>
2626

27-
<ItemGroup>
27+
<ItemGroup>
28+
<AssemblyAttribute Include="System.Runtime.CompilerServices.InternalsVisibleTo">
29+
<_Parameter1>$(MSBuildProjectName).Test</_Parameter1>
30+
</AssemblyAttribute>
31+
</ItemGroup>
32+
33+
<ItemGroup>
2834
<PackageReference Include="Microsoft.CodeAnalysis.CSharp.Workspaces" PrivateAssets="all" />
2935
</ItemGroup>
3036

Diff for: src/EffectiveCSharp.Analyzers/SquiggleCop.Baseline.yaml

+1-1
Original file line numberDiff line numberDiff line change
@@ -752,7 +752,7 @@
752752
- {Id: RCS1198, Title: Avoid unnecessary boxing of value type, Category: Roslynator, DefaultSeverity: Warning, IsEnabledByDefault: false, EffectiveSeverities: [None], IsEverSuppressed: true}
753753
- {Id: RCS1199, Title: Unnecessary null check, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
754754
- {Id: RCS1200, Title: Call 'Enumerable.ThenBy' instead of 'Enumerable.OrderBy', Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
755-
- {Id: RCS1201, Title: Use method chaining, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
755+
- {Id: RCS1201, Title: Use method chaining, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: true}
756756
- {Id: RCS1202, Title: Avoid NullReferenceException, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}
757757
- {Id: RCS1203, Title: Use AttributeUsageAttribute, Category: Roslynator, DefaultSeverity: Warning, IsEnabledByDefault: true, EffectiveSeverities: [Error], IsEverSuppressed: false}
758758
- {Id: RCS1204, Title: Use EventArgs.Empty, Category: Roslynator, DefaultSeverity: Note, IsEnabledByDefault: true, EffectiveSeverities: [Note], IsEverSuppressed: false}

0 commit comments

Comments
 (0)