From a94a3ae6151cf2a47e6b4fe3a8e5f4d352cbeb33 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Tue, 9 Jul 2024 15:41:04 +0200 Subject: [PATCH 1/3] CaYC commit --- .../FieldsShouldBeEncapsulatedInProperties.cs | 81 +++++++++---------- 1 file changed, 40 insertions(+), 41 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs index 0e4e349ab77..8877da14fe9 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs @@ -18,54 +18,53 @@ * 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 FieldsShouldBeEncapsulatedInProperties : SonarDiagnosticAnalyzer { - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class FieldsShouldBeEncapsulatedInProperties : SonarDiagnosticAnalyzer - { - private const string DiagnosticId = "S1104"; - private const string MessageFormat = "Make this field 'private' and encapsulate it in a 'public' property."; + private const string DiagnosticId = "S1104"; + private const string MessageFormat = "Make this field 'private' and encapsulate it in a 'public' property."; - private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); + private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat); - public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); + private static readonly ISet ValidModifiers = new HashSet + { + SyntaxKind.PrivateKeyword, + SyntaxKind.ProtectedKeyword, + SyntaxKind.InternalKeyword, + SyntaxKind.ReadOnlyKeyword, + SyntaxKind.ConstKeyword + }; - private static readonly ISet ValidModifiers = new HashSet - { - SyntaxKind.PrivateKeyword, - SyntaxKind.ProtectedKeyword, - SyntaxKind.InternalKeyword, - SyntaxKind.ReadOnlyKeyword, - SyntaxKind.ConstKeyword - }; + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction( - c => + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction( + c => + { + var fieldDeclaration = (FieldDeclarationSyntax)c.Node; + if (fieldDeclaration.Modifiers.Any(m => ValidModifiers.Contains(m.Kind()))) { - var fieldDeclaration = (FieldDeclarationSyntax)c.Node; - if (fieldDeclaration.Modifiers.Any(m => ValidModifiers.Contains(m.Kind()))) - { - return; - } + return; + } - var firstVariable = fieldDeclaration.Declaration.Variables[0]; - var symbol = c.SemanticModel.GetDeclaredSymbol(firstVariable); - var parentSymbol = c.SemanticModel.GetDeclaredSymbol(fieldDeclaration.Parent); - if (parentSymbol.HasAttribute(KnownType.System_Runtime_InteropServices_StructLayoutAttribute) || Serializable(symbol, parentSymbol)) - { - return; - } + var firstVariable = fieldDeclaration.Declaration.Variables[0]; + var symbol = c.SemanticModel.GetDeclaredSymbol(firstVariable); + var parentSymbol = c.SemanticModel.GetDeclaredSymbol(fieldDeclaration.Parent); + if (parentSymbol.HasAttribute(KnownType.System_Runtime_InteropServices_StructLayoutAttribute) || Serializable(symbol, parentSymbol)) + { + return; + } - if (symbol.GetEffectiveAccessibility() == Accessibility.Public) - { - c.ReportIssue(Rule, firstVariable); - } - }, - SyntaxKind.FieldDeclaration); + if (symbol.GetEffectiveAccessibility() == Accessibility.Public) + { + c.ReportIssue(Rule, firstVariable); + } + }, + SyntaxKind.FieldDeclaration); - private static bool Serializable(ISymbol symbol, ISymbol parentSymbol) => - parentSymbol.HasAttribute(KnownType.System_SerializableAttribute) - && !symbol.HasAttribute(KnownType.System_NonSerializedAttribute); - } + private static bool Serializable(ISymbol symbol, ISymbol parentSymbol) => + parentSymbol.HasAttribute(KnownType.System_SerializableAttribute) + && !symbol.HasAttribute(KnownType.System_NonSerializedAttribute); } From 0825155068829a3ac217ab5f16a32cd23b1dabeb Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Tue, 9 Jul 2024 16:02:20 +0200 Subject: [PATCH 2/3] Fix S1104 FP: Do not report in Unity serializable classes --- .../FieldsShouldBeEncapsulatedInProperties.cs | 8 +++++++- ...eldsShouldBeEncapsulatedInPropertiesTest.cs | 7 +++++++ ...ShouldBeEncapsulatedInProperties.Unity3D.cs | 18 ++++++++++++++++++ 3 files changed, 32 insertions(+), 1 deletion(-) create mode 100644 analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs index 8877da14fe9..37359d5dd4e 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs @@ -37,6 +37,10 @@ public sealed class FieldsShouldBeEncapsulatedInProperties : SonarDiagnosticAnal SyntaxKind.ConstKeyword }; + private static readonly ImmutableArray IgnoredTypes = ImmutableArray.Create( + KnownType.UnityEngine_MonoBehaviour, + KnownType.UnityEngine_ScriptableObject); + public override ImmutableArray SupportedDiagnostics { get; } = ImmutableArray.Create(Rule); protected override void Initialize(SonarAnalysisContext context) => @@ -52,7 +56,9 @@ protected override void Initialize(SonarAnalysisContext context) => var firstVariable = fieldDeclaration.Declaration.Variables[0]; var symbol = c.SemanticModel.GetDeclaredSymbol(firstVariable); var parentSymbol = c.SemanticModel.GetDeclaredSymbol(fieldDeclaration.Parent); - if (parentSymbol.HasAttribute(KnownType.System_Runtime_InteropServices_StructLayoutAttribute) || Serializable(symbol, parentSymbol)) + if (symbol.ContainingType.DerivesFromAny(IgnoredTypes) + || parentSymbol.HasAttribute(KnownType.System_Runtime_InteropServices_StructLayoutAttribute) + || Serializable(symbol, parentSymbol)) { return; } diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs index 9144f5cf603..2ea5343b4e6 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs @@ -31,6 +31,13 @@ public class FieldsShouldBeEncapsulatedInPropertiesTest public void FieldsShouldBeEncapsulatedInProperties() => builder.AddPaths("FieldsShouldBeEncapsulatedInProperties.cs").Verify(); + [TestMethod] + public void FieldsShouldBeEncapsulatedInProperties_Unity3D_Ignored() => + builder.AddPaths("FieldsShouldBeEncapsulatedInProperties.Unity3D.cs") + // Concurrent analysis puts fake Unity3D class into the Concurrent namespace + .WithConcurrentAnalysis(false) + .VerifyNoIssues(); + #if NET [TestMethod] diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs new file mode 100644 index 00000000000..16643e10c11 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs @@ -0,0 +1,18 @@ +// https://github.com/SonarSource/sonar-dotnet/issues/7522 +public class UnityMonoBehaviour : UnityEngine.MonoBehaviour +{ + public int Field1; // Compliant +} + +public class UnityScriptableObject : UnityEngine.ScriptableObject +{ + public int Field1; // Compliant +} + +// Unity3D does not seem to be available as a nuget package and we cannot use the original classes +// Cannot run this test case in Concurrent mode because of the Concurrent namespace +namespace UnityEngine +{ + public class MonoBehaviour { } + public class ScriptableObject { } +} From 350fd8975b9181d6c8094d0a69d396407fc7fcb7 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Thu, 11 Jul 2024 14:38:56 +0200 Subject: [PATCH 3/3] Add tests for Unity custom serializable class --- ...FieldsShouldBeEncapsulatedInPropertiesTest.cs | 2 +- ...dsShouldBeEncapsulatedInProperties.Unity3D.cs | 16 +++++++++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs index 2ea5343b4e6..cd3d09c5130 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs @@ -36,7 +36,7 @@ public void FieldsShouldBeEncapsulatedInProperties_Unity3D_Ignored() => builder.AddPaths("FieldsShouldBeEncapsulatedInProperties.Unity3D.cs") // Concurrent analysis puts fake Unity3D class into the Concurrent namespace .WithConcurrentAnalysis(false) - .VerifyNoIssues(); + .Verify(); #if NET diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs index 16643e10c11..4d1d0544c48 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs @@ -1,4 +1,6 @@ -// https://github.com/SonarSource/sonar-dotnet/issues/7522 +using System; + +// https://github.com/SonarSource/sonar-dotnet/issues/7522 public class UnityMonoBehaviour : UnityEngine.MonoBehaviour { public int Field1; // Compliant @@ -9,10 +11,22 @@ public class UnityScriptableObject : UnityEngine.ScriptableObject public int Field1; // Compliant } +public class InvalidCustomSerializableClass1 : UnityEngine.Object +{ + public int Field1; // Noncompliant +} + +[Serializable] +public class ValidCustomSerializableClass : UnityEngine.Object +{ + public int Field1; // Compliant +} + // Unity3D does not seem to be available as a nuget package and we cannot use the original classes // Cannot run this test case in Concurrent mode because of the Concurrent namespace namespace UnityEngine { public class MonoBehaviour { } public class ScriptableObject { } + public class Object { } }