Skip to content
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

Update RSPECs as part of the LayC migration (sixth batch) #7454

Merged
merged 1 commit into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions analyzers/rspec/cs/S108.html
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
<h2>Why is this an issue?</h2>
<p>Most of the time a block of code is empty when a piece of code is really missing. So such empty block must be either filled or removed.</p>
<h3>Noncompliant code example</h3>
<p>An empty code block is confusing. It will require some effort from maintainers to determine if it is intentional or indicates the implementation is
incomplete.</p>
<pre>
for (int i = 0; i &lt; 42; i++){} // Empty on purpose or missing piece of code ?
for (int i = 0; i &lt; 42; i++){} // Noncompliant: is the block empty on purpose, or is code missing?
</pre>
<p>Removing or filling the empty code blocks takes away ambiguity and generally results in a more straightforward and less surprising code.</p>
<h3>Exceptions</h3>
<p>When a block contains a comment, this block is not considered to be empty.</p>
<p>The rule ignores code blocks that contain comments.</p>

6 changes: 2 additions & 4 deletions analyzers/rspec/cs/S1110.html
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ <h2>Why is this an issue?</h2>
<p>The use of parentheses, even those not required to enforce a desired order of operations, can clarify the intent behind a piece of code. But
redundant pairs of parentheses could be misleading, and should be removed.</p>
<h3>Noncompliant code example</h3>
<pre>
<pre data-diff-id="1" data-diff-type="noncompliant">
if (a &amp;&amp; ((x + y &gt; 0))) // Noncompliant
{
//...
Expand All @@ -11,14 +11,12 @@ <h3>Noncompliant code example</h3>
return ((x + 1)); // Noncompliant
</pre>
<h3>Compliant solution</h3>
<pre>
<pre data-diff-id="1" data-diff-type="compliant">
if (a &amp;&amp; (x + y &gt; 0))
{
//...
}

return x + 1;

return (x + 1);
</pre>

17 changes: 12 additions & 5 deletions analyzers/rspec/cs/S1117.html
Original file line number Diff line number Diff line change
@@ -1,17 +1,24 @@
<h2>Why is this an issue?</h2>
<p>Overriding or shadowing a variable declared in an outer scope can strongly impact the readability, and therefore the maintainability, of a piece of
code. Further, it could lead maintainers to introduce bugs because they think they’re using one variable but are really using another.</p>
<h3>Noncompliant code example</h3>
<p>Overriding or shadowing a field or a property declared in an outer scope can strongly impact the readability, and therefore the maintainability, of
a piece of code. Developers may mistakenly assume they are modifying or accessing the class field/property when, in fact, they are working with the
local variable.</p>
<pre>
class Foo
{
public int myField;
public int MyProperty { get; set; }

public void DoSomething()
{
int myField = 0; // Noncompliant
...
int myField = 0; // Noncompliant
int MyProperty = 0; // Noncompliant
}
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/fields">Fields</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/properties">Properties</a> </li>
</ul>

4 changes: 2 additions & 2 deletions analyzers/rspec/cs/S1117.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Local variables should not shadow class fields",
"title": "Local variables should not shadow class fields or properties",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
Expand All @@ -14,5 +14,5 @@
"ruleSpecification": "RSPEC-1117",
"sqKey": "S1117",
"scope": "All",
"quickfix": "unknown"
"quickfix": "infeasible"
}
49 changes: 35 additions & 14 deletions analyzers/rspec/cs/S1479.html
Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
<h2>Why is this an issue?</h2>
<p>When <code>switch</code> statements have large sets of case clauses, it is usually an attempt to map two sets of data. A <code>Dictionary</code>
should be used instead to make the code more readable and maintainable.</p>
<h3>Noncompliant code example</h3>
<p>With a "Maximum number of case" set to 4</p>
<pre>
<p>When <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/selection-statements#the-switch-statement">switch</a>
statements have large sets of case clauses, it is usually an attempt to map two sets of data. A <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.dictionary-2">Dictionary</a> should be used instead to make the code
more readable and maintainable.</p>
<h3>Exceptions</h3>
<p>This rule ignores <code>switch</code> statements over <code>Enum</code> arguments and empty, fall-through cases.</p>
<h2>How to fix it</h2>
<p>Store all the cases apart from the <code>default</code> one in a dictionary and try to get the matching value by calling the <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.dictionary-2.trygetvalue">TryGetValue</a> method.</p>
<h3>Code examples</h3>
<p>The example below are using the "Maximum number of case" property set to <code>4</code>.</p>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
public class TooManyCase
{
public int switchCase(char ch)
public int mapValues(char ch)
{
switch(ch) { // Noncompliant
switch(ch) { // Noncompliant: 5 cases, "default" excluded, more than maximum = 4
case 'a':
return 1;
case 'b':
Expand All @@ -28,19 +36,25 @@ <h3>Noncompliant code example</h3>
}
}
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
using System.Collections.Generic;

public class TooManyCase
{
Dictionary&lt;char, int&gt; matching = new Dictionary&lt;char, int&gt;()
{
{'a', 1}, {'b', 2}, {'c', 2}, {'d', 3},
{'e', 4}, {'f', 5}, {'g', 5}, {'h', 5}
{ 'a', 1 },
{ 'b', 2 },
{ 'c', 2 },
{ 'd', 3 },
{ 'e', 4 },
{ 'f', 5 },
{ 'g', 5 },
{ 'h', 5 }
};

public int withDictionary(char ch)
public int mapValues(char ch)
{
int value;
if (this.matching.TryGetValue(ch, out value)) {
Expand All @@ -51,6 +65,13 @@ <h3>Compliant solution</h3>
}
}
</pre>
<h3>Exceptions</h3>
<p>This rule ignores <code>switch</code>es over <code>Enum</code>s and empty, fall-through cases.</p>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.dictionary-2">Dictionary&lt;TKey,TValue&gt; Class</a> </li>
<li> <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.collections.generic.dictionary-2.trygetvalue">Dictionary&lt;TKey,TValue&gt;.TryGetValue(TKey, TValue) Method</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/selection-statements#the-switch-statement">The
<code>switch</code> statement</a> </li>
</ul>

2 changes: 1 addition & 1 deletion analyzers/rspec/cs/S1479.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,5 @@
"ruleSpecification": "RSPEC-1479",
"sqKey": "S1479",
"scope": "All",
"quickfix": "unknown"
"quickfix": "infeasible"
}
174 changes: 142 additions & 32 deletions analyzers/rspec/cs/S2190.html
Original file line number Diff line number Diff line change
@@ -1,50 +1,160 @@
<h2>Why is this an issue?</h2>
<p>Recursion happens when control enters a loop that has no exit. This can happen a method invokes itself, when a pair of methods invoke each other,
or when <code>goto</code> statements are used to move between two segments of code. It can be a useful tool, but unless the method includes a
provision to break out of the recursion and <code>return</code>, the recursion will continue until the stack overflows and the program crashes.</p>
<h3>Noncompliant code example</h3>
<p>Having an infinite loop or recursion will lead to a program failure or a program never finishing the execution.</p>
<pre>
int Pow(int num, int exponent) // Noncompliant; no condition under which pow isn't re-called
public int Sum()
{
num = num * Pow(num, exponent-1);
return num; // this is never reached
var i = 0;
var result = 0;
while (true) // Noncompliant: the program will never stop
{
result += i;
i++;
}
return result;
}

void WhileLoop() // Noncompliant; no condition under which while loop would exit
</pre>
<p>This can happen in multiple scenarios.</p>
<h3>Loop statements</h3>
<p><code>while</code> and <code>for</code> loops with no <code>break</code> or <code>return</code> statements and with the exit condition always
<code>false</code> will be indefinitely executed.</p>
<h3>"goto" statements</h3>
<p><code>goto</code> statement with nothing that stops it from being executed over and over again will prevent the program from the completion.</p>
<h3>Recursion</h3>
<p>When a <a href="https://en.wikipedia.org/wiki/Recursion_(computer_science)">recursive</a> method call chain lacks an exit condition, the <a
href="https://en.wikipedia.org/wiki/Call_stack">call stack</a> will reach its limit and the program will crash due to a <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.stackoverflowexception">StackOverflowException</a>.</p>
<pre>
int Pow(int num, int exponent)
{
while (true)
{
var line = Console.ReadLine();
Console.WriteLine(line);
}
return num * Pow(num, exponent - 1); // Noncompliant: no condition under which Pow isn't re-called
}

void InternalRecursion(int i)
</pre>
<p>In this example, <code>Pow</code> will keep calling <code>Pow</code> with <code>exponent - 1</code> forever, until the program crashes with a
StackOverflowException.</p>
<p>Recursion provides some benefits.</p>
<ul>
<li> <strong>Simplified code</strong>: recursion can often lead to more concise and elegant code by breaking down complex problems into smaller,
more manageable parts. </li>
<li> <strong>Improved code readability</strong>: compared to iterative solutions, recursive solutions can be easier to understand and reason about.
</li>
</ul>
<p>However, it has disadvantages as well.</p>
<ul>
<li> <strong>Stack overflow</strong>: Recursive functions can lead to <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.stackoverflowexception">stack overflow</a> if the recursion is too deep, potentially
causing the program to crash. </li>
<li> <strong>Performance overhead</strong>: Recursive function calls can lead to poor performance due to the need to push and pop <a
href="https://en.citizendium.org/wiki/Stack_frame#:~:text=In%20computer%20science%2C%20a%20stack,only%20exist%20at%20run%2Dtime">stack frames</a>,
making them potentially slower than iterative solutions. </li>
<li> <strong>Difficulty in debugging</strong>: Debugging recursive code can be challenging, as multiple recursive calls can make it harder to track
the flow of execution and identify logical errors. </li>
<li> <strong>Space complexity</strong>: Recursive algorithms may require more memory compared to iterative approaches, as each recursive call adds a
new frame to the call stack. </li>
<li> <strong>Lack of control</strong>: Recursion can sometimes lead to infinite loops or unexpected behavior if not properly implemented or
terminated, making it crucial to have proper base cases and exit conditions. </li>
</ul>
<h2>How to fix it</h2>
<p>The program’s logic should incorporate a mechanism to break out of the control flow loop. Here are some examples.</p>
<h3>Code examples</h3>
<ul>
<li> Use a loop condition which eventually evaluates to <code>false</code> </li>
</ul>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
public int Sum()
{
start:
goto end;
end:
goto start; // Noncompliant; there's no way to break out of this method
var i = 0;
var result = 0;
while (true) // Noncompliant: the program will never stop
{
result += i;
i++;
}
return result;
}
</pre>
<h3>Compliant solution</h3>
<pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
public int Sum()
{
var i = 0;
var result = 0;
while (result &lt; 1000)
{
result += i;
i++;
}
return result;
}
</pre>
<ul>
<li> As <a href="https://rules.sonarsource.com/csharp/RSPEC-907">{rule:csharpsquid:S907}</a> generally suggests, avoid using <code>goto</code>
statements. Instead, you can use a loop statement or explicit recursion. </li>
</ul>
<h4>Noncompliant code example</h4>
<pre data-diff-id="2" data-diff-type="noncompliant">
public int Sum()
{
var result = 0;
var i = 0;
iteration:
// Noncompliant: program never ends
result += i;
i++;
goto iteration;
return result;
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="2" data-diff-type="compliant">
public int Sum()
{
var i = 0;
var result = 0;
while (result &lt; 1000)
{
result += i;
i++;
}
return result;
}
</pre>
<ul>
<li> For a recursion make sure there is a base case when the recursive method is not re-called. </li>
</ul>
<h4>Noncompliant code example</h4>
<pre data-diff-id="3" data-diff-type="noncompliant">
int Pow(int num, int exponent)
{
if (exponent &gt; 1) // recursion now conditional and stop-able
{
num = num * Pow(num, exponent-1);
}
return num;
return num * Pow(num, exponent - 1); // Noncompliant: no condition under which Pow isn't re-called
}

void WhileLoop()
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="3" data-diff-type="compliant">
int Pow(int num, int exponent)
{
string line;
while ((line = Console.ReadLine()) != null) // loop has clear exit condition
if (exponent &gt; 1) // recursion is now conditional and stoppable
{
Console.WriteLine(line);
num = num * Pow(num, exponent - 1);
}
return num;
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/iteration-statements#the-for-statement">The "for"
statement</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/iteration-statements#the-while-statement">The "while"
statement</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/statements/jump-statements#the-goto-statement">The "goto"
statement</a> </li>
<li> <a href="https://en.wikipedia.org/wiki/Recursion_(computer_science)">Recursion - wiki</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/api/system.stackoverflowexception?view=net-7.0">StackOverflowException class</a> </li>
<li> <a href="https://rules.sonarsource.com/csharp/RSPEC-907">{rule:csharpsquid:S907}: "goto" statement should not be used</a> </li>
</ul>
<h3>Articles &amp; blog posts</h3>
<ul>
<li> <a href="https://www.cs.utexas.edu/users/EWD/transcriptions/EWD02xx/EWD215.html">Edsger Dijkstra: A Case against the GO TO Statement</a> </li>
</ul>

4 changes: 2 additions & 2 deletions analyzers/rspec/cs/S2190.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"title": "Recursion should not be infinite",
"title": "Loops and recursions should not be infinite",
"type": "BUG",
"status": "ready",
"remediation": {
Expand All @@ -13,5 +13,5 @@
"ruleSpecification": "RSPEC-2190",
"sqKey": "S2190",
"scope": "All",
"quickfix": "unknown"
"quickfix": "infeasible"
}
Loading