-
Notifications
You must be signed in to change notification settings - Fork 1
Adding Item 12 Analyzer, Provider and Tests #62
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
Changes from all commits
c06ff97
55ca307
4dd9f2b
aaae648
74e8931
31cd29b
8f352f7
cc88272
cba51c9
12662a8
d886813
b07c606
b877bd3
3761ec5
52b2850
58d7878
b678fdb
ef99b90
ceaa593
d2de257
dde05f8
d6a903b
570bc93
e3cdf69
0b13830
97c51a4
2c031a2
6d7a03a
2636b70
c11f9cf
24d0ddd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,17 +16,18 @@ | |
- https://github.com/dotnet/roslyn/blob/main/docs/wiki/NuGet-packages.md | ||
- https://learn.microsoft.com/en-us/visualstudio/productinfo/vs-servicing | ||
--> | ||
|
||
<PackageVersion Include="BenchmarkDotNet.Diagnostics.dotMemory" Version="0.14.0" /> | ||
<PackageVersion Include="BenchmarkDotNet.Diagnostics.dotTrace" Version="0.14.0" /> | ||
<PackageVersion Include="Microsoft.CodeAnalysis.CSharp.Workspaces" Version="4.3.1" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<PackageVersion Include="BenchmarkDotNet" Version="0.13.12" /> | ||
<PackageVersion Include="BenchmarkDotNet" Version="0.14.0" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why'd this get bumped? |
||
<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" /> | ||
<PackageVersion Include="System.CommandLine.Rendering" Version="2.0.0-beta1.20074.1" /> | ||
<PackageVersion Include="Newtonsoft.Json" Version="13.0.3" /> | ||
<PackageVersion Include="Perfolizer" Version="0.2.1" /> | ||
<PackageVersion Include="Perfolizer" Version="0.3.17" /> | ||
<PackageVersion Include="Microsoft.Extensions.Logging" Version="8.0.0" /> | ||
<PackageVersion Include="Microsoft.Diagnostics.Tracing.TraceEvent" Version="3.1.13" /> | ||
</ItemGroup> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,4 +6,4 @@ | |
"msbuild-sdks": { | ||
"DotNet.ReproducibleBuilds.Isolated": "1.2.4" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,11 @@ | ||
; 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 | PreferDeclarationInitializersToAssignmentStatement, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1200.md) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are having these rules confusing to end users? They can't be distilled down into fewer (or one)? |
||
ECS1201 | Maintainability | Info | PreferDeclarationInitializersExceptNullOrZero, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1201.md) | ||
ECS1202 | Maintainability | Info | PreferDeclarationInitializersExceptWhenVaryingInitializations, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1202.md) | ||
ECS1203 | Maintainability | Info | PreferDeclarationInitializersWhenNoInitializationPresent, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/97c51a41b53d059cfcd06f0c8ce5ab178b070e6b/docs/rules/ECS1203.md) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,4 +15,8 @@ internal static class DiagnosticIds | |
internal const string MinimizeBoxingUnboxing = "ECS0900"; | ||
internal const string BeAwareOfValueTypeCopyInReferenceTypes = "ECS0901"; | ||
internal const string UseSpanInstead = "ECS1000"; | ||
internal const string PreferDeclarationInitializersToAssignmentStatement = "ECS1200"; | ||
internal const string PreferDeclarationInitializersExceptNullOrZero = "ECS1201"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In other cases this is covered by CA1805: Do not initialize unnecessarily. In those cases we want to instead update our global configuration to enable that analyzer. If there is a gap in that analyzer, then we should file an issue for triage and contribute a fix. There is also some cases of this is covered in Roslynator RCS1129 (code, docs). |
||
internal const string PreferDeclarationInitializersExceptWhenVaryingInitializations = "ECS1202"; | ||
internal const string PreferDeclarationInitializersWhenNoInitializationPresent = "ECS1203"; | ||
} |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,194 @@ | ||
namespace EffectiveCSharp.Analyzers; | ||
|
||
/// <summary> | ||
/// A <see cref="CodeFixProvider"/> that provides a code fix for the <see cref="PreferDeclarationInitializersToAssignmentStatementsAnalyzer"/>. | ||
/// </summary> | ||
/// <seealso cref="Microsoft.CodeAnalysis.CodeFixes.CodeFixProvider" /> | ||
[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(PreferDeclarationInitializersToAssignmentStatementsCodeFixProvider))] | ||
[Shared] | ||
public class PreferDeclarationInitializersToAssignmentStatementsCodeFixProvider : CodeFixProvider | ||
{ | ||
private static readonly string EquivalenceKey = "ECS1200CodeFix"; | ||
|
||
/// <inheritdoc /> | ||
public override ImmutableArray<string> FixableDiagnosticIds => ImmutableArray.Create( | ||
DiagnosticIds.PreferDeclarationInitializersToAssignmentStatement, | ||
DiagnosticIds.PreferDeclarationInitializersExceptNullOrZero, | ||
DiagnosticIds.PreferDeclarationInitializersExceptWhenVaryingInitializations, | ||
DiagnosticIds.PreferDeclarationInitializersWhenNoInitializationPresent); | ||
|
||
/// <inheritdoc /> | ||
public override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; | ||
|
||
/// <inheritdoc /> | ||
public override async Task RegisterCodeFixesAsync(CodeFixContext context) | ||
{ | ||
SyntaxNode? root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); | ||
SemanticModel? semanticModel = await context.Document.GetSemanticModelAsync(context.CancellationToken).ConfigureAwait(false); | ||
|
||
if (root is null || semanticModel is null) | ||
{ | ||
return; | ||
} | ||
|
||
foreach (Diagnostic diagnostic in context.Diagnostics) | ||
{ | ||
TextSpan diagnosticSpan = diagnostic.Location.SourceSpan; | ||
|
||
SyntaxNode diagnosticNode = root.FindNode(diagnosticSpan); | ||
|
||
switch (diagnostic.Id) | ||
{ | ||
case DiagnosticIds.PreferDeclarationInitializersToAssignmentStatement: | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: "Initialize in field declaration instead", | ||
createChangedSolution: cancellationToken => ReplaceAssignmentsWithFieldDeclarationInitializerAsync(context, semanticModel, diagnosticNode, cancellationToken), | ||
equivalenceKey: EquivalenceKey), | ||
context.Diagnostics); | ||
break; | ||
case DiagnosticIds.PreferDeclarationInitializersExceptNullOrZero: | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: "Do not initialize to null or zero", | ||
createChangedSolution: cancellationToken => EnforceFieldIsNotInitializedAsync(context.Document, diagnosticNode, cancellationToken), | ||
equivalenceKey: EquivalenceKey), | ||
diagnostic); | ||
break; | ||
case DiagnosticIds.PreferDeclarationInitializersExceptWhenVaryingInitializations: | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: "Do not use field initializer when varying initializations", | ||
createChangedSolution: cancellationToken => EnforceFieldIsNotInitializedAsync(context.Document, diagnosticNode, cancellationToken), | ||
equivalenceKey: EquivalenceKey), | ||
diagnostic); | ||
break; | ||
case DiagnosticIds.PreferDeclarationInitializersWhenNoInitializationPresent: | ||
context.RegisterCodeFix( | ||
CodeAction.Create( | ||
title: "Use initializer in field declaration", | ||
createChangedSolution: cancellationToken => EnforceFieldDeclarationInitializationAsync(context.Document, semanticModel, diagnosticNode, cancellationToken), | ||
equivalenceKey: EquivalenceKey), | ||
diagnostic); | ||
break; | ||
default: | ||
continue; | ||
} | ||
} | ||
} | ||
Comment on lines
+24
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main Async Registration Method The Consider refactoring each case in the switch statement into separate methods to improve readability and maintainability. |
||
|
||
private static async Task<Solution> ReplaceAssignmentsWithFieldDeclarationInitializerAsync(CodeFixContext context, SemanticModel semanticModel, SyntaxNode diagnosticNode, CancellationToken cancellationToken) | ||
Check failure on line 80 in src/EffectiveCSharp.Analyzers/PreferDeclarationInitializersToAssignmentStatementsCodeFixProvider.cs
|
||
{ | ||
SyntaxNode? root = await context.Document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
if (root is null | ||
Check failure on line 84 in src/EffectiveCSharp.Analyzers/PreferDeclarationInitializersToAssignmentStatementsCodeFixProvider.cs
|
||
|| diagnosticNode.Parent is not AssignmentExpressionSyntax assignmentExpression // The diagnostic should be an expression statement | ||
|| assignmentExpression.Parent is not ExpressionStatementSyntax expressionStatementSyntax | ||
|| semanticModel.GetSymbolInfo(assignmentExpression.Left, context.CancellationToken).Symbol is not IFieldSymbol fieldSymbol // The left side of the assignment should be a field | ||
|| fieldSymbol.DeclaringSyntaxReferences.Length != 1 // The field should have a single declaration | ||
|| fieldSymbol.DeclaringSyntaxReferences[0] is not { } syntaxReference // Let's get a reference to the field declaration | ||
|| (await syntaxReference.GetSyntaxAsync(cancellationToken).ConfigureAwait(false)).Parent?.Parent is not FieldDeclarationSyntax existingFieldDeclaration // Let's get the field declaration | ||
|| root.RemoveNode(expressionStatementSyntax, SyntaxRemoveOptions.KeepNoTrivia) is not { } newRoot // Let's remove the assignment statement since we've found the field and have an initializer | ||
|| CreateFieldDeclaration(existingFieldDeclaration, GetInitializerFromExpressionSyntax(assignmentExpression.Right)) is not { } fieldDeclarationWithNewInitializer) | ||
{ | ||
// If any of the above conditions are not met, return the current solution | ||
return context.Document.Project.Solution; | ||
} | ||
|
||
// Replace the existing field declaration with the new field declaration | ||
return context.Document.WithSyntaxRoot(newRoot.ReplaceNode(existingFieldDeclaration, fieldDeclarationWithNewInitializer)).Project.Solution; | ||
} | ||
Comment on lines
+80
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Complex Conditional Logic The method Consider simplifying the conditional logic or breaking down the method into smaller, more focused methods. Additionally, ensure that the comments are updated to reflect any changes in logic or method structure. |
||
|
||
private static EqualsValueClauseSyntax GetInitializerFromExpressionSyntax(ExpressionSyntax assignmentExpression) | ||
{ | ||
// We use a given expression syntax node, duplicate it, and create an EqualsValueClauseSyntax node from it | ||
// which can serve as an initializer for a field declaration | ||
return SyntaxFactory.EqualsValueClause(assignmentExpression.WithTriviaFrom(assignmentExpression)); | ||
} | ||
|
||
private static async Task<Solution> EnforceFieldIsNotInitializedAsync(Document document, SyntaxNode declaration, CancellationToken cancellationToken) | ||
{ | ||
SyntaxNode? root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
if (root is null | ||
|| declaration is not FieldDeclarationSyntax fieldDeclaration // The declaration should be a field declaration | ||
|| CreateFieldDeclaration(fieldDeclaration) is not { } newFieldDeclaration) | ||
{ | ||
// If any of the above conditions are not met, return the current solution | ||
return document.Project.Solution; | ||
} | ||
|
||
// Replace the existing field declaration with the new field declaration | ||
return document.WithSyntaxRoot(root.ReplaceNode(fieldDeclaration, newFieldDeclaration)).Project.Solution; | ||
} | ||
Comment on lines
+109
to
+123
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Field Initialization Enforcement The method Adding more detailed comments would help other developers understand the decision points within this method. |
||
|
||
private static async Task<Solution> EnforceFieldDeclarationInitializationAsync(Document document, SemanticModel semanticModel, SyntaxNode declaration, CancellationToken cancellationToken) | ||
{ | ||
SyntaxNode? root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); | ||
|
||
if (root is null | ||
|| declaration is not FieldDeclarationSyntax fieldDeclaration // The declaration should be a field declaration | ||
|| fieldDeclaration.Declaration.Variables.Count != 1 // The field declaration should have a variable declarator | ||
|| fieldDeclaration.Declaration.Variables[0] is not { } variableDeclarator // The field declaration should have a variable declarator | ||
|| semanticModel.GetDeclaredSymbol(variableDeclarator, cancellationToken) is not IFieldSymbol fieldSymbol // Let's get the field symbol | ||
|| GetDefaultInitializerForType(fieldSymbol.Type) is not { } newInitializer // Let's try to get an initializer for the field type | ||
|| CreateFieldDeclaration(fieldDeclaration, newInitializer) is not { } fieldDeclarationWithInitializer) | ||
{ | ||
// If any of the above conditions are not met, return the current solution | ||
return document.Project.Solution; | ||
} | ||
|
||
// Replace the existing field declaration with the new field declaration | ||
return document.WithSyntaxRoot(root.ReplaceNode(fieldDeclaration, fieldDeclarationWithInitializer)).Project.Solution; | ||
} | ||
Comment on lines
+125
to
+143
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Default Initializer Handling The method Refactor to handle multiple declarators and simplify the conditional logic to improve clarity and robustness. Consider handling cases where multiple declarators are present in a field declaration. |
||
|
||
private static EqualsValueClauseSyntax? GetDefaultInitializerForType(ITypeSymbol fieldType) | ||
{ | ||
// If the field type is an interface or delegate, we do not want to initialize it | ||
if (fieldType.TypeKind == TypeKind.Interface | ||
|| fieldType.TypeKind == TypeKind.Delegate) | ||
{ | ||
return null; | ||
} | ||
|
||
// If the field type is a string, we can initialize it to string.Empty | ||
if (fieldType.SpecialType == SpecialType.System_String) | ||
{ | ||
return SyntaxFactory.EqualsValueClause(SyntaxFactory.MemberAccessExpression(SyntaxKind.SimpleMemberAccessExpression, SyntaxFactory.IdentifierName("string"), SyntaxFactory.IdentifierName("Empty"))); | ||
} | ||
|
||
// If the field type is a generic type, we need to create a generic object creation expression | ||
if (fieldType is INamedTypeSymbol { IsGenericType: true } namedTypeSymbol) | ||
{ | ||
string genericTypeName = namedTypeSymbol.Name; | ||
string genericArguments = string.Join(", ", namedTypeSymbol.TypeArguments.Select(arg => arg.ToDisplayString())); | ||
string fullTypeName = $"{genericTypeName}<{genericArguments}>"; | ||
return SyntaxFactory.EqualsValueClause(SyntaxFactory.ObjectCreationExpression(SyntaxFactory.ParseTypeName(fullTypeName)).WithArgumentList(SyntaxFactory.ArgumentList())); | ||
} | ||
|
||
return SyntaxFactory.EqualsValueClause(SyntaxFactory.ObjectCreationExpression(SyntaxFactory.ParseTypeName(fieldType.Name)).WithArgumentList(SyntaxFactory.ArgumentList())); | ||
} | ||
Comment on lines
+145
to
+170
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Type-Specific Initializers The method Consider using a strategy pattern or a dictionary of type handlers to simplify the method and make it easier to extend in the future. |
||
|
||
private static FieldDeclarationSyntax? CreateFieldDeclaration(FieldDeclarationSyntax existingFieldDeclaration, EqualsValueClauseSyntax? newInitializer = null) | ||
{ | ||
// Extract the existing variable declarator | ||
VariableDeclaratorSyntax? existingVariableDeclarator = existingFieldDeclaration.Declaration.Variables.FirstOrDefault(); | ||
|
||
if (existingVariableDeclarator is null) | ||
{ | ||
return null; | ||
} | ||
|
||
// Create a new variable declarator with the initializer | ||
VariableDeclaratorSyntax newVariableDeclarator = SyntaxFactory.VariableDeclarator(existingVariableDeclarator.Identifier) | ||
.WithInitializer(newInitializer); | ||
|
||
// Create a new variable declaration with the new variable declarator | ||
VariableDeclarationSyntax newVariableDeclaration = SyntaxFactory.VariableDeclaration(existingFieldDeclaration.Declaration.Type) | ||
.WithVariables(SyntaxFactory.SingletonSeparatedList(newVariableDeclarator)); | ||
|
||
// Create a new field declaration with the new variable declaration and existing modifiers | ||
return SyntaxFactory.FieldDeclaration(newVariableDeclaration) | ||
.WithModifiers(existingFieldDeclaration.Modifiers); | ||
} | ||
Comment on lines
+172
to
+193
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Field Declaration Creation The method Refactor to support multiple variable declarators in the field declaration. Consider refactoring to handle multiple variable declarators. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
using BenchmarkDotNet.Diagnostics.dotMemory; | ||
Check failure on line 1 in tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs1200Benchmarks.cs
|
||
using BenchmarkDotNet.Diagnostics.dotTrace; | ||
|
||
namespace EffectiveCSharp.Analyzers.Benchmarks; | ||
|
||
[InProcess] | ||
[MemoryDiagnoser] | ||
public class Ecs1200Benchmarks | ||
{ | ||
private static CompilationWithAnalyzers? BaselineCompilation { get; set; } | ||
|
||
private static CompilationWithAnalyzers? TestCompilation { get; set; } | ||
|
||
[IterationSetup] | ||
[SuppressMessage("Usage", "VSTHRD002:Avoid problematic synchronous waits", Justification = "Async setup not supported in BenchmarkDotNet.See https://github.com/dotnet/BenchmarkDotNet/issues/2442.")] | ||
public static void SetupCompilation() | ||
{ | ||
List<(string Name, string Content)> sources = []; | ||
for (int index = 0; index < Constants.NumberOfCodeFiles; index++) | ||
{ | ||
string name = "TypeName" + index; | ||
sources.Add((name, @$" | ||
internal class {name} | ||
{{ | ||
private string labels; | ||
|
||
public {name}() | ||
{{ | ||
labels = string.Empty; | ||
}} | ||
}} | ||
")); | ||
} | ||
|
||
(BaselineCompilation, TestCompilation) = | ||
BenchmarkCSharpCompilationFactory | ||
.CreateAsync<PreferDeclarationInitializersToAssignmentStatementsAnalyzer>(sources.ToArray()) | ||
.GetAwaiter() | ||
.GetResult(); | ||
} | ||
|
||
[Benchmark] | ||
public async Task Ecs1200WithDiagnostics() | ||
{ | ||
ImmutableArray<Diagnostic> diagnostics = | ||
(await TestCompilation! | ||
.GetAnalysisResultAsync(CancellationToken.None) | ||
.ConfigureAwait(false)) | ||
.AssertValidAnalysisResult() | ||
.GetAllDiagnostics(); | ||
|
||
if (diagnostics.Length != Constants.NumberOfCodeFiles) | ||
{ | ||
throw new InvalidOperationException($"Expected '{Constants.NumberOfCodeFiles:N0}' analyzer diagnostics but found '{diagnostics.Length}'"); | ||
} | ||
} | ||
|
||
[Benchmark(Baseline = true)] | ||
public async Task Ecs1200Baseline() | ||
{ | ||
ImmutableArray<Diagnostic> diagnostics = | ||
(await BaselineCompilation! | ||
.GetAnalysisResultAsync(CancellationToken.None) | ||
.ConfigureAwait(false)) | ||
.AssertValidAnalysisResult() | ||
.GetAllDiagnostics(); | ||
|
||
if (diagnostics.Length != 0) | ||
{ | ||
throw new InvalidOperationException($"Expected no analyzer diagnostics but found '{diagnostics.Length}'"); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These got inserted into the incorrect section