Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix S1104 FP: Do not report in Unity serializable classes #9514

Merged
merged 3 commits into from
Jul 11, 2024
Merged

Fix S1104 FP: Do not report in Unity serializable classes #9514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a94a3ae
CaYC commit
sebastien-marichal Jul 9, 2024
0825155
Fix S1104 FP: Do not report in Unity serializable classes
sebastien-marichal Jul 9, 2024
350fd89
Add tests for Unity custom serializable class
sebastien-marichal Jul 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 46 additions & 41 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/FieldsShouldBeEncapsulatedInProperties.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,54 +18,59 @@
* 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);
private static readonly ISet<SyntaxKind> ValidModifiers = new HashSet<SyntaxKind>
{
SyntaxKind.PrivateKeyword,
SyntaxKind.ProtectedKeyword,
SyntaxKind.InternalKeyword,
SyntaxKind.ReadOnlyKeyword,
SyntaxKind.ConstKeyword
};

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);
private static readonly ImmutableArray<KnownType> IgnoredTypes = ImmutableArray.Create(
KnownType.UnityEngine_MonoBehaviour,
KnownType.UnityEngine_ScriptableObject);
Comment on lines +41 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding from the docs is that you should include UnityEngine.Object.
Which, if I'm not wrong, is parent to UnityEngine_ScriptableObject so you can replace one with the other.

But for custom classes which don’t derive from UnityEngine.Object, Unity includes the state of the instance directly in the serialized data of the MonoBehaviour or ScriptableObject that references them. There are two ways that this can happen: inline and by [SerializeReference]
.```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding UnityEngine.Object is not enough, the class should also have the Serializable attribute.
I can add it so it also supports custom serializable unity class/object.
I will rework a bit the logic then.

Copy link
Contributor Author

@sebastien-marichal sebastien-marichal Jul 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a closer look, the required attribute for custom objects is System.Serializable, and this is already an exception to the rule.

Edit 1: Never mind, I didn't realize we were only checking if the field had the attribute. I have pushed my updates.

Edit 2: I am having strokes, we do check if the parent class has the Serializable field. In any case, I have added more UTs to check for custom Unity serializable classes.


private static readonly ISet<SyntaxKind> ValidModifiers = new HashSet<SyntaxKind>
{
SyntaxKind.PrivateKeyword,
SyntaxKind.ProtectedKeyword,
SyntaxKind.InternalKeyword,
SyntaxKind.ReadOnlyKeyword,
SyntaxKind.ConstKeyword
};
public override ImmutableArray<DiagnosticDescriptor> 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 (symbol.ContainingType.DerivesFromAny(IgnoredTypes)
|| 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);
}
7 changes: 7 additions & 0 deletions analyzers/tests/SonarAnalyzer.Test/Rules/FieldsShouldBeEncapsulatedInPropertiesTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
.Verify();

#if NET

[TestMethod]
Expand Down
32 changes: 32 additions & 0 deletions ...zers/tests/SonarAnalyzer.Test/TestCases/FieldsShouldBeEncapsulatedInProperties.Unity3D.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
using System;

// 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
}

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 { }
}
Loading