Skip to content

Add Item 12: Prefer Member Initializers to Assignment Statements #63

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 54 commits into from
Sep 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
c06ff97
started project. Must actually write analyzer
rorozcov Jul 30, 2024
55ca307
very rough draft. ugly code. need to test
rorozcov Jul 31, 2024
4dd9f2b
analyzer running correctly but off by one
rorozcov Aug 2, 2024
aaae648
works except that source file error ocurring
rorozcov Aug 2, 2024
74e8931
passed first test. Issue was in the analyzer instance being reused in…
rorozcov Aug 3, 2024
31cd29b
massive update. All tests passing except for initializing in declarat…
rorozcov Aug 4, 2024
8f352f7
analyzer passing all tests!
rorozcov Aug 5, 2024
cc88272
updated diagnostic ids. Implemented first part of code provider. Fixe…
rorozcov Aug 7, 2024
cba51c9
provider mostly working. Unexpected iterations issue + diagnostics ar…
rorozcov Aug 12, 2024
12662a8
fixed final bug. All tests pass. V1 is done! Time to optimize
rorozcov Aug 14, 2024
d886813
Added new edge case. Improved runtime and memory efficiency. Must cle…
rorozcov Aug 16, 2024
b07c606
Added untested benchmark
rorozcov Aug 17, 2024
b877bd3
cleaned up analyzer
rorozcov Aug 17, 2024
3761ec5
renaming files and classes
rorozcov Aug 20, 2024
52b2850
Update src/EffectiveCSharp.Analyzers/PreferDeclarationInitializersToA…
rorozcov Aug 21, 2024
58d7878
Update src/EffectiveCSharp.Analyzers/PreferDeclarationInitializersToA…
rorozcov Aug 21, 2024
b678fdb
removed IDictionary. Added back lines in tests.csproj
rorozcov Aug 21, 2024
ef99b90
finished a comment
rorozcov Aug 21, 2024
ceaa593
Add additional tests for corner cases
rjmurillo Aug 21, 2024
d2de257
Merge remote-tracking branch 'github-desktop-rorozcov/main' into pr/62
rjmurillo Aug 21, 2024
dde05f8
addressed comments and improved code fix provider
rorozcov Aug 22, 2024
d6a903b
Added logic to handle tests: FieldInitializedWithMethodDirectly and F…
rorozcov Aug 22, 2024
570bc93
handled all remaining tests except for one in question
rorozcov Aug 23, 2024
e3cdf69
Merge remote-tracking branch 'upstream/main'
rorozcov Aug 27, 2024
0b13830
changed ctor overload test to not expect diagnostic because it is a v…
rorozcov Aug 27, 2024
97c51a4
removed GetSymbolInfo calls since they were too expensive. Reduced ru…
rorozcov Aug 27, 2024
2c031a2
fixed all alerts except for boxing alerts
rorozcov Aug 28, 2024
85b9dfc
WIP
rjmurillo Aug 28, 2024
10cd64a
WIP: Support init via method
rjmurillo Aug 28, 2024
addd00a
Update IsDefaultValue handling
rjmurillo Aug 28, 2024
1d08ebd
WIP: Cover redundant field initialization
rjmurillo Aug 28, 2024
25295cf
Update foreach to for loop to reduce GC
rjmurillo Aug 29, 2024
72dd42e
Fix spacing around for statements
rjmurillo Aug 30, 2024
bffb223
Remove additional foreach to reduce gc
rjmurillo Aug 30, 2024
14a8a08
Support detection of custom types being the same as default
rjmurillo Aug 30, 2024
c6cc54e
Allow `default` for fields
rjmurillo Aug 30, 2024
aa6c00b
Cover condition where field is initialized with parameter but paramet…
rjmurillo Aug 30, 2024
b876bad
Support instances where types are created using values defined elsewh…
rjmurillo Aug 30, 2024
b7597af
Clean up some static analysis warnings
rjmurillo Aug 30, 2024
431c5df
Add helper method as extension
rjmurillo Aug 30, 2024
df4f185
Add documentation for ECS1200
rjmurillo Aug 30, 2024
6d8dd69
Refactor helper methods to extension methods
rjmurillo Aug 30, 2024
0d16a21
Clean up tests
rjmurillo Aug 30, 2024
a3d8d98
Fix static analysis warnings
rjmurillo Aug 30, 2024
afd9108
Add CA1805 to configuration and update documentation
rjmurillo Sep 10, 2024
c31c0c8
Update analyzer shipping documentation manifest
rjmurillo Sep 10, 2024
54b7fc4
Remove duplicate implementation for analyzer, consolidate unit tests
rjmurillo Sep 10, 2024
a738a84
Remove unused CreateDiagnostic extension methods
rjmurillo Sep 10, 2024
af68eff
Update CreateDiagnostic extensions for inlining
rjmurillo Sep 10, 2024
5800482
Fix code review comments
rjmurillo Sep 10, 2024
24d3aae
Add new line at EOF to align with static analysis guidelines
rjmurillo Sep 10, 2024
984e3cc
Update suppression phrasing to avoid two adverbs in a row
rjmurillo Sep 10, 2024
e91a232
Update SquiggleCop baselines
rjmurillo Sep 10, 2024
26b5491
Update SquiggleCop baselines
rjmurillo Sep 10, 2024
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
2 changes: 1 addition & 1 deletion Directory.Packages.props
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@
- https://github.com/dotnet/roslyn/blob/main/docs/wiki/NuGet-packages.md
- https://learn.microsoft.com/en-us/visualstudio/productinfo/vs-servicing
-->

<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.3.1" />
</ItemGroup>
<ItemGroup>
<PackageVersion Include="BenchmarkDotNet" Version="0.13.12" />
<PackageVersion Include="BenchmarkDotNet.Diagnostics.dotTrace" Version="0.13.12" />
<PackageVersion Include="GetPackFromProject" Version="1.0.6" />
<PackageVersion Include="Nerdbank.GitVersioning" Version="3.6.139" />
<PackageVersion Include="System.CommandLine" Version="2.0.0-beta1.21216.1" />
Expand Down
89 changes: 89 additions & 0 deletions docs/rules/ECS1200.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
# ECS1200: Prefer member initializers to assignment statements

This rule is described in detail in [Effective C#: 50 Specific Ways to Improve your C#](https://www.oreilly.com/library/view/effective-c-50/9780134579290/).

## Cause

The rule identifies fields in a type that are being initialized both at the point of declaration and again within a constructor. This can lead to redundancy and potential confusion, as the field is unnecessarily re-initialized, which could also introduce bugs if the constructor logic changes.

## Rule description

Fields should be initialized at the point of declaration rather than within constructors when the value being assigned does not depend on any constructor parameters or instance members. This ensures consistency, reduces redundancy, and avoids potential issues with fields being out of sync.

The rule will trigger when it detects that a field is initialized both at the declaration and again within a constructor with the same or a redundant value. It encourages developers to consolidate this initialization into a single place—__the field declaration__—whenever possible.

## How to fix violations

To fix violations, remove the redundant initialization from the constructor if the field is already initialized at the point of declaration with the same value. If the initialization must depend on constructor logic, ensure that it's not duplicated unnecessarily.

## When to suppress warnings

It is always safe to suppress the warning, as it highlights potentially unnecessary code and work that may be avoided.

### Suppress a warning

If the initialization in the constructor is necessary due to specific requirements or conditions, you might want to suppress the warning.

#pragma warning disable ECS1200

Check notice on line 27 in docs/rules/ECS1200.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1200.md#L27

No space after hash on atx style heading
// The code that's violating the rule
#pragma warning restore ECS1200

Check notice on line 29 in docs/rules/ECS1200.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1200.md#L29

No space after hash on atx style heading
Comment on lines +27 to +29
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space after the hash symbol in the ATX-style headings.

The static analysis tools suggest adding a space after the hash symbol in the ATX-style headings at lines 27 and 29 to follow the correct Markdown formatting.

Apply this diff to fix the formatting issue:

-#pragma warning disable ECS1200
+# pragma warning disable ECS1200
 // The code that's violating the rule
-#pragma warning restore ECS1200
+# pragma warning restore ECS1200

Committable suggestion was skipped due to low confidence.

Tools
Markdownlint

27-27: null
No space after hash on atx style heading

(MD018, no-missing-space-atx)


29-29: null
No space after hash on atx style heading

(MD018, no-missing-space-atx)

GitHub Check: Codacy Static Code Analysis

[notice] 27-27: docs/rules/ECS1200.md#L27
No space after hash on atx style heading


[notice] 29-29: docs/rules/ECS1200.md#L29
No space after hash on atx style heading


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.ECS1200.severity = none
```

## Example of a violation

### Description

The following example shows a violation where a field `listOfString` is initialized both at its declaration and within the default constructor.

### Code

```csharp
public class MyClass
{
private List<string> listOfString = new List<string>();

public MyClass()
{
listOfString = new List<string>();
}

public MyClass(int size)
{
listOfString = new List<string>(size);
}
}
```

## Example of how to fix

### Description

Check notice on line 65 in docs/rules/ECS1200.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1200.md#L65

Multiple headings with the same content

To fix the violation, remove the redundant initialization from the default constructor, as the field `listOfString` is already initialized with the same value at the point of declaration.

### Code

Check notice on line 69 in docs/rules/ECS1200.md

View check run for this annotation

Codacy Production / Codacy Static Code Analysis

docs/rules/ECS1200.md#L69

Multiple headings with the same content
Comment on lines +65 to +69
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using the same content for multiple headings.

The static analysis tool suggests avoiding using the same content for multiple headings, as it may be confusing for readers.

Consider using different content for the headings to make them more descriptive and distinguishable. For example:

-### Description
+### Violation Example
 
 The following example shows a violation where a field `listOfString` is initialized both at its declaration and within the default constructor.
 
-### Code
+### Violation Code
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
### Description
To fix the violation, remove the redundant initialization from the default constructor, as the field `listOfString` is already initialized with the same value at the point of declaration.
### Code
### Violation Example
To fix the violation, remove the redundant initialization from the default constructor, as the field `listOfString` is already initialized with the same value at the point of declaration.
### Violation Code
Tools
GitHub Check: Codacy Static Code Analysis

[notice] 65-65: docs/rules/ECS1200.md#L65
Multiple headings with the same content


[notice] 69-69: docs/rules/ECS1200.md#L69
Multiple headings with the same content


```csharp
public class MyClass
{
private List<string> listOfString = new List<string>();

public MyClass()
{
}

public MyClass(int size)
{
listOfString = new List<string>(size);
}
}
```

## Related Rules

[CA1805: Do not initialize unnecessarily](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1805)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a single newline character at the end of the file.

The static analysis tool suggests adding a single newline character at the end of the file to follow the Markdown formatting conventions.

Add a single newline character at the end of the file.

Tools
Markdownlint

89-89: null
Files should end with a single newline character

(MD047, single-trailing-newline)

2 changes: 1 addition & 1 deletion global.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@
"msbuild-sdks": {
"DotNet.ReproducibleBuilds.Isolated": "1.2.4"
}
}
}
6 changes: 6 additions & 0 deletions src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
; Unshipped analyzer release
; https://github.com/dotnet/roslyn-analyzers/blob/main/src/Microsoft.CodeAnalysis.Analyzers/ReleaseTrackingAnalyzers.Help.md

### New Rules

Rule ID | Category | Severity | Notes
--------|----------|----------|-------
ECS1200 | Maintainability | Info | PreferMemberInitializerAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1200.md)
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,10 @@ csharp_style_prefer_pattern_matching = true:warning
#
# Effective C# Item #10 - Use the new modifier only to react to base class Updates
dotnet_diagnostic.CA1061.severity = warning

# Title: : Do not initialize unnecessarily
# Category : Performance
# Help Link: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1805
#
# Effective C# Item #12 - Prefer member initializers to assignment statements
dotnet_diagnostic.CA1805.severity = warning
42 changes: 7 additions & 35 deletions src/EffectiveCSharp.Analyzers/Common/DiagnosticExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,42 +1,11 @@
namespace EffectiveCSharp.Analyzers.Common;
using System.Runtime.CompilerServices;

namespace EffectiveCSharp.Analyzers.Common;

internal static class DiagnosticExtensions
{
[DebuggerStepThrough]
internal static Diagnostic CreateDiagnostic(
this SyntaxNode node,
DiagnosticDescriptor rule,
params object?[]? messageArgs)
=> node.CreateDiagnostic(rule, properties: null, messageArgs);

[DebuggerStepThrough]
internal static Diagnostic CreateDiagnostic(
this SyntaxNode node,
DiagnosticDescriptor rule,
ImmutableDictionary<string, string?>? properties,
params object?[]? messageArgs)
=> node.CreateDiagnostic(
rule,
additionalLocations: null,
properties,
messageArgs);

[DebuggerStepThrough]
internal static Diagnostic CreateDiagnostic(
this SyntaxNode node,
DiagnosticDescriptor rule,
IEnumerable<Location>? additionalLocations,
ImmutableDictionary<string, string?>? properties,
params object?[]? messageArgs)
=> node
.GetLocation()
.CreateDiagnostic(
rule: rule,
additionalLocations: additionalLocations,
properties: properties,
messageArgs: messageArgs);

[DebuggerStepThrough]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static Diagnostic CreateDiagnostic(
this Location location,
DiagnosticDescriptor rule,
Expand All @@ -47,6 +16,8 @@ internal static Diagnostic CreateDiagnostic(
properties: null,
messageArgs: messageArgs);

[DebuggerStepThrough]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static Diagnostic CreateDiagnostic(
this Location location,
DiagnosticDescriptor rule,
Expand All @@ -59,6 +30,7 @@ internal static Diagnostic CreateDiagnostic(
messageArgs: messageArgs);

[DebuggerStepThrough]
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static Diagnostic CreateDiagnostic(
this Location location,
DiagnosticDescriptor rule,
Expand Down
1 change: 1 addition & 0 deletions src/EffectiveCSharp.Analyzers/Common/DiagnosticIds.cs
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@ internal static class DiagnosticIds
internal const string MinimizeBoxingUnboxing = "ECS0900";
internal const string BeAwareOfValueTypeCopyInReferenceTypes = "ECS0901";
internal const string UseSpanInstead = "ECS1000";
internal const string PreferDeclarationInitializersToAssignmentStatement = "ECS1200";
}
Loading
Loading