Skip to content

Commit 7624b2d

Browse files
bcollamorebcollamore
authored
Merge branch 'main' into copilot/fix-719
2 parents d9d6f73 + 134423e commit 7624b2d

8 files changed

Lines changed: 634 additions & 18 deletions

File tree

‎Documentation/Diagnostics/PH2144.md‎

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
# PH2144: Backwards for-loop boundary check should use >= 0 instead of > 0
2+
3+
| Property | Value |
4+
|--|--|
5+
| Package | [Philips.CodeAnalysis.MaintainabilityAnalyzers](https://www.nuget.org/packages/Philips.CodeAnalysis.MaintainabilityAnalyzers) |
6+
| Diagnostic ID | PH2144 |
7+
| Category | [Maintainability](../Maintainability.md) |
8+
| Analyzer | [AvoidIncorrectForLoopConditionAnalyzer](https://github.com/philips-software/roslyn-analyzers/blob/main/Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidIncorrectForLoopConditionAnalyzer.cs)
9+
| CodeFix | Yes |
10+
| Severity | Error |
11+
| Enabled By Default | Yes |
12+
13+
## Introduction
14+
15+
When looping backwards from a collection count to 0, use `>= 0` instead of `> 0` to ensure the element at index 0 is processed. Using `> 0` in the loop condition causes the loop to skip the element at index 0, which is often unintended.
16+
17+
## How to solve
18+
19+
Change the loop condition from `> 0` to `>= 0` to include the element at index 0.
20+
21+
## Example
22+
23+
Code that triggers a diagnostic:
24+
``` cs
25+
class BadExample
26+
{
27+
public void BadMethod()
28+
{
29+
var list = new List<int> { 1, 2, 3, 4, 5 };
30+
31+
// This loop skips the element at index 0
32+
for (var index = list.Count - 1; index > 0; index--)
33+
{
34+
Console.WriteLine(list[index]);
35+
}
36+
}
37+
}
38+
39+
```
40+
41+
And the replacement code:
42+
``` cs
43+
class GoodExample
44+
{
45+
public void GoodMethod()
46+
{
47+
var list = new List<int> { 1, 2, 3, 4, 5 };
48+
49+
// This loop processes all elements including index 0
50+
for (var index = list.Count - 1; index >= 0; index--)
51+
{
52+
Console.WriteLine(list[index]);
53+
}
54+
}
55+
}
56+
57+
```
58+
59+
## Configuration
60+
61+
This analyzer does not offer any special configuration. The general ways of [suppressing](https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/suppress-warnings) diagnostics apply.
62+
63+
## Legitimate Use Cases and Workarounds
64+
65+
In rare cases, you may intentionally want to skip the element at index 0 in a backwards loop. When this is the case, the analyzer will produce a false positive. Here are several clean workarounds to avoid triggering the analyzer while preserving the intended behavior:
66+
67+
### Workaround 1: Use `>= 1` instead of `> 0`
68+
``` cs
69+
// Instead of: for (var i = array.Length - 1; i > 0; i--)
70+
for (var i = array.Length - 1; i >= 1; i--) // Mathematically equivalent but clearer intent
71+
{
72+
// Process elements from index 1 to array.Length - 1 (skipping index 0)
73+
}
74+
```
75+
76+
### Workaround 2: Make the exclusion explicit with variables
77+
``` cs
78+
// Make it clear that you're intentionally excluding the first element
79+
var startIndex = array.Length - 1;
80+
var stopBeforeIndex = 1; // Stop before index 0, not including it
81+
for (var i = startIndex; i >= stopBeforeIndex; i--)
82+
{
83+
// Process elements from index 1 to array.Length - 1 (skipping index 0)
84+
}
85+
```
86+
87+
### Workaround 3: Use array slicing (C# 8.0+)
88+
``` cs
89+
// Process only the sub-array excluding the first element, then iterate normally
90+
var elementsToProcess = array[1..]; // Skip index 0
91+
for (var i = elementsToProcess.Length - 1; i >= 0; i--)
92+
{
93+
// Process elements (original indices 1 to array.Length - 1)
94+
DoSomething(elementsToProcess[i]);
95+
}
96+
```
97+
98+
### Workaround 4: Use LINQ when appropriate
99+
``` cs
100+
// For simple operations, LINQ can be clearer
101+
array.Skip(1).Reverse().ToList().ForEach(element => DoSomething(element));
102+
```
103+
104+
These workarounds make the intent clearer to both the analyzer and future maintainers of the code.

‎Philips.CodeAnalysis.Common/DiagnosticId.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,7 @@ public enum DiagnosticId
137137
AvoidEmptyRegions = 2141,
138138
AvoidCastToString = 2142,
139139
AvoidAssemblyGetEntryAssembly = 2143,
140+
AvoidIncorrectForLoopCondition = 2144,
140141
AvoidStringFormatInInterpolatedString = 2145,
141142
AvoidVariableNamedUnderscore = 2147,
142143
}

‎Philips.CodeAnalysis.MaintainabilityAnalyzers/Documentation/XmlDocumentationShouldAddValueAnalyzer.cs‎

Lines changed: 51 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,12 @@ private void AnalyzeNamedNode(SyntaxNodeAnalysisContext context, Func<string> na
116116

117117
var lowercaseName = name.ToLowerInvariant();
118118

119+
// If any element contains useful information, skip all diagnostics
120+
if (HasUsefulElements(xmlElements, lowercaseName))
121+
{
122+
return;
123+
}
124+
119125
foreach (XmlElementSyntax xmlElement in xmlElements)
120126
{
121127
if (xmlElement.StartTag.Name.LocalName.Text != @"summary")
@@ -133,25 +139,53 @@ private void AnalyzeNamedNode(SyntaxNodeAnalysisContext context, Func<string> na
133139
continue;
134140
}
135141

136-
// Find the 'value' in the XML documentation content by:
137-
// 1. Splitting it into separate words.
138-
// 2. Filtering a predefined and a configurable list of words that add no value.
139-
// 3. Filter words that are part of the method name.
140-
// 4. Throw a Diagnostic if no words remain. This boils down to the content only containing 'low value' words.
141-
IEnumerable<string> words =
142-
SplitInWords(content)
143-
.Where(u => !additionalUselessWords.Contains(u) && !UselessWords.Contains(u))
144-
.Where(s => !lowercaseName.Contains(s));
145-
146-
// We assume here that every remaining word adds value to the documentation text.
147-
if (!words.Any())
148-
{
149-
var loc = Location.Create(context.Node.SyntaxTree, xmlElement.Content.FullSpan);
150-
var diagnostic = Diagnostic.Create(ValueRule, loc);
151-
context.ReportDiagnostic(diagnostic);
152-
}
142+
// Summary already evaluated by HasUsefulElements
143+
var loc = Location.Create(context.Node.SyntaxTree, xmlElement.Content.FullSpan);
144+
var valueDiagnostic = Diagnostic.Create(ValueRule, loc);
145+
context.ReportDiagnostic(valueDiagnostic);
146+
}
147+
}
148+
}
149+
150+
/// <summary>
151+
/// Checks if there are any XML documentation elements that have useful content.
152+
/// Elements like summary, param, returns, exception, etc. are considered if they have useful information.
153+
/// </summary>
154+
/// <param name="xmlElements">All XML documentation elements</param>
155+
/// <param name="lowercaseName">The lowercase name of the documented element</param>
156+
/// <returns>True if there are elements with useful content, false otherwise</returns>
157+
private bool HasUsefulElements(IEnumerable<XmlElementSyntax> xmlElements, string lowercaseName)
158+
{
159+
var xmlDocElements = new[] { "summary", "param", "returns", "exception", "remarks", "example", "value" };
160+
161+
foreach (XmlElementSyntax xmlElement in xmlElements)
162+
{
163+
var tagName = xmlElement.StartTag.Name.LocalName.Text;
164+
if (!xmlDocElements.Contains(tagName))
165+
{
166+
continue;
167+
}
168+
169+
var content = GetContent(xmlElement);
170+
171+
if (string.IsNullOrWhiteSpace(content))
172+
{
173+
continue;
174+
}
175+
176+
IEnumerable<string> words =
177+
SplitInWords(content)
178+
.Where(u => !additionalUselessWords.Contains(u) && !UselessWords.Contains(u))
179+
.Where(s => !lowercaseName.Contains(s));
180+
181+
// If there are remaining words, assume this element has useful content
182+
if (words.Any())
183+
{
184+
return true;
153185
}
154186
}
187+
188+
return false;
155189
}
156190

157191
private static string GetContent(XmlElementSyntax xmlElement)

‎Philips.CodeAnalysis.MaintainabilityAnalyzers/Maintainability/AvoidIncorrectForLoopConditionAnalyzer.cs‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,177 @@
1+
// © 2024 Koninklijke Philips N.V. See License.md in the project root for license information.
2+
3+
using Microsoft.CodeAnalysis;
4+
using Microsoft.CodeAnalysis.CSharp;
5+
using Microsoft.CodeAnalysis.CSharp.Syntax;
6+
using Microsoft.CodeAnalysis.Diagnostics;
7+
using Philips.CodeAnalysis.Common;
8+
9+
namespace Philips.CodeAnalysis.MaintainabilityAnalyzers.Maintainability
10+
{
11+
/// <summary>
12+
/// Diagnostic for incorrect condition in backwards for-loops.
13+
/// </summary>
14+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
15+
public class AvoidIncorrectForLoopConditionAnalyzer : SingleDiagnosticAnalyzer
16+
{
17+
private const string Title = "Backwards for-loop boundary check should use >= 0 instead of > 0";
18+
private const string MessageFormat = "Use '>= 0' instead of '> 0' in backwards for-loop condition to include element at index 0";
19+
private const string Description = "When looping backwards from a collection count to 0, use '>= 0' to ensure the element at index 0 is processed";
20+
21+
public AvoidIncorrectForLoopConditionAnalyzer()
22+
: base(DiagnosticId.AvoidIncorrectForLoopCondition, Title, MessageFormat, Description, Categories.Maintainability)
23+
{ }
24+
25+
/// <summary>
26+
/// <inheritdoc cref="DiagnosticAnalyzer"/>
27+
/// </summary>
28+
protected override void InitializeCompilation(CompilationStartAnalysisContext context)
29+
{
30+
context.RegisterSyntaxNodeAction(AnalyzeForStatement, SyntaxKind.ForStatement);
31+
}
32+
33+
private void AnalyzeForStatement(SyntaxNodeAnalysisContext context)
34+
{
35+
if (Helper.ForGeneratedCode.IsGeneratedCode(context))
36+
{
37+
return;
38+
}
39+
40+
var forStatement = (ForStatementSyntax)context.Node;
41+
42+
// Check if this is a backwards loop with problematic condition
43+
if (IsBackwardsLoopWithIncorrectCondition(forStatement))
44+
{
45+
Location location = forStatement.Condition.GetLocation();
46+
context.ReportDiagnostic(Diagnostic.Create(Rule, location));
47+
}
48+
}
49+
50+
private static bool IsBackwardsLoopWithIncorrectCondition(ForStatementSyntax forStatement)
51+
{
52+
// Must have a condition
53+
if (forStatement.Condition == null)
54+
{
55+
return false;
56+
}
57+
58+
// Must have incrementors (decrementors in this case)
59+
if (forStatement.Incrementors.Count == 0)
60+
{
61+
return false;
62+
}
63+
64+
// Check if any incrementor is decrementing
65+
var hasDecrement = false;
66+
var decrementedVariable = string.Empty;
67+
68+
foreach (ExpressionSyntax incrementor in forStatement.Incrementors)
69+
{
70+
if (IsDecrementOperation(incrementor, out var varName))
71+
{
72+
hasDecrement = true;
73+
decrementedVariable = varName;
74+
break;
75+
}
76+
}
77+
78+
if (!hasDecrement || string.IsNullOrEmpty(decrementedVariable))
79+
{
80+
return false;
81+
}
82+
83+
// Check if condition is "variable > 0"
84+
return IsGreaterThanZeroCondition(forStatement.Condition, decrementedVariable);
85+
}
86+
87+
private static bool IsDecrementOperation(ExpressionSyntax expression, out string variableName)
88+
{
89+
variableName = null;
90+
91+
switch (expression)
92+
{
93+
// Handle i--, --i
94+
case PostfixUnaryExpressionSyntax postfix when postfix.IsKind(SyntaxKind.PostDecrementExpression):
95+
if (postfix.Operand is IdentifierNameSyntax postfixIdentifier)
96+
{
97+
variableName = postfixIdentifier.Identifier.ValueText;
98+
return true;
99+
}
100+
break;
101+
102+
case PrefixUnaryExpressionSyntax prefix when prefix.IsKind(SyntaxKind.PreDecrementExpression):
103+
if (prefix.Operand is IdentifierNameSyntax prefixIdentifier)
104+
{
105+
variableName = prefixIdentifier.Identifier.ValueText;
106+
return true;
107+
}
108+
break;
109+
110+
// Handle i -= 1
111+
case AssignmentExpressionSyntax assignment when assignment.IsKind(SyntaxKind.SubtractAssignmentExpression):
112+
if (assignment.Left is IdentifierNameSyntax subtractIdentifier && IsLiteralOne(assignment.Right))
113+
{
114+
variableName = subtractIdentifier.Identifier.ValueText;
115+
return true;
116+
}
117+
break;
118+
119+
// Handle i = i - 1
120+
case AssignmentExpressionSyntax assignment when assignment.IsKind(SyntaxKind.SimpleAssignmentExpression):
121+
if (assignment.Left is IdentifierNameSyntax leftId &&
122+
assignment.Right is BinaryExpressionSyntax binary &&
123+
binary.IsKind(SyntaxKind.SubtractExpression) &&
124+
binary.Left is IdentifierNameSyntax rightId &&
125+
leftId.Identifier.ValueText == rightId.Identifier.ValueText &&
126+
IsLiteralOne(binary.Right))
127+
{
128+
variableName = leftId.Identifier.ValueText;
129+
return true;
130+
}
131+
break;
132+
}
133+
134+
return false;
135+
}
136+
137+
private static bool IsLiteralOne(ExpressionSyntax expression)
138+
{
139+
return expression is LiteralExpressionSyntax literal &&
140+
literal.IsKind(SyntaxKind.NumericLiteralExpression) &&
141+
literal.Token.ValueText == "1";
142+
}
143+
144+
private static bool IsGreaterThanZeroCondition(ExpressionSyntax condition, string variableName)
145+
{
146+
if (condition is BinaryExpressionSyntax binary)
147+
{
148+
// Check for "variable > 0"
149+
if (binary.IsKind(SyntaxKind.GreaterThanExpression) &&
150+
binary.Left is IdentifierNameSyntax leftId &&
151+
leftId.Identifier.ValueText == variableName &&
152+
IsLiteralZero(binary.Right))
153+
{
154+
return true;
155+
}
156+
157+
// Check for "0 < variable" (equivalent but less common)
158+
if (binary.IsKind(SyntaxKind.LessThanExpression) &&
159+
IsLiteralZero(binary.Left) &&
160+
binary.Right is IdentifierNameSyntax rightId &&
161+
rightId.Identifier.ValueText == variableName)
162+
{
163+
return true;
164+
}
165+
}
166+
167+
return false;
168+
}
169+
170+
private static bool IsLiteralZero(ExpressionSyntax expression)
171+
{
172+
return expression is LiteralExpressionSyntax literal &&
173+
literal.IsKind(SyntaxKind.NumericLiteralExpression) &&
174+
literal.Token.ValueText == "0";
175+
}
176+
}
177+
}

‎Philips.CodeAnalysis.MaintainabilityAnalyzers/Naming/NamespaceMatchFilePathAnalyzer.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public override void Analyze()
6363
private bool IsNamespacePartOfPath(string ns, string path)
6464
{
6565
var nodes = path.Split(Path.DirectorySeparatorChar);
66-
for (var i = nodes.Length - 2; i > 0; i--) // Exclude file.cs (i.e., the end) and the drive (i.e., the start). Start from back to succeed quickly.
66+
for (var i = nodes.Length - 2; i >= 1; i--) // Exclude file.cs (i.e., the end) and the drive (i.e., the start). Start from back to succeed quickly.
6767
{
6868
if (string.Equals(nodes[i], ns, StringComparison.OrdinalIgnoreCase))
6969
{

‎Philips.CodeAnalysis.MaintainabilityAnalyzers/Philips.CodeAnalysis.MaintainabilityAnalyzers.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
<PropertyGroup>
1010
<Description>Roslyn Diagnostic Analyzers for helping maintainability or readability of C# code</Description>
1111
<PackageTags>CSharp Maintainability Roslyn CodeAnalysis analyzers Philips</PackageTags>
12+
<PackageReleaseNotes>Added PH2144: AvoidIncorrectForLoopCondition analyzer to detect backwards for-loop boundary check issues</PackageReleaseNotes>
1213
</PropertyGroup>
1314

1415
<ItemGroup>

0 commit comments

Comments
 (0)