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.

Sign up for GitHub

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 RSPEC before 9.3 release #7336

Merged
merged 3 commits into from
Jun 6, 2023
Merged

Update RSPEC before 9.3 release #7336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4dcf1d
C# Changes
gregory-paidis-sonarsource Jun 6, 2023
ccdeae0
VB.NET changes
gregory-paidis-sonarsource Jun 6, 2023
149820b
Fix S3260 benchmarks
gregory-paidis-sonarsource Jun 6, 2023
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
165 changes: 159 additions & 6 deletions analyzers/rspec/cs/S3260.html
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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