diff --git a/Documentation/Diagnostics/PH2147.md b/Documentation/Diagnostics/PH2147.md index 5d47c9b3a..31db9e1aa 100644 --- a/Documentation/Diagnostics/PH2147.md +++ b/Documentation/Diagnostics/PH2147.md @@ -1,4 +1,4 @@ -# PH2147: Avoid variables named exactly '_' +# PH2147: Avoid variables named '_' | Property | Value | |--|--| @@ -12,7 +12,9 @@ ## Introduction -This rule flags variables (local variables, foreach variables, for loop variables, using variables, and out parameters) that are named exactly `_` (single underscore). This rule does not flag proper C# discards (both anonymous discards `_` and typed discards like `string _`). +This rule flags variables that are named exactly `_` (single underscore). This includes local variables, foreach variables, for loop variables, using variables, and out parameters that are explicitly named `_`. + +This rule does not flag proper C# anonymous discards (`_`). ## Reason @@ -24,7 +26,7 @@ Either use a proper discard (if you don't need the value) or use a more descript ## Examples -### Incorrect +### Variables Named '_' (Flagged) ```csharp // Local variable - looks like discard but is actually a variable @@ -43,36 +45,24 @@ if (int.TryParse(input, out var _)) } ``` -### Correct +### Correct Usage (NOT Flagged) ```csharp -// Use a proper discard if you don't need the value -_ = ExtractData(reader); +// Use proper anonymous discards - these are NOT flagged +_ = ExtractData(reader); // Anonymous discard assignment +GetValue(out _); // Anonymous discard in out parameter +TryParseHelper("123", out _); // Anonymous discard in out parameter -// Or use a descriptive name if you do need the value +// Use descriptive names when you need the value byte[] data = ExtractData(reader); - -// ForEach with discard -foreach (var _ in items) -{ - // Use discard syntax instead -} - -// Use proper discards - these are NOT flagged by this rule -if (int.TryParse(input, out _)) // Anonymous discard +foreach (var item in items) { - // code -} - -if (int.TryParse(input, out int _)) // Typed discard -{ - // code + // Use item } -// Or use a descriptive name if you need the value if (int.TryParse(input, out int result)) { - // code using result + // Use result } ``` @@ -83,5 +73,5 @@ This rule is enabled by default with Error severity. ## Suppression ```csharp -[SuppressMessage("Philips.CodeAnalysis.MaintainabilityAnalyzers", "PH2147:Avoid variables named exactly '_'", Justification = "Reviewed.")] +[SuppressMessage("Philips.CodeAnalysis.MaintainabilityAnalyzers", "PH2147:Avoid variables named '_'", Justification = "Reviewed.")] ``` \ No newline at end of file diff --git a/Documentation/Diagnostics/PH2152.md b/Documentation/Diagnostics/PH2152.md new file mode 100644 index 000000000..398b1669e --- /dev/null +++ b/Documentation/Diagnostics/PH2152.md @@ -0,0 +1,72 @@ +# PH2152: Avoid unnecessary typed discard + +| Property | Value | +|--|--| +| Package | [Philips.CodeAnalysis.MaintainabilityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MaintainabilityAnalyzers) | +| Diagnostic ID | PH2152 | +| Category | [Naming](../Naming.md) | +| Analyzer | [AvoidVariableNamedUnderscoreAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/AvoidVariableNamedUnderscoreAnalyzer.cs) +| CodeFix | No | +| Severity | Error | +| Enabled By Default | Yes | + +## Introduction + +This rule flags unnecessary typed discards like `out string _` when anonymous discard `out _` would work equally well. + +This rule does not flag typed discards that are necessary for overload resolution. + +## Reason + +Anonymous discards have clearer intent and are more concise than typed discards when the type information is not needed for overload resolution. + +## How to fix violations + +Replace typed discards with anonymous discards when the type is not needed for overload resolution. + +## Examples + +### Unnecessary Typed Discards (Flagged) + +```csharp +// These are flagged - unnecessary typed discards when anonymous would work +GetValue(out string _); // Should be: GetValue(out _); +TryParseHelper("123", out int _); // Should be: TryParseHelper("123", out _); + +public void GetValue(out string value) { value = "test"; } +public bool TryParseHelper(string input, out int result) { result = 42; return true; } +``` + +### Necessary Typed Discards (NOT Flagged) + +```csharp +// These are NOT flagged - necessary for overload resolution +Parse("123", out int _); // Disambiguates from Parse(string, out string) +Parse("test", out string _); // Disambiguates from Parse(string, out int) + +// Example overloaded methods +public bool Parse(string input, out int result) { result = 42; return true; } +public bool Parse(string input, out string result) { result = input; return true; } +``` + +### Correct Usage (NOT Flagged) + +```csharp +// Use proper anonymous discards - these are NOT flagged +GetValue(out _); // Anonymous discard in out parameter +TryParseHelper("123", out _); // Anonymous discard in out parameter + +// Typed discards are allowed when needed for overload resolution +Parse("123", out int _); // Needed to select correct overload +Parse("test", out string _); // Needed to select correct overload +``` + +## Configuration + +This rule is enabled by default with Error severity. + +## Suppression + +```csharp +[SuppressMessage("Philips.CodeAnalysis.MaintainabilityAnalyzers", "PH2152:Avoid unnecessary typed discard", Justification = "Reviewed.")] +``` \ No newline at end of file diff --git a/Philips.CodeAnalysis.Common/DiagnosticId.cs b/Philips.CodeAnalysis.Common/DiagnosticId.cs index 60aa34ec8..791b63020 100644 --- a/Philips.CodeAnalysis.Common/DiagnosticId.cs +++ b/Philips.CodeAnalysis.Common/DiagnosticId.cs @@ -141,6 +141,7 @@ public enum DiagnosticId AvoidVariableNamedUnderscore = 2147, AvoidProblematicUsingPatterns = 2149, AvoidTodoComments = 2151, + AvoidUnnecessaryTypedDiscard = 2152, AvoidUnusedToString = 2153, AvoidUnlicensedPackages = 2155, AvoidPkcsPaddingWithRsaEncryption = 2158, diff --git a/Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/AvoidVariableNamedUnderscoreAnalyzer.cs b/Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/AvoidVariableNamedUnderscoreAnalyzer.cs index 8e081e430..36f10eae1 100644 --- a/Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/AvoidVariableNamedUnderscoreAnalyzer.cs +++ b/Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/AvoidVariableNamedUnderscoreAnalyzer.cs @@ -10,16 +10,38 @@ namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Naming { [DiagnosticAnalyzer(LanguageNames.CSharp)] - public class AvoidVariableNamedUnderscoreAnalyzer : SingleDiagnosticAnalyzer + public class AvoidVariableNamedUnderscoreAnalyzer : DiagnosticAnalyzerBase { - private const string Title = @"Avoid variables named exactly '_'"; - private const string MessageFormat = @"Variable '{0}' is named '_' which can be confused with a discard. Consider using a discard or a more descriptive variable name"; - private const string Description = @"Variables named exactly '_' can be confused with C# discards. Use either a proper discard or a more descriptive variable name."; - - public AvoidVariableNamedUnderscoreAnalyzer() - : base(DiagnosticId.AvoidVariableNamedUnderscore, Title, MessageFormat, Description, Categories.Naming) - { - } + private const string VariableNamedUnderscoreTitle = @"Avoid variables named '_'"; + private const string VariableNamedUnderscoreMessageFormat = @"Avoid variable named '{0}' as it can be confused with discards"; + private const string VariableNamedUnderscoreDescription = @"Avoid variables named exactly '_' which can be confused with discards."; + + private const string UnnecessaryTypedDiscardTitle = @"Avoid unnecessary typed discard"; + private const string UnnecessaryTypedDiscardMessageFormat = @"Use anonymous discard '_' instead of typed discard '{0}' when type is not needed"; + private const string UnnecessaryTypedDiscardDescription = @"Prefer anonymous discards over typed discards when the type is not needed for overload resolution."; + + public static readonly DiagnosticDescriptor VariableNamedUnderscoreRule = new( + DiagnosticId.AvoidVariableNamedUnderscore.ToId(), + VariableNamedUnderscoreTitle, + VariableNamedUnderscoreMessageFormat, + Categories.Naming, + DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: VariableNamedUnderscoreDescription, + helpLinkUri: DiagnosticId.AvoidVariableNamedUnderscore.ToHelpLinkUrl()); + + public static readonly DiagnosticDescriptor UnnecessaryTypedDiscardRule = new( + DiagnosticId.AvoidUnnecessaryTypedDiscard.ToId(), + UnnecessaryTypedDiscardTitle, + UnnecessaryTypedDiscardMessageFormat, + Categories.Naming, + DiagnosticSeverity.Error, + isEnabledByDefault: true, + description: UnnecessaryTypedDiscardDescription, + helpLinkUri: DiagnosticId.AvoidUnnecessaryTypedDiscard.ToHelpLinkUrl()); + + public override System.Collections.Immutable.ImmutableArray SupportedDiagnostics => + System.Collections.Immutable.ImmutableArray.Create(VariableNamedUnderscoreRule, UnnecessaryTypedDiscardRule); protected override void InitializeCompilation(CompilationStartAnalysisContext context) { @@ -43,7 +65,7 @@ private void AnalyzeForEachStatement(SyntaxNodeAnalysisContext context) } Location location = foreachStatement.GetLocation(); - var diagnostic = Diagnostic.Create(Rule, location, foreachStatement.Identifier.ValueText); + var diagnostic = Diagnostic.Create(VariableNamedUnderscoreRule, location, foreachStatement.Identifier.ValueText); context.ReportDiagnostic(diagnostic); } @@ -68,7 +90,7 @@ private void AnalyzeVariableDeclaration(SyntaxNodeAnalysisContext context) } Location location = variableDeclaration.GetLocation(); - var diagnostic = Diagnostic.Create(Rule, location, identifier); + var diagnostic = Diagnostic.Create(VariableNamedUnderscoreRule, location, identifier); context.ReportDiagnostic(diagnostic); } } @@ -88,21 +110,99 @@ private void AnalyzeArgument(SyntaxNodeAnalysisContext context) return; } - // Check if it's a variable declaration + // Check if it's a typed discard (e.g., out int _) if (argument.Expression is DeclarationExpressionSyntax declaration && - declaration.Designation is SingleVariableDesignationSyntax variable) + declaration.Designation is DiscardDesignationSyntax discard) { - if (variable.Identifier.ValueText != "_") + // This is a typed discard (e.g., out int _) + // Only flag if the type is not needed for overload resolution + if (!IsTypedDiscardNecessaryForOverloadResolution(context, argument)) { - return; + Location location = discard.GetLocation(); + var diagnostic = Diagnostic.Create(UnnecessaryTypedDiscardRule, location, "_"); + context.ReportDiagnostic(diagnostic); } + return; + } + // Check if it's a variable declaration with identifier "_" + if (argument.Expression is DeclarationExpressionSyntax declaration2 && + declaration2.Designation is SingleVariableDesignationSyntax variable && + variable.Identifier.ValueText == "_") + { Location location = variable.Identifier.GetLocation(); - var diagnostic = Diagnostic.Create(Rule, location, variable.Identifier.ValueText); + var diagnostic = Diagnostic.Create(VariableNamedUnderscoreRule, location, variable.Identifier.ValueText); context.ReportDiagnostic(diagnostic); - // Note: DiscardDesignationSyntax represents proper discards (e.g., "out _" or "out string _") - // and should not be flagged, so we don't handle this case. } + + // Note: We don't flag IdentifierNameSyntax("_") because that represents + // anonymous discards like "out _" which are the preferred form + } + + private static bool IsTypedDiscardNecessaryForOverloadResolution(SyntaxNodeAnalysisContext context, ArgumentSyntax argument) + { + // Find the method call containing this argument + InvocationExpressionSyntax invocation = argument.Ancestors() + .OfType() + .FirstOrDefault(); + if (invocation is null) + { + return false; // Can't find method call, allow the flag + } + + // Early check: if the invocation doesn't look like it could have overloads, + // we can avoid semantic model access entirely + if (invocation.Expression is not (MemberAccessExpressionSyntax or IdentifierNameSyntax)) + { + return false; // Complex expression, allow the flag + } + + // Get semantic information about the method - moved lower for performance + SymbolInfo symbolInfo = context.SemanticModel.GetSymbolInfo(invocation); + if (symbolInfo.Symbol is not IMethodSymbol method) + { + return false; // Can't resolve method, allow the flag + } + + // Get the argument position - handle named arguments + var argumentIndex = GetArgumentIndex(invocation.ArgumentList.Arguments, argument, method); + if (argumentIndex < 0 || argumentIndex >= method.Parameters.Length) + { + return false; // Invalid position, allow the flag + } + + // Simple heuristic: check if there are method overloads with different out parameter types at this position + INamedTypeSymbol containingType = method.ContainingType; + System.Collections.Generic.IEnumerable methodsWithSameName = containingType.GetMembers(method.Name).OfType(); + + // Count how many overloads have an out parameter at this position + var overloadCount = methodsWithSameName + .Where(m => argumentIndex < m.Parameters.Length) + .Count(m => m.Parameters[argumentIndex].RefKind == RefKind.Out); + + // If there are multiple overloads with out parameters at this position, + // then the typed discard might be necessary for overload resolution + return overloadCount > 1; + } + + private static int GetArgumentIndex(SeparatedSyntaxList arguments, ArgumentSyntax targetArgument, IMethodSymbol method) + { + // Handle named arguments by checking if the target argument has a name + if (targetArgument.NameColon != null) + { + var parameterName = targetArgument.NameColon.Name.Identifier.ValueText; + for (var i = 0; i < method.Parameters.Length; i++) + { + if (method.Parameters[i].Name == parameterName) + { + return i; + } + } + return -1; // Parameter name not found + } + + // For positional arguments, just get the index + return arguments.IndexOf(targetArgument); } } } diff --git a/Philips.CodeAnalysis.Test/Maintainability/Naming/AvoidVariableNamedUnderscoreAnalyzerTest.cs b/Philips.CodeAnalysis.Test/Maintainability/Naming/AvoidVariableNamedUnderscoreAnalyzerTest.cs index fc42f7b1a..b09bc8a99 100644 --- a/Philips.CodeAnalysis.Test/Maintainability/Naming/AvoidVariableNamedUnderscoreAnalyzerTest.cs +++ b/Philips.CodeAnalysis.Test/Maintainability/Naming/AvoidVariableNamedUnderscoreAnalyzerTest.cs @@ -1,6 +1,7 @@ // © 2019 Koninklijke Philips N.V. See License.md in the project root for license information. using System.Threading.Tasks; +using Microsoft.CodeAnalysis; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.VisualStudio.TestTools.UnitTesting; using Philips.CodeAnalysis.Common; @@ -60,7 +61,7 @@ public void TestMethod() [TestMethod] [TestCategory(TestDefinitions.UnitTests)] - public async Task OutParameterNamedUnderscoreShouldNotFlag() + public async Task OutParameterNamedUnderscoreShouldFlag() { var test = @" using System; @@ -78,7 +79,7 @@ private void TestHelper(out int value) } }"; - await VerifySuccessfulCompilation(test).ConfigureAwait(false); + await VerifyDiagnostic(test, DiagnosticId.AvoidUnnecessaryTypedDiscard).ConfigureAwait(false); } [TestMethod] @@ -273,5 +274,155 @@ public void GetIpV4(string adapter, out IPAddress addr, out int mask, out byte[] await VerifySuccessfulCompilation(test).ConfigureAwait(false); } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task UnnecessaryTypedDiscardShouldFlag() + { + var test = @" +using System; + +class TestClass +{ + public void TestMethod() + { + // Typed discard when anonymous discard would work + GetValue(out string _); + TryParseHelper(""123"", out int _); + } + + private void GetValue(out string value) + { + value = ""test""; + } + + private bool TryParseHelper(string input, out int result) + { + result = 42; + return true; + } +}"; + + DiagnosticResult[] expected = CreateTypedDiscardDiagnostics( + (9, 23), (10, 33) + ); + + await VerifyDiagnostic(test, expected).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task NecessaryTypedDiscardForOverloadResolutionShouldNotFlag() + { + var test = @" +using System; + +class TestClass +{ + public void TestMethod() + { + // These typed discards are needed for overload resolution + Parse(""123"", out int _); // Disambiguates from Parse(string, out string) + Parse(""test"", out string _); // Disambiguates from Parse(string, out int) + } + + private bool Parse(string input, out int result) + { + result = 42; + return true; + } + + private bool Parse(string input, out string result) + { + result = input; + return true; + } +}"; + + await VerifySuccessfulCompilation(test).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task UnnecessaryTypedDiscardWithNamedArgumentsShouldFlag() + { + var test = @" +using System; + +class TestClass +{ + public void TestMethod() + { + // Typed discard with named argument when anonymous discard would work + GetStringValue(value: out string _); + TryParseInt(input: ""123"", result: out int _); + } + + private void GetStringValue(out string value) + { + value = ""test""; + } + + private bool TryParseInt(string input, out int result) + { + result = 42; + return true; + } +}"; + + DiagnosticResult[] expected = CreateTypedDiscardDiagnostics( + (9, 36), (10, 45) + ); + + await VerifyDiagnostic(test, expected).ConfigureAwait(false); + } + + [TestMethod] + [TestCategory(TestDefinitions.UnitTests)] + public async Task NecessaryTypedDiscardWithNamedArgumentsForOverloadResolutionShouldNotFlag() + { + var test = @" +using System; + +class TestClass +{ + public void TestMethod() + { + // These typed discards with named arguments are needed for overload resolution + Parse(input: ""123"", result: out int _); // Disambiguates from Parse(string, out string) + Parse(input: ""test"", result: out string _); // Disambiguates from Parse(string, out int) + } + + private bool Parse(string input, out int result) + { + result = 42; + return true; + } + + private bool Parse(string input, out string result) + { + result = input; + return true; + } +}"; + + await VerifySuccessfulCompilation(test).ConfigureAwait(false); + } + + private static DiagnosticResult[] CreateTypedDiscardDiagnostics(params (int line, int column)[] locations) + { + var results = new DiagnosticResult[locations.Length]; + for (var i = 0; i < locations.Length; i++) + { + results[i] = new DiagnosticResult() + { + Id = DiagnosticId.AvoidUnnecessaryTypedDiscard.ToId(), + Location = new DiagnosticResultLocation("Test0.cs", locations[i].line, locations[i].column), + Message = new System.Text.RegularExpressions.Regex(".*"), + Severity = DiagnosticSeverity.Error, + }; + } + return results; + } } }