From 22b02cc8733d2d000ff7a46ef03f093d8f2e7ee8 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Fri, 24 Feb 2023 10:44:30 +0100 Subject: [PATCH] Code review 2 --- .../LockedFieldShouldBePrivateAndReadonly.cs | 44 +++++++++++-------- ...ckedFieldShouldBePrivateAndReadonlyTest.cs | 8 ++++ ...eAndReadonly.CSharp9.TopLevelStatements.cs | 40 +++++++++++++++++ .../LockedFieldShouldBePrivateAndReadonly.cs | 6 +-- 4 files changed, 76 insertions(+), 22 deletions(-) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs index 7137fc7773a..029ba72e8ed 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/LockedFieldShouldBePrivateAndReadonly.cs @@ -42,7 +42,7 @@ private static void CheckLockStatement(SonarSyntaxNodeReportingContext context) else { var symbol = context.SemanticModel.GetSymbolInfo(expression).Symbol; - if (IsOfTypeString(expression, symbol)) + if (IsOfTypeString(context.SemanticModel, expression)) { ReportIssue("Strings can be interned, and should not be used for locking."); } @@ -50,18 +50,11 @@ private static void CheckLockStatement(SonarSyntaxNodeReportingContext context) { ReportIssue($"'{lockedSymbol.Name}' is a local variable, and should not be used for locking."); } - else if (expression is (IdentifierNameSyntax or MemberAccessExpressionSyntax) - && symbol is IFieldSymbol lockedField - && (!lockedField.IsReadOnly || lockedField.GetEffectiveAccessibility() != Accessibility.Private)) + else if (FieldNotReadonlyOrNotPrivate(expression, symbol) is { } lockedField) { - if (lockedField.ContainingType is { } lockedFieldType && context.ContainingSymbol?.ContainingType is { } containingType && !lockedFieldType.Equals(containingType)) - { - ReportIssue($"Use members from '{containingType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart)}' for locking."); - } - else - { - ReportIssue($"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."); - } + ReportIssue(FieldInSameTypeAs(lockedField, context.ContainingSymbol?.ContainingType) is { } containingType + ? $"Use members from '{containingType.ToMinimalDisplayString(context.SemanticModel, expression.SpanStart)}' for locking." + : $"'{lockedField.Name}' is not 'private readonly', and should not be used for locking."); } } @@ -70,13 +63,26 @@ private static void CheckLockStatement(SonarSyntaxNodeReportingContext context) } private static bool IsCreation(ExpressionSyntax expression) => - expression is ObjectCreationExpressionSyntax - or AnonymousObjectCreationExpressionSyntax - or ArrayCreationExpressionSyntax - or ImplicitArrayCreationExpressionSyntax - or QueryExpressionSyntax; + expression.IsAnyKind( + SyntaxKind.ObjectCreationExpression, + SyntaxKind.AnonymousObjectCreationExpression, + SyntaxKind.ArrayCreationExpression, + SyntaxKind.ImplicitArrayCreationExpression, + SyntaxKind.QueryExpression); - private static bool IsOfTypeString(ExpressionSyntax expression, ISymbol symbol) => + private static bool IsOfTypeString(SemanticModel model, ExpressionSyntax expression) => expression.IsAnyKind(SyntaxKind.StringLiteralExpression, SyntaxKind.InterpolatedStringExpression) - || (symbol.GetSymbolType() is { } type && type.Is(KnownType.System_String)); + || expression.IsKnownType(KnownType.System_String, model); + + private static IFieldSymbol FieldNotReadonlyOrNotPrivate(ExpressionSyntax expression, ISymbol symbol) => + expression.IsAnyKind(SyntaxKind.IdentifierName, SyntaxKind.SimpleMemberAccessExpression) + && symbol is IFieldSymbol lockedField + && (!lockedField.IsReadOnly || lockedField.GetEffectiveAccessibility() != Accessibility.Private) + ? lockedField + : null; + + private static ITypeSymbol FieldInSameTypeAs(IFieldSymbol field, INamedTypeSymbol type) => + field.ContainingType is { } fieldType && type is { } && !fieldType.Equals(type) + ? type + : null; } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs index ed6a414f847..f59f72770b5 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/LockedFieldShouldBePrivateAndReadonlyTest.cs @@ -45,6 +45,14 @@ public class LockedFieldShouldBePrivateAndReadonlyTest .WithOptions(ParseOptionsHelper.FromCSharp9) .Verify(); + [TestMethod] + public void LockedFieldShouldBePrivateAndReadonly_CSharp9_TopLevelStatements() => + builder + .AddPaths("LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs") + .WithOptions(ParseOptionsHelper.FromCSharp9) + .WithTopLevelStatements() + .Verify(); + [TestMethod] public void LockedFieldShouldBePrivateAndReadonly_CSharp11() => builder diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs new file mode 100644 index 00000000000..3fcd5a91737 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.CSharp9.TopLevelStatements.cs @@ -0,0 +1,40 @@ +lock ("a string") { } // Noncompliant +// ^^^^^^^^^^ +lock (MethodReturningString()) { } // Noncompliant +lock (new object()) { } // Noncompliant + +string MethodReturningString() => "a string"; + +class ClassNestedAtTopLevel +{ + readonly object readonlyField = new(); + object readWriteField = new(); + + static readonly object staticReadonlyField = new(); + static object staticReadWriteField = new(); + + void Test() + { + lock (readonlyField) { } + lock (readWriteField) { } // Noncompliant + lock (staticReadonlyField) { } + lock (staticReadWriteField) { } // Noncompliant + } + + class SecondLevelNesting + { + readonly object readonlyField = new(); + object readWriteField = new(); + + static readonly object staticReadonlyField = new(); + static object staticReadWriteField = new(); + + void Test() + { + lock (readonlyField) { } + lock (readWriteField) { } // Noncompliant + lock (staticReadonlyField) { } + lock (staticReadWriteField) { } // Noncompliant + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs index f8d713321c5..06796fbbd96 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/LockedFieldShouldBePrivateAndReadonly.cs @@ -24,8 +24,8 @@ void OnAStaticField() lock (Test.staticReadonlyField) { } lock (Test.staticReadWriteField) { } // Noncompliant {{'staticReadWriteField' is not 'private readonly', and should not be used for locking.}} // ^^^^^^^^^^^^^^^^^^^^^^^^^ - lock (AnotherClass.staticReadonlyField) { } // Noncompliant {{Use fields from 'Test' for locking.}} - lock (AnotherClass.staticReadWriteField) { } // Noncompliant {{Use fields from 'Test' for locking.}} + lock (AnotherClass.staticReadonlyField) { } // Noncompliant {{Use members from 'Test' for locking.}} + lock (AnotherClass.staticReadWriteField) { } // Noncompliant {{Use members from 'Test' for locking.}} // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } @@ -149,7 +149,7 @@ class TestExplicitCast void Test() { - lock ((AnotherClass)readonlyField) { } // Compliant, the cast operator can build + lock ((AnotherClass)readonlyField) { } // Compliant, the cast operator can run arbitrary code } }