From 874001a022c4e82f5889e8c7abb4cd52a9f3d3df Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Thu, 16 Feb 2023 11:16:19 +0100 Subject: [PATCH 01/14] Add more UTs to be more explicit about access modifiers --- .../src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 4 ++-- .../SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs | 5 +++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 6a412c10be3..6a7863aee15 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -44,8 +44,8 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer private static IEnumerable GetDiagnostics(VariableDeclarationSyntax declaration, SemanticModel semanticModel) => declaration.Variables - .Where(x => FieldIsRelevant(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol)) - .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText)); + .Where(x => FieldIsRelevant(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol)) + .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText)); private static bool FieldIsRelevant(IFieldSymbol fieldSymbol) => fieldSymbol is {IsStatic: true, IsConst: false, IsReadOnly: false} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index 441f2428d9c..28ba5029cfa 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -9,6 +9,11 @@ public class StaticFieldVisible public const double Pi2 = 3.14; public double Pi3 = 3.14; + protected static double Pi4 = 3.14; // Noncompliant + internal static double Pi5 = 3.14; // Noncompliant + internal static double Pi6 = 3.14; // Noncompliant + protected internal static double Pi7 = 3.14; // Noncompliant + [ThreadStatic] public static int value; // Compliant, thread static field values are not shared between threads } From 438e2dc842bd2ec41eb062d0ff35eb3678e224b2 Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Thu, 16 Feb 2023 14:29:42 +0100 Subject: [PATCH 02/14] replace semantic model operations with AST ones --- .../Rules/StaticFieldVisible.cs | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 6a7863aee15..786665489da 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -34,22 +34,26 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer context.RegisterNodeAction( c => { - var fieldDeclaration = (FieldDeclarationSyntax)c.Node; - foreach (var diagnostic in GetDiagnostics(fieldDeclaration.Declaration, c.SemanticModel)) + foreach (var diagnostic in GetDiagnostics((FieldDeclarationSyntax)c.Node, c.SemanticModel)) { c.ReportIssue(diagnostic); } }, SyntaxKind.FieldDeclaration); - private static IEnumerable GetDiagnostics(VariableDeclarationSyntax declaration, SemanticModel semanticModel) => - declaration.Variables - .Where(x => FieldIsRelevant(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol)) - .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText)); + private static IEnumerable GetDiagnostics(FieldDeclarationSyntax declaration, SemanticModel semanticModel) => + FieldIsRelevant(declaration) + ? declaration.Declaration.Variables + .Where(x => !FieldIsThreadSafe(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol)) + .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText)) + : Enumerable.Empty(); - private static bool FieldIsRelevant(IFieldSymbol fieldSymbol) => - fieldSymbol is {IsStatic: true, IsConst: false, IsReadOnly: false} - && fieldSymbol.DeclaredAccessibility != Accessibility.Private - && !fieldSymbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.System_ThreadStaticAttribute)); + private static bool FieldIsRelevant(FieldDeclarationSyntax node) => + !node.Modifiers.Any(SyntaxKind.PrivateKeyword) + && node.Modifiers.Any(SyntaxKind.StaticKeyword) + && !(node.Modifiers.Any(SyntaxKind.ConstKeyword) || node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)); + + private static bool FieldIsThreadSafe(IFieldSymbol fieldSymbol) => + fieldSymbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.System_ThreadStaticAttribute)); } } From eed966719ed3b766aecb5879a46052a5f0092698 Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Thu, 16 Feb 2023 14:38:56 +0100 Subject: [PATCH 03/14] Add test with multiple filed declarations in one line --- .../SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index 28ba5029cfa..ff8a597f6f3 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -6,6 +6,10 @@ public class StaticFieldVisible { public static double Pi = 3.14; // Noncompliant // ^^ + public static int X = 1, Y, Z = 100; +// ^ +// ^@-1 +// ^@-2 public const double Pi2 = 3.14; public double Pi3 = 3.14; From 73f5639364072ef8c7aef88f1932bff5c561d61e Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Thu, 16 Feb 2023 14:55:18 +0100 Subject: [PATCH 04/14] more UTS --- .../src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 3 ++- .../SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs | 5 +++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 786665489da..5d1d9e071fc 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -49,7 +49,8 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer : Enumerable.Empty(); private static bool FieldIsRelevant(FieldDeclarationSyntax node) => - !node.Modifiers.Any(SyntaxKind.PrivateKeyword) + node.Modifiers.Any() + && !node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.StaticKeyword) && !(node.Modifiers.Any(SyntaxKind.ConstKeyword) || node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index ff8a597f6f3..371ba25e890 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -18,6 +18,11 @@ public class StaticFieldVisible internal static double Pi6 = 3.14; // Noncompliant protected internal static double Pi7 = 3.14; // Noncompliant + private static double Pi8 = 3.14; + private double Pi9 = 3.14; + static double Pi10 = 3.14; // Noncompliant "internal" is the default if no access modifier is specified + // https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/access-modifiers + [ThreadStatic] public static int value; // Compliant, thread static field values are not shared between threads } From 016aebf461b918f69005a9cf0bec60bec153f35f Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Thu, 16 Feb 2023 16:30:55 +0100 Subject: [PATCH 05/14] update ITs --- .../expected/Nancy/Nancy--net452-S2223.json | 78 +++++++++++++++++++ .../Nancy/Nancy--netstandard2.0-S2223.json | 78 +++++++++++++++++++ .../its/expected/Net5/Net5--net5.0-S2223.json | 30 +++++++ 3 files changed, 186 insertions(+) create mode 100644 analyzers/its/expected/Net5/Net5--net5.0-S2223.json diff --git a/analyzers/its/expected/Nancy/Nancy--net452-S2223.json b/analyzers/its/expected/Nancy/Nancy--net452-S2223.json index f33562d4425..8c705f0d29c 100644 --- a/analyzers/its/expected/Nancy/Nancy--net452-S2223.json +++ b/analyzers/its/expected/Nancy/Nancy--net452-S2223.json @@ -15,6 +15,71 @@ }, { "id": "S2223", +"message": "Change the visibility of 'hexChars' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 47, +"startColumn": 23, +"endLine": 47, +"endColumn": 31 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'entitiesLock' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 48, +"startColumn": 23, +"endLine": 48, +"endColumn": 35 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'entities' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 49, +"startColumn": 47, +"endLine": 49, +"endColumn": 55 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'defaultEncoder' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 54, +"startColumn": 28, +"endLine": 54, +"endColumn": 42 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'currentEncoder' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 56, +"startColumn": 28, +"endLine": 56, +"endColumn": 42 +} +} +}, +{ +"id": "S2223", "message": "Change the visibility of 'DEFAULT_SWITCHOVER_THRESHOLD' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\IO\RequestStream.cs", @@ -28,6 +93,19 @@ }, { "id": "S2223", +"message": "Change the visibility of 'mimeTypes' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\MimeTypes.cs", +"region": { +"startLine": 41, +"startColumn": 44, +"endLine": 41, +"endColumn": 53 +} +} +}, +{ +"id": "S2223", "message": "Change the visibility of 'NoBody' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\Response.cs", diff --git a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json index f33562d4425..8c705f0d29c 100644 --- a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json +++ b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json @@ -15,6 +15,71 @@ }, { "id": "S2223", +"message": "Change the visibility of 'hexChars' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 47, +"startColumn": 23, +"endLine": 47, +"endColumn": 31 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'entitiesLock' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 48, +"startColumn": 23, +"endLine": 48, +"endColumn": 35 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'entities' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 49, +"startColumn": 47, +"endLine": 49, +"endColumn": 55 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'defaultEncoder' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 54, +"startColumn": 28, +"endLine": 54, +"endColumn": 42 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'currentEncoder' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", +"region": { +"startLine": 56, +"startColumn": 28, +"endLine": 56, +"endColumn": 42 +} +} +}, +{ +"id": "S2223", "message": "Change the visibility of 'DEFAULT_SWITCHOVER_THRESHOLD' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\IO\RequestStream.cs", @@ -28,6 +93,19 @@ }, { "id": "S2223", +"message": "Change the visibility of 'mimeTypes' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Nancy\src\Nancy\MimeTypes.cs", +"region": { +"startLine": 41, +"startColumn": 44, +"endLine": 41, +"endColumn": 53 +} +} +}, +{ +"id": "S2223", "message": "Change the visibility of 'NoBody' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\Response.cs", diff --git a/analyzers/its/expected/Net5/Net5--net5.0-S2223.json b/analyzers/its/expected/Net5/Net5--net5.0-S2223.json new file mode 100644 index 00000000000..950afd59eb7 --- /dev/null +++ b/analyzers/its/expected/Net5/Net5--net5.0-S2223.json @@ -0,0 +1,30 @@ +{ +"issues": [ +{ +"id": "S2223", +"message": "Change the visibility of 'foo' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Net5\Net5\S3604.cs", +"region": { +"startLine": 7, +"startColumn": 20, +"endLine": 7, +"endColumn": 23 +} +} +}, +{ +"id": "S2223", +"message": "Change the visibility of 'bar' or make it 'const' or 'readonly'.", +"location": { +"uri": "sources\Net5\Net5\S3604.cs", +"region": { +"startLine": 8, +"startColumn": 20, +"endLine": 8, +"endColumn": 23 +} +} +} +] +} From 9537be332e780422d1dfae218f2369a44d35c2fc Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Thu, 16 Feb 2023 16:49:26 +0100 Subject: [PATCH 06/14] delete not useful condition and update UTs --- analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 3 +-- .../SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 5d1d9e071fc..786665489da 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -49,8 +49,7 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer : Enumerable.Empty(); private static bool FieldIsRelevant(FieldDeclarationSyntax node) => - node.Modifiers.Any() - && !node.Modifiers.Any(SyntaxKind.PrivateKeyword) + !node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.StaticKeyword) && !(node.Modifiers.Any(SyntaxKind.ConstKeyword) || node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index 371ba25e890..2d14a810fa8 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -22,6 +22,7 @@ public class StaticFieldVisible private double Pi9 = 3.14; static double Pi10 = 3.14; // Noncompliant "internal" is the default if no access modifier is specified // https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/access-modifiers + public readonly double Pi11 = 3.14; [ThreadStatic] public static int value; // Compliant, thread static field values are not shared between threads From c906d0234891794592bc2322f218d4853d44083c Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Fri, 17 Feb 2023 10:48:06 +0100 Subject: [PATCH 07/14] remove check for const as if there is static keyword then const will never be there --- analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 786665489da..f74c67be822 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -51,7 +51,7 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer private static bool FieldIsRelevant(FieldDeclarationSyntax node) => !node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.StaticKeyword) - && !(node.Modifiers.Any(SyntaxKind.ConstKeyword) || node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword)); + && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword); private static bool FieldIsThreadSafe(IFieldSymbol fieldSymbol) => fieldSymbol.GetAttributes().Any(x => x.AttributeClass.Is(KnownType.System_ThreadStaticAttribute)); From 47f6635480e57ac8c82221a29fd0f5b301efc2c7 Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Fri, 17 Feb 2023 14:17:43 +0100 Subject: [PATCH 08/14] if no access modifier exist the field is private --- .../src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 5 +++-- .../SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs | 3 +-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index f74c67be822..839ff9c5d94 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -49,8 +49,9 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer : Enumerable.Empty(); private static bool FieldIsRelevant(FieldDeclarationSyntax node) => - !node.Modifiers.Any(SyntaxKind.PrivateKeyword) - && node.Modifiers.Any(SyntaxKind.StaticKeyword) + node.Modifiers.Any(SyntaxKind.StaticKeyword) + && node.Modifiers.Count > 1 + && !node.Modifiers.Any(SyntaxKind.PrivateKeyword) && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword); private static bool FieldIsThreadSafe(IFieldSymbol fieldSymbol) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index 2d14a810fa8..eb67997293f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -20,8 +20,7 @@ public class StaticFieldVisible private static double Pi8 = 3.14; private double Pi9 = 3.14; - static double Pi10 = 3.14; // Noncompliant "internal" is the default if no access modifier is specified - // https://learn.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/access-modifiers + static double Pi10 = 3.14; // Compliant - if not access modifier exist the field is private public readonly double Pi11 = 3.14; [ThreadStatic] From f6cf3ad21f3d6f7000b432c395d77d971e6fb840 Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Fri, 17 Feb 2023 14:18:07 +0100 Subject: [PATCH 09/14] Revert "update ITs" This reverts commit 016aebf461b918f69005a9cf0bec60bec153f35f. --- .../expected/Nancy/Nancy--net452-S2223.json | 78 ------------------- .../Nancy/Nancy--netstandard2.0-S2223.json | 78 ------------------- .../its/expected/Net5/Net5--net5.0-S2223.json | 30 ------- 3 files changed, 186 deletions(-) delete mode 100644 analyzers/its/expected/Net5/Net5--net5.0-S2223.json diff --git a/analyzers/its/expected/Nancy/Nancy--net452-S2223.json b/analyzers/its/expected/Nancy/Nancy--net452-S2223.json index 8c705f0d29c..f33562d4425 100644 --- a/analyzers/its/expected/Nancy/Nancy--net452-S2223.json +++ b/analyzers/its/expected/Nancy/Nancy--net452-S2223.json @@ -15,71 +15,6 @@ }, { "id": "S2223", -"message": "Change the visibility of 'hexChars' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 47, -"startColumn": 23, -"endLine": 47, -"endColumn": 31 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'entitiesLock' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 48, -"startColumn": 23, -"endLine": 48, -"endColumn": 35 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'entities' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 49, -"startColumn": 47, -"endLine": 49, -"endColumn": 55 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'defaultEncoder' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 54, -"startColumn": 28, -"endLine": 54, -"endColumn": 42 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'currentEncoder' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 56, -"startColumn": 28, -"endLine": 56, -"endColumn": 42 -} -} -}, -{ -"id": "S2223", "message": "Change the visibility of 'DEFAULT_SWITCHOVER_THRESHOLD' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\IO\RequestStream.cs", @@ -93,19 +28,6 @@ }, { "id": "S2223", -"message": "Change the visibility of 'mimeTypes' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\MimeTypes.cs", -"region": { -"startLine": 41, -"startColumn": 44, -"endLine": 41, -"endColumn": 53 -} -} -}, -{ -"id": "S2223", "message": "Change the visibility of 'NoBody' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\Response.cs", diff --git a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json index 8c705f0d29c..f33562d4425 100644 --- a/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json +++ b/analyzers/its/expected/Nancy/Nancy--netstandard2.0-S2223.json @@ -15,71 +15,6 @@ }, { "id": "S2223", -"message": "Change the visibility of 'hexChars' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 47, -"startColumn": 23, -"endLine": 47, -"endColumn": 31 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'entitiesLock' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 48, -"startColumn": 23, -"endLine": 48, -"endColumn": 35 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'entities' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 49, -"startColumn": 47, -"endLine": 49, -"endColumn": 55 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'defaultEncoder' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 54, -"startColumn": 28, -"endLine": 54, -"endColumn": 42 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'currentEncoder' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\Helpers\HttpEncoder.cs", -"region": { -"startLine": 56, -"startColumn": 28, -"endLine": 56, -"endColumn": 42 -} -} -}, -{ -"id": "S2223", "message": "Change the visibility of 'DEFAULT_SWITCHOVER_THRESHOLD' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\IO\RequestStream.cs", @@ -93,19 +28,6 @@ }, { "id": "S2223", -"message": "Change the visibility of 'mimeTypes' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Nancy\src\Nancy\MimeTypes.cs", -"region": { -"startLine": 41, -"startColumn": 44, -"endLine": 41, -"endColumn": 53 -} -} -}, -{ -"id": "S2223", "message": "Change the visibility of 'NoBody' or make it 'const' or 'readonly'.", "location": { "uri": "sources\Nancy\src\Nancy\Response.cs", diff --git a/analyzers/its/expected/Net5/Net5--net5.0-S2223.json b/analyzers/its/expected/Net5/Net5--net5.0-S2223.json deleted file mode 100644 index 950afd59eb7..00000000000 --- a/analyzers/its/expected/Net5/Net5--net5.0-S2223.json +++ /dev/null @@ -1,30 +0,0 @@ -{ -"issues": [ -{ -"id": "S2223", -"message": "Change the visibility of 'foo' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Net5\Net5\S3604.cs", -"region": { -"startLine": 7, -"startColumn": 20, -"endLine": 7, -"endColumn": 23 -} -} -}, -{ -"id": "S2223", -"message": "Change the visibility of 'bar' or make it 'const' or 'readonly'.", -"location": { -"uri": "sources\Net5\Net5\S3604.cs", -"region": { -"startLine": 8, -"startColumn": 20, -"endLine": 8, -"endColumn": 23 -} -} -} -] -} From 1f6a2c1440dcb5832b1634db61eddcae8ab975ff Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Tue, 21 Feb 2023 14:16:50 +0100 Subject: [PATCH 10/14] cover also case of private protected modifier --- .../src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 6 +++--- .../TestCases/StaticFieldVisible.CSharp9.cs | 3 +++ 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 839ff9c5d94..ad4784d3db7 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -43,15 +43,15 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer private static IEnumerable GetDiagnostics(FieldDeclarationSyntax declaration, SemanticModel semanticModel) => FieldIsRelevant(declaration) - ? declaration.Declaration.Variables + ? declaration.Declaration.Variables .Where(x => !FieldIsThreadSafe(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol)) .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText)) - : Enumerable.Empty(); + : Enumerable.Empty(); private static bool FieldIsRelevant(FieldDeclarationSyntax node) => node.Modifiers.Any(SyntaxKind.StaticKeyword) && node.Modifiers.Count > 1 - && !node.Modifiers.Any(SyntaxKind.PrivateKeyword) + && (!node.Modifiers.Any(SyntaxKind.PrivateKeyword) || (node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.ProtectedKeyword))) && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword); private static bool FieldIsThreadSafe(IFieldSymbol fieldSymbol) => diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs index 52f4a2b135c..801c4867761 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.CSharp9.cs @@ -6,6 +6,9 @@ public record StaticFieldVisible // ^^ public const double Pi2 = 3.14; public double Pi3 = 3.14; + private protected static double Pi4 = 3.14; // Noncompliant + protected private static double Pi15 = 3.14; // Noncompliant + public static Shape Empty = Shape.Empty; // Noncompliant {{Change the visibility of 'Empty' or make it 'const' or 'readonly'.}} [ThreadStatic] From 0b61972dcb1dcfa151426eff360904b9e14116d8 Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Tue, 21 Feb 2023 14:34:54 +0100 Subject: [PATCH 11/14] add more UTs --- .../SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 4 ++-- .../TestCases/StaticFieldVisible.cs | 10 +++++++--- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index ad4784d3db7..4c98e58cbca 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -49,8 +49,8 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer : Enumerable.Empty(); private static bool FieldIsRelevant(FieldDeclarationSyntax node) => - node.Modifiers.Any(SyntaxKind.StaticKeyword) - && node.Modifiers.Count > 1 + node.Modifiers.Count > 1 + && node.Modifiers.Any(SyntaxKind.StaticKeyword) && (!node.Modifiers.Any(SyntaxKind.PrivateKeyword) || (node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.ProtectedKeyword))) && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index eb67997293f..0f1e41dafbf 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -20,11 +20,15 @@ public class StaticFieldVisible private static double Pi8 = 3.14; private double Pi9 = 3.14; - static double Pi10 = 3.14; // Compliant - if not access modifier exist the field is private - public readonly double Pi11 = 3.14; + static double Pi10 = 3.14; // Compliant - if no access modifier exist, the field is private + double Pi11 = 3.14; + public readonly double Pi12 = 3.14; [ThreadStatic] - public static int value; // Compliant, thread static field values are not shared between threads + public static int Value; // Compliant, thread static field values are not shared between threads + + [NonSerialized] + public static int Value2; // Noncompliant } public class Shape From 1ad2631a5b362e60034e008102748dc066dfb5dd Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Wed, 22 Feb 2023 11:59:06 +0100 Subject: [PATCH 12/14] exclude volatile fields from the raising issues --- .../src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs | 1 + .../SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs index 4c98e58cbca..c10d19f189e 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs @@ -51,6 +51,7 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer private static bool FieldIsRelevant(FieldDeclarationSyntax node) => node.Modifiers.Count > 1 && node.Modifiers.Any(SyntaxKind.StaticKeyword) + && !node.Modifiers.Any(SyntaxKind.VolatileKeyword) && (!node.Modifiers.Any(SyntaxKind.PrivateKeyword) || (node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.ProtectedKeyword))) && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs index 0f1e41dafbf..981176f2d7c 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs @@ -24,11 +24,15 @@ public class StaticFieldVisible double Pi11 = 3.14; public readonly double Pi12 = 3.14; + public static volatile int VolatileValue = 3; // Compliant - volatile keyword indicates that a field might be modified by multiple threads that are executing at the same time. + // see: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile + [ThreadStatic] public static int Value; // Compliant, thread static field values are not shared between threads [NonSerialized] public static int Value2; // Noncompliant + } public class Shape From 53bab059bd0708aa2ce3183cd2a3a7842e1547cf Mon Sep 17 00:00:00 2001 From: mary-georgiou-sonarsource Date: Wed, 22 Feb 2023 13:35:49 +0100 Subject: [PATCH 13/14] update rsepc for rule S2223 --- analyzers/rspec/cs/S2223_c#.html | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/analyzers/rspec/cs/S2223_c#.html b/analyzers/rspec/cs/S2223_c#.html index 7757a715351..a255243326e 100644 --- a/analyzers/rspec/cs/S2223_c#.html +++ b/analyzers/rspec/cs/S2223_c#.html @@ -1,7 +1,10 @@ -

A static field that is neither constant nor read-only is not thread-safe. Correctly accessing these fields from different threads -needs synchronization with locks. Improper synchronization may lead to unexpected results, thus publicly visible static fields are best -suited for storing non-changing data shared by many consumers. To enforce this intent, these fields should be marked readonly or -converted to constants.

+

Non-private static fields that are neither const nor readonly can lead to errors and unpredictable +behavior.

+

This can happen because: * Any object can modify these fields and alter the global state. This makes the code more difficult to read, debug and +test. * Correctly accessing these fields from different threads needs synchronization with lock. Improper synchronization may lead to +unexpected results.

+

Publicly visible static fields should only be used to store shared data that does not change. To enforce this intent, these fields should be marked +readonly or converted to const.

Noncompliant Code Example

 public class Math

From 1947abaa01a5505c457be11f32dbcfc34e76c3f0 Mon Sep 17 00:00:00 2001
From: mary-georgiou-sonarsource 
Date: Wed, 22 Feb 2023 15:53:49 +0100
Subject: [PATCH 14/14] apply comments

---
 .../Rules/StaticFieldVisible.cs                     |  8 ++++----
 .../TestCases/StaticFieldVisible.cs                 | 13 ++++++-------
 2 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs
index c10d19f189e..8d9c852d685 100644
--- a/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs
+++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/StaticFieldVisible.cs
@@ -34,17 +34,17 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer
             context.RegisterNodeAction(
                 c =>
                 {
-                    foreach (var diagnostic in GetDiagnostics((FieldDeclarationSyntax)c.Node, c.SemanticModel))
+                    foreach (var diagnostic in GetDiagnostics(c.SemanticModel, (FieldDeclarationSyntax)c.Node))
                     {
                         c.ReportIssue(diagnostic);
                     }
                 },
                 SyntaxKind.FieldDeclaration);
 
-        private static IEnumerable GetDiagnostics(FieldDeclarationSyntax declaration, SemanticModel semanticModel) =>
+        private static IEnumerable GetDiagnostics(SemanticModel model, FieldDeclarationSyntax declaration) =>
             FieldIsRelevant(declaration)
                 ? declaration.Declaration.Variables
-                    .Where(x => !FieldIsThreadSafe(semanticModel.GetDeclaredSymbol(x) as IFieldSymbol))
+                    .Where(x => !FieldIsThreadSafe(model.GetDeclaredSymbol(x) as IFieldSymbol))
                     .Select(x => Diagnostic.Create(Rule, x.Identifier.GetLocation(), x.Identifier.ValueText))
                 : Enumerable.Empty();
 
@@ -52,7 +52,7 @@ public sealed class StaticFieldVisible : SonarDiagnosticAnalyzer
             node.Modifiers.Count > 1
             && node.Modifiers.Any(SyntaxKind.StaticKeyword)
             && !node.Modifiers.Any(SyntaxKind.VolatileKeyword)
-            && (!node.Modifiers.Any(SyntaxKind.PrivateKeyword) || (node.Modifiers.Any(SyntaxKind.PrivateKeyword) && node.Modifiers.Any(SyntaxKind.ProtectedKeyword)))
+            && (!node.Modifiers.Any(SyntaxKind.PrivateKeyword) || node.Modifiers.Any(SyntaxKind.ProtectedKeyword))
             && !node.Modifiers.Any(SyntaxKind.ReadOnlyKeyword);
 
         private static bool FieldIsThreadSafe(IFieldSymbol fieldSymbol) =>
diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs
index 981176f2d7c..5df9b1628be 100644
--- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs
+++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/StaticFieldVisible.cs
@@ -13,19 +13,18 @@ public class StaticFieldVisible
         public const double Pi2 = 3.14;
         public double Pi3 = 3.14;
 
-        protected static double Pi4 = 3.14; // Noncompliant
-        internal static double Pi5 = 3.14; // Noncompliant
-        internal static double Pi6 = 3.14; // Noncompliant
-        protected internal static double Pi7 = 3.14; // Noncompliant
+        protected static double Pi4 = 3.14;             // Noncompliant
+        internal static double Pi5 = 3.14;              // Noncompliant
+        internal static double Pi6 = 3.14;              // Noncompliant
+        protected internal static double Pi7 = 3.14;    // Noncompliant
 
         private static double Pi8 = 3.14;
         private double Pi9 = 3.14;
-        static double Pi10 = 3.14; // Compliant - if no access modifier exist, the field is private
+        static double Pi10 = 3.14; // Compliant - if no access modifier exists, the field is private
         double Pi11 = 3.14;
         public readonly double Pi12 = 3.14;
 
-        public static volatile int VolatileValue = 3; // Compliant - volatile keyword indicates that a field might be modified by multiple threads that are executing at the same time.
-                                                      // see: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile
+        public static volatile int VolatileValue = 3; // Compliant - see: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/volatile
 
         [ThreadStatic]
         public static int Value; // Compliant, thread static field values are not shared between threads