From b48e7f687ebada9656877b2613a1c8e494b0ef5b Mon Sep 17 00:00:00 2001 From: Andrei Epure <38876598+andrei-epure-sonarsource@users.noreply.github.com> Date: Wed, 1 Mar 2023 14:13:56 +0100 Subject: [PATCH 01/12] Coding style: Improve wording (#6787) --- docs/coding-style.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/coding-style.md b/docs/coding-style.md index ba816b65615..35820c2e51a 100644 --- a/docs/coding-style.md +++ b/docs/coding-style.md @@ -27,16 +27,16 @@ Static fields and properties should be placed before instance ones. Static methods are preferred to be after instance methods. -Methods which are called by other methods in the same category should be placed below the callers. +Once grouped as specified above, methods which are called by other methods in the same group should be placed below the callers. ```csharp -public int Bar() => 2; +public int PublicMethod() => 42; -int CallerOne(int x) => Foo() + x; +int CallerOne() => Leaf(); -int CallerTwo(int x) => Foo() + x + Bar(); +int CallerTwo() => Leaf() + PublicMethod(); -int Foo() => 1; // this is placed at the bottom, as it's a "leaf" method within the same category +int Leaf() => 42; ``` Local functions should always be placed at the end of a method. From cc3e1fd3eecef65ee15ff7b1a704edceff69857b Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com> Date: Wed, 1 Mar 2023 14:15:03 +0100 Subject: [PATCH 02/12] Improve S3398: Fix code smells (#6827) --- .../Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs index 44998f457b3..bfb3748020a 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs @@ -178,7 +178,8 @@ private sealed record MethodUsedByTypes(MethodDeclarationSyntax Method, TypeDecl /// /// Collects all the potential references to a set of methods inside the given syntax node. /// The collector looks for identifiers which match any of the methods' names, but does not try to resolve them to symbols with the semantic model. - /// Performance gains: by only using the syntax tree to find matches we can eliminate certain methods (which are only used by the type which has declared it) without using the more costly symbolic lookup. + /// Performance gains: by only using the syntax tree to find matches we can eliminate certain methods (which are only used by the type which has declared it) + /// without using the more costly symbolic lookup. /// private sealed class PotentialMethodReferenceCollector : CSharpSyntaxWalker { From 9715c02980ed8176d30b520a3bbba1612daf5e9a Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Thu, 2 Mar 2023 08:55:38 +0100 Subject: [PATCH 03/12] SyntaxFacade: fix the order of parameters in IsKnownAttributeType (#6838) --- analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs | 2 +- analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs | 2 +- .../Rules/DebuggerDisplayUsesExistingMembersBase.cs | 2 +- .../SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs index 21f0560d744..5c1b8a8f12f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxFacade.cs @@ -50,7 +50,7 @@ internal sealed class CSharpSyntaxFacade : SyntaxFacade public override bool IsNullLiteral(SyntaxNode node) => node.IsNullLiteral(); - public override bool IsKnownAttributeType(SyntaxNode attribute, KnownType knownType, SemanticModel model) => + public override bool IsKnownAttributeType(SemanticModel model, SyntaxNode attribute, KnownType knownType) => AttributeSyntaxExtensions.IsKnownType(Cast(attribute), knownType, model); public override IEnumerable ArgumentExpressions(SyntaxNode node) => diff --git a/analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs index f08ceed8ff4..a195916ef39 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs +++ b/analyzers/src/SonarAnalyzer.Common/Facade/SyntaxFacade.cs @@ -32,7 +32,7 @@ public abstract class SyntaxFacade public abstract bool IsAnyKind(SyntaxNode node, ISet syntaxKinds); public abstract bool IsAnyKind(SyntaxNode node, params TSyntaxKind[] syntaxKinds); public abstract bool IsAnyKind(SyntaxTrivia trivia, params TSyntaxKind[] syntaxKinds); - public abstract bool IsKnownAttributeType(SyntaxNode attribute, KnownType knownType, SemanticModel model); + public abstract bool IsKnownAttributeType(SemanticModel model, SyntaxNode attribute, KnownType knownType); public abstract IEnumerable ArgumentExpressions(SyntaxNode node); public abstract ImmutableArray AssignmentTargets(SyntaxNode assignment); public abstract SyntaxNode AssignmentLeft(SyntaxNode assignment); diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/DebuggerDisplayUsesExistingMembersBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/DebuggerDisplayUsesExistingMembersBase.cs index 1453d0d5150..1ae5dc4f1c6 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/DebuggerDisplayUsesExistingMembersBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/DebuggerDisplayUsesExistingMembersBase.cs @@ -43,7 +43,7 @@ public abstract class DebuggerDisplayUsesExistingMembersBase { var attribute = (TAttributeSyntax)c.Node; - if (Language.Syntax.IsKnownAttributeType(attribute, KnownType.System_Diagnostics_DebuggerDisplayAttribute, c.SemanticModel) + if (Language.Syntax.IsKnownAttributeType(c.SemanticModel, attribute, KnownType.System_Diagnostics_DebuggerDisplayAttribute) && AttributeFormatString(attribute) is { } formatString && Language.Syntax.StringValue(formatString, c.SemanticModel) is { } formatStringText && FirstInvalidMemberName(c, formatStringText, attribute) is { } firstInvalidMember) diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs index 66a63dfaa0e..1145946ad4e 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicSyntaxFacade.cs @@ -50,7 +50,7 @@ internal sealed class VisualBasicSyntaxFacade : SyntaxFacade public override bool IsAnyKind(SyntaxTrivia trivia, params SyntaxKind[] syntaxKinds) => trivia.IsAnyKind(syntaxKinds); - public override bool IsKnownAttributeType(SyntaxNode attribute, KnownType knownType, SemanticModel model) => + public override bool IsKnownAttributeType(SemanticModel model, SyntaxNode attribute, KnownType knownType) => AttributeSyntaxExtensions.IsKnownType(Cast(attribute), knownType, model); public override IEnumerable ArgumentExpressions(SyntaxNode node) => From 4903462ee837b7e7192d81b5e76d79323e1cefd6 Mon Sep 17 00:00:00 2001 From: Mary Georgiou <89914005+mary-georgiou-sonarsource@users.noreply.github.com> Date: Thu, 2 Mar 2023 11:32:37 +0100 Subject: [PATCH 04/12] Add reproducer for #6836 (#6842) --- .../Rules/DeclareTypesInNamespacesTest.cs | 5 ++++- ...lareTypesInNamespaces.AfterCSharp9.PartialProgramClass.cs | 3 +++ 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/DeclareTypesInNamespaces.AfterCSharp9.PartialProgramClass.cs diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/DeclareTypesInNamespacesTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/DeclareTypesInNamespacesTest.cs index c2e0b7848e9..6bbe4bfd2c5 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/DeclareTypesInNamespacesTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/DeclareTypesInNamespacesTest.cs @@ -45,7 +45,10 @@ public class DeclareTypesInNamespacesTest [TestMethod] public void DeclareTypesInNamespaces_CS_AfterCSharp9() => - builder.AddPaths("DeclareTypesInNamespaces.AfterCSharp9.cs").WithTopLevelStatements().Verify(); + builder + .AddPaths("DeclareTypesInNamespaces.AfterCSharp9.cs", "DeclareTypesInNamespaces.AfterCSharp9.PartialProgramClass.cs") + .WithTopLevelStatements() + .Verify(); [TestMethod] public void DeclareTypesInNamespaces_CS_AfterCSharp10() => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/DeclareTypesInNamespaces.AfterCSharp9.PartialProgramClass.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/DeclareTypesInNamespaces.AfterCSharp9.PartialProgramClass.cs new file mode 100644 index 00000000000..c405e6516b7 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/DeclareTypesInNamespaces.AfterCSharp9.PartialProgramClass.cs @@ -0,0 +1,3 @@ +// reproducer for https://github.com/SonarSource/sonar-dotnet/issues/6836 +public partial class Program { } // Noncompliant, FP this is partial class for the class Program generated by the compiler when using top-level statements + From dffe8ff1aaeffb92431d5fa5e811c07b00da4808 Mon Sep 17 00:00:00 2001 From: Zsolt Kolbay <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com> Date: Thu, 2 Mar 2023 12:01:17 +0100 Subject: [PATCH 05/12] Update RSPEC (#6843) --- analyzers/rspec/cs/S4433_c#.html | 5 +++-- analyzers/rspec/cs/S4545_c#.html | 9 +++++---- analyzers/rspec/vbnet/S4545_vb.net.html | 9 +++++---- analyzers/src/SonarAnalyzer.CSharp/sonarpedia.json | 2 +- analyzers/src/SonarAnalyzer.VisualBasic/sonarpedia.json | 2 +- 5 files changed, 15 insertions(+), 12 deletions(-) diff --git a/analyzers/rspec/cs/S4433_c#.html b/analyzers/rspec/cs/S4433_c#.html index 01d7be99fff..867a6f35e80 100644 --- a/analyzers/rspec/cs/S4433_c#.html +++ b/analyzers/rspec/cs/S4433_c#.html @@ -1,5 +1,5 @@

An LDAP client authenticates to an LDAP server with a "bind request" which provides, among other, a simple authentication method.

+href="https://web.archive.org/web/20220922153922/https://ldapwiki.com/wiki/Simple%20Authentication">simple authentication method.

Simple authentication in LDAP can be used with three different mechanisms:

diff --git a/analyzers/rspec/cs/S4545_c#.html b/analyzers/rspec/cs/S4545_c#.html index 1ddd69d9df0..833d7af462b 100644 --- a/analyzers/rspec/cs/S4545_c#.html +++ b/analyzers/rspec/cs/S4545_c#.html @@ -1,8 +1,9 @@

The DebuggerDisplayAttribute is used to determine how an object is displayed in the debugger window.

-

The DebuggerDisplayAttribute constructor takes a single argument: the string to be displayed in the value column for instances of the -type. Any text within curly braces is evaluated as the name of a field, property, or method.

-

Naming a non-existent field, property or method between curly braces will result in a CS0103 error in the debug window when debugging objects. -Although there is no impact on the production code, providing a wrong value can lead to difficulties when debugging the application.

+

The DebuggerDisplayAttribute constructor takes a single mandatory argument: the string to be displayed in the value column for +instances of the type. Any text within curly braces is evaluated as the name of a field or property, or any complex expression containing method calls +and operators.

+

Naming a non-existent member between curly braces will result in a CS0103 error in the debug window when debugging objects. Although there is no +impact on the production code, providing a wrong value can lead to difficulties when debugging the application.

This rule raises an issue when text specified between curly braces refers to members that don’t exist in the current context.

Noncompliant Code Example

diff --git a/analyzers/rspec/vbnet/S4545_vb.net.html b/analyzers/rspec/vbnet/S4545_vb.net.html
index 80db2002279..41584eec577 100644
--- a/analyzers/rspec/vbnet/S4545_vb.net.html
+++ b/analyzers/rspec/vbnet/S4545_vb.net.html
@@ -1,8 +1,9 @@
 

The DebuggerDisplayAttribute is used to determine how an object is displayed in the debugger window.

-

The DebuggerDisplayAttribute constructor takes a single argument: the string to be displayed in the value column for instances of the -type. Any text within curly braces is evaluated as the name of a field, property, or method.

-

Naming a non-existent field, property or method between curly braces will result in a BC30451 error in the debug window when debugging objects. -Although there is no impact on the production code, providing a wrong value can lead to difficulties when debugging the application.

+

The DebuggerDisplayAttribute constructor takes a single mandatory argument: the string to be displayed in the value column for +instances of the type. Any text within curly braces is evaluated as the name of a field or property, or any complex expression containing method calls +and operators.

+

Naming a non-existent member between curly braces will result in a BC30451 error in the debug window when debugging objects. Although there is no +impact on the production code, providing a wrong value can lead to difficulties when debugging the application.

This rule raises an issue when text specified between curly braces refers to members that don’t exist in the current context.

Noncompliant Code Example

diff --git a/analyzers/src/SonarAnalyzer.CSharp/sonarpedia.json b/analyzers/src/SonarAnalyzer.CSharp/sonarpedia.json
index 4c00453c9e5..7138b901485 100644
--- a/analyzers/src/SonarAnalyzer.CSharp/sonarpedia.json
+++ b/analyzers/src/SonarAnalyzer.CSharp/sonarpedia.json
@@ -3,5 +3,5 @@
   "languages": [
     "CSH"
   ],
-  "latest-update": "2023-02-16T13:29:06.541771900Z"
+  "latest-update": "2023-03-02T10:14:13.012961900Z"
 }
\ No newline at end of file
diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/sonarpedia.json b/analyzers/src/SonarAnalyzer.VisualBasic/sonarpedia.json
index b46d8a27db4..519076fbca6 100644
--- a/analyzers/src/SonarAnalyzer.VisualBasic/sonarpedia.json
+++ b/analyzers/src/SonarAnalyzer.VisualBasic/sonarpedia.json
@@ -3,5 +3,5 @@
   "languages": [
     "VBNET"
   ],
-  "latest-update": "2023-02-16T13:29:34.979573400Z"
+  "latest-update": "2023-03-02T10:14:52.653036700Z"
 }
\ No newline at end of file

From 0f74c565b752d3743c99d10b0eba7c7d5df52ad9 Mon Sep 17 00:00:00 2001
From: Zsolt Kolbay
 <121798625+zsolt-kolbay-sonarsource@users.noreply.github.com>
Date: Fri, 3 Mar 2023 15:49:19 +0100
Subject: [PATCH 06/12] Replace SyntaxWalker with SafeSyntaxWalker (#6852)

---
 .../Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs         | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs
index bfb3748020a..bd886ec8212 100644
--- a/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs
+++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/PrivateStaticMethodUsedOnlyByNestedClass.cs
@@ -134,7 +134,7 @@ private static TypeDeclarationSyntax LowestCommonAncestorOrSelf(IEnumerable methods, TypeDeclarationSyntax outerType, SemanticModel model)
     {
         var collector = new PotentialMethodReferenceCollector(methods);
-        collector.Visit(outerType);
+        collector.SafeVisit(outerType);
 
         return collector.PotentialMethodReferences
                 .Where(x => !OnlyUsedByOuterType(x))
@@ -181,7 +181,7 @@ private sealed record MethodUsedByTypes(MethodDeclarationSyntax Method, TypeDecl
     /// Performance gains: by only using the syntax tree to find matches we can eliminate certain methods (which are only used by the type which has declared it)
     /// without using the more costly symbolic lookup.
     /// 
-    private sealed class PotentialMethodReferenceCollector : CSharpSyntaxWalker
+    private sealed class PotentialMethodReferenceCollector : SafeCSharpSyntaxWalker
     {
         private readonly ISet methodsToFind;
         private readonly Dictionary> potentialMethodReferences;

From 8dab9119bbdafaa57e40bf98323c2a5575bd201d Mon Sep 17 00:00:00 2001
From: Antonio Aversa 
Date: Mon, 6 Mar 2023 13:15:28 +0100
Subject: [PATCH 07/12] New rule S6507: Blocks should not be synchronized on
 local variables (#6854)

---
 ...445.json => AutoMapper--net461-S6507.json} |  2 +-
 ... => AutoMapper--netstandard2.0-S6507.json} |  2 +-
 analyzers/rspec/cs/S2445_c#.html              |  4 +--
 analyzers/rspec/cs/S6507_c#.html              | 35 +++++++++++++++++++
 analyzers/rspec/cs/S6507_c#.json              | 24 +++++++++++++
 .../Rules/LockedFieldShouldBeReadonly.cs      | 20 +++++------
 .../PackagingTests/RuleTypeMappingCS.cs       |  2 +-
 7 files changed, 74 insertions(+), 15 deletions(-)
 rename analyzers/its/expected/Automapper/{AutoMapper--net461-S2445.json => AutoMapper--net461-S6507.json} (94%)
 rename analyzers/its/expected/Automapper/{AutoMapper--netstandard2.0-S2445.json => AutoMapper--netstandard2.0-S6507.json} (94%)
 create mode 100644 analyzers/rspec/cs/S6507_c#.html
 create mode 100644 analyzers/rspec/cs/S6507_c#.json

diff --git a/analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json b/analyzers/its/expected/Automapper/AutoMapper--net461-S6507.json
similarity index 94%
rename from analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json
rename to analyzers/its/expected/Automapper/AutoMapper--net461-S6507.json
index c9f27b7abe2..cd1d53d403f 100644
--- a/analyzers/its/expected/Automapper/AutoMapper--net461-S2445.json
+++ b/analyzers/its/expected/Automapper/AutoMapper--net461-S6507.json
@@ -1,7 +1,7 @@
 {
 "issues":  [
 {
-"id":  "S2445",
+"id":  "S6507",
 "message":  "Do not lock on local variable 'typeMap', use a readonly field instead.",
 "location":  {
 "uri":  "sources\Automapper\src\AutoMapper\Configuration\MapperConfiguration.cs",
diff --git a/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json b/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S6507.json
similarity index 94%
rename from analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json
rename to analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S6507.json
index c9f27b7abe2..cd1d53d403f 100644
--- a/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S2445.json
+++ b/analyzers/its/expected/Automapper/AutoMapper--netstandard2.0-S6507.json
@@ -1,7 +1,7 @@
 {
 "issues":  [
 {
-"id":  "S2445",
+"id":  "S6507",
 "message":  "Do not lock on local variable 'typeMap', use a readonly field instead.",
 "location":  {
 "uri":  "sources\Automapper\src\AutoMapper\Configuration\MapperConfiguration.cs",
diff --git a/analyzers/rspec/cs/S2445_c#.html b/analyzers/rspec/cs/S2445_c#.html
index 21ef51420e3..36a28fa9354 100644
--- a/analyzers/rspec/cs/S2445_c#.html
+++ b/analyzers/rspec/cs/S2445_c#.html
@@ -3,8 +3,8 @@
 
  1. Locking on a non-readonly field makes it possible for the field’s value to change while a thread is in the code block locked on the old value. This allows another thread to lock on the new value and access the same block concurrently.
  2. -
  3. Locking on a local variable or a new instance of an object can undermine synchronization because two different threads running the same method - in parallel will potentially lock on different instances of the same object, allowing them to access the synchronized block at the same time.
  4. +
  5. Locking on a new instance of an object undermines synchronization because two different threads running the same method in parallel will lock + on different instances of the same object, allowing them to access the synchronized block at the same time.
  6. Locking on a string literal is also dangerous since, depending on whether the string is interned or not, different threads may or may not synchronize on the same object instance.
diff --git a/analyzers/rspec/cs/S6507_c#.html b/analyzers/rspec/cs/S6507_c#.html new file mode 100644 index 00000000000..5b61f94376b --- /dev/null +++ b/analyzers/rspec/cs/S6507_c#.html @@ -0,0 +1,35 @@ +

Locking on a local variable can undermine synchronization because two different threads running the same method in parallel will potentially lock +on different instances of the same object, allowing them to access the synchronized block at the same time.

+

Noncompliant Code Example

+
+private void DoSomething()
+{
+  object local = new object();
+  // Code potentially modifying the local variable ...
+
+  lock (local) // Noncompliant
+  {
+    // ...
+  }
+}
+
+

Compliant Solution

+
+private readonly object lockObj = new object();
+
+private void DoSomething()
+{
+  lock (lockObj)
+  {
+    //...
+  }
+}
+
+

See

+ + diff --git a/analyzers/rspec/cs/S6507_c#.json b/analyzers/rspec/cs/S6507_c#.json new file mode 100644 index 00000000000..f01c758da85 --- /dev/null +++ b/analyzers/rspec/cs/S6507_c#.json @@ -0,0 +1,24 @@ +{ + "title": "Blocks should not be synchronized on local variables", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "15min" + }, + "tags": [ + "cwe", + "multi-threading" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6507", + "sqKey": "S6507", + "scope": "All", + "securityStandards": { + "CWE": [ + 412, + 413 + ] + }, + "quickfix": "unknown" +} diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs index 104b9592a54..524ac4eea34 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBeReadonly.cs @@ -23,11 +23,14 @@ namespace SonarAnalyzer.Rules.CSharp; [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class LockedFieldShouldBeReadonly : SonarDiagnosticAnalyzer { - private const string DiagnosticId = "S2445"; + private const string LockedFieldDiagnosticId = "S2445"; + private const string LocalVariableDiagnosticId = "S6507"; + private const string MessageFormat = "Do not lock on {0}, use a readonly field instead."; - private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, "Do not lock on {0}, use a readonly field instead."); + private static readonly DiagnosticDescriptor LockedFieldRule = DescriptorFactory.Create(LockedFieldDiagnosticId, MessageFormat); + private static readonly DiagnosticDescriptor LocalVariableRule = DescriptorFactory.Create(LocalVariableDiagnosticId, MessageFormat); - public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(Rule); + public override ImmutableArray SupportedDiagnostics => ImmutableArray.Create(LockedFieldRule, LocalVariableRule); protected override void Initialize(SonarAnalysisContext context) => context.RegisterNodeAction(CheckLockStatement, SyntaxKind.LockStatement); @@ -37,27 +40,24 @@ private static void CheckLockStatement(SonarSyntaxNodeReportingContext context) var expression = ((LockStatementSyntax)context.Node).Expression?.RemoveParentheses(); if (IsCreation(expression)) { - ReportIssue("a new instance because is a no-op"); + context.ReportIssue(Diagnostic.Create(LockedFieldRule, expression.GetLocation(), "a new instance because is a no-op")); } else { var lazySymbol = new Lazy(() => context.SemanticModel.GetSymbolInfo(expression).Symbol); if (IsOfTypeString(expression, lazySymbol)) { - ReportIssue("strings as they can be interned"); + context.ReportIssue(Diagnostic.Create(LockedFieldRule, expression.GetLocation(), "strings as they can be interned")); } else if (expression is IdentifierNameSyntax && lazySymbol.Value is ILocalSymbol localSymbol) { - ReportIssue($"local variable '{localSymbol.Name}'"); + context.ReportIssue(Diagnostic.Create(LocalVariableRule, expression.GetLocation(), $"local variable '{localSymbol.Name}'")); } else if (FieldWritable(expression, lazySymbol) is { } field) { - ReportIssue($"writable field '{field.Name}'"); + context.ReportIssue(Diagnostic.Create(LockedFieldRule, expression.GetLocation(), $"writable field '{field.Name}'")); } } - - void ReportIssue(string message) => - context.ReportIssue(Diagnostic.Create(Rule, expression.GetLocation(), message)); } private static bool IsCreation(ExpressionSyntax expression) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs index 919122b3e4a..a79296bd910 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeMappingCS.cs @@ -6431,7 +6431,7 @@ internal static class RuleTypeMappingCS // ["S6504"], // ["S6505"], // ["S6506"], - // ["S6507"], + ["S6507"] = "BUG", // ["S6508"], // ["S6509"], // ["S6510"], From dfe1da2277c9f3d54038dfb57d66b1e962ec60c0 Mon Sep 17 00:00:00 2001 From: Martin Strecker <103252490+martin-strecker-sonarsource@users.noreply.github.com> Date: Wed, 8 Mar 2023 09:01:33 +0100 Subject: [PATCH 08/12] Bump version to 8.55 (#6873) --- analyzers/packaging/SonarAnalyzer.CSharp.nuspec | 4 ++-- analyzers/packaging/SonarAnalyzer.VisualBasic.nuspec | 4 ++-- analyzers/src/AssemblyInfo.Shared.cs | 6 +++--- analyzers/src/SonarAnalyzer.CFG/SonarAnalyzer.CFG.cs.nuspec | 2 +- its/pom.xml | 2 +- pom.xml | 2 +- scripts/version/Version.props | 4 ++-- sonar-csharp-plugin/pom.xml | 2 +- sonar-dotnet-shared-library/pom.xml | 2 +- sonar-vbnet-plugin/pom.xml | 2 +- 10 files changed, 15 insertions(+), 15 deletions(-) diff --git a/analyzers/packaging/SonarAnalyzer.CSharp.nuspec b/analyzers/packaging/SonarAnalyzer.CSharp.nuspec index 05379cab859..ed53052fce1 100644 --- a/analyzers/packaging/SonarAnalyzer.CSharp.nuspec +++ b/analyzers/packaging/SonarAnalyzer.CSharp.nuspec @@ -2,7 +2,7 @@ SonarAnalyzer.CSharp - 8.54.0.0 + 8.55.0.0 SonarAnalyzer for C# SonarSource SonarSource @@ -13,7 +13,7 @@ false Roslyn analyzers that spot Bugs, Vulnerabilities and Code Smells in your code. For an even better overall experience, you can use SonarLint for Visual Studio, which is a free extension that can be used standalone or with SonarQube and/or SonarCloud. Roslyn analyzers that spot Bugs, Vulnerabilities and Code Smells in your code. For an even better overall experience, you can use SonarLint for Visual Studio, which is a free extension (https://www.sonarlint.org/visualstudio/) that can be used standalone or with SonarQube (https://www.sonarqube.org/) and/or SonarCloud (https://sonarcloud.io/). - https://github.com/SonarSource/sonar-dotnet/releases/tag/8.54.0.0 + https://github.com/SonarSource/sonar-dotnet/releases/tag/8.55.0.0 en-US Copyright © 2015-2023 SonarSource SA Roslyn Analyzers Refactoring CodeAnalysis CleanCode Clean Code diff --git a/analyzers/packaging/SonarAnalyzer.VisualBasic.nuspec b/analyzers/packaging/SonarAnalyzer.VisualBasic.nuspec index 9f58f44f4d9..1031667efef 100644 --- a/analyzers/packaging/SonarAnalyzer.VisualBasic.nuspec +++ b/analyzers/packaging/SonarAnalyzer.VisualBasic.nuspec @@ -2,7 +2,7 @@ SonarAnalyzer.VisualBasic - 8.54.0.0 + 8.55.0.0 SonarAnalyzer for Visual Basic SonarSource SonarSource @@ -13,7 +13,7 @@ false Roslyn analyzers that spot Bugs, Vulnerabilities and Code Smells in your code. For an even better overall experience, you can use SonarLint for Visual Studio, which is a free extension that can be used standalone or with SonarQube and/or SonarCloud. Roslyn analyzers that spot Bugs, Vulnerabilities and Code Smells in your code. For an even better overall experience, you can use SonarLint for Visual Studio, which is a free extension (https://www.sonarlint.org/visualstudio/) that can be used standalone or with SonarQube (https://www.sonarqube.org/) and/or SonarCloud (https://sonarcloud.io/). - https://github.com/SonarSource/sonar-dotnet/releases/tag/8.54.0.0 + https://github.com/SonarSource/sonar-dotnet/releases/tag/8.55.0.0 en-US Copyright © 2015-2023 SonarSource SA Roslyn Analyzers Refactoring CodeAnalysis CleanCode Clean Code diff --git a/analyzers/src/AssemblyInfo.Shared.cs b/analyzers/src/AssemblyInfo.Shared.cs index 8e01b65b832..7a168d90c04 100644 --- a/analyzers/src/AssemblyInfo.Shared.cs +++ b/analyzers/src/AssemblyInfo.Shared.cs @@ -23,10 +23,10 @@ using System.Resources; using System.Runtime.InteropServices; -[assembly: AssemblyVersion("8.54.0")] -[assembly: AssemblyFileVersion("8.54.0.0")] +[assembly: AssemblyVersion("8.55.0")] +[assembly: AssemblyFileVersion("8.55.0.0")] // The value should look like "Version:X.X.X.X Branch:not-set Sha1:not-set" -[assembly: AssemblyInformationalVersion("Version:8.54.0.0 Branch: Sha1:")] +[assembly: AssemblyInformationalVersion("Version:8.55.0.0 Branch: Sha1:")] [assembly: AssemblyConfiguration("")] [assembly: AssemblyCompany("SonarSource")] [assembly: AssemblyCopyright("Copyright © 2015-2023 SonarSource SA")] diff --git a/analyzers/src/SonarAnalyzer.CFG/SonarAnalyzer.CFG.cs.nuspec b/analyzers/src/SonarAnalyzer.CFG/SonarAnalyzer.CFG.cs.nuspec index c672f08a7a9..469933fb779 100644 --- a/analyzers/src/SonarAnalyzer.CFG/SonarAnalyzer.CFG.cs.nuspec +++ b/analyzers/src/SonarAnalyzer.CFG/SonarAnalyzer.CFG.cs.nuspec @@ -2,7 +2,7 @@ SonarAnalyzer.CFG.CSharp - 8.54.0.0 + 8.55.0.0 C# CFG library for SonarAnalyzer SonarSource SonarSource diff --git a/its/pom.xml b/its/pom.xml index a7df9b4dc80..dfef0589db9 100644 --- a/its/pom.xml +++ b/its/pom.xml @@ -6,7 +6,7 @@ org.sonarsource.dotnet sonar-dotnet - 8.54-SNAPSHOT + 8.55-SNAPSHOT com.sonarsource.it diff --git a/pom.xml b/pom.xml index 744662db52f..4932859286e 100644 --- a/pom.xml +++ b/pom.xml @@ -11,7 +11,7 @@ org.sonarsource.dotnet sonar-dotnet - 8.54-SNAPSHOT + 8.55-SNAPSHOT pom .NET Analyzers parent diff --git a/scripts/version/Version.props b/scripts/version/Version.props index 249a4685d3d..1c97984ec69 100644 --- a/scripts/version/Version.props +++ b/scripts/version/Version.props @@ -1,8 +1,8 @@ - 8.54.0 + 8.55.0 0 - 8.54 + 8.55 $(Sha1) $(BranchName) $(MainVersion).$(BuildNumber) diff --git a/sonar-csharp-plugin/pom.xml b/sonar-csharp-plugin/pom.xml index 5c1295a8c19..133af0a27a9 100644 --- a/sonar-csharp-plugin/pom.xml +++ b/sonar-csharp-plugin/pom.xml @@ -6,7 +6,7 @@ org.sonarsource.dotnet sonar-dotnet - 8.54-SNAPSHOT + 8.55-SNAPSHOT sonar-csharp-plugin diff --git a/sonar-dotnet-shared-library/pom.xml b/sonar-dotnet-shared-library/pom.xml index 682a15994a6..01aed0aa944 100644 --- a/sonar-dotnet-shared-library/pom.xml +++ b/sonar-dotnet-shared-library/pom.xml @@ -6,7 +6,7 @@ org.sonarsource.dotnet sonar-dotnet - 8.54-SNAPSHOT + 8.55-SNAPSHOT sonar-dotnet-shared-library diff --git a/sonar-vbnet-plugin/pom.xml b/sonar-vbnet-plugin/pom.xml index b9e3f2d0d04..274af424e41 100644 --- a/sonar-vbnet-plugin/pom.xml +++ b/sonar-vbnet-plugin/pom.xml @@ -6,7 +6,7 @@ org.sonarsource.dotnet sonar-dotnet - 8.54-SNAPSHOT + 8.55-SNAPSHOT sonar-vbnet-plugin From d73c2cce42f48ca4f91c6003a2186157292bf5be Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Wed, 8 Mar 2023 09:03:06 +0100 Subject: [PATCH 09/12] Fix S3415 FP/FN: Support named arguments (#6759) --- .../Facade/CSharpFacade.cs | 15 +- .../Helpers/CSharpMethodParameterLookup.cs | 31 +- .../Helpers/CSharpSyntaxHelper.cs | 12 +- ...sertionArgsShouldBePassedInCorrectOrder.cs | 120 ++-- .../Trackers/CSharpObjectCreationTracker.cs | 10 +- .../Wrappers/ObjectCreationFactory.cs | 6 + .../Facade/ILanguageFacade.cs | 1 + .../Helpers/MethodParameterLookupBase.cs | 192 +++--- .../Facade/VisualBasicFacade.cs | 11 +- .../VisualBasicMethodParameterLookup.cs | 30 +- .../Helpers/VisualBasicSyntaxHelper.cs | 14 +- .../VisualBasicObjectCreationTracker.cs | 14 +- .../Facade/CSharpFacadeTests.cs | 60 +- .../Facade/VisualBasicFacadeTests.cs | 58 +- .../Helpers/MethodParameterLookupTest.cs | 591 +++++++++++------- ...ionArgsShouldBePassedInCorrectOrderTest.cs | 85 ++- ...ArgsShouldBePassedInCorrectOrder.MsTest.cs | 91 ++- ...nArgsShouldBePassedInCorrectOrder.NUnit.cs | 88 ++- ...nArgsShouldBePassedInCorrectOrder.Xunit.cs | 82 ++- 19 files changed, 971 insertions(+), 540 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs index c0ecf611a64..cde171858b6 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpFacade.cs @@ -18,6 +18,7 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ +using Microsoft.CodeAnalysis; using SonarAnalyzer.Helpers.Facade; namespace SonarAnalyzer.Helpers; @@ -51,13 +52,19 @@ internal sealed class CSharpFacade : ILanguageFacade node.FindConstantValue(model); public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol) => + invocation != null ? new CSharpMethodParameterLookup(GetArgumentList(invocation), methodSymbol) : null; + + public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, SemanticModel semanticModel) => + invocation != null ? new CSharpMethodParameterLookup(GetArgumentList(invocation), semanticModel) : null; + + private static ArgumentListSyntax GetArgumentList(SyntaxNode invocation) => invocation switch { - null => null, - ObjectCreationExpressionSyntax x => new CSharpMethodParameterLookup(x.ArgumentList, methodSymbol), - InvocationExpressionSyntax x => new CSharpMethodParameterLookup(x, methodSymbol), + ArgumentListSyntax x => x, + ObjectCreationExpressionSyntax x => x.ArgumentList, + InvocationExpressionSyntax x => x.ArgumentList, _ when ImplicitObjectCreationExpressionSyntaxWrapper.IsInstance(invocation) => - new CSharpMethodParameterLookup(((ImplicitObjectCreationExpressionSyntaxWrapper)invocation).ArgumentList, methodSymbol), + ((ImplicitObjectCreationExpressionSyntaxWrapper)invocation).ArgumentList, _ => throw new ArgumentException($"{invocation.GetType()} does not contain an ArgumentList.", nameof(invocation)), }; diff --git a/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs b/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs index 700f93f9196..fcea469e16b 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpMethodParameterLookup.cs @@ -18,26 +18,25 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Helpers +namespace SonarAnalyzer.Helpers; + +internal class CSharpMethodParameterLookup : MethodParameterLookupBase { - internal class CSharpMethodParameterLookup : MethodParameterLookupBase - { - public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, SemanticModel semanticModel) - : this(invocation.ArgumentList, semanticModel) { } + public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, SemanticModel semanticModel) + : this(invocation.ArgumentList, semanticModel) { } - public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) - : this(invocation.ArgumentList, methodSymbol) { } + public CSharpMethodParameterLookup(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) + : this(invocation.ArgumentList, methodSymbol) { } - public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel) - : base(argumentList?.Arguments, argumentList == null ? null : semanticModel.GetSymbolInfo(argumentList.Parent).Symbol as IMethodSymbol) { } + public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel) + : base(argumentList.Arguments, semanticModel.GetSymbolInfo(argumentList.Parent)) { } - public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol) - : base(argumentList?.Arguments, methodSymbol) { } + public CSharpMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol) + : base(argumentList.Arguments, methodSymbol) { } - protected override SyntaxNode Expression(ArgumentSyntax argument) => - argument.Expression; + protected override SyntaxNode Expression(ArgumentSyntax argument) => + argument.Expression; - protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) => - argument.NameColon?.Name.Identifier; - } + protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) => + argument.NameColon?.Name.Identifier; } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs b/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs index 4dd747bae43..e029a59ea5c 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs @@ -338,12 +338,10 @@ public static bool IsComment(this SyntaxTrivia trivia) /// /// There can be zero, one or more results based on parameter type (Optional or ParamArray/params). /// - public static ImmutableArray ArgumentValuesForParameter(SemanticModel semanticModel, ArgumentListSyntax argumentList, string parameterName) - { - var methodParameterLookup = new CSharpMethodParameterLookup(argumentList, semanticModel); - return methodParameterLookup.TryGetSyntax(parameterName, out var expressions) - ? expressions - : ImmutableArray.Empty; - } + public static ImmutableArray ArgumentValuesForParameter(SemanticModel semanticModel, ArgumentListSyntax argumentList, string parameterName) => + argumentList != null + && new CSharpMethodParameterLookup(argumentList, semanticModel).TryGetSyntax(parameterName, out var expressions) + ? expressions + : ImmutableArray.Empty; } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs index a6acd55abb0..73b5562d287 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/AssertionArgsShouldBePassedInCorrectOrder.cs @@ -18,63 +18,85 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Rules.CSharp +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class AssertionArgsShouldBePassedInCorrectOrder : SonarDiagnosticAnalyzer { - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class AssertionArgsShouldBePassedInCorrectOrder : SonarDiagnosticAnalyzer - { - internal const string DiagnosticId = "S3415"; - private const string MessageFormat = "Make sure these 2 arguments are in the correct order: expected value, actual value."; + internal const string DiagnosticId = "S3415"; + private const string MessageFormat = "Make sure these 2 arguments are in the correct order: expected value, actual value."; - private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); - private static readonly IDictionary> MethodsWithType = new Dictionary> + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(c => { - ["AreEqual"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert), - ["AreNotEqual"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert), - ["AreSame"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert), - ["AreNotSame"] = ImmutableArray.Create(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, KnownType.NUnit_Framework_Assert), - ["Equal"] = ImmutableArray.Create(KnownType.Xunit_Assert), - ["Same"] = ImmutableArray.Create(KnownType.Xunit_Assert), - ["NotSame"] = ImmutableArray.Create(KnownType.Xunit_Assert) - }; - - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(c => + if (c.Node is InvocationExpressionSyntax { ArgumentList: { Arguments.Count: >= 2 } argumentList } invocation + && GetParameters(invocation.GetName()) is { } knownAssertParameters + && c.SemanticModel.GetSymbolInfo(invocation).AllSymbols() + .SelectMany(symbol => + symbol is IMethodSymbol { IsStatic: true, ContainingSymbol: INamedTypeSymbol container } methodSymbol + ? knownAssertParameters.Select(knownParameters => FindWrongArguments(c.SemanticModel, container, methodSymbol, argumentList, knownParameters)) + : Enumerable.Empty()) + .FirstOrDefault(x => x is not null) is (Expected: var expected, Actual: var actual)) { - var methodCall = (InvocationExpressionSyntax)c.Node; - if (!methodCall.Expression.IsKind(SyntaxKind.SimpleMemberAccessExpression) - || methodCall.ArgumentList.Arguments.Count < 2) - { - return; - } + c.ReportIssue(Diagnostic.Create(Rule, CreateLocation(expected, actual))); + } + }, + SyntaxKind.InvocationExpression); - var firstArgument = methodCall.ArgumentList.Arguments[0]; - var secondArgument = methodCall.ArgumentList.Arguments[1]; - if (firstArgument.Expression is LiteralExpressionSyntax - || secondArgument.Expression is not LiteralExpressionSyntax) + private static KnownAssertParameters[] GetParameters(string name) => + name switch + { + "AreEqual" => new KnownAssertParameters[] { - return; - } - - var methodCallExpression = (MemberAccessExpressionSyntax)methodCall.Expression; - - var methodKnownTypes = MethodsWithType.GetValueOrDefault(methodCallExpression.Name.Identifier.ValueText); - if (methodKnownTypes == null) + new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "expected", "actual"), + new(KnownType.NUnit_Framework_Assert, "expected", "actual") + }, + "AreNotEqual" => new KnownAssertParameters[] { - return; - } - - var symbolInfo = c.SemanticModel.GetSymbolInfo(methodCallExpression.Expression).Symbol; - var isAnyTrackedAssertType = (symbolInfo as INamedTypeSymbol).IsAny(methodKnownTypes); - if (!isAnyTrackedAssertType) + new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "notExpected", "actual"), + new(KnownType.NUnit_Framework_Assert, "expected", "actual") + }, + "AreSame" => new KnownAssertParameters[] + { + new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "expected", "actual"), + new(KnownType.NUnit_Framework_Assert, "expected", "actual") + }, + "AreNotSame" => new KnownAssertParameters[] + { + new(KnownType.Microsoft_VisualStudio_TestTools_UnitTesting_Assert, "notExpected", "actual"), + new(KnownType.NUnit_Framework_Assert, "expected", "actual") + }, + "Equal" or "NotEqual" or "Same" or "NotSame" => new KnownAssertParameters[] { - return; - } + new(KnownType.Xunit_Assert, "expected", "actual") + }, + _ => null + }; + + private static WrongArguments? FindWrongArguments(SemanticModel semanticModel, + INamedTypeSymbol container, + IMethodSymbol symbol, + ArgumentListSyntax argumentList, + KnownAssertParameters knownParameters) => + container.Is(knownParameters.AssertClass) + && CSharpFacade.Instance.MethodParameterLookup(argumentList, symbol) is var parameterLookup + && parameterLookup.TryGetSyntax(knownParameters.ExpectedParameterName, out var expectedArguments) + && expectedArguments.FirstOrDefault() is { } expected + && semanticModel.GetConstantValue(expected).HasValue is false + && parameterLookup.TryGetSyntax(knownParameters.ActualParameterName, out var actualArguments) + && actualArguments.FirstOrDefault() is { } actual + && semanticModel.GetConstantValue(actual).HasValue + ? new(expected, actual) + : null; + + private static Location CreateLocation(SyntaxNode argument1, SyntaxNode argument2) => + argument1.Span.CompareTo(argument2.Span) < 0 + ? argument1.Parent.CreateLocation(argument2.Parent) + : argument2.Parent.CreateLocation(argument1.Parent); - c.ReportIssue(Diagnostic.Create(Rule, firstArgument.CreateLocation(secondArgument))); - }, - SyntaxKind.InvocationExpression); - } + private readonly record struct KnownAssertParameters(KnownType AssertClass, string ExpectedParameterName, string ActualParameterName); + private readonly record struct WrongArguments(SyntaxNode Expected, SyntaxNode Actual); } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpObjectCreationTracker.cs b/analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpObjectCreationTracker.cs index d19e9201409..f2e98e0bbd7 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpObjectCreationTracker.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Trackers/CSharpObjectCreationTracker.cs @@ -29,13 +29,11 @@ public class CSharpObjectCreationTracker : ObjectCreationTracker && argumentList.Arguments.Count > index && argumentList.Arguments[index].Expression.HasConstantValue(context.SemanticModel); - internal override object ConstArgumentForParameter(ObjectCreationContext context, string parameterName) - { - var argumentList = ObjectCreationFactory.Create(context.Node).ArgumentList; - var values = CSharpSyntaxHelper.ArgumentValuesForParameter(context.SemanticModel, argumentList, parameterName); - return values.Length == 1 && values[0] is ExpressionSyntax valueSyntax + internal override object ConstArgumentForParameter(ObjectCreationContext context, string parameterName) => + ObjectCreationFactory.TryCreate(context.Node, out var objectCreation) + && CSharpSyntaxHelper.ArgumentValuesForParameter(context.SemanticModel, objectCreation.ArgumentList, parameterName) is { Length: 1 } values + && values[0] is ExpressionSyntax valueSyntax ? valueSyntax.FindConstantValue(context.SemanticModel) : null; - } } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs b/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs index 1f9aa8edff8..7ac8e0f47c7 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Wrappers/ObjectCreationFactory.cs @@ -49,6 +49,12 @@ node switch ? Create(node) : null; + public static bool TryCreate(SyntaxNode node, out IObjectCreation objectCreation) + { + objectCreation = TryCreate(node); + return objectCreation is not null; + } + private class ObjectCreation : IObjectCreation { private readonly ObjectCreationExpressionSyntax objectCreation; diff --git a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs index 4c675baa6d5..30b09520334 100644 --- a/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs +++ b/analyzers/src/SonarAnalyzer.Common/Facade/ILanguageFacade.cs @@ -33,6 +33,7 @@ public interface ILanguageFacade DiagnosticDescriptor CreateDescriptor(string id, string messageFormat, bool? isEnabledByDefault = null, bool fadeOutCode = false); object FindConstantValue(SemanticModel model, SyntaxNode node); IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol); + IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, SemanticModel semanticModel); string GetName(SyntaxNode expression); } diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs index 334c88e241f..03b9589b9ff 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/MethodParameterLookupBase.cs @@ -18,116 +18,132 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Helpers +namespace SonarAnalyzer.Helpers; + +public interface IMethodParameterLookup { - public interface IMethodParameterLookup + bool TryGetSymbol(SyntaxNode argument, out IParameterSymbol parameter); + bool TryGetSyntax(IParameterSymbol parameter, out ImmutableArray expressions); + bool TryGetSyntax(string parameterName, out ImmutableArray expressions); + bool TryGetNonParamsSyntax(IParameterSymbol parameter, out SyntaxNode expression); +} + +// This should come from the Roslyn API (https://github.com/dotnet/roslyn/issues/9) +internal abstract class MethodParameterLookupBase : IMethodParameterLookup + where TArgumentSyntax : SyntaxNode +{ + private readonly SeparatedSyntaxList argumentList; + + protected abstract SyntaxToken? GetNameColonArgumentIdentifier(TArgumentSyntax argument); + protected abstract SyntaxNode Expression(TArgumentSyntax argument); + + public IMethodSymbol MethodSymbol { get; } + private ImmutableArray MethodSymbolOrCandidates { get; } + + protected MethodParameterLookupBase(SeparatedSyntaxList argumentList, SymbolInfo? methodSymbolInfo) + : this(argumentList, methodSymbolInfo?.Symbol as IMethodSymbol, methodSymbolInfo?.AllSymbols().OfType()) { } + + protected MethodParameterLookupBase(SeparatedSyntaxList argumentList, IMethodSymbol methodSymbol) + : this(argumentList, methodSymbol, new[] { methodSymbol }) { } + + private MethodParameterLookupBase(SeparatedSyntaxList argumentList, IMethodSymbol methodSymbol, IEnumerable methodSymbolOrCandidates) { - bool TryGetSymbol(SyntaxNode argument, out IParameterSymbol parameter); - bool TryGetSyntax(IParameterSymbol parameter, out ImmutableArray expressions); - bool TryGetSyntax(string parameterName, out ImmutableArray expressions); - bool TryGetNonParamsSyntax(IParameterSymbol parameter, out SyntaxNode expression); + this.argumentList = argumentList; + MethodSymbol = methodSymbol; + MethodSymbolOrCandidates = methodSymbolOrCandidates?.ToImmutableArray() ?? ImmutableArray.Create(); } - // This should come from the Roslyn API (https://github.com/dotnet/roslyn/issues/9) - internal abstract class MethodParameterLookupBase : IMethodParameterLookup - where TArgumentSyntax : SyntaxNode - { - private readonly SeparatedSyntaxList? argumentList; + public bool TryGetSymbol(SyntaxNode argument, out IParameterSymbol parameter) => + TryGetSymbol(argument, MethodSymbol, out parameter); - protected abstract SyntaxToken? GetNameColonArgumentIdentifier(TArgumentSyntax argument); - protected abstract SyntaxNode Expression(TArgumentSyntax argument); + private bool TryGetSymbol(SyntaxNode argument, IMethodSymbol methodSymbol, out IParameterSymbol parameter) + { + parameter = null; + var arg = argument as TArgumentSyntax ?? throw new ArgumentException($"{nameof(argument)} must be of type {typeof(TArgumentSyntax)}", nameof(argument)); - public IMethodSymbol MethodSymbol { get; } + if (!argumentList.Contains(arg) + || methodSymbol == null + || methodSymbol.IsVararg) + { + return false; + } - protected MethodParameterLookupBase(SeparatedSyntaxList? argumentList, IMethodSymbol methodSymbol) + if (GetNameColonArgumentIdentifier(arg) is { } nameColonArgumentIdentifier) { - this.argumentList = argumentList; - MethodSymbol = methodSymbol; + parameter = methodSymbol.Parameters.FirstOrDefault(symbol => symbol.Name == nameColonArgumentIdentifier.ValueText); + return parameter != null; } - public bool TryGetSymbol(SyntaxNode argument, out IParameterSymbol parameter) + var index = argumentList.IndexOf(arg); + if (index >= methodSymbol.Parameters.Length) { - parameter = null; - var arg = argument as TArgumentSyntax ?? throw new ArgumentException($"{nameof(argument)} must be of type {typeof(TArgumentSyntax)}", nameof(argument)); + var lastParameter = methodSymbol.Parameters.Last(); + parameter = lastParameter.IsParams ? lastParameter : null; + return parameter != null; + } + parameter = methodSymbol.Parameters[index]; + return true; + } - if (!argumentList.HasValue - || !argumentList.Value.Contains(arg) - || MethodSymbol == null - || MethodSymbol.IsVararg) - { - return false; - } + /// + /// Method returns array of argument syntaxes that represents all syntaxes passed to the parameter. + /// + /// There could be multiple syntaxes for ParamArray/params. + /// There could be zero or one result for optional parameters. + /// There will be single result for normal parameters. + /// + public bool TryGetSyntax(IParameterSymbol parameter, out ImmutableArray expressions) => + TryGetSyntax(parameter.Name, out expressions); - if (GetNameColonArgumentIdentifier(arg) is { } nameColonArgumentIdentifier) - { - parameter = MethodSymbol.Parameters.FirstOrDefault(symbol => symbol.Name == nameColonArgumentIdentifier.ValueText); - return parameter != null; - } + /// + /// Method returns array of argument syntaxes that represents all syntaxes passed to the parameter. + /// + /// There could be multiple syntaxes for ParamArray/params. + /// There could be zero or one result for optional parameters. + /// There will be single result for normal parameters. + public bool TryGetSyntax(string parameterName, out ImmutableArray expressions) + { + var candidateArgumentLists = MethodSymbolOrCandidates + .Select(x => GetAllArgumentParameterMappings(x).Where(x => x.Symbol.Name == parameterName).Select(x => Expression(x.Node)).ToImmutableArray()).ToImmutableArray(); + expressions = candidateArgumentLists.Any() && AllArgumentsAreTheSame(candidateArgumentLists) + ? candidateArgumentLists[0] + : Enumerable.Empty().ToImmutableArray(); + return !expressions.IsEmpty; - var index = argumentList.Value.IndexOf(arg); - if (index >= MethodSymbol.Parameters.Length) - { - var lastParameter = MethodSymbol.Parameters.Last(); - parameter = lastParameter.IsParams ? lastParameter : null; - return parameter != null; - } - parameter = MethodSymbol.Parameters[index]; - return true; - } + static bool AllArgumentsAreTheSame(ImmutableArray> candidateArgumentLists) => + candidateArgumentLists.Skip(1).All(x => x.SequenceEqual(candidateArgumentLists[0])); + } - /// - /// Method returns array of argument syntaxes that represents all syntaxes passed to the parameter. - /// - /// There could be multiple syntaxes for ParamArray/params. - /// There could be zero or one result for optional parameters. - /// There will be single result for normal parameters. - /// - public bool TryGetSyntax(IParameterSymbol parameter, out ImmutableArray expressions) => - TryGetSyntax(parameter.Name, out expressions); - - /// - /// Method returns array of argument syntaxes that represents all syntaxes passed to the parameter. - /// - /// There could be multiple syntaxes for ParamArray/params. - /// There could be zero or one result for optional parameters. - /// There will be single result for normal parameters. - public bool TryGetSyntax(string parameterName, out ImmutableArray expressions) + /// + /// Method returns zero or one argument syntax that represents syntax passed to the parameter. + /// + /// Caller must ensure that given parameter is not ParamArray/params. + /// + public bool TryGetNonParamsSyntax(IParameterSymbol parameter, out SyntaxNode expression) + { + if (parameter.IsParams) { - expressions = GetAllArgumentParameterMappings().Where(x => x.Symbol.Name == parameterName).Select(x => Expression(x.Node)).ToImmutableArray(); - return !expressions.IsEmpty; + throw new InvalidOperationException("Cannot call TryGetNonParamsSyntax on ParamArray/params parameters."); } - - /// - /// Method returns zero or one argument syntax that represents syntax passed to the parameter. - /// - /// Caller must ensure that given parameter is not ParamArray/params. - /// - public bool TryGetNonParamsSyntax(IParameterSymbol parameter, out SyntaxNode expression) + if (TryGetSyntax(parameter, out var all)) { - if (parameter.IsParams) - { - throw new System.InvalidOperationException("Cannot call TryGetNonParamsSyntax on ParamArray/params parameters."); - } - if (TryGetSyntax(parameter, out var all)) - { - expression = all.Single(); - return true; - } - expression = null; - return false; + expression = all.Single(); + return true; } + expression = null; + return false; + } - internal IEnumerable> GetAllArgumentParameterMappings() + internal IEnumerable> GetAllArgumentParameterMappings() => + GetAllArgumentParameterMappings(MethodSymbol); + + private IEnumerable> GetAllArgumentParameterMappings(IMethodSymbol methodSymbol) + { + foreach (var argument in argumentList) { - if (argumentList.HasValue) + if (TryGetSymbol(argument, methodSymbol, out var parameter)) { - foreach (var argument in argumentList) - { - if (TryGetSymbol(argument, out var parameter)) - { - yield return new NodeAndSymbol(argument, parameter); - } - } + yield return new NodeAndSymbol(argument, parameter); } } } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs index 83d6afc1fab..f3848639460 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Facade/VisualBasicFacade.cs @@ -51,11 +51,16 @@ internal sealed class VisualBasicFacade : ILanguageFacade node.FindConstantValue(model); public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, IMethodSymbol methodSymbol) => + invocation != null ? new VisualBasicMethodParameterLookup(GetArgumentList(invocation), methodSymbol) : null; + public IMethodParameterLookup MethodParameterLookup(SyntaxNode invocation, SemanticModel semanticModel) => + invocation != null ? new VisualBasicMethodParameterLookup(GetArgumentList(invocation), semanticModel) : null; + + private static ArgumentListSyntax GetArgumentList(SyntaxNode invocation) => invocation switch { - null => null, - ObjectCreationExpressionSyntax x => new VisualBasicMethodParameterLookup(x.ArgumentList, methodSymbol), - InvocationExpressionSyntax x => new VisualBasicMethodParameterLookup(x, methodSymbol), + ArgumentListSyntax x => x, + ObjectCreationExpressionSyntax x => x.ArgumentList, + InvocationExpressionSyntax x => x.ArgumentList, _ => throw new ArgumentException($"{invocation.GetType()} does not contain an ArgumentList.", nameof(invocation)), }; diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodParameterLookup.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodParameterLookup.cs index 8b9768523ab..eac07d2d0f1 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodParameterLookup.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicMethodParameterLookup.cs @@ -18,23 +18,25 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Helpers +namespace SonarAnalyzer.Helpers; + +internal class VisualBasicMethodParameterLookup : MethodParameterLookupBase { - internal class VisualBasicMethodParameterLookup : MethodParameterLookupBase - { - public VisualBasicMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol) - : base(argumentList?.Arguments, methodSymbol) { } + public VisualBasicMethodParameterLookup(InvocationExpressionSyntax invocation, SemanticModel semanticModel) + : this(invocation.ArgumentList, semanticModel) { } + + public VisualBasicMethodParameterLookup(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) + : this(invocation.ArgumentList, methodSymbol) { } - public VisualBasicMethodParameterLookup(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) - : this(invocation.ArgumentList, methodSymbol) { } + public VisualBasicMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel) + : base(argumentList.Arguments, semanticModel.GetSymbolInfo(argumentList.Parent)) { } - public VisualBasicMethodParameterLookup(ArgumentListSyntax argumentList, SemanticModel semanticModel) - : base(argumentList?.Arguments, argumentList == null ? null : semanticModel.GetSymbolInfo(argumentList.Parent).Symbol as IMethodSymbol) { } + public VisualBasicMethodParameterLookup(ArgumentListSyntax argumentList, IMethodSymbol methodSymbol) + : base(argumentList.Arguments, methodSymbol) { } - protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) => - (argument as SimpleArgumentSyntax)?.NameColonEquals?.Name.Identifier; + protected override SyntaxToken? GetNameColonArgumentIdentifier(ArgumentSyntax argument) => + (argument as SimpleArgumentSyntax)?.NameColonEquals?.Name.Identifier; - protected override SyntaxNode Expression(ArgumentSyntax argument) => - argument.GetExpression(); - } + protected override SyntaxNode Expression(ArgumentSyntax argument) => + argument.GetExpression(); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs index c7c30eff044..a063a726a80 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs @@ -259,13 +259,9 @@ public static string GetIdentifierText(this MethodBlockSyntax method) /// /// There can be zero, one or more results based on parameter type (Optional or ParamArray/params). /// - public static ImmutableArray ArgumentValuesForParameter(SemanticModel semanticModel, ArgumentListSyntax argumentList, string parameterName) - { - var methodParameterLookup = new VisualBasicMethodParameterLookup(argumentList, semanticModel); - if (methodParameterLookup.TryGetSyntax(parameterName, out var expressions)) - { - return expressions; - } - return ImmutableArray.Empty; - } + public static ImmutableArray ArgumentValuesForParameter(SemanticModel semanticModel, ArgumentListSyntax argumentList, string parameterName) => + argumentList != null + && new VisualBasicMethodParameterLookup(argumentList, semanticModel).TryGetSyntax(parameterName, out var expressions) + ? expressions + : ImmutableArray.Empty; } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Trackers/VisualBasicObjectCreationTracker.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Trackers/VisualBasicObjectCreationTracker.cs index 9b45f3a4ec4..88452496107 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Trackers/VisualBasicObjectCreationTracker.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Trackers/VisualBasicObjectCreationTracker.cs @@ -29,13 +29,11 @@ public class VisualBasicObjectCreationTracker : ObjectCreationTracker index && argumentList.Arguments[index].GetExpression().HasConstantValue(context.SemanticModel); - internal override object ConstArgumentForParameter(ObjectCreationContext context, string parameterName) - { - var argumentList = ((ObjectCreationExpressionSyntax)context.Node).ArgumentList; - var values = VisualBasicSyntaxHelper.ArgumentValuesForParameter(context.SemanticModel, argumentList, parameterName); - return values.Length == 1 && values[0] is ExpressionSyntax valueSyntax - ? valueSyntax.FindConstantValue(context.SemanticModel) - : null; - } + internal override object ConstArgumentForParameter(ObjectCreationContext context, string parameterName) => + ((ObjectCreationExpressionSyntax)context.Node).ArgumentList is { } argumentList + && VisualBasicSyntaxHelper.ArgumentValuesForParameter(context.SemanticModel, argumentList, parameterName) is { Length: 1 } values + && values[0] is ExpressionSyntax valueSyntax + ? valueSyntax.FindConstantValue(context.SemanticModel) + : null; } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Facade/CSharpFacadeTests.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Facade/CSharpFacadeTests.cs index cfea667c640..67202ed4dac 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Facade/CSharpFacadeTests.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Facade/CSharpFacadeTests.cs @@ -28,7 +28,7 @@ namespace SonarAnalyzer.UnitTest.Facade; public class CSharpFacadeTests { [TestMethod] - public void MethodParameterLookupForInvocation() + public void MethodParameterLookup_ForInvocation() { var sut = CSharpFacade.Instance; var code = """ @@ -47,7 +47,22 @@ public class C } [TestMethod] - public void MethodParameterLookupForObjectCreation() + public void MethodParameterLookup_SemanticModelOverload() + { + var code = """ + public class C + { + public int M(int arg) => + M(1); + } + """; + var (tree, model) = TestHelper.CompileCS(code); + var lookup = CSharpFacade.Instance.MethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().First(), model); + lookup.Should().NotBeNull().And.BeOfType(); + } + + [TestMethod] + public void MethodParameterLookup_ForObjectCreation() { var sut = CSharpFacade.Instance; var code = """ @@ -68,7 +83,7 @@ public class C } [TestMethod] - public void MethodParameterLookupForImplicitObjectCreation() + public void MethodParameterLookup_ForImplicitObjectCreation() { var sut = CSharpFacade.Instance; var code = """ @@ -89,7 +104,26 @@ public class C } [TestMethod] - public void MethodParameterLookupUnsupportedSyntaxKind() + public void MethodParameterLookup_ForArgumentList() + { + var sut = CSharpFacade.Instance; + var code = """ + public class C + { + public int M(int arg) => + M(1); + } + """; + var (tree, model) = TestHelper.CompileCS(code); + var root = tree.GetRoot(); + var invocation = root.DescendantNodes().OfType().First(); + var method = model.GetDeclaredSymbol(root.DescendantNodes().OfType().First()); + var actual = sut.MethodParameterLookup(invocation, method); + actual.Should().NotBeNull().And.BeOfType(); + } + + [TestMethod] + public void MethodParameterLookup_UnsupportedSyntaxKind() { var sut = CSharpFacade.Instance; var code = """ @@ -108,7 +142,7 @@ public class C } [TestMethod] - public void MethodParameterLookupNull() + public void MethodParameterLookup_Null() { var sut = CSharpFacade.Instance; var code = """ @@ -124,4 +158,20 @@ public class C var actual = sut.MethodParameterLookup(null, method); actual.Should().BeNull(); } + + [TestMethod] + public void MethodParameterLookup_Null_SemanticModelOverload() + { + var sut = CSharpFacade.Instance; + var code = """ + public class C + { + public int M(int arg) => + M(1); + } + """; + var (_, model) = TestHelper.CompileCS(code); + var actual = sut.MethodParameterLookup(null, model); + actual.Should().BeNull(); + } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Facade/VisualBasicFacadeTests.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Facade/VisualBasicFacadeTests.cs index 228b98dfffd..89a11532d84 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Facade/VisualBasicFacadeTests.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Facade/VisualBasicFacadeTests.cs @@ -27,7 +27,7 @@ namespace SonarAnalyzer.UnitTest.Facade; public class VisualBasicFacadeTests { [TestMethod] - public void MethodParameterLookupForInvocation() + public void MethodParameterLookup_ForInvocation() { var sut = VisualBasicFacade.Instance; var code = """ @@ -46,7 +46,22 @@ End Class } [TestMethod] - public void MethodParameterLookupForObjectCreation() + public void MethodParameterLookup_SemanticModelOverload() + { + var code = """ + Public Class C + Public Function M(arg As Integer) As Integer + Return M(1) + End Function + End Class + """; + var (tree, model) = TestHelper.CompileVB(code); + var actual = VisualBasicFacade.Instance.MethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().First(), model); + actual.Should().NotBeNull().And.BeOfType(); + } + + [TestMethod] + public void MethodParameterLookup_ForObjectCreation() { var sut = VisualBasicFacade.Instance; var code = """ @@ -68,7 +83,26 @@ End Class } [TestMethod] - public void MethodParameterLookupUnsupportedSyntaxKind() + public void MethodParameterLookup_ForArgumentList() + { + var sut = VisualBasicFacade.Instance; + var code = """ + Public Class C + Public Function M(arg As Integer) As Integer + Return M(1) + End Function + End Class + """; + var (tree, model) = TestHelper.CompileVB(code); + var root = tree.GetRoot(); + var argumentList = root.DescendantNodes().OfType().First(); + var method = model.GetDeclaredSymbol(root.DescendantNodes().OfType().First()); + var actual = sut.MethodParameterLookup(argumentList, method); + actual.Should().NotBeNull().And.BeOfType(); + } + + [TestMethod] + public void MethodParameterLookup_UnsupportedSyntaxKind() { var sut = VisualBasicFacade.Instance; var code = """ @@ -87,7 +121,7 @@ End Class } [TestMethod] - public void MethodParameterLookupNull() + public void MethodParameterLookup_Null() { var sut = VisualBasicFacade.Instance; var code = """ @@ -103,4 +137,20 @@ End Class var actual = sut.MethodParameterLookup(null, method); actual.Should().BeNull(); } + + [TestMethod] + public void MethodParameterLookup_Null_SemanticModelOverload() + { + var sut = VisualBasicFacade.Instance; + var code = """ + Public Class C + Public Function M(arg As Integer) As Integer + Return M(1) + End Function + End Class + """; + var (_, model) = TestHelper.CompileVB(code); + var actual = sut.MethodParameterLookup(null, model); + actual.Should().BeNull(); + } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs index 902d2a2e1a2..7207fe969cd 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Helpers/MethodParameterLookupTest.cs @@ -25,289 +25,442 @@ using VBCodeAnalysis = Microsoft.CodeAnalysis.VisualBasic; using VBSyntax = Microsoft.CodeAnalysis.VisualBasic.Syntax; -namespace SonarAnalyzer.UnitTest.Helpers -{ - [TestClass] - public class MethodParameterLookupTest - { - private const string SourceCS = @" -namespace Test +namespace SonarAnalyzer.UnitTest.Helpers; + +[TestClass] +public class MethodParameterLookupTest { - class TestClass - { - void Main() - { - DoNothing(); - DoSomething(1, true); - DoSomething(b: true, a: 1); - WithOptional(1); - WithOptional(1, ""Ipsum""); - WithOptional(opt: ""Ipsum"", a: 1); - WithParams(); - WithParams(1, 2, 3); - } + private const string SourceCS = """ + namespace Test + { + class TestClass + { + void Main() + { + DoNothing(); + DoSomething(1, true); + DoSomething(b: true, a: 1); + WithOptional(1); + WithOptional(1, "Ipsum"); + WithOptional(opt: "Ipsum", a: 1); + WithParams(); + WithParams(1, 2, 3); + } - void ThisShouldNotBeFoundInMain() - { - SpecialMethod(65535); - } + void ThisShouldNotBeFoundInMain() + { + SpecialMethod(65535); + } - void DoNothing() - { - } + void DoNothing() + { + } - void DoSomething(int a, bool b) - { - } + void DoSomething(int a, bool b) + { + } - void WithOptional(int a, string opt = ""Lorem"") - { - } + void WithOptional(int a, string opt = "Lorem") + { + } - void WithParams(params int[] arr) - { - } + void WithParams(params int[] arr) + { + } - void SpecialMethod(int specialParameter) - { - } - } -}"; + void SpecialMethod(int specialParameter) + { + } + } + } + """; - private const string SourceVB = @" -Module MainModule + private const string SourceVB = """ + Module MainModule - Sub Main() - DoNothing() - DoSomething(1, True) - WithOptional(1) - WithOptional(1, ""Ipsum"") - WithParams() - WithParams(1, 2, 3) - End Sub + Sub Main() + DoNothing() + DoSomething(1, True) + WithOptional(1) + WithOptional(1, "Ipsum") + WithParams() + WithParams(1, 2, 3) + End Sub - Sub ThisShouldNotBeFoundInMain() - SpecialMethod(65535) - End Sub + Sub ThisShouldNotBeFoundInMain() + SpecialMethod(65535) + End Sub - Sub DoNothing() - End Sub + Sub DoNothing() + End Sub - Sub DoSomething(a As Integer, b As Boolean) - End Sub + Sub DoSomething(a As Integer, b As Boolean) + End Sub - Sub WithOptional(a As Integer, Optional opt As String = ""Lorem"") - End Sub + Sub WithOptional(a As Integer, Optional opt As String = "Lorem") + End Sub - Sub WithParams(ParamArray arr() As Integer) - End Sub + Sub WithParams(ParamArray arr() As Integer) + End Sub - Sub SpecialMethod(SpecialParameter As Integer) - End Sub -End Module -"; + Sub SpecialMethod(SpecialParameter As Integer) + End Sub + End Module + """; - [TestMethod] - public void TestMethodParameterLookup_CS() - { - var c = new CSharpInspection(SourceCS); - c.CheckExpectedParameterMappings(0, "DoNothing", new { }); - c.CheckExpectedParameterMappings(1, "DoSomething", new { a = 1, b = true }); - c.CheckExpectedParameterMappings(2, "DoSomething", new { a = 1, b = true }); - c.CheckExpectedParameterMappings(3, "WithOptional", new { a = 1 }); - c.CheckExpectedParameterMappings(4, "WithOptional", new { a = 1, opt = "Ipsum" }); - c.CheckExpectedParameterMappings(5, "WithOptional", new { a = 1, opt = "Ipsum" }); - c.CheckExpectedParameterMappings(6, "WithParams", new { }); - c.CheckExpectedParameterMappings(7, "WithParams", new { arr = new[] { 1, 2, 3 } }); - - c.MainInvocations.Length.Should().Be(8); // Self-Test of this test. If new Invocation is added to the Main(), this number has to be updated and test should be written for that case. - } + [TestMethod] + public void TestMethodParameterLookup_CS() + { + var c = new CSharpInspection(SourceCS); + c.CheckExpectedParameterMappings(0, "DoNothing", new { }); + c.CheckExpectedParameterMappings(1, "DoSomething", new { a = 1, b = true }); + c.CheckExpectedParameterMappings(2, "DoSomething", new { a = 1, b = true }); + c.CheckExpectedParameterMappings(3, "WithOptional", new { a = 1 }); + c.CheckExpectedParameterMappings(4, "WithOptional", new { a = 1, opt = "Ipsum" }); + c.CheckExpectedParameterMappings(5, "WithOptional", new { a = 1, opt = "Ipsum" }); + c.CheckExpectedParameterMappings(6, "WithParams", new { }); + c.CheckExpectedParameterMappings(7, "WithParams", new { arr = new[] { 1, 2, 3 } }); + + c.MainInvocations.Length.Should().Be(8); // Self-Test of this test. If new Invocation is added to the Main(), this number has to be updated and test should be written for that case. + } - [TestMethod] - public void TestMethodParameterLookup_VB() - { - var c = new VisualBasicInspection(SourceVB); - c.CheckExpectedParameterMappings(0, "DoNothing", new { }); - c.CheckExpectedParameterMappings(1, "DoSomething", new { a = 1, b = true }); - c.CheckExpectedParameterMappings(2, "WithOptional", new { a = 1 }); - c.CheckExpectedParameterMappings(3, "WithOptional", new { a = 1, opt = "Ipsum" }); - c.CheckExpectedParameterMappings(4, "WithParams", new { }); - c.CheckExpectedParameterMappings(5, "WithParams", new { arr = new[] { 1, 2, 3 } }); - - c.MainInvocations.Length.Should().Be(6); // Self-Test of this test. If new Invocation is added to the Main(), this number has to be updated and test should be written for that case. - } + [TestMethod] + public void TestMethodParameterLookup_VB() + { + var c = new VisualBasicInspection(SourceVB); + c.CheckExpectedParameterMappings(0, "DoNothing", new { }); + c.CheckExpectedParameterMappings(1, "DoSomething", new { a = 1, b = true }); + c.CheckExpectedParameterMappings(2, "WithOptional", new { a = 1 }); + c.CheckExpectedParameterMappings(3, "WithOptional", new { a = 1, opt = "Ipsum" }); + c.CheckExpectedParameterMappings(4, "WithParams", new { }); + c.CheckExpectedParameterMappings(5, "WithParams", new { arr = new[] { 1, 2, 3 } }); + + c.MainInvocations.Length.Should().Be(6); // Self-Test of this test. If new Invocation is added to the Main(), this number has to be updated and test should be written for that case. + } - [TestMethod] - public void TestMethodParameterLookup_CS_ThrowsException() - { - var c = new CSharpInspection(SourceCS); - var lookupThrow = c.CreateLookup(1, "DoSomething"); + [TestMethod] + public void TestMethodParameterLookup_CS_ThrowsException() + { + var c = new CSharpInspection(SourceCS); + var lookupThrow = c.CreateLookup(1, "DoSomething"); - var invalidOperationEx = Assert.ThrowsException(() => lookupThrow.TryGetNonParamsSyntax(lookupThrow.MethodSymbol.Parameters.Single(), out var argument)); - invalidOperationEx.Message.Should().Be("Sequence contains more than one element"); + var invalidOperationEx = Assert.ThrowsException(() => lookupThrow.TryGetNonParamsSyntax(lookupThrow.MethodSymbol.Parameters.Single(), out var argument)); + invalidOperationEx.Message.Should().Be("Sequence contains more than one element"); - var argumentEx = Assert.ThrowsException(() => - lookupThrow.TryGetSymbol(CSharpCodeAnalysis.SyntaxFactory.LiteralExpression(CSharpCodeAnalysis.SyntaxKind.StringLiteralExpression), out var parameter)); - argumentEx.Message.Should().StartWith("argument must be of type Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentSyntax"); - } + var argumentEx = Assert.ThrowsException(() => + lookupThrow.TryGetSymbol(CSharpCodeAnalysis.SyntaxFactory.LiteralExpression(CSharpCodeAnalysis.SyntaxKind.StringLiteralExpression), out var parameter)); + argumentEx.Message.Should().StartWith("argument must be of type Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentSyntax"); + } - [TestMethod] - public void TestMethodParameterLookup_VB_ThrowsException() - { - var c = new VisualBasicInspection(SourceVB); - var lookupThrow = c.CreateLookup(1, "DoSomething"); + [TestMethod] + public void TestMethodParameterLookup_VB_ThrowsException() + { + var c = new VisualBasicInspection(SourceVB); + var lookupThrow = c.CreateLookup(1, "DoSomething"); - var invalidOperationEx = Assert.ThrowsException(() => lookupThrow.TryGetNonParamsSyntax(lookupThrow.MethodSymbol.Parameters.Single(), out var argument)); - invalidOperationEx.Message.Should().Be("Sequence contains more than one element"); + var invalidOperationEx = Assert.ThrowsException(() => lookupThrow.TryGetNonParamsSyntax(lookupThrow.MethodSymbol.Parameters.Single(), out var argument)); + invalidOperationEx.Message.Should().Be("Sequence contains more than one element"); - var argumentEx = Assert.ThrowsException(() => - lookupThrow.TryGetSymbol(VBCodeAnalysis.SyntaxFactory.StringLiteralExpression(VBCodeAnalysis.SyntaxFactory.StringLiteralToken(string.Empty, string.Empty)), out var parameter)); - argumentEx.Message.Should().StartWith("argument must be of type Microsoft.CodeAnalysis.VisualBasic.Syntax.ArgumentSyntax"); - } + var argumentEx = Assert.ThrowsException(() => + lookupThrow.TryGetSymbol(VBCodeAnalysis.SyntaxFactory.StringLiteralExpression(VBCodeAnalysis.SyntaxFactory.StringLiteralToken(string.Empty, string.Empty)), out var parameter)); + argumentEx.Message.Should().StartWith("argument must be of type Microsoft.CodeAnalysis.VisualBasic.Syntax.ArgumentSyntax"); + } - private abstract class InspectionBase - where TArgumentSyntax : SyntaxNode - where TInvocationSyntax : SyntaxNode - { - public SnippetCompiler Compiler { get; protected set; } - public TInvocationSyntax[] MainInvocations { get; protected set; } - public TArgumentSyntax SpecialArgument { get; private set; } - public IParameterSymbol SpecialParameter { get; private set; } + [TestMethod] + public void TestMethodParameterLookup_CS_ThrowsException_NonParams() + { + var c = new CSharpInspection(SourceCS); + var lookupThrow = c.CreateLookup(7, "WithParams"); - public abstract TInvocationSyntax[] FindInvocationsIn(string name); - public abstract object ExtractArgumentValue(TArgumentSyntax argumentSyntax); - public abstract TArgumentSyntax[] GetArguments(TInvocationSyntax invocation); - public abstract MethodParameterLookupBase CreateLookup(TInvocationSyntax invocation, IMethodSymbol method); + var invalidOperationEx = Assert.ThrowsException(() => lookupThrow.TryGetNonParamsSyntax(lookupThrow.MethodSymbol.Parameters.Single(), out var argument)); + invalidOperationEx.Message.Should().Be("Cannot call TryGetNonParamsSyntax on ParamArray/params parameters."); + } - protected InspectionBase(string source, AnalyzerLanguage language) - { - Compiler = new SnippetCompiler(source, false, language); - MainInvocations = FindInvocationsIn("Main"); - } + [TestMethod] + public void TestMethodParameterLookup_VB_ThrowsException_NonParams() + { + var c = new VisualBasicInspection(SourceVB); + var lookupThrow = c.CreateLookup(5, "WithParams"); - public MethodParameterLookupBase CreateLookup(int invocationIndex, string expectedMethod) - { - var invocation = MainInvocations[invocationIndex]; - var method = Compiler.SemanticModel.GetSymbolInfo(invocation).Symbol as IMethodSymbol; - method.Name.Should().Be(expectedMethod); - return CreateLookup(invocation, method); - } + var invalidOperationEx = Assert.ThrowsException(() => lookupThrow.TryGetNonParamsSyntax(lookupThrow.MethodSymbol.Parameters.Single(), out var argument)); + invalidOperationEx.Message.Should().Be("Cannot call TryGetNonParamsSyntax on ParamArray/params parameters."); + } - public void CheckExpectedParameterMappings(int invocationIndex, string expectedMethod, object expectedArguments) + [TestMethod] + public void TestMethodParameterLookup_CS_MultipleCandidates() + { + var source = """ + namespace Test { - var lookup = CreateLookup(invocationIndex, expectedMethod); - InspectTryGetSyntax(lookup, expectedArguments, lookup.MethodSymbol); - InspectTryGetSymbol(lookup, expectedArguments, GetArguments(MainInvocations[invocationIndex])); + class TestClass + { + void Main(dynamic d) => + AmbiguousCall(d); + + void AmbiguousCall(int p) { } + + void AmbiguousCall(string p) { } + } } + """; + var (tree, model) = TestHelper.CompileCS(source); + var lookup = new CSharpMethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().Single(), model); - protected void InitSpecial(TInvocationSyntax specialInvocation) + lookup.TryGetSyntax("p", out var expressions).Should().BeTrue(); + expressions.Should().BeEquivalentTo(new[] { new { Identifier = new { ValueText = "d" } } }); + } + + [TestMethod] + public void TestMethodParameterLookup_VB_MultipleCandidates() + { + var source = """ + Module Test + Sub Main() + Overloaded(42, "") + End Sub + + Sub Overloaded(a As Integer, b As Boolean) + End Sub + + Sub Overloaded(a As Integer, b As Integer) + End Sub + End Module + """; + var (tree, model) = TestHelper.CompileIgnoreErrorsVB(source); + var lookup = new VisualBasicMethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().Single(), model); + + lookup.TryGetSyntax("a", out var expressions).Should().BeTrue(); + expressions.Should().BeEquivalentTo(new[] { new { Token = new { ValueText = "42" } } }); + } + + [TestMethod] + public void TestMethodParameterLookup_CS_MultipleCandidatesWithDifferentParameters() + { + var source = """ + namespace Test { - SpecialArgument = GetArguments(specialInvocation).Single(); - SpecialParameter = (Compiler.SemanticModel.GetSymbolInfo(specialInvocation).Symbol as IMethodSymbol).Parameters.Single(); + class TestClass + { + void Main(dynamic d) => + AmbiguousCall(d, d); + + void AmbiguousCall(int a, int b) { } + + void AmbiguousCall(string b, string a) { } + } } + """; + var (tree, model) = TestHelper.CompileCS(source); + var lookup = new CSharpMethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().Single(), model); - private void InspectTryGetSyntax(MethodParameterLookupBase lookup, object expectedArguments, IMethodSymbol method) - { - lookup.TryGetSyntax(SpecialParameter, out var symbol).Should().Be(false); + lookup.TryGetSyntax("a", out var expressions).Should().BeFalse(); + expressions.Should().BeEmpty(); + } + + [TestMethod] + public void TestMethodParameterLookup_VB_MultipleCandidatesWithDifferentParameters() + { + var source = """ + Module Test + Sub Main() + Overloaded(42, "") + End Sub + + Sub Overloaded(a As Integer, b As Integer) + End Sub + + Sub Overloaded(b As Boolean, a As Boolean) + End Sub + End Module + """; + var (tree, model) = TestHelper.CompileIgnoreErrorsVB(source); + var lookup = new VisualBasicMethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().Single(), model); + + lookup.TryGetSyntax("a", out var expressions).Should().BeFalse(); + expressions.Should().BeEmpty(); + } - foreach (var parameter in method.Parameters) + [TestMethod] + public void TestMethodParameterLookup_CS_UnknownMethod() + { + var source = """ + namespace Test + { + class TestClass { - if (parameter.IsParams && lookup.TryGetSyntax(parameter, out var expressions)) - { - var expected = ExtractExpectedValue(expectedArguments, parameter.Name).Should().BeAssignableTo().Subject.Cast(); - expressions.Select(x => ConstantValue(x)).Should().Equal(expected); - } - else if (!parameter.IsParams && lookup.TryGetNonParamsSyntax(parameter, out var expression)) + void Main() { - ConstantValue(expression).Should().Be(ExtractExpectedValue(expectedArguments, parameter.Name)); + UnknownMethod(42); } - else if (!parameter.IsOptional && !parameter.IsParams) - { - Assert.Fail($"TryGetSyntax missing {parameter.Name}"); - } // Else it's OK } } + """; + var (tree, model) = TestHelper.CompileIgnoreErrorsCS(source); + var lookup = new CSharpMethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().Single(), model); + + lookup.TryGetSyntax("p", out var expressions).Should().BeFalse(); + expressions.Should().BeEmpty(); + } + + [TestMethod] + public void TestMethodParameterLookup_VB_UnknownMethod() + { + var source = """ + Module Test + Sub Main() + UnknownMethod(42) + End Sub + End Module + """; + var (tree, model) = TestHelper.CompileIgnoreErrorsVB(source); + var lookup = new VisualBasicMethodParameterLookup(tree.GetRoot().DescendantNodes().OfType().Single(), model); + + lookup.TryGetSyntax("a", out var expressions).Should().BeFalse(); + expressions.Should().BeEmpty(); + } + + private abstract class InspectionBase + where TArgumentSyntax : SyntaxNode + where TInvocationSyntax : SyntaxNode + { + public SnippetCompiler Compiler { get; protected set; } + public TInvocationSyntax[] MainInvocations { get; protected set; } + public TArgumentSyntax SpecialArgument { get; private set; } + public IParameterSymbol SpecialParameter { get; private set; } + + public abstract TInvocationSyntax[] FindInvocationsIn(string name); + public abstract object ExtractArgumentValue(TArgumentSyntax argumentSyntax); + public abstract TArgumentSyntax[] GetArguments(TInvocationSyntax invocation); + public abstract MethodParameterLookupBase CreateLookup(TInvocationSyntax invocation, IMethodSymbol method); + + protected InspectionBase(string source, AnalyzerLanguage language) + { + Compiler = new SnippetCompiler(source, false, language); + MainInvocations = FindInvocationsIn("Main"); + } + + public MethodParameterLookupBase CreateLookup(int invocationIndex, string expectedMethod) + { + var invocation = MainInvocations[invocationIndex]; + var method = Compiler.SemanticModel.GetSymbolInfo(invocation).Symbol as IMethodSymbol; + method.Name.Should().Be(expectedMethod); + return CreateLookup(invocation, method); + } + + public void CheckExpectedParameterMappings(int invocationIndex, string expectedMethod, object expectedArguments) + { + var lookup = CreateLookup(invocationIndex, expectedMethod); + InspectTryGetSyntax(lookup, expectedArguments, lookup.MethodSymbol); + InspectTryGetSymbol(lookup, expectedArguments, GetArguments(MainInvocations[invocationIndex])); + } + + protected void InitSpecial(TInvocationSyntax specialInvocation) + { + SpecialArgument = GetArguments(specialInvocation).Single(); + SpecialParameter = (Compiler.SemanticModel.GetSymbolInfo(specialInvocation).Symbol as IMethodSymbol).Parameters.Single(); + } - private void InspectTryGetSymbol(MethodParameterLookupBase lookup, object expectedArguments, TArgumentSyntax[] arguments) + private void InspectTryGetSyntax(MethodParameterLookupBase lookup, object expectedArguments, IMethodSymbol method) + { + lookup.TryGetSyntax(SpecialParameter, out var symbol).Should().Be(false); + + foreach (var parameter in method.Parameters) { - lookup.TryGetSymbol(SpecialArgument, out var parameter).Should().Be(false); + if (parameter.IsParams && lookup.TryGetSyntax(parameter, out var expressions)) + { + var expected = ExtractExpectedValue(expectedArguments, parameter.Name).Should().BeAssignableTo().Subject.Cast(); + expressions.Select(x => ConstantValue(x)).Should().Equal(expected); + } + else if (!parameter.IsParams && lookup.TryGetNonParamsSyntax(parameter, out var expression)) + { + ConstantValue(expression).Should().Be(ExtractExpectedValue(expectedArguments, parameter.Name)); + } + else if (!parameter.IsOptional && !parameter.IsParams) + { + Assert.Fail($"TryGetSyntax missing {parameter.Name}"); + } // Else it's OK + } + } + + private void InspectTryGetSymbol(MethodParameterLookupBase lookup, object expectedArguments, TArgumentSyntax[] arguments) + { + lookup.TryGetSymbol(SpecialArgument, out var parameter).Should().Be(false); - foreach (var argument in arguments) + foreach (var argument in arguments) + { + if (lookup.TryGetSymbol(argument, out var symbol)) { - if (lookup.TryGetSymbol(argument, out var symbol)) + var value = ExtractArgumentValue(argument); + var expected = ExtractExpectedValue(expectedArguments, symbol.Name); + if (symbol.IsParams) { - var value = ExtractArgumentValue(argument); - var expected = ExtractExpectedValue(expectedArguments, symbol.Name); - if (symbol.IsParams) - { - // Expected contains all values {1, 2, 3} for ParamArray/params, but foreach is probing one at a time - expected.Should().BeAssignableTo().Which.Cast().Should().Contain(value); - } - else - { - value.Should().Be(expected); - } + // Expected contains all values {1, 2, 3} for ParamArray/params, but foreach is probing one at a time + expected.Should().BeAssignableTo().Which.Cast().Should().Contain(value); } else { - Assert.Fail($"TryGetParameterSymbol missing {argument.ToString()}"); + value.Should().Be(expected); } } - } - - private static object ExtractExpectedValue(object expected, string name) - { - var pi = expected.GetType().GetProperty(name); - if (pi == null) + else { - Assert.Fail($"Parameter name {name} was not expected."); + Assert.Fail($"TryGetParameterSymbol missing {argument.ToString()}"); } - return pi.GetValue(expected, null); } - - private object ConstantValue(SyntaxNode node) => - Compiler.SemanticModel.GetConstantValue(node).Value; } - private class CSharpInspection : InspectionBase + private static object ExtractExpectedValue(object expected, string name) { - public CSharpInspection(string source) : base(source, AnalyzerLanguage.CSharp) => - InitSpecial(Compiler.GetNodes() - .Single(x => x.Expression is CSharpSyntax.IdentifierNameSyntax identifier - && identifier.Identifier.ValueText == "SpecialMethod")); + var pi = expected.GetType().GetProperty(name); + if (pi == null) + { + Assert.Fail($"Parameter name {name} was not expected."); + } + return pi.GetValue(expected, null); + } + + private object ConstantValue(SyntaxNode node) => + Compiler.SemanticModel.GetConstantValue(node).Value; + } - public override CSharpSyntax.InvocationExpressionSyntax[] FindInvocationsIn(string name) => - Compiler.GetNodes().Single(x => x.Identifier.ValueText == name).DescendantNodes().OfType().ToArray(); + private class CSharpInspection : InspectionBase + { + public CSharpInspection(string source) : base(source, AnalyzerLanguage.CSharp) => + InitSpecial(Compiler.GetNodes() + .Single(x => x.Expression is CSharpSyntax.IdentifierNameSyntax identifier + && identifier.Identifier.ValueText == "SpecialMethod")); - public override CSharpSyntax.ArgumentSyntax[] GetArguments(CSharpSyntax.InvocationExpressionSyntax invocation) => - invocation.ArgumentList.Arguments.ToArray(); + public override CSharpSyntax.InvocationExpressionSyntax[] FindInvocationsIn(string name) => + Compiler.GetNodes().Single(x => x.Identifier.ValueText == name).DescendantNodes().OfType().ToArray(); - public override object ExtractArgumentValue(CSharpSyntax.ArgumentSyntax argumentSyntax) => - Compiler.SemanticModel.GetConstantValue(argumentSyntax.Expression).Value; + public override CSharpSyntax.ArgumentSyntax[] GetArguments(CSharpSyntax.InvocationExpressionSyntax invocation) => + invocation.ArgumentList.Arguments.ToArray(); - public override MethodParameterLookupBase CreateLookup(CSharpSyntax.InvocationExpressionSyntax invocation, IMethodSymbol method) => - new CSharpMethodParameterLookup(invocation.ArgumentList, method); - } + public override object ExtractArgumentValue(CSharpSyntax.ArgumentSyntax argumentSyntax) => + Compiler.SemanticModel.GetConstantValue(argumentSyntax.Expression).Value; - private class VisualBasicInspection : InspectionBase - { - public VisualBasicInspection(string source) : base(source, AnalyzerLanguage.VisualBasic) => - InitSpecial(Compiler.GetNodes() - .Single(x => x.Expression is VBSyntax.IdentifierNameSyntax identifier - && identifier.Identifier.ValueText == "SpecialMethod")); + public override MethodParameterLookupBase CreateLookup(CSharpSyntax.InvocationExpressionSyntax invocation, IMethodSymbol method) => + new CSharpMethodParameterLookup(invocation.ArgumentList, method); + } + + private class VisualBasicInspection : InspectionBase + { + public VisualBasicInspection(string source) : base(source, AnalyzerLanguage.VisualBasic) => + InitSpecial(Compiler.GetNodes() + .Single(x => x.Expression is VBSyntax.IdentifierNameSyntax identifier + && identifier.Identifier.ValueText == "SpecialMethod")); - public override VBSyntax.InvocationExpressionSyntax[] FindInvocationsIn(string name) => - Compiler.GetNodes().Single(x => x.SubOrFunctionStatement.Identifier.ValueText == "Main") - .DescendantNodes().OfType().ToArray(); + public override VBSyntax.InvocationExpressionSyntax[] FindInvocationsIn(string name) => + Compiler.GetNodes().Single(x => x.SubOrFunctionStatement.Identifier.ValueText == "Main") + .DescendantNodes().OfType().ToArray(); - public override VBSyntax.ArgumentSyntax[] GetArguments(VBSyntax.InvocationExpressionSyntax invocation) => - invocation.ArgumentList.Arguments.ToArray(); + public override VBSyntax.ArgumentSyntax[] GetArguments(VBSyntax.InvocationExpressionSyntax invocation) => + invocation.ArgumentList.Arguments.ToArray(); - public override MethodParameterLookupBase CreateLookup(VBSyntax.InvocationExpressionSyntax invocation, IMethodSymbol method) => - new VisualBasicMethodParameterLookup(invocation.ArgumentList, Compiler.SemanticModel); + public override MethodParameterLookupBase CreateLookup(VBSyntax.InvocationExpressionSyntax invocation, IMethodSymbol method) => + new VisualBasicMethodParameterLookup(invocation.ArgumentList, Compiler.SemanticModel); - public override object ExtractArgumentValue(VBSyntax.ArgumentSyntax argumentSyntax) => - Compiler.SemanticModel.GetConstantValue(argumentSyntax.GetExpression()).Value; - } + public override object ExtractArgumentValue(VBSyntax.ArgumentSyntax argumentSyntax) => + Compiler.SemanticModel.GetConstantValue(argumentSyntax.GetExpression()).Value; } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/AssertionArgsShouldBePassedInCorrectOrderTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/AssertionArgsShouldBePassedInCorrectOrderTest.cs index 2abfc020a40..6bc022c75c5 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/AssertionArgsShouldBePassedInCorrectOrderTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/AssertionArgsShouldBePassedInCorrectOrderTest.cs @@ -20,36 +20,59 @@ using SonarAnalyzer.Rules.CSharp; -namespace SonarAnalyzer.UnitTest.Rules +namespace SonarAnalyzer.UnitTest.Rules; + +[TestClass] +public class AssertionArgsShouldBePassedInCorrectOrderTest { - [TestClass] - public class AssertionArgsShouldBePassedInCorrectOrderTest - { - private readonly VerifierBuilder builder = new VerifierBuilder(); - - [DataTestMethod] - [DataRow("1.1.11")] - [DataRow(Constants.NuGetLatestVersion)] - public void AssertionArgsShouldBePassedInCorrectOrder_MsTest(string testFwkVersion) => - builder.AddPaths("AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs") - .AddReferences(NuGetMetadataReference.MSTestTestFramework(testFwkVersion)) - .Verify(); - - [DataTestMethod] - [DataRow("2.5.7.10213")] - [DataRow(Constants.NuGetLatestVersion)] - public void AssertionArgsShouldBePassedInCorrectOrder_NUnit(string testFwkVersion) => - builder.AddPaths("AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs") - .AddReferences(NuGetMetadataReference.NUnit(testFwkVersion)) - .Verify(); - - [DataTestMethod] - [DataRow("2.0.0")] - [DataRow(Constants.NuGetLatestVersion)] - public void AssertionArgsShouldBePassedInCorrectOrder_XUnit(string testFwkVersion) => - builder.AddPaths("AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs") - .AddReferences(NuGetMetadataReference.XunitFramework(testFwkVersion) - .Concat(NetStandardMetadataReference.Netstandard)) - .Verify(); - } + private readonly VerifierBuilder builder = new VerifierBuilder(); + + [DataTestMethod] + [DataRow("1.1.11")] + [DataRow(Constants.NuGetLatestVersion)] + public void AssertionArgsShouldBePassedInCorrectOrder_MsTest(string testFwkVersion) => + builder.AddPaths("AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs") + .AddReferences(NuGetMetadataReference.MSTestTestFramework(testFwkVersion)) + .Verify(); + + [TestMethod] + public void AssertionArgsShouldBePassedInCorrectOrder_MsTest_Static() => + builder.WithTopLevelStatements().AddReferences(NuGetMetadataReference.MSTestTestFramework(Constants.NuGetLatestVersion)).AddSnippet(""" + using static Microsoft.VisualStudio.TestTools.UnitTesting.Assert; + var str = ""; + AreEqual(str, ""); // Noncompliant + """).Verify(); + + [DataTestMethod] + [DataRow("2.5.7.10213")] + [DataRow(Constants.NuGetLatestVersion)] + public void AssertionArgsShouldBePassedInCorrectOrder_NUnit(string testFwkVersion) => + builder.AddPaths("AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs") + .AddReferences(NuGetMetadataReference.NUnit(testFwkVersion)) + .Verify(); + + [TestMethod] + public void AssertionArgsShouldBePassedInCorrectOrder_NUnit_Static() => + builder.WithTopLevelStatements().AddReferences(NuGetMetadataReference.NUnit(Constants.NuGetLatestVersion)).AddSnippet(""" + using static NUnit.Framework.Assert; + var str = ""; + AreEqual(str, ""); // Noncompliant + """).Verify(); + + [DataTestMethod] + [DataRow("2.0.0")] + [DataRow(Constants.NuGetLatestVersion)] + public void AssertionArgsShouldBePassedInCorrectOrder_XUnit(string testFwkVersion) => + builder.AddPaths("AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs") + .AddReferences(NuGetMetadataReference.XunitFramework(testFwkVersion) + .Concat(NetStandardMetadataReference.Netstandard)) + .Verify(); + + [TestMethod] + public void AssertionArgsShouldBePassedInCorrectOrder_XUnit_Static() => + builder.WithTopLevelStatements().AddReferences(NuGetMetadataReference.XunitFramework(Constants.NuGetLatestVersion)).AddSnippet(""" + using static Xunit.Assert; + var str = ""; + Equal(str, ""); // Noncompliant + """).Verify(); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs index d1fa58c03cc..87b937b36aa 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.MsTest.cs @@ -7,29 +7,61 @@ namespace Tests.Diagnostics class Program { [TestMethod] - public void Foo() + public void Simple(string str, double d) { - var str = ""; - Assert.AreEqual(str, ""); // Noncompliant {{Make sure these 2 arguments are in the correct order: expected value, actual value.}} -// ^^^^^^^ - Assert.AreSame(str, ""); // Noncompliant -// ^^^^^^^ - - double d = 42; - Assert.AreEqual(d, 42); // Noncompliant -// ^^^^^ - Assert.AreNotEqual(d, 42); // Noncompliant + Assert.AreEqual("", str); // Compliant + Assert.AreEqual(str, ""); // Noncompliant {{Make sure these 2 arguments are in the correct order: expected value, actual value.}} + // ^^^^^^^ + Assert.AreEqual(42, d); // Compliant + Assert.AreEqual(d, 42); // Noncompliant + // ^^^^^ + Assert.AreNotEqual("", str); // Compliant + Assert.AreNotEqual(str, ""); // Noncompliant + // ^^^^^^^ + Assert.AreNotEqual(42, d); // Compliant + Assert.AreNotEqual(d, 42); // Noncompliant + // ^^^^^ + Assert.AreSame("", str); // Compliant + Assert.AreSame(str, ""); // Noncompliant + // ^^^^^^^ + Assert.AreSame(42, d); // Compliant + Assert.AreSame(d, 42); // Noncompliant + // ^^^^^ + Assert.AreNotSame("", str); // Compliant + Assert.AreNotSame(str, ""); // Noncompliant + // ^^^^^^^ + Assert.AreNotSame(42, d); // Compliant + Assert.AreNotSame(d, 42); // Noncompliant + // ^^^^^ - Assert.AreSame(d, 42); // Noncompliant - Assert.AreEqual(d, 42, 1, "message"); // Noncompliant - Assert.AreNotEqual(d, 42, 1, "message"); // Noncompliant + Assert.AreEqual("", str, "message"); // Compliant + Assert.AreEqual(str, "", "message"); // Noncompliant + Assert.AreNotEqual("", str, "message"); // Compliant + Assert.AreNotEqual(str, "", "message"); // Noncompliant + Assert.AreSame("", str, "message"); // Compliant + Assert.AreSame(str, "", "message"); // Noncompliant + Assert.AreNotSame("", str, "message"); // Compliant + Assert.AreNotSame(str, "", "message"); // Noncompliant - Assert.AreEqual("", str); - Assert.AreSame("", str); - Assert.AreEqual(42, d, 1, "message"); - Assert.AreNotEqual(42, d, 1, "message"); Assert.IsNull(str); } + + [TestMethod] + public void Dynamic() + { + dynamic d = 42; + Assert.AreEqual(d, 35); // Noncompliant + Assert.AreEqual(35, d); // Compliant + Assert.AreEqual(actual: d, expected: 35); // Compliant + Assert.AreEqual(actual: 35, expected: d); // Noncompliant + } + + [TestMethod] + public void BrokeSyntax() + { + double d = 42; + Assert.Equual(d, 42); // Error + } } } @@ -43,18 +75,23 @@ class Program public void Foo() { var str = ""; - Assert.AreEqual(actual: "", expected: str); // Compliant FN + Assert.AreEqual(actual: "", expected: str); // Noncompliant Assert.AreEqual(expected: "", actual: str); // Compliant - Assert.AreEqual(actual: str, expected: ""); // Noncompliant FP + Assert.AreEqual(actual: str, expected: ""); // Compliant Assert.AreEqual(expected: str, actual: ""); // Noncompliant - Assert.AreNotEqual(actual: "", notExpected: str); // Compliant FN - Assert.AreSame(actual: "", expected: str); // Compliant FN - Assert.AreNotSame(actual: "", notExpected: str); // Compliant FN + Assert.AreNotEqual(actual: "", notExpected: str); // Noncompliant + Assert.AreSame(actual: "", expected: str); // Noncompliant + Assert.AreNotSame(actual: "", notExpected: str); // Noncompliant int d = 42; - Assert.AreEqual(actual: 1, expected: d); // Compliant FN - Assert.AreEqual(actual: null, expected: new Program()); // Compliant FN + Assert.AreEqual(actual: 1, expected: d); // Noncompliant + Assert.AreEqual(actual: null, expected: new Program()); // Noncompliant + + Assert.AreEqual(message: "", expected: str, actual: ""); // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^^^^^ + Assert.AreEqual(expected: str, message: "", actual: ""); // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } } } @@ -73,7 +110,7 @@ public void TestString() string stringToTest = RetrieveString(); const string constString = "Spring"; - Assert.AreEqual(expected: stringToTest, actual: constString); // FN + Assert.AreEqual(expected: stringToTest, actual: constString); // Noncompliant Assert.AreEqual(expected: constString, actual: stringToTest); // Compliant } @@ -82,7 +119,7 @@ public void TestEnum() { Seasons seasonToTest = RetrieveSeason(); - Assert.AreEqual(expected: seasonToTest, actual: Seasons.Spring); // FN + Assert.AreEqual(expected: seasonToTest, actual: Seasons.Spring); // Noncompliant Assert.AreEqual(expected: Seasons.Spring, actual: seasonToTest); // Compliant } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs index 74d8e89fec2..22f564a4363 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.NUnit.cs @@ -9,25 +9,43 @@ class Program void FakeAssert(object a, object b) { } [Test] - public void Foo() + public void Simple(string str, double d) { - var str = ""; - Assert.AreEqual(str, ""); // Noncompliant {{Make sure these 2 arguments are in the correct order: expected value, actual value.}} -// ^^^^^^^ - Assert.AreSame(str, ""); // Noncompliant -// ^^^^^^^ - Assert.AreNotSame(str, ""); // Noncompliant - - double d = 42; - Assert.AreEqual(d, 42); // Noncompliant -// ^^^^^ - Assert.AreSame(d, 42); // Noncompliant - Assert.AreEqual(d, 42, 1, "message"); // Noncompliant + Assert.AreEqual("", str); // Compliant + Assert.AreEqual(str, ""); // Noncompliant {{Make sure these 2 arguments are in the correct order: expected value, actual value.}} + // ^^^^^^^ + Assert.AreEqual(42, d); // Compliant + Assert.AreEqual(d, 42); // Noncompliant + // ^^^^^ + Assert.AreNotEqual("", str); // Compliant + Assert.AreNotEqual(str, ""); // Noncompliant + // ^^^^^^^ + Assert.AreNotEqual(42, d); // Compliant + Assert.AreNotEqual(d, 42); // Noncompliant + // ^^^^^ + Assert.AreSame("", str); // Compliant + Assert.AreSame(str, ""); // Noncompliant + // ^^^^^^^ + Assert.AreSame(42, d); // Compliant + Assert.AreSame(d, 42); // Noncompliant + // ^^^^^ + Assert.AreNotSame("", str); // Compliant + Assert.AreNotSame(str, ""); // Noncompliant + // ^^^^^^^ + Assert.AreNotSame(42, d); // Compliant + Assert.AreNotSame(d, 42); // Noncompliant + // ^^^^^ - Assert.AreEqual("", str); - Assert.AreSame("", str); - Assert.AreEqual(42, d, 1, "message"); + Assert.AreEqual("", str, "message"); // Compliant + Assert.AreEqual(str, "", "message"); // Noncompliant + Assert.AreNotEqual("", str, "message"); // Compliant + Assert.AreNotEqual(str, "", "message"); // Noncompliant + Assert.AreSame("", str, "message"); // Compliant + Assert.AreSame(str, "", "message"); // Noncompliant + Assert.AreNotSame("", str, "message"); // Compliant + Assert.AreNotSame(str, "", "message"); // Noncompliant + Assert.IsNull(str); FakeAssert(d, 42); } } @@ -43,16 +61,38 @@ class Program public void Foo() { var str = ""; - Assert.AreEqual(actual: "", expected: str); // Compliant FN + Assert.AreEqual(actual: "", expected: str); // Noncompliant Assert.AreEqual(expected: "", actual: str); // Compliant - Assert.AreEqual(actual: str, expected: ""); // Noncompliant FP + Assert.AreEqual(actual: str, expected: ""); // Compliant Assert.AreEqual(expected: str, actual: ""); // Noncompliant - Assert.AreNotEqual(actual: "", expected: str); // Compliant FN - Assert.AreSame(actual: "", expected: str); // Compliant FN - Assert.AreNotSame(actual: "", expected: str); // Compliant FN + Assert.AreNotEqual(actual: "", expected: str); // Noncompliant + Assert.AreSame(actual: "", expected: str); // Noncompliant + Assert.AreNotSame(actual: "", expected: str); // Noncompliant + + Assert.AreEqual(actual: null, expected: new Program()); // Noncompliant + + Assert.AreEqual(message: "", expected: str, actual: ""); // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^^^^^ + Assert.AreEqual(expected: str, message: "", actual: ""); // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + } + + [Test] + public void Dynamic() + { + dynamic d = 42; + Assert.AreEqual(d, 35); // Noncompliant + Assert.AreEqual(35, d); // Compliant + Assert.AreEqual(actual: d, expected: 35); // Compliant + Assert.AreEqual(actual: 35, expected: d); // Noncompliant + } - Assert.AreEqual(actual: null, expected: new Program()); // Compliant FN + [Test] + public void BrokeSyntax() + { + double d = 42; + Assert.Equual(d, 42); // Error } } } @@ -71,7 +111,7 @@ public void TestString() string stringToTest = RetrieveString(); const string constString = "Spring"; - Assert.AreEqual(expected: stringToTest, actual: constString); // FN + Assert.AreEqual(expected: stringToTest, actual: constString); // Noncompliant Assert.AreEqual(expected: constString, actual: stringToTest); // Compliant } @@ -80,7 +120,7 @@ public void TestEnum() { Seasons seasonToTest = RetrieveSeason(); - Assert.AreEqual(expected: seasonToTest, actual: Seasons.Spring); //FN + Assert.AreEqual(expected: seasonToTest, actual: Seasons.Spring); //Noncompliant Assert.AreEqual(expected: Seasons.Spring, actual: seasonToTest); // Compliant } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs index 845dfd67ff9..11319d585ca 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/AssertionArgsShouldBePassedInCorrectOrder.Xunit.cs @@ -6,24 +6,51 @@ namespace Tests.Diagnostics class Program { [Fact] - public void Foo() + public void Simple(string str, double d) { - var str = ""; - Assert.Equal(str, ""); // Noncompliant {{Make sure these 2 arguments are in the correct order: expected value, actual value.}} -// ^^^^^^^ - Assert.Same(str, ""); // Noncompliant -// ^^^^^^^ - double d = 42; - Assert.Equal(d, 42); // Noncompliant -// ^^^^^ - Assert.Same(d, 42); // Noncompliant - Assert.NotSame(d, 42); // Noncompliant - Assert.NotSame(42, d); - Assert.Equal(d, 42, 1); // Noncompliant + Assert.Equal("", str); // Compliant + Assert.Equal(str, ""); // Noncompliant {{Make sure these 2 arguments are in the correct order: expected value, actual value.}} + // ^^^^^^^ + Assert.Equal(42, d); // Compliant + Assert.Equal(d, 42); // Noncompliant + // ^^^^^ + Assert.NotEqual("", str); // Compliant + Assert.NotEqual(str, ""); // Noncompliant + // ^^^^^^^ + Assert.NotEqual(42, d); // Compliant + Assert.NotEqual(d, 42); // Noncompliant + // ^^^^^ + Assert.Same("", str); // Compliant + Assert.Same(str, ""); // Noncompliant + // ^^^^^^^ + Assert.Same(42, d); // Compliant + Assert.Same(d, 42); // Noncompliant + // ^^^^^ + Assert.NotSame("", str); // Compliant + Assert.NotSame(str, ""); // Noncompliant + // ^^^^^^^ + Assert.NotSame(42, d); // Compliant + Assert.NotSame(d, 42); // Noncompliant + // ^^^^^ - Assert.Equal("", str); - Assert.Same("", str); - Assert.Equal(42, d, 1); + Assert.Null(str); + } + + [Fact] + public void Dynamic() + { + dynamic d = 42; + Assert.Equal(d, 35); // Noncompliant + Assert.Equal(35, d); // Compliant + Assert.Equal(actual: d, expected: 35); // Compliant + Assert.Equal(actual: 35, expected: d); // Noncompliant + } + + [Fact] + public void BrokeSyntax() + { + double d = 42; + Assert.Equual(d, 42); // Error } } } @@ -37,17 +64,20 @@ class Program public void Foo() { var str = ""; - Assert.Equal(actual: "", expected: str); // Compliant FN - Assert.Equal(expected: "", actual: str); // Compliant - Assert.Equal(actual: str, expected: ""); // Noncompliant FP - Assert.Equal(expected: str, actual: ""); // Noncompliant + Assert.Equal(actual: "", expected: str); // Noncompliant + Assert.Equal(expected: "", actual: str); // Compliant + Assert.Equal(actual: str, expected: ""); // Compliant + Assert.Equal(expected: str, actual: ""); // Noncompliant - Assert.Same(actual: "", expected: str); // Compliant FN - Assert.NotSame(actual: "", expected: str); // Compliant FN + Assert.Same(actual: "", expected: str); // Noncompliant + Assert.NotSame(actual: "", expected: str); // Noncompliant int d = 42; - Assert.Equal(actual: 1, expected: d); // Compliant FN - Assert.Equal(actual: null, expected: new Program()); // Compliant FN + Assert.Equal(actual: 1, expected: d); // Noncompliant + Assert.Equal(actual: null, expected: new Program()); // Noncompliant + + Assert.Equal(expected: str, actual: ""); // Noncompliant + // ^^^^^^^^^^^^^^^^^^^^^^^^^ } } } @@ -65,7 +95,7 @@ public void TestString() string stringToTest = RetrieveString(); const string constString = "Spring"; - Assert.Same(expected: stringToTest, actual: constString); // FN + Assert.Same(expected: stringToTest, actual: constString); // Noncompliant Assert.Same(expected: constString, actual: stringToTest); // Compliant } @@ -74,7 +104,7 @@ public void TestEnum() { Seasons seasonToTest = RetrieveSeason(); - Assert.Same(expected: seasonToTest, actual: Seasons.Spring); //FN + Assert.Same(expected: seasonToTest, actual: Seasons.Spring); // Noncompliant Assert.Same(expected: Seasons.Spring, actual: seasonToTest); // Compliant } From dd58aeac8d2d98793b32c3b3476a58ec5eb43073 Mon Sep 17 00:00:00 2001 From: Costin Zaharia <56015273+costin-zaharia-sonarsource@users.noreply.github.com> Date: Tue, 14 Mar 2023 09:23:43 +0100 Subject: [PATCH 10/12] Fix build: update msbuild path (#6921) --- azure-pipelines.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index beb7a9bc65d..205b67374a8 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -17,7 +17,7 @@ variables: - group: sonarsource-build-variables # To make sure we don`t use MsBuild 17. - name: MsBuildPath - value: 'C:\Program Files\Microsoft Visual Studio\2022\Community\Msbuild\Current\Bin\msbuild.exe' + value: 'C:\Program Files (x86)\Microsoft Visual Studio\2022\BuildTools\MSBuild\Current\Bin\MSBuild.exe' - name: UnitTestProjectPath value: 'analyzers\tests\SonarAnalyzer.UnitTest\' - name: UnitTestResultsPath From d5f094980bca8aeda9511fca62f9d9dfda4f2127 Mon Sep 17 00:00:00 2001 From: Pavel Mikula <57188685+pavel-mikula-sonarsource@users.noreply.github.com> Date: Tue, 14 Mar 2023 10:19:36 +0100 Subject: [PATCH 11/12] Move Tree from `SonarReportingContextBase` to `SonarTreeReportingContextBase` (#6837) --- .../AnalysisContext/SonarAnalysisContext.cs | 8 +++++++- .../AnalysisContext/SonarAnalysisContextBase.cs | 1 - .../AnalysisContext/SonarCodeBlockStartAnalysisContext.cs | 1 - .../AnalysisContext/SonarCompilationReportingContext.cs | 1 - .../SonarCompilationStartAnalysisContext.cs | 1 - .../AnalysisContext/SonarReportingContextBase.cs | 4 +++- .../AnalysisContext/SonarSymbolReportingContext.cs | 1 - .../src/SonarAnalyzer.Common/Helpers/ReportingContext.cs | 4 ++-- .../SonarCodeBlockStartAnalysisContextTest.cs | 1 - .../SonarCompilationReportingContextTest.cs | 7 +++---- .../SonarCompilationStartAnalysisContextTest.cs | 7 +++---- .../AnalysisContext/SonarSymbolReportingContextTest.cs | 1 - 12 files changed, 18 insertions(+), 19 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContext.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContext.cs index 35bacc2e212..322e1f1063b 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContext.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContext.cs @@ -74,6 +74,12 @@ internal SonarAnalysisContext(RoslynAnalysisContext analysisContext, IEnumerable internal static bool LegacyIsRegisteredActionEnabled(IEnumerable diagnostics, SyntaxTree tree) => ShouldExecuteRegisteredAction == null || tree == null || ShouldExecuteRegisteredAction(diagnostics, tree); + /// + /// Legacy API for backward compatibility with SonarLint v4.0 - v5.5. See . + /// + internal static bool LegacyIsRegisteredActionEnabled(DiagnosticDescriptor diagnostic, SyntaxTree tree) => + ShouldExecuteRegisteredAction == null || tree == null || ShouldExecuteRegisteredAction(new[] { diagnostic }, tree); + public void RegisterCodeBlockStartAction(GeneratedCodeRecognizer generatedCodeRecognizer, Action> action) where TSyntaxKind : struct => analysisContext.RegisterCodeBlockStartAction( @@ -121,7 +127,7 @@ public void RegisterNodeAction(GeneratedCodeRecognizer generatedCod // the decision is made on based on whether the project contains the analyzer as NuGet). if (context.HasMatchingScope(supportedDiagnostics) && context.ShouldAnalyzeTree(sourceTree, generatedCodeRecognizer) - && LegacyIsRegisteredActionEnabled(supportedDiagnostics, context.Tree)) + && LegacyIsRegisteredActionEnabled(supportedDiagnostics, sourceTree)) { action(context); } diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContextBase.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContextBase.cs index b0ef9dc8e76..e18e7365ae7 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContextBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarAnalysisContextBase.cs @@ -42,7 +42,6 @@ public class SonarAnalysisContextBase public abstract class SonarAnalysisContextBase : SonarAnalysisContextBase { - public abstract SyntaxTree Tree { get; } public abstract Compilation Compilation { get; } public abstract AnalyzerOptions Options { get; } public abstract CancellationToken Cancel { get; } diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCodeBlockStartAnalysisContext.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCodeBlockStartAnalysisContext.cs index af02d74a241..8f62d293af1 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCodeBlockStartAnalysisContext.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCodeBlockStartAnalysisContext.cs @@ -22,7 +22,6 @@ namespace SonarAnalyzer.AnalysisContext; public sealed class SonarCodeBlockStartAnalysisContext : SonarAnalysisContextBase> where TSyntaxKind : struct { - public override SyntaxTree Tree => Context.CodeBlock.SyntaxTree; public override Compilation Compilation => Context.SemanticModel.Compilation; public override AnalyzerOptions Options => Context.Options; public override CancellationToken Cancel => Context.CancellationToken; diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationReportingContext.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationReportingContext.cs index 2f5548c580c..55be60aac79 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationReportingContext.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationReportingContext.cs @@ -29,7 +29,6 @@ public sealed class SonarCompilationReportingContext : SonarCompilationReporting private static readonly Regex WebConfigRegex = new(@"[\\\/]web\.([^\\\/]+\.)?config$", RegexOptions.IgnoreCase, FileNameTimeout); private static readonly Regex AppSettingsRegex = new(@"[\\\/]appsettings\.([^\\\/]+\.)?json$", RegexOptions.IgnoreCase, FileNameTimeout); - public override SyntaxTree Tree => Context.Compilation.SyntaxTrees.FirstOrDefault(); public override Compilation Compilation => Context.Compilation; public override AnalyzerOptions Options => Context.Options; public override CancellationToken Cancel => Context.CancellationToken; diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationStartAnalysisContext.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationStartAnalysisContext.cs index bd0373b0677..e99d7c04a99 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationStartAnalysisContext.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarCompilationStartAnalysisContext.cs @@ -22,7 +22,6 @@ namespace SonarAnalyzer.AnalysisContext; public sealed class SonarCompilationStartAnalysisContext : SonarAnalysisContextBase { - public override SyntaxTree Tree => Context.Compilation.SyntaxTrees.FirstOrDefault(); public override Compilation Compilation => Context.Compilation; public override AnalyzerOptions Options => Context.Options; public override CancellationToken Cancel => Context.CancellationToken; diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarReportingContextBase.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarReportingContextBase.cs index 5801f0d13af..d8e0c77e0d4 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarReportingContextBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarReportingContextBase.cs @@ -28,7 +28,7 @@ public abstract class SonarReportingContextBase : SonarAnalysisContext protected void ReportIssueCore(Diagnostic diagnostic) { - if (HasMatchingScope(diagnostic.Descriptor)) + if (HasMatchingScope(diagnostic.Descriptor) && SonarAnalysisContext.LegacyIsRegisteredActionEnabled(diagnostic.Descriptor, diagnostic.Location?.SourceTree)) { var reportingContext = CreateReportingContext(diagnostic); if (reportingContext is { Compilation: { } compilation, Diagnostic.Location: { Kind: LocationKind.SourceFile, SourceTree: { } tree } } && !compilation.ContainsSyntaxTree(tree)) @@ -58,6 +58,8 @@ protected void ReportIssueCore(Diagnostic diagnostic) /// public abstract class SonarTreeReportingContextBase : SonarReportingContextBase { + public abstract SyntaxTree Tree { get; } + protected SonarTreeReportingContextBase(SonarAnalysisContext analysisContext, TContext context) : base(analysisContext, context) { } public void ReportIssue(Diagnostic diagnostic) => diff --git a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarSymbolReportingContext.cs b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarSymbolReportingContext.cs index 86c43f071ea..5172b22aa52 100644 --- a/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarSymbolReportingContext.cs +++ b/analyzers/src/SonarAnalyzer.Common/AnalysisContext/SonarSymbolReportingContext.cs @@ -22,7 +22,6 @@ namespace SonarAnalyzer.AnalysisContext; public sealed class SonarSymbolReportingContext : SonarCompilationReportingContextBase { - public override SyntaxTree Tree => Context.Symbol.Locations.Select(x => x.SourceTree).FirstOrDefault(x => x is not null); public override Compilation Compilation => Context.Compilation; public override AnalyzerOptions Options => Context.Options; public override CancellationToken Cancel => Context.CancellationToken; diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/ReportingContext.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/ReportingContext.cs index a5f2990b702..b720d97e4b5 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/ReportingContext.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/ReportingContext.cs @@ -35,10 +35,10 @@ public ReportingContext(SonarSyntaxTreeReportingContext context, Diagnostic diag : this(diagnostic, context.Context.ReportDiagnostic, context.Compilation, context.Tree) { } public ReportingContext(SonarCompilationReportingContext context, Diagnostic diagnostic) - : this(diagnostic, context.Context.ReportDiagnostic, context.Compilation, diagnostic.Location?.SourceTree ?? context.Tree) { } + : this(diagnostic, context.Context.ReportDiagnostic, context.Compilation, diagnostic.Location?.SourceTree) { } public ReportingContext(SonarSymbolReportingContext context, Diagnostic diagnostic) - : this(diagnostic, context.Context.ReportDiagnostic, context.Compilation, diagnostic.Location?.SourceTree ?? context.Tree) { } + : this(diagnostic, context.Context.ReportDiagnostic, context.Compilation, diagnostic.Location?.SourceTree) { } public ReportingContext(SonarCodeBlockReportingContext context, Diagnostic diagnostic) : this(diagnostic, context.Context.ReportDiagnostic, context.Compilation, context.Tree) { } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeBlockStartAnalysisContextTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeBlockStartAnalysisContextTest.cs index ec47ba9d376..26f8d2e760c 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeBlockStartAnalysisContextTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCodeBlockStartAnalysisContextTest.cs @@ -38,7 +38,6 @@ public void Properties_ArePropagated() var context = new Mock>(codeBlock, owningSymbol, model, options, cancel).Object; var sut = new SonarCodeBlockStartAnalysisContext(AnalysisScaffolding.CreateSonarAnalysisContext(), context); - sut.Tree.Should().BeSameAs(codeBlock.SyntaxTree); sut.Compilation.Should().BeSameAs(model.Compilation); sut.Options.Should().BeSameAs(options); sut.Cancel.Should().Be(cancel); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationReportingContextTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationReportingContextTest.cs index cd500aba52f..58f33bc30f2 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationReportingContextTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationReportingContextTest.cs @@ -29,13 +29,12 @@ public class SonarCompilationReportingContextTest public void Properties_ArePropagated() { var cancel = new CancellationToken(true); - var (tree, model) = TestHelper.CompileCS("// Nothing to see here"); + var compilation = TestHelper.CompileCS("// Nothing to see here").Model.Compilation; var options = AnalysisScaffolding.CreateOptions(); - var context = new CompilationAnalysisContext(model.Compilation, options, _ => { }, _ => true, cancel); + var context = new CompilationAnalysisContext(compilation, options, _ => { }, _ => true, cancel); var sut = new SonarCompilationReportingContext(AnalysisScaffolding.CreateSonarAnalysisContext(), context); - sut.Tree.Should().BeSameAs(tree); - sut.Compilation.Should().BeSameAs(model.Compilation); + sut.Compilation.Should().BeSameAs(compilation); sut.Options.Should().BeSameAs(options); sut.Cancel.Should().Be(cancel); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationStartAnalysisContextTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationStartAnalysisContextTest.cs index a91a7f66909..dcf054f19c5 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationStartAnalysisContextTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarCompilationStartAnalysisContextTest.cs @@ -30,13 +30,12 @@ public class SonarCompilationStartAnalysisContextTest public void Properties_ArePropagated() { var cancel = new CancellationToken(true); - var (tree, model) = TestHelper.CompileCS("// Nothing to see here"); + var compilation = TestHelper.CompileCS("// Nothing to see here").Model.Compilation; var options = AnalysisScaffolding.CreateOptions(); - var context = new Mock(model.Compilation, options, cancel).Object; + var context = new Mock(compilation, options, cancel).Object; var sut = new SonarCompilationStartAnalysisContext(AnalysisScaffolding.CreateSonarAnalysisContext(), context); - sut.Tree.Should().BeSameAs(tree); - sut.Compilation.Should().BeSameAs(model.Compilation); + sut.Compilation.Should().BeSameAs(compilation); sut.Options.Should().BeSameAs(options); sut.Cancel.Should().Be(cancel); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarSymbolReportingContextTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarSymbolReportingContextTest.cs index 5de2d2c8a0d..6ea9eaf607f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarSymbolReportingContextTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/AnalysisContext/SonarSymbolReportingContextTest.cs @@ -37,7 +37,6 @@ public void Properties_ArePropagated() var context = new SymbolAnalysisContext(symbol, model.Compilation, options, _ => { }, _ => true, cancel); var sut = new SonarSymbolReportingContext(AnalysisScaffolding.CreateSonarAnalysisContext(), context); - sut.Tree.Should().BeSameAs(tree); sut.Compilation.Should().BeSameAs(model.Compilation); sut.Options.Should().BeSameAs(options); sut.Cancel.Should().Be(cancel); From 8e33179c17980d27ca93ae805a36d5a1de8d09cb Mon Sep 17 00:00:00 2001 From: Tim Pohlmann Date: Tue, 14 Mar 2023 10:20:23 +0100 Subject: [PATCH 12/12] Fix S1905 FP: Nullability context and array of anonymous types (#6753) --- .../Rules/RedundantCast.cs | 240 ++++++++---------- .../Rules/RedundantCastTest.cs | 118 ++++++--- .../TestCases/RedundantCast.CSharp8.cs | 88 ++++++- 3 files changed, 282 insertions(+), 164 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantCast.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantCast.cs index a23a168971f..9c24d53abc8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantCast.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/RedundantCast.cs @@ -18,161 +18,137 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Rules.CSharp -{ - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class RedundantCast : SonarDiagnosticAnalyzer - { - internal const string DiagnosticId = "S1905"; - private const string MessageFormat = "Remove this unnecessary cast to '{0}'."; - - private static readonly DiagnosticDescriptor rule = - DescriptorFactory.Create(DiagnosticId, MessageFormat); - - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(rule); +namespace SonarAnalyzer.Rules.CSharp; - private static readonly ISet CastIEnumerableMethods = new HashSet { "Cast", "OfType" }; - - protected override void Initialize(SonarAnalysisContext context) - { - context.RegisterNodeAction( - c => - { - var castExpression = (CastExpressionSyntax)c.Node; - CheckCastExpression(c, castExpression.Expression, castExpression.Type, castExpression.Type.GetLocation()); - }, - SyntaxKind.CastExpression); +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class RedundantCast : SonarDiagnosticAnalyzer +{ + internal const string DiagnosticId = "S1905"; + private const string MessageFormat = "Remove this unnecessary cast to '{0}'."; - context.RegisterNodeAction( - c => - { - var castExpression = (BinaryExpressionSyntax)c.Node; - CheckCastExpression(c, castExpression.Left, castExpression.Right, - castExpression.OperatorToken.CreateLocation(castExpression.Right)); - }, - SyntaxKind.AsExpression); - - context.RegisterNodeAction( - CheckExtensionMethodInvocation, - SyntaxKind.InvocationExpression); - } + private static readonly DiagnosticDescriptor Rule = + DescriptorFactory.Create(DiagnosticId, MessageFormat); - private static void CheckCastExpression(SonarSyntaxNodeReportingContext context, ExpressionSyntax expression, ExpressionSyntax type, Location location) - { - if (expression.IsKind(SyntaxKindEx.DefaultLiteralExpression)) - { - return; - } + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); - var expressionType = context.SemanticModel.GetTypeInfo(expression).Type; - if (expressionType == null) - { - return; - } + private static readonly ISet CastIEnumerableMethods = new HashSet { "Cast", "OfType" }; - var castType = context.SemanticModel.GetTypeInfo(type).Type; - if (castType == null) + protected override void Initialize(SonarAnalysisContext context) + { + context.RegisterNodeAction( + c => { - return; - } + var castExpression = (CastExpressionSyntax)c.Node; + CheckCastExpression(c, castExpression.Expression, castExpression.Type, castExpression.Type.GetLocation()); + }, + SyntaxKind.CastExpression); - if (expressionType.Equals(castType)) + context.RegisterNodeAction( + c => { - context.ReportIssue(Diagnostic.Create(rule, location, - castType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart))); - } - } + var castExpression = (BinaryExpressionSyntax)c.Node; + CheckCastExpression(c, castExpression.Left, castExpression.Right, + castExpression.OperatorToken.CreateLocation(castExpression.Right)); + }, + SyntaxKind.AsExpression); + + context.RegisterNodeAction( + CheckExtensionMethodInvocation, + SyntaxKind.InvocationExpression); + } - private static void CheckExtensionMethodInvocation(SonarSyntaxNodeReportingContext context) + private static void CheckCastExpression(SonarSyntaxNodeReportingContext context, ExpressionSyntax expression, ExpressionSyntax type, Location location) + { + if (!expression.IsKind(SyntaxKindEx.DefaultLiteralExpression) + && context.SemanticModel.GetTypeInfo(expression) is { Type: { } expressionType } expressionTypeInfo + && context.SemanticModel.GetTypeInfo(type) is { Type: { } castType } + && expressionType.Equals(castType) + && FlowStateEquals(expressionTypeInfo, type)) { - var invocation = (InvocationExpressionSyntax)context.Node; - if (GetEnumerableExtensionSymbol(invocation, context.SemanticModel) is { } methodSymbol) - { - var returnType = methodSymbol.ReturnType; - if (GetGenericTypeArgument(returnType) is { } castType) - { - if (methodSymbol.Name == "OfType" && CanHaveNullValue(castType)) - { - // OfType() filters 'null' values from enumerables - return; - } - - var elementType = GetElementType(invocation, methodSymbol, context.SemanticModel); - // Generic types {T} and {T?} are equal and there is no way to access NullableAnnotation field right now - // See https://github.com/SonarSource/sonar-dotnet/issues/3273 - if (elementType != null && elementType.Equals(castType) && string.Equals(elementType.ToString(), castType.ToString(), System.StringComparison.Ordinal)) - { - var methodCalledAsStatic = methodSymbol.MethodKind == MethodKind.Ordinary; - context.ReportIssue(Diagnostic.Create(rule, GetReportLocation(invocation, methodCalledAsStatic), - returnType.ToMinimalDisplayString(context.SemanticModel, invocation.SpanStart))); - } - } - } + ReportIssue(context, expression, castType, location); } + } - /// If the invocation one of the extensions, returns the method symbol. - private static IMethodSymbol GetEnumerableExtensionSymbol(InvocationExpressionSyntax invocation, SemanticModel semanticModel) => - invocation.GetMethodCallIdentifier() is { } methodName - && CastIEnumerableMethods.Contains(methodName.ValueText) - && semanticModel.GetSymbolInfo(invocation).Symbol is IMethodSymbol methodSymbol - && methodSymbol.IsExtensionOn(KnownType.System_Collections_IEnumerable) - ? methodSymbol - : null; - - private static ITypeSymbol GetGenericTypeArgument(ITypeSymbol type) => - type is INamedTypeSymbol returnType && returnType.Is(KnownType.System_Collections_Generic_IEnumerable_T) - ? returnType.TypeArguments.Single() - : null; - - private static bool CanHaveNullValue(ITypeSymbol type) => type.IsReferenceType || type.Name == "Nullable"; - - private static Location GetReportLocation(InvocationExpressionSyntax invocation, bool methodCalledAsStatic) + private static bool FlowStateEquals(TypeInfo expressionTypeInfo, ExpressionSyntax type) + { + var castingToNullable = type.IsKind(SyntaxKind.NullableType); + return expressionTypeInfo.Nullability().FlowState switch { - if (!(invocation.Expression is MemberAccessExpressionSyntax memberAccess)) - { - return invocation.Expression.GetLocation(); - } - - return methodCalledAsStatic - ? memberAccess.GetLocation() - : memberAccess.OperatorToken.CreateLocation(invocation); - } + NullableFlowState.None => true, + NullableFlowState.MaybeNull => castingToNullable, + NullableFlowState.NotNull => !castingToNullable, + _ => true, + }; + } - private static ITypeSymbol GetElementType(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol, - SemanticModel semanticModel) + private static void CheckExtensionMethodInvocation(SonarSyntaxNodeReportingContext context) + { + var invocation = (InvocationExpressionSyntax)context.Node; + if (GetEnumerableExtensionSymbol(invocation, context.SemanticModel) is { } methodSymbol) { - ExpressionSyntax collection; - if (methodSymbol.MethodKind == MethodKind.Ordinary) + var returnType = methodSymbol.ReturnType; + if (GetGenericTypeArgument(returnType) is { } castType) { - if (!invocation.ArgumentList.Arguments.Any()) + if (methodSymbol.Name == "OfType" && CanHaveNullValue(castType)) { - return null; + // OfType() filters 'null' values from enumerables + return; } - collection = invocation.ArgumentList.Arguments.First().Expression; - } - else - { - if (!(invocation.Expression is MemberAccessExpressionSyntax memberAccess)) + + var elementType = GetElementType(invocation, methodSymbol, context.SemanticModel); + // Generic types {T} and {T?} are equal and there is no way to access NullableAnnotation field right now + // See https://github.com/SonarSource/sonar-dotnet/issues/3273 + if (elementType != null && elementType.Equals(castType) && string.Equals(elementType.ToString(), castType.ToString(), StringComparison.Ordinal)) { - return null; + var methodCalledAsStatic = methodSymbol.MethodKind == MethodKind.Ordinary; + ReportIssue(context, invocation, returnType, GetReportLocation(invocation, methodCalledAsStatic)); } - collection = memberAccess.Expression; } + } + } - var typeInfo = semanticModel.GetTypeInfo(collection); - if (typeInfo.Type is INamedTypeSymbol collectionType && - collectionType.TypeArguments.Length == 1) - { - return collectionType.TypeArguments.First(); - } + private static void ReportIssue(SonarSyntaxNodeReportingContext context, ExpressionSyntax expression, ITypeSymbol castType, Location location) => + context.ReportIssue(Diagnostic.Create(Rule, location, castType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart))); - if (typeInfo.Type is IArrayTypeSymbol arrayType && - arrayType.Rank == 1) // casting is necessary for multidimensional arrays - { - return arrayType.ElementType; - } + /// If the invocation one of the extensions, returns the method symbol. + private static IMethodSymbol GetEnumerableExtensionSymbol(InvocationExpressionSyntax invocation, SemanticModel semanticModel) => + invocation.GetMethodCallIdentifier() is { } methodName + && CastIEnumerableMethods.Contains(methodName.ValueText) + && semanticModel.GetSymbolInfo(invocation).Symbol is IMethodSymbol methodSymbol + && methodSymbol.IsExtensionOn(KnownType.System_Collections_IEnumerable) + ? methodSymbol + : null; - return null; - } + private static ITypeSymbol GetGenericTypeArgument(ITypeSymbol type) => + type is INamedTypeSymbol returnType && returnType.Is(KnownType.System_Collections_Generic_IEnumerable_T) + ? returnType.TypeArguments.Single() + : null; + + private static bool CanHaveNullValue(ITypeSymbol type) => + type.IsReferenceType || type.Is(KnownType.System_Nullable_T); + + private static Location GetReportLocation(InvocationExpressionSyntax invocation, bool methodCalledAsStatic) => + methodCalledAsStatic is false && invocation.Expression is MemberAccessExpressionSyntax memberAccess + ? memberAccess.OperatorToken.CreateLocation(invocation) + : invocation.Expression.GetLocation(); + + private static ITypeSymbol GetElementType(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol, SemanticModel semanticModel) + { + return semanticModel.GetTypeInfo(CollectionExpression(invocation, methodSymbol)).Type switch + { + INamedTypeSymbol { TypeArguments: { Length: 1 } typeArguments } => typeArguments.First(), + IArrayTypeSymbol { Rank: 1 } arrayType => arrayType.ElementType, // casting is necessary for multidimensional arrays + _ => null + }; + + static ExpressionSyntax CollectionExpression(InvocationExpressionSyntax invocation, IMethodSymbol methodSymbol) => + methodSymbol.MethodKind is MethodKind.ReducedExtension + ? ReducedExtensionExpression(invocation) + : invocation.ArgumentList.Arguments.FirstOrDefault()?.Expression; + + static ExpressionSyntax ReducedExtensionExpression(InvocationExpressionSyntax invocation) => + invocation.Expression is MemberAccessExpressionSyntax { Expression: { } memberAccessExpression } + ? memberAccessExpression + : invocation.GetParentConditionalAccessExpression()?.Expression; } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/RedundantCastTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/RedundantCastTest.cs index a183818ceb7..7e707e05d17 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/RedundantCastTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/RedundantCastTest.cs @@ -21,45 +21,103 @@ using Microsoft.CodeAnalysis.CSharp; using SonarAnalyzer.Rules.CSharp; -namespace SonarAnalyzer.UnitTest.Rules +namespace SonarAnalyzer.UnitTest.Rules; + +[TestClass] +public class RedundantCastTest { - [TestClass] - public class RedundantCastTest - { - private readonly VerifierBuilder builder = new VerifierBuilder(); + private readonly VerifierBuilder builder = new VerifierBuilder(); - [TestMethod] - public void RedundantCast() => - builder.AddPaths("RedundantCast.cs").Verify(); + [TestMethod] + public void RedundantCast() => + builder.AddPaths("RedundantCast.cs").Verify(); - [TestMethod] - public void RedundantCast_CSharp8() => - builder.AddPaths("RedundantCast.CSharp8.cs").WithOptions(ParseOptionsHelper.FromCSharp8).Verify(); + [TestMethod] + public void RedundantCast_CSharp8() => + builder.AddPaths("RedundantCast.CSharp8.cs").WithOptions(ParseOptionsHelper.FromCSharp8).Verify(); #if NET - [TestMethod] - public void RedundantCast_CSharp9() => - builder.AddPaths("RedundantCast.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify(); + [TestMethod] + public void RedundantCast_CSharp9() => + builder.AddPaths("RedundantCast.CSharp9.cs").WithOptions(ParseOptionsHelper.FromCSharp9).Verify(); #endif - [TestMethod] - public void RedundantCast_CodeFix() => - builder.AddPaths("RedundantCast.cs").WithCodeFix().WithCodeFixedPaths("RedundantCast.Fixed.cs").VerifyCodeFix(); + [TestMethod] + public void RedundantCast_CodeFix() => + builder.AddPaths("RedundantCast.cs").WithCodeFix().WithCodeFixedPaths("RedundantCast.Fixed.cs").VerifyCodeFix(); - [TestMethod] - public void RedundantCast_DefaultLiteral() => - builder.AddSnippet(@" -using System; -public static class MyClass -{ - public static void RunAction(Action action) + [TestMethod] + public void RedundantCast_DefaultLiteral() => + builder.AddSnippet(""" + using System; + public static class MyClass + { + public static void RunAction(Action action) + { + bool myBool = (bool)default; // FN - the cast is unneeded + RunFunc(() => { action(); return default; }, (bool)default); // should not raise because of the generic the cast is mandatory + RunFunc(() => { action(); return default; }, (bool)default); // FN - the cast is unneeded + } + + public static T RunFunc(Func func, T returnValue = default) => returnValue; + } + """).WithLanguageVersion(LanguageVersion.CSharp7_1).Verify(); + + [TestMethod] + [DynamicData(nameof(NullableTestDataWithFlowState))] + public void RedundantCast_NullableEnabled(string snippet, bool compliant) + => VerifyNullableTests(snippet, "enable", compliant); + + [TestMethod] + [DynamicData(nameof(NullableTestDataWithFlowState))] + public void RedundantCast_NullableWarnings(string snippet, bool compliant) + => VerifyNullableTests(snippet, "enable warnings", compliant); + + [TestMethod] + [DynamicData(nameof(NullableTestDataWithoutFlowState))] + public void RedundantCast_NullableDisabled(string snippet, bool compliant) + => VerifyNullableTests(snippet, "disable", compliant); + + [TestMethod] + [DynamicData(nameof(NullableTestDataWithoutFlowState))] + public void RedundantCast_NullableAnnotations(string snippet, bool compliant) + => VerifyNullableTests(snippet, "enable annotations", compliant); + + private void VerifyNullableTests(string snippet, string nullableContext, bool compliant) { - bool myBool = (bool)default; // FN - the cast is unneeded - RunFunc(() => { action(); return default; }, (bool)default); // should not raise because of the generic the cast is mandatory - RunFunc(() => { action(); return default; }, (bool)default); // FN - the cast is unneeded + var code = $$""" + #nullable {{nullableContext}} + void Test(string nonNullable, string? nullable) + { + {{snippet}} // {{compliant switch { true => "Compliant", false => "Noncompliant" }}} + } + """; + builder.AddSnippet(code).WithTopLevelStatements().Verify(); } - public static T RunFunc(Func func, T returnValue = default) => returnValue; -}").WithLanguageVersion(LanguageVersion.CSharp7_1).Verify(); - } + private static IEnumerable<(string Snippet, bool CompliantWithFlowState, bool CompliantWithoutFlowState)> NullableTestData => new[] + { + ("""_ = (string)"Test";""", false, false), + ("""_ = (string?)"Test";""", true, false), + ("""_ = (string)null;""", true, true), + ("""_ = (string?)null;""", true, true), + ("""_ = (string)nullable;""", true, false), + ("""_ = (string?)nullable;""", false, false), + ("""_ = (string)nonNullable;""", false, false), + ("""_ = (string?)nonNullable;""", true, false), + ("""_ = nullable as string;""", true, false), + ("""_ = nonNullable as string;""", false, false), + ("""if (nullable != null) _ = (string)nullable;""", false, false), + ("""if (nullable != null) _ = (string?)nullable;""", true, false), + ("""if (nullable != null) _ = nullable as string;""", false, false), + ("""if (nonNullable == null) _ = (string)nonNullable;""", true, false), + ("""if (nonNullable == null) _ = (string?)nonNullable;""", false, false), + ("""if (nonNullable == null) _ = nonNullable as string;""", true, false), + }; + + private static IEnumerable NullableTestDataWithFlowState => + NullableTestData.Select(x => new object[] { x.Snippet, x.CompliantWithFlowState }); + + private static IEnumerable NullableTestDataWithoutFlowState => + NullableTestData.Select(x => new object[] { x.Snippet, x.CompliantWithoutFlowState }); } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/RedundantCast.CSharp8.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/RedundantCast.CSharp8.cs index adab201f62f..1e623abffc3 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/RedundantCast.CSharp8.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/RedundantCast.CSharp8.cs @@ -2,15 +2,99 @@ using System.Collections.Generic; using System.Linq; +#nullable enable namespace Tests.Diagnostics { + public class InvocationTests + { + public static void Invocations() + { + var ints = new int[1]; + var objects= new object[1]; + var moreInts = new int[1][]; + var moreObjects = new object[1][]; + ints?.Cast(); // Noncompliant + objects?.Cast(); // Compliant + moreInts[0].Cast(); // Noncompliant + moreObjects[0].Cast(); // Compliant + moreInts[0]?.Cast(); // Noncompliant + moreObjects[0]?.Cast(); // Compliant + GetInts().Cast(); // Noncompliant + GetObjects().Cast(); // Compliant + GetInts()?.Cast(); // Noncompliant + GetObjects()?.Cast(); // Compliant + Enumerable.Cast(); // Error - overload resolution failure + } + + public static int[] GetInts() => null; + public static object[] GetObjects() => null; + } + // https://github.com/SonarSource/sonar-dotnet/issues/3273 public class CastOnNullable { - public static IEnumerable UsefulCast() + public static IEnumerable Array() { var nullableStrings = new string?[] { "one", "two", null, "three" }; return nullableStrings.OfType(); // Compliant - filters out the null } - } + + public void Tuple() + { + _ = (a: (string?)"", b: ""); // Compliant + } + + public void ValueTypes(int nonNullable, int? nullable) + { + _ = (int?)nonNullable; // Compliant + _ = (int?)nullable; // Noncompliant + _ = (int)nonNullable; // Noncompliant + _ = (int)nullable; // Compliant + } + } + + // https://github.com/SonarSource/sonar-dotnet/issues/6438 + public class AnonTypes + { + public void Simple(string nonNullable, string? nullable) + { + _ = new { X = (string?)nonNullable }; // Compliant + _ = new { X = (string?)nullable }; // Noncompliant + _ = new { X = (string)nonNullable }; // Noncompliant + _ = new { X = (string)nullable }; // Compliant + } + + public void Array(string nonNullable, string? nullable) + { + _ = new[] { new { X = (string?)nonNullable }, new { X = (string?)null } }; // Compliant + _ = new[] { new { X = (string?)nullable }, new { X = (string?)null } }; // Noncompliant + _ = new[] { new { X = (string?)nonNullable } }; // Compliant + _ = new[] { new { X = (string?)nullable } }; // Noncompliant + _ = new[] { new HoldsObject(new { X = (string?)nonNullable }) }; // Compliant + _ = new[] { new HoldsObject(new { X = (string?)nullable }) }; // Noncompliant + } + + public void SwitchExpression(string nonNullable, string? nullable) + { + _ = true switch + { + true => new { X = (string?)nonNullable }, // Compliant + false => new { X = (string?)null } // Compliant + }; + _ = true switch + { + true => new { X = (string?)nullable }, // Noncompliant + false => new { X = (string?)null } // Compliant + }; + } + } + + internal class HoldsObject + { + object O { get; } + public HoldsObject(object o) + { + O = o; + } + } }