Skip to content

Commit

Permalink
Update RSPEC before 9.3 release (#7336)
Browse files Browse the repository at this point in the history
  • Loading branch information
@gregory-paidis-sonarsource
gregory-paidis-sonarsource authored Jun 6, 2023
1 parent e63e4bc commit e47cf88
Show file tree
Hide file tree
Showing 14 changed files with 483 additions and 381 deletions.
165 changes: 159 additions & 6 deletions analyzers/rspec/cs/S3260.html
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,15 @@
<h2>Why is this an issue?</h2>
<p><code>private</code> classes and records aren’t visible outside of their assemblies anyway, so if they’re not extended inside the assemblies, they
should be made explicitly non-extensible with the addition of the <code>sealed</code> keyword.</p>
<h3>Noncompliant code example</h3>
<pre>
<p>Classes and records with either <code>private</code> or <code>file</code> access modifiers aren’t visible outside of their assemblies or files, so
if they’re not extended inside their scope, they should be made explicitly non-extensible with the addition of the <code>sealed</code> keyword.</p>
<h3>What is the potential impact?</h3>
<p>We measured at least 4x improvement in execution time. For more details see the <code>Benchmarks</code> section from the <code>More info</code>
tab.</p>
<h2>How to fix it</h2>
<p>The code can be improved by adding the <code>sealed</code> keyword in front of the <code>class</code> or <code>record</code> types that have no
inheritors.</p>
<h3>Code examples</h3>
<h4>Noncompliant code example</h4>
<pre data-diff-id="1" data-diff-type="noncompliant">
private class MyClass // Noncompliant
{
// ...
Expand All @@ -13,8 +20,19 @@ <h3>Noncompliant code example</h3>
// ...
}
</pre>
<h3>Compliant solution</h3>
<pre>
<pre data-diff-id="2" data-diff-type="noncompliant">
file class MyClass // Noncompliant
{
// ...
}

file record MyRecord // Noncompliant
{
// ...
}
</pre>
<h4>Compliant solution</h4>
<pre data-diff-id="1" data-diff-type="compliant">
private sealed class MyClass
{
// ...
Expand All @@ -25,4 +43,139 @@ <h3>Compliant solution</h3>
// ...
}
</pre>
<pre data-diff-id="2" data-diff-type="compliant">
file sealed class MyClass
{
// ...
}

file sealed record MyRecord
{
// ...
}
</pre>
<h2>Resources</h2>
<h3>Documentation</h3>
<ul>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/sealed">The <code>sealed</code> keyword</a> </li>
</ul>
<h3>Articles &amp; blog posts</h3>
<ul>
<li> <a href="https://code-maze.com/improve-performance-sealed-classes-dotnet">Boosting Performance With Sealed Classes in .NET</a> </li>
<li> <a href="https://devblogs.microsoft.com/dotnet/performance-improvements-in-net-6/#peanut-butter">Performance Improvements in .NET 6</a> </li>
</ul>
<h3>Benchmarks</h3>
<table>
<colgroup>
<col style="width: 20%;">
<col style="width: 20%;">
<col style="width: 20%;">
<col style="width: 20%;">
<col style="width: 20%;">
</colgroup>
<thead>
<tr>
<th>Method</th>
<th>Runtime</th>
<th>Mean</th>
<th>StdDev</th>
<th>Ratio</th>
</tr>
</thead>
<tbody>
<tr>
<td><p>UnsealedType</p></td>
<td><p>.NET 5.0</p></td>
<td><p>918.7 us</p></td>
<td><p>10.72 us</p></td>
<td><p>1.00</p></td>
</tr>
<tr>
<td><p>SealedType</p></td>
<td><p>.NET 5.0</p></td>
<td><p>231.2 us</p></td>
<td><p>3.20 us</p></td>
<td><p>0.25</p></td>
</tr>
<tr>
<td><p>UnsealedType</p></td>
<td><p>.NET 6.0</p></td>
<td><p>867.9 us</p></td>
<td><p>5.65 us</p></td>
<td><p>1.00</p></td>
</tr>
<tr>
<td><p>SealedType</p></td>
<td><p>.NET 6.0</p></td>
<td><p>218.4 us</p></td>
<td><p>0.59 us</p></td>
<td><p>0.25</p></td>
</tr>
<tr>
<td><p>UnsealedType</p></td>
<td><p>.NET 7.0</p></td>
<td><p>1,074.5 us</p></td>
<td><p>3.15 us</p></td>
<td><p>1.00</p></td>
</tr>
<tr>
<td><p>SealedType</p></td>
<td><p>.NET 7.0</p></td>
<td><p>216.1 us</p></td>
<td><p>1.19 us</p></td>
<td><p>0.20</p></td>
</tr>
</tbody>
</table>
<p>The results were generated by running the following snippet with <a href="https://github.com/dotnet/BenchmarkDotNet">BenchmarkDotNet</a>:</p>
<pre>
[Params(1_000_000)]
public int Iterations { get; set; }

private readonly UnsealedClass unsealedType = new UnsealedClass();
private readonly SealedClass sealedType = new SealedClass();

[Benchmark(Baseline = true)]
public void UnsealedType()
{
for (int i = 0; i &lt; Iterations; i++)
{
unsealedType.DoNothing();
}
}

[Benchmark]
public void SealedType()
{
for (int i = 0; i &lt; Iterations; i++)
{
sealedType.DoNothing();
}
}

private class BaseType
{
public virtual void DoNothing() { }
}

private class UnsealedClass : BaseType
{
public override void DoNothing() { }
}

private sealed class SealedClass : BaseType
{
public override void DoNothing() { }
}
</pre>
<p>Hardware Configuration:</p>
<pre>
BenchmarkDotNet=v0.13.5, OS=Windows 10 (10.0.19045.2846/22H2/2022Update)
12th Gen Intel Core i7-12800H, 1 CPU, 20 logical and 14 physical cores
.NET SDK=7.0.203
[Host] : .NET 7.0.5 (7.0.523.17405), X64 RyuJIT AVX2
.NET 5.0 : .NET 5.0.17 (5.0.1722.21314), X64 RyuJIT AVX2
.NET 6.0 : .NET 6.0.16 (6.0.1623.17311), X64 RyuJIT AVX2
.NET 7.0 : .NET 7.0.5 (7.0.523.17405), X64 RyuJIT AVX2
</pre>

21 changes: 10 additions & 11 deletions analyzers/rspec/cs/S3972.html
View file
Original file line number Diff line number Diff line change
@@ -1,31 +1,30 @@
<h2>Why is this an issue?</h2>
<p>Code is clearest when each statement has its own line. Nonetheless, it is a common pattern to combine on the same line an <code>if</code> and its
resulting <em>then</em> statement. However, when an <code>if</code> is placed on the same line as the closing <code>}</code> from a preceding
<em>then</em>, <em>else</em> or <em>else if</em> part, it is either an error - <code>else</code> is missing - or the invitation to a future error as
maintainers fail to understand that the two statements are unconnected.</p>
<h3>Noncompliant code example</h3>
<p>When an <code>if</code> is placed on the same line as the closing <code>}</code> from a preceding <code>if</code>, <code>else</code> or <code>else
if</code> part, it is either an error - <code>else</code> is missing - or the invitation to a future error as maintainers fail to understand that the
two statements are unconnected.</p>
<p>This code is confusing</p>
<pre>
if (condition1) {
// ...
} if (condition2) { // Noncompliant
//...
}
</pre>
<h3>Compliant solution</h3>
<p>Either the two conditions are unrelated and they should be visually separated</p>
<pre>
if (condition1) {
// ...
} else if (condition2) {
}

if (condition2) {
//...
}
</pre>
<p>Or</p>
<p>Or they were supposed to be exclusive and you should use <code>else if</code> instead</p>
<pre>
if (condition1) {
// ...
}

if (condition2) {
} else if (condition2) {
//...
}
</pre>
Expand Down
32 changes: 19 additions & 13 deletions analyzers/rspec/cs/S3973.html
View file
Original file line number Diff line number Diff line change
@@ -1,25 +1,31 @@
<h2>Why is this an issue?</h2>
<p>In the absence of enclosing curly braces, the line immediately after a conditional is the one that is conditionally executed. By both convention
and good practice, such lines are indented. In the absence of both curly braces and indentation the intent of the original programmer is entirely
unclear and perhaps not actually what is executed. Additionally, such code is highly likely to be confusing to maintainers.</p>
<h3>Noncompliant code example</h3>
<p>When the line immediately after a conditional has neither curly braces nor indentation, the intent of the code is unclear and perhaps not what is
executed. Additionally, such code is confusing to maintainers.</p>
<pre>
if (condition) // Noncompliant
DoTheThing();

DoTheOtherThing();
SomethingElseEntirely();

Foo();
DoTheOtherThing(); // Was the intent to call this function unconditionally?
</pre>
<h3>Compliant solution</h3>
<p>It becomes even more confusing and bug-prone if lines get commented out.</p>
<pre>
if (condition) // Noncompliant
// DoTheThing();
DoTheOtherThing(); // Was the intent to call this function conditionally?
</pre>
<p>Indentation alone or together with curly braces makes the intent clear.</p>
<pre>
if (condition)
DoTheThing();
DoTheOtherThing(); // Clear intent to call this function unconditionally

DoTheOtherThing();
SomethingElseEntirely();
// or

Foo();
if (condition)
{
DoTheThing();
}
DoTheOtherThing(); // Clear intent to call this function unconditionally
</pre>
<p>This rule raises an issue if the line controlled by a conditional has the same indentation as the conditional and is not enclosed in curly
braces.</p>

20 changes: 10 additions & 10 deletions analyzers/rspec/cs/S4144.html
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
<h2>Why is this an issue?</h2>
<p>When two methods have the same implementation, either it was a mistake - something else was intended - or the duplication was intentional, but may
be confusing to maintainers. In the latter case, one implementation should invoke the other.</p>
<h3>Noncompliant code example</h3>
<pre>
private const string CODE = "bounteous";
<p>Two methods having the same implementation are suspicious. It might be that something else was intended. Or the duplication is intentional, which
becomes a maintenance burden.</p>
<pre data-diff-id="1" data-diff-type="noncompliant">
private const string CODE = "secret";
private int callCount = 0;

public string GetCode()
Expand All @@ -12,15 +11,16 @@ <h3>Noncompliant code example</h3>
return CODE;
}

public string GetName() // Noncompliant
public string GetName() // Noncompliant: duplicates GetCode
{
callCount++;
return CODE;
}
</pre>
<h3>Compliant solution</h3>
<pre>
private const string CODE = "bounteous";
<p>If the identical logic is intentional, the code should be refactored to avoid duplication. For example, by having both methods call the same method
or by having one implementation invoke the other.</p>
<pre data-diff-id="1" data-diff-type="compliant">
private const string CODE = "secret";
private int callCount = 0;

public string GetCode()
Expand All @@ -29,7 +29,7 @@ <h3>Compliant solution</h3>
return CODE;
}

public string GetName()
public string GetName() // Intent is clear
{
return GetCode();
}
Expand Down
6 changes: 6 additions & 0 deletions analyzers/rspec/cs/S6605.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ <h3>What is the potential impact?</h3>
<p>We measured at least 3x improvement in execution time. For more details see the <code>Benchmarks</code> section from the <code>More info</code>
tab.</p>
<p>Also, no memory allocations were needed for the <code>Exists</code> method, since the search is done in-place.</p>
<h3>Exceptions</h3>
<p>Since <code><a href="https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/ef/language-reference/linq-to-entities">LINQ to
Entities</a></code> relies a lot on <code>System.Linq</code> for <a
href="https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/ef/language-reference/linq-to-entities#query-conversion">query conversion</a>,
this rule won’t raise when used within LINQ to Entities syntaxes.</p>
<h2>How to fix it</h2>
<p>The <code>Exists</code> method is defined on the collection class, and it has the same signature as <code>Any</code> extension method if a
predicate is used. The method can be replaced in place.</p>
Expand Down Expand Up @@ -44,6 +49,7 @@ <h3>Documentation</h3>
<li> <a
href="https://learn.microsoft.com/en-us/dotnet/api/system.collections.immutable.immutablelist-1.exists">ImmutableList&lt;T&gt;.Exists(Predicate&lt;T&gt;)</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/api/system.linq.enumerable.any">Enumerable.Any(Predicate&lt;T&gt;)</a> </li>
<li> <a href="https://learn.microsoft.com/en-us/dotnet/framework/data/adonet/ef/language-reference/linq-to-entities">LINQ to Entities</a> </li>
</ul>
<h3>Benchmarks</h3>
<table>
Expand Down
Loading

0 comments on commit e47cf88

Please sign in to comment.