diff --git a/README.md b/README.md index d9a9c7b4..d9f479fc 100755 --- a/README.md +++ b/README.md @@ -197,7 +197,7 @@ If you are already using other analyzers, you can check [which rules are duplica |[MA0180](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0180.md)|Design|ILogger type parameter should match containing type|⚠️|❌|✔️| |[MA0181](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0181.md)|Style|Do not use cast|ℹ️|❌|❌| |[MA0182](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0182.md)|Design|Avoid unused internal types|ℹ️|✔️|✔️| -|[MA0183](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0183.md)|Usage|string.Format should use a format string with placeholders|⚠️|✔️|❌| +|[MA0183](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0183.md)|Usage|The format string should use placeholders|⚠️|✔️|❌| |[MA0184](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0184.md)|Style|Do not use interpolated string without parameters|👻|✔️|✔️| |[MA0185](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0185.md)|Performance|Simplify string.Create when all parameters are culture invariant|ℹ️|✔️|✔️| |[MA0186](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0186.md)|Design|Equals method should use \[NotNullWhen(true)\] on the parameter|ℹ️|❌|❌| diff --git a/docs/README.md b/docs/README.md index 257d2d64..67803cce 100755 --- a/docs/README.md +++ b/docs/README.md @@ -181,7 +181,7 @@ |[MA0180](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0180.md)|Design|ILogger type parameter should match containing type|⚠️|❌|✔️| |[MA0181](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0181.md)|Style|Do not use cast|ℹ️|❌|❌| |[MA0182](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0182.md)|Design|Avoid unused internal types|ℹ️|✔️|✔️| -|[MA0183](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0183.md)|Usage|string.Format should use a format string with placeholders|⚠️|✔️|❌| +|[MA0183](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0183.md)|Usage|The format string should use placeholders|⚠️|✔️|❌| |[MA0184](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0184.md)|Style|Do not use interpolated string without parameters|👻|✔️|✔️| |[MA0185](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0185.md)|Performance|Simplify string.Create when all parameters are culture invariant|ℹ️|✔️|✔️| |[MA0186](https://github.com/meziantou/Meziantou.Analyzer/blob/main/docs/Rules/MA0186.md)|Design|Equals method should use \[NotNullWhen(true)\] on the parameter|ℹ️|❌|❌| @@ -744,7 +744,7 @@ dotnet_diagnostic.MA0181.severity = none # MA0182: Avoid unused internal types dotnet_diagnostic.MA0182.severity = suggestion -# MA0183: string.Format should use a format string with placeholders +# MA0183: The format string should use placeholders dotnet_diagnostic.MA0183.severity = warning # MA0184: Do not use interpolated string without parameters @@ -1300,7 +1300,7 @@ dotnet_diagnostic.MA0181.severity = none # MA0182: Avoid unused internal types dotnet_diagnostic.MA0182.severity = none -# MA0183: string.Format should use a format string with placeholders +# MA0183: The format string should use placeholders dotnet_diagnostic.MA0183.severity = none # MA0184: Do not use interpolated string without parameters diff --git a/docs/Rules/MA0183.md b/docs/Rules/MA0183.md index 83f800a7..8e3c5560 100644 --- a/docs/Rules/MA0183.md +++ b/docs/Rules/MA0183.md @@ -1,4 +1,13 @@ -# MA0183 - string.Format should use a format string with placeholders +# MA0183 - The format string should use placeholders Source: [StringFormatShouldBeConstantAnalyzer.cs](https://github.com/meziantou/Meziantou.Analyzer/blob/main/src/Meziantou.Analyzer/Rules/StringFormatShouldBeConstantAnalyzer.cs) + +This rule detects calls to formatting methods where the format string has no placeholders but formatting arguments are provided, or where a formatting method is called with no arguments at all (making the format call redundant). + +The following methods are checked: + +- `string.Format` +- `Console.Write` +- `Console.WriteLine` +- `System.Text.StringBuilder.AppendFormat` diff --git a/src/Meziantou.Analyzer.Pack/configuration/default.editorconfig b/src/Meziantou.Analyzer.Pack/configuration/default.editorconfig index 4b5016fe..dc1a96a4 100644 --- a/src/Meziantou.Analyzer.Pack/configuration/default.editorconfig +++ b/src/Meziantou.Analyzer.Pack/configuration/default.editorconfig @@ -542,7 +542,7 @@ dotnet_diagnostic.MA0181.severity = none # MA0182: Avoid unused internal types dotnet_diagnostic.MA0182.severity = suggestion -# MA0183: string.Format should use a format string with placeholders +# MA0183: The format string should use placeholders dotnet_diagnostic.MA0183.severity = warning # MA0184: Do not use interpolated string without parameters diff --git a/src/Meziantou.Analyzer.Pack/configuration/none.editorconfig b/src/Meziantou.Analyzer.Pack/configuration/none.editorconfig index 09a71638..1c12ff8e 100644 --- a/src/Meziantou.Analyzer.Pack/configuration/none.editorconfig +++ b/src/Meziantou.Analyzer.Pack/configuration/none.editorconfig @@ -542,7 +542,7 @@ dotnet_diagnostic.MA0181.severity = none # MA0182: Avoid unused internal types dotnet_diagnostic.MA0182.severity = none -# MA0183: string.Format should use a format string with placeholders +# MA0183: The format string should use placeholders dotnet_diagnostic.MA0183.severity = none # MA0184: Do not use interpolated string without parameters diff --git a/src/Meziantou.Analyzer/Rules/StringFormatShouldBeConstantAnalyzer.cs b/src/Meziantou.Analyzer/Rules/StringFormatShouldBeConstantAnalyzer.cs index 059a0ec3..ec394a99 100644 --- a/src/Meziantou.Analyzer/Rules/StringFormatShouldBeConstantAnalyzer.cs +++ b/src/Meziantou.Analyzer/Rules/StringFormatShouldBeConstantAnalyzer.cs @@ -11,8 +11,8 @@ public sealed class StringFormatShouldBeConstantAnalyzer : DiagnosticAnalyzer { private static readonly DiagnosticDescriptor Rule = new( RuleIdentifiers.StringFormatShouldBeConstant, - title: "string.Format should use a format string with placeholders", - messageFormat: "Use string literal instead of string.Format when the format string has no placeholders", + title: "The format string should use placeholders", + messageFormat: "Use a string literal instead of a format method when the format string has no placeholders", RuleCategories.Usage, DiagnosticSeverity.Warning, isEnabledByDefault: true, @@ -25,54 +25,99 @@ public override void Initialize(AnalysisContext context) { context.EnableConcurrentExecution(); context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); - context.RegisterOperationAction(Analyze, OperationKind.Invocation); + context.RegisterCompilationStartAction(compilationStartContext => + { + var formatProviderType = compilationStartContext.Compilation.GetBestTypeByMetadataName("System.IFormatProvider"); + var consoleType = compilationStartContext.Compilation.GetBestTypeByMetadataName("System.Console"); + var stringBuilderType = compilationStartContext.Compilation.GetBestTypeByMetadataName("System.Text.StringBuilder"); + + compilationStartContext.RegisterOperationAction( + context => Analyze(context, formatProviderType, consoleType, stringBuilderType), + OperationKind.Invocation); + }); } - private static void Analyze(OperationAnalysisContext context) + private static void Analyze(OperationAnalysisContext context, ITypeSymbol? formatProviderType, ITypeSymbol? consoleType, ITypeSymbol? stringBuilderType) { var operation = (IInvocationOperation)context.Operation; + var method = operation.TargetMethod; - // Check if it's a string.Format call - if (operation.TargetMethod.Name != "Format") + if (operation.Arguments.Length == 0) return; - if (operation.TargetMethod.ContainingType.SpecialType != SpecialType.System_String) + // Determine if this is a known format method, what the format arg index is, + // and whether to report when there are no formatting arguments. + int formatArgumentIndex; + bool reportWhenNoFormattingArgs; + + if (method.ContainingType.SpecialType == SpecialType.System_String && method.Name == "Format") + { + // string.Format - report even when called with no args (e.g. string.Format("abc")) + reportWhenNoFormattingArgs = true; + formatArgumentIndex = GetFormatArgIndex(operation, formatProviderType); + } + else if (consoleType is not null && method.ContainingType.IsEqualTo(consoleType) && + (method.Name == "Write" || method.Name == "WriteLine")) + { + // Console.Write / Console.WriteLine - only report when formatting arguments are supplied + // (Console.Write("abc") is a valid non-format call) + if (method.Parameters.Length <= 1) + return; + + reportWhenNoFormattingArgs = false; + formatArgumentIndex = 0; + } + else if (stringBuilderType is not null && method.ContainingType.IsEqualTo(stringBuilderType) && + method.Name == "AppendFormat") + { + // StringBuilder.AppendFormat - report even when called with no args + reportWhenNoFormattingArgs = true; + formatArgumentIndex = GetFormatArgIndex(operation, formatProviderType); + } + else + { return; + } - if (operation.Arguments.Length == 0) + if (formatArgumentIndex < 0 || formatArgumentIndex >= operation.Arguments.Length) return; - var formatProviderType = context.Compilation.GetTypeByMetadataName("System.IFormatProvider"); + var formatArgument = operation.Arguments[formatArgumentIndex]; - // Find the format string argument (either first or second parameter depending on overload) - IArgumentOperation? formatArgument = null; - var formatArgumentIndex = -1; + // Check if there are any formatting arguments after the format string + var hasFormattingArguments = HasFormattingArguments(operation, formatArgumentIndex); - if (operation.Arguments.Length > 0) + // Case 1: No formatting arguments at all + if (!hasFormattingArguments) { - var firstArg = operation.Arguments[0]; - if (firstArg.Parameter?.Type.IsEqualTo(formatProviderType) == true) + if (reportWhenNoFormattingArgs) { - // First argument is IFormatProvider, so format string is the second argument - if (operation.Arguments.Length > 1) - { - formatArgument = operation.Arguments[1]; - formatArgumentIndex = 1; - } + context.ReportDiagnostic(Rule, operation); } - else + + return; + } + + // Case 2: Has formatting arguments but constant format string has no placeholders + if (formatArgument.Value.ConstantValue.HasValue && formatArgument.Value.ConstantValue.Value is string formatString) + { + if (!HasPlaceholders(formatString)) { - // First argument is the format string - formatArgument = firstArg; - formatArgumentIndex = 0; + context.ReportDiagnostic(Rule, operation); } } + } - if (formatArgument is null) - return; + private static int GetFormatArgIndex(IInvocationOperation operation, ITypeSymbol? formatProviderType) + { + if (operation.Arguments.Length > 0 && operation.Arguments[0].Parameter?.Type.IsEqualTo(formatProviderType) == true) + return 1; - // Check if there are any formatting arguments after the format string - var hasFormattingArguments = false; + return 0; + } + + private static bool HasFormattingArguments(IInvocationOperation operation, int formatArgumentIndex) + { for (var i = formatArgumentIndex + 1; i < operation.Arguments.Length; i++) { var arg = operation.Arguments[i]; @@ -82,10 +127,8 @@ private static void Analyze(OperationAnalysisContext context) { // Check if the array has any elements if (arrayCreation.Initializer is not null && arrayCreation.Initializer.ElementValues.Length > 0) - { - hasFormattingArguments = true; - break; - } + return true; + // Skip empty params arrays continue; } @@ -94,10 +137,7 @@ private static void Analyze(OperationAnalysisContext context) if (arg.ArgumentKind is ArgumentKind.ParamCollection && arg.Value is ICollectionExpressionOperation collectionExpression) { if (collectionExpression.Elements.Length > 0) - { - hasFormattingArguments = true; - break; - } + return true; continue; } @@ -107,25 +147,10 @@ private static void Analyze(OperationAnalysisContext context) if (arg.IsImplicit) continue; - hasFormattingArguments = true; - break; - } - - // Case 1: No formatting arguments at all - report regardless of format string - if (!hasFormattingArguments) - { - context.ReportDiagnostic(Rule, operation); - return; + return true; } - // Case 2: Has formatting arguments but constant format string has no placeholders - if (formatArgument.Value.ConstantValue.HasValue && formatArgument.Value.ConstantValue.Value is string formatString) - { - if (!HasPlaceholders(formatString)) - { - context.ReportDiagnostic(Rule, operation); - } - } + return false; } private static bool HasPlaceholders(string formatString) diff --git a/tests/Meziantou.Analyzer.Test/Rules/StringFormatShouldBeConstantAnalyzerTests.cs b/tests/Meziantou.Analyzer.Test/Rules/StringFormatShouldBeConstantAnalyzerTests.cs index f68bf175..05b9eb6c 100644 --- a/tests/Meziantou.Analyzer.Test/Rules/StringFormatShouldBeConstantAnalyzerTests.cs +++ b/tests/Meziantou.Analyzer.Test/Rules/StringFormatShouldBeConstantAnalyzerTests.cs @@ -473,4 +473,265 @@ await CreateProjectBuilder() .WithOutputKind(OutputKind.ConsoleApplication) .ValidateAsync(); } + + [Fact] + public async Task ConsoleWrite_NoArgs_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + Console.Write("NO PLACEHOLDERS"); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWrite_WithArgButNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + [|Console.Write("NO PLACEHOLDERS", true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWrite_WithArgAndPlaceholder_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + Console.Write("Value: {0}", true); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWrite_MultipleArgsNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + [|Console.Write("NO PLACEHOLDERS", true, true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWriteLine_NoArgs_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + Console.WriteLine("NO PLACEHOLDERS"); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWriteLine_WithArgButNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + [|Console.WriteLine("NO PLACEHOLDERS", true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWriteLine_WithArgAndPlaceholder_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + Console.WriteLine("Value: {0}", true); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWriteLine_MultipleArgsNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + [|Console.WriteLine("NO PLACEHOLDERS", true, true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task ConsoleWriteLine_ThreeArgsNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + + class Test + { + void Method() + { + [|Console.WriteLine("NO PLACEHOLDERS", true, true, true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task StringBuilderAppendFormat_NoFormattingArgs_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System.Text; + + class Test + { + void Method() + { + var sb = new StringBuilder(); + [|sb.AppendFormat("NO PLACEHOLDERS")|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task StringBuilderAppendFormat_WithArgButNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System.Text; + + class Test + { + void Method() + { + var sb = new StringBuilder(); + [|sb.AppendFormat("NO PLACEHOLDERS", true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task StringBuilderAppendFormat_WithArgAndPlaceholder_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System.Text; + + class Test + { + void Method() + { + var sb = new StringBuilder(); + sb.AppendFormat("Value: {0}", true); + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task StringBuilderAppendFormat_WithIFormatProviderAndNoPlaceholder_ShouldReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + using System.Globalization; + using System.Text; + + class Test + { + void Method() + { + var sb = new StringBuilder(); + [|sb.AppendFormat(CultureInfo.InvariantCulture, "NO PLACEHOLDERS", true)|]; + } + } + """) + .ValidateAsync(); + } + + [Fact] + public async Task StringBuilderAppendFormat_WithIFormatProviderAndPlaceholder_ShouldNotReportDiagnostic() + { + await CreateProjectBuilder() + .WithSourceCode(""" + using System; + using System.Globalization; + using System.Text; + + class Test + { + void Method() + { + var sb = new StringBuilder(); + sb.AppendFormat(CultureInfo.InvariantCulture, "Value: {0}", true); + } + } + """) + .ValidateAsync(); + } }