diff --git a/docs/rules/ECS0006.md b/docs/rules/ECS0006.md new file mode 100644 index 0000000..4694ee2 --- /dev/null +++ b/docs/rules/ECS0006.md @@ -0,0 +1,65 @@ +# ECS0006: Avoid stringly-typed APIs + +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 + +Using string literals to represent member names or parameter names in APIs. + +## Rule description + +This rule identifies instances where string literals are used to refer to member names or parameter names. Using string literals in such contexts is prone to errors, especially during refactoring, as the string literals do not update automatically. The `nameof` operator should be used instead to ensure type safety and to facilitate easier refactoring. + +## How to fix violations + +Replace the string literal with the `nameof` operator to reference the member or parameter name. + +## When to suppress warnings + +Suppress warnings only if you have a valid reason for using string literals that cannot be replaced with the `nameof` operator. For example, if the string literal represents a dynamic value that cannot be determined at compile time. + +## Example of a violation + +### Description + +Using a string literal to reference a member name. + +### Code + +```csharp +public class MyClass +{ + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + "thisCantBeNull", + "We told you this cant be null"); + } + } +} +``` + +## Example of how to fix + +### Description + +Replacing the string literal with the `nameof` operator. + +### Code + +```csharp +public class MyClass +{ + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + nameof(thisCantBeNull), + "We told you this cant be null"); + } + } +} +``` diff --git a/effectivecsharpanalyzers.lutconfig b/effectivecsharpanalyzers.lutconfig new file mode 100644 index 0000000..596a860 --- /dev/null +++ b/effectivecsharpanalyzers.lutconfig @@ -0,0 +1,6 @@ + + + true + true + 180000 + \ No newline at end of file diff --git a/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md b/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md index 6656d71..4b0114a 100644 --- a/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md +++ b/src/EffectiveCSharp.Analyzers/AnalyzerReleases.Unshipped.md @@ -5,4 +5,5 @@ Rule ID | Category | Severity | Notes --------|----------|----------|------- ECS0002 | Maintainability | Info | PreferReadonlyOverConstAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/10c2d53afd688efe5a59097f76cb4edf33f6a474/docs/ECS0002.md) +ECS0006 | Refactoring | Warning | AvoidStringlyTypedApisAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/main/docs/ECS0006.md) ECS1000 | Performance | Info | SpanAnalyzer, [Documentation](https://github.com/rjmurillo/EffectiveCSharp.Analyzers/blob/d00a4cc9f61e7d5b392894aad859e46c43a5611c/docs/ECS1000.md) \ No newline at end of file diff --git a/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs new file mode 100644 index 0000000..15ed2fe --- /dev/null +++ b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisAnalyzer.cs @@ -0,0 +1,76 @@ +namespace EffectiveCSharp.Analyzers; + +/// +/// A for Effective C# Item #6 - Avoid stringly typed APIs. +/// +/// +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public class AvoidStringlyTypedApisAnalyzer : DiagnosticAnalyzer +{ + private const string DiagnosticId = DiagnosticIds.AvoidStringlyTypedApis; + private const string Title = "Avoid stringly-typed APIs"; + private const string MessageFormat = "Use 'nameof({0})' instead of the string literal \"{0}\""; + + private const string Description = + "Replace string literals representing member names with the nameof operator to ensure type safety."; + + private const string Category = "Refactoring"; + + private static readonly DiagnosticDescriptor Rule = new( + DiagnosticId, + Title, + MessageFormat, + Category, + DiagnosticSeverity.Warning, + isEnabledByDefault: true, + description: Description, + helpLinkUri: + $"https://github.com/rjmurillo/EffectiveCSharp.Analyzers{ThisAssembly.GitCommitId}/docs/{DiagnosticId}.md"); + + /// + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + + /// + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSyntaxNodeAction(AnalyzeLiteralExpression, SyntaxKind.StringLiteralExpression); + } + + private static void AnalyzeLiteralExpression(SyntaxNodeAnalysisContext context) + { + LiteralExpressionSyntax literalExpression = (LiteralExpressionSyntax)context.Node; + string literalValue = literalExpression.Token.ValueText; + + // Walk up the syntax tree to find the containing class or method + TypeDeclarationSyntax? containingClass = literalExpression.FirstAncestorOrSelf(); + MethodDeclarationSyntax? containingMethod = literalExpression.FirstAncestorOrSelf(); + + SemanticModel semanticModel = context.SemanticModel; + + if (containingClass != null + && semanticModel.GetDeclaredSymbol(containingClass, context.CancellationToken) is { } containingTypeSymbol) + { + IEnumerable memberNames = containingTypeSymbol.GetMembers().Select(member => member.Name); + + if (memberNames.Contains(literalValue, StringComparer.Ordinal)) + { + Diagnostic diagnostic = literalExpression.GetLocation().CreateDiagnostic(Rule, literalValue); + context.ReportDiagnostic(diagnostic); + } + } + + if (containingMethod != null + && semanticModel.GetDeclaredSymbol(containingMethod, context.CancellationToken) is { } methodSymbol) + { + IEnumerable parameterNames = methodSymbol.Parameters.Select(parameter => parameter.Name); + + if (parameterNames.Contains(literalValue, StringComparer.Ordinal)) + { + Diagnostic diagnostic = literalExpression.GetLocation().CreateDiagnostic(Rule, literalValue); + context.ReportDiagnostic(diagnostic); + } + } + } +} diff --git a/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs new file mode 100644 index 0000000..d097007 --- /dev/null +++ b/src/EffectiveCSharp.Analyzers/AvoidStringlyTypedApisCodeFixProvider.cs @@ -0,0 +1,111 @@ +namespace EffectiveCSharp.Analyzers; + +/// +/// A that provides a code fix for the . +/// +/// +[ExportCodeFixProvider(LanguageNames.CSharp, Name = nameof(AvoidStringlyTypedApisCodeFixProvider))] +[Shared] +public class AvoidStringlyTypedApisCodeFixProvider : CodeFixProvider +{ + private const string Title = "Use nameof operator"; + + /// + public sealed override ImmutableArray FixableDiagnosticIds => ImmutableArray.Create(DiagnosticIds.AvoidStringlyTypedApis); + + /// + public sealed override FixAllProvider GetFixAllProvider() => WellKnownFixAllProviders.BatchFixer; + + /// + public sealed override async Task RegisterCodeFixesAsync(CodeFixContext context) + { + SyntaxNode? root = await context.Document.GetSyntaxRootAsync(context.CancellationToken).ConfigureAwait(false); + Diagnostic diagnostic = context.Diagnostics.First(); + TextSpan diagnosticSpan = diagnostic.Location.SourceSpan; + + SyntaxNode? node = root?.FindNode(diagnosticSpan); + + LiteralExpressionSyntax? literalExpression = node switch + { + // Check if the node is a LiteralExpressionSyntax directly + LiteralExpressionSyntax literalNode => literalNode, + + // Check if the node is an ArgumentSyntax containing a LiteralExpressionSyntax + ArgumentSyntax { Expression: LiteralExpressionSyntax argLiteralNode } => argLiteralNode, + + _ => null + }; + + if (literalExpression != null) + { + context.RegisterCodeFix( + CodeAction.Create( + title: Title, + createChangedSolution: c => UseNameofOperatorAsync(context.Document, literalExpression, c), + equivalenceKey: Title), + diagnostic); + } + } + + private static async Task UseNameofOperatorAsync(Document document, LiteralExpressionSyntax? literalExpression, CancellationToken cancellationToken) + { + string literalValue = literalExpression?.Token.ValueText; + + // Walk up the syntax tree to find the containing class or method + TypeDeclarationSyntax? containingClass = literalExpression?.FirstAncestorOrSelf(); + MethodDeclarationSyntax? containingMethod = literalExpression?.FirstAncestorOrSelf(); + + string? nameofExpressionText = null; + + if (containingClass != null) + { + SemanticModel? semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + + if (semanticModel.GetDeclaredSymbol(containingClass, cancellationToken) is { } containingTypeSymbol) + { + IEnumerable memberNames = containingTypeSymbol.GetMembers().Select(member => member.Name); + + if (memberNames.Contains(literalValue)) + { + nameofExpressionText = $"nameof({literalValue})"; + } + } + } + + if (nameofExpressionText == null && containingMethod != null) + { + SemanticModel? semanticModel = await document.GetSemanticModelAsync(cancellationToken).ConfigureAwait(false); + + if (semanticModel.GetDeclaredSymbol(containingMethod, cancellationToken) is { } methodSymbol) + { + IEnumerable parameterNames = methodSymbol.Parameters.Select(parameter => parameter.Name); + + if (parameterNames.Contains(literalValue)) + { + nameofExpressionText = $"nameof({literalValue})"; + } + } + } + + if (nameofExpressionText == null) + { + return document.Project.Solution; + } + + if (literalExpression != null) + { + ExpressionSyntax nameofExpression = SyntaxFactory.ParseExpression(nameofExpressionText) + .WithTriviaFrom(literalExpression); + + SyntaxNode? root = await document.GetSyntaxRootAsync(cancellationToken).ConfigureAwait(false); + SyntaxNode? newRoot = root?.ReplaceNode(literalExpression, nameofExpression); + + if (newRoot != null) + { + return document.WithSyntaxRoot(newRoot).Project.Solution; + } + } + + return document.Project.Solution; + } +} diff --git a/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs b/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs index 24c21a4..a1b62d3 100644 --- a/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs +++ b/src/EffectiveCSharp.Analyzers/DiagnosticIds.cs @@ -3,5 +3,6 @@ internal static class DiagnosticIds { internal const string PreferReadonlyOverConst = "ECS0002"; + internal const string AvoidStringlyTypedApis = "ECS0006"; internal const string UseSpanInstead = "ECS1000"; } diff --git a/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0002Benchmarks.cs b/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0002Benchmarks.cs index 56e563f..336180f 100644 --- a/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0002Benchmarks.cs +++ b/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0002Benchmarks.cs @@ -34,7 +34,7 @@ internal class {name} } [Benchmark] - public async Task Ecs1000WithDiagnostics() + public async Task Ecs0002WithDiagnostics() { ImmutableArray diagnostics = (await TestCompilation! @@ -50,7 +50,7 @@ public async Task Ecs1000WithDiagnostics() } [Benchmark(Baseline = true)] - public async Task Ecs1000Baseline() + public async Task Ecs0002Baseline() { ImmutableArray diagnostics = (await BaselineCompilation! diff --git a/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0006Benchmarks.cs b/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0006Benchmarks.cs new file mode 100644 index 0000000..fac389d --- /dev/null +++ b/tests/EffectiveCSharp.Analyzers.Benchmarks/Ecs0006Benchmarks.cs @@ -0,0 +1,75 @@ +namespace EffectiveCSharp.Analyzers.Benchmarks; + +[InProcess] +[MemoryDiagnoser] +public class Ecs0006Benchmarks +{ + 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, @$" +using System; + +public class {name} +{{ + public static void ExceptionMessage(object thisCantBeNull) + {{ + if (thisCantBeNull == null) + {{ + throw new ArgumentNullException( + ""thisCantBeNull"", + ""We told you this cant be null""); + }} + }} +}} +")); + } + + (BaselineCompilation, TestCompilation) = + BenchmarkCSharpCompilationFactory + .CreateAsync(sources.ToArray()) + .GetAwaiter() + .GetResult(); + } + + [Benchmark] + public async Task Ecs0006WithDiagnostics() + { + ImmutableArray 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 Ecs0006Baseline() + { + ImmutableArray 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}'"); + } + } +} diff --git a/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs new file mode 100644 index 0000000..59cfc32 --- /dev/null +++ b/tests/EffectiveCSharp.Analyzers.Tests/AvoidStringlyTypedApisTests.cs @@ -0,0 +1,89 @@ +using CodeFixVerifier = EffectiveCSharp.Analyzers.Tests.Helpers.AnalyzerAndCodeFixVerifier; +using Verifier = EffectiveCSharp.Analyzers.Tests.Helpers.AnalyzerVerifier; + +namespace EffectiveCSharp.Analyzers.Tests; + +public class AvoidStringlyTypedApisTests +{ + public static IEnumerable TestData() + { + return new object[][] + { + // This should not fire because it's using nameof + ["""nameof(thisCantBeNull)"""], + + // This should fire because it's referring to a member name using a string literal + [""" + {|ECS0006:"thisCantBeNull"|} + """], + + // This should not fire because it's suppressed + [ + """ + #pragma warning disable ECS0006 + "thisCantBeNull" + #pragma warning restore ECS0006 + """ + ], + }.WithReferenceAssemblyGroups(); + } + + [Theory] + [MemberData(nameof(TestData))] + public async Task Analyzer(string referenceAssemblyGroup, string source) + { + await Verifier.VerifyAnalyzerAsync( + $$""" + public class MyClass + { + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + {{source}} + , + "We told you this cant be null"); + } + } + } + """, + referenceAssemblyGroup); + } + + [Fact] + public async Task CodeFix() + { + string testCode = """ + public class MyClass + { + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + {|ECS0006:"thisCantBeNull"|}, + "We told you this cant be null"); + } + } + } + """; + + string fixedCode = """ + public class MyClass + { + public static void ExceptionMessage(object thisCantBeNull) + { + if (thisCantBeNull == null) + { + throw new ArgumentNullException( + nameof(thisCantBeNull), + "We told you this cant be null"); + } + } + } + """; + + await CodeFixVerifier.VerifyCodeFixAsync(testCode, fixedCode, ReferenceAssemblyCatalog.Net80); + } +} diff --git a/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs b/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs index 16ec9c8..1bafc8e 100644 --- a/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs +++ b/tests/EffectiveCSharp.Analyzers.Tests/SpanAnalyzerTests.cs @@ -29,7 +29,7 @@ public static IEnumerable TestData() [Theory] [MemberData(nameof(TestData))] - public async Task SpanAnalyzer(string referenceAssemblyGroup, string source) + public async Task Analyzer(string referenceAssemblyGroup, string source) { await Verifier.VerifyAnalyzerAsync( $$""" @@ -45,7 +45,7 @@ void Method() } [Fact(Skip = "Reporting an analyzer failure when the unit test code above shows it is correct")] - public async Task TestArraySpanFix() + public async Task CodeFix() { string testCode = """ class Program