-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
ce40605
Add base code for ECS1300
rjmurillo 1077839
Fix static analysis warnings
rjmurillo 26a8d9a
Add word to shared dictionary
rjmurillo 04ecfba
Set correct rule suppression
rjmurillo 9b1fb5a
Add rules to dogfood project
rjmurillo 31a0189
Add capability to set symbols and types on safe list
rjmurillo 103878a
Reduce lookups from SemanticModel
rjmurillo 2d2f144
Plumb through CancellationToken
rjmurillo 3953b6c
Add support for RegEx and Version types
rjmurillo d4197e0
Add support for binary expressions (for divisible math)
rjmurillo 45f596e
Add basic benchmark for ECS1300
rjmurillo 1ae0715
Enable parallelism (was off for debugging)
rjmurillo 26583c1
Update to use a local immutable hashset of items rather than mutable …
rjmurillo 79cc6bc
Add symbol check for safety while analyzing collection init
rjmurillo 245fe96
Use Stringbuilder to reduce string overhead (slightly)
rjmurillo 2a9bb2e
Fix static analysis warnings
rjmurillo f39ba6f
Update SquiggleCop baselines
rjmurillo 9c05291
Add break from header to content
rjmurillo File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
# 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_methods` Option: Use the `dotnet_diagnostic.ECS1300.safe_methods` key to specify a comma-separated list of fully qualified method names that you want to treat as safe. | ||
|
||
```ini | ||
[*.cs] | ||
dotnet_diagnostic.ECS1300.safe_methods = System.MySafeClass.MySafeMethod, System.AnotherClass.AnotherSafeMethod | ||
``` | ||
|
||
>Note: Ensure that the method names are fully qualified, including the namespace and class name. | ||
|
||
#### Example Configuration | ||
|
||
Suppose you have methods in your codebase that you know are safe for static initialization: | ||
|
||
- `Utilities.GetDefaultSettings` | ||
- `Constants.GetMaxValue` | ||
|
||
You can add them to the safe methods list: | ||
|
||
```ini | ||
[*.cs] | ||
dotnet_diagnostic.ECS1300.safe_methods = Utilities.GetDefaultSettings, Constants.GetMaxValue | ||
``` | ||
|
||
#### Effect on the Analyzer | ||
rjmurillo marked this conversation as resolved.
Show resolved
Hide resolved
|
||
By configuring these methods 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 | ||
- **Ensure Safety**: Only add methods that are guaranteed not to throw exceptions during static initialization. | ||
- **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 | ||
|
||
```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 | ||
} | ||
``` | ||
rjmurillo marked this conversation as resolved.
Show resolved
Hide resolved
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -234,4 +234,10 @@ | |
|
||
return false; | ||
} | ||
|
||
internal static bool IsCompileTimeConstant(this SemanticModel semanticModel, ExpressionSyntax expression) | ||
Check failure on line 238 in src/EffectiveCSharp.Analyzers/Common/SemanticModelExtensions.cs
|
||
{ | ||
Optional<object?> constantValue = semanticModel.GetConstantValue(expression); | ||
return constantValue.HasValue; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.