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.

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

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

Merged
merged 3 commits into from
Jul 11, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ public sealed class FieldsShouldBeEncapsulatedInProperties : SonarDiagnosticAnal
SyntaxKind.ConstKeyword
};

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

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.


public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(Rule);

protected override void Initialize(SonarAnalysisContext context) =>
Expand All @@ -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;
}
Expand Down
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)
.VerifyNoIssues();

#if NET

[TestMethod]
Expand Down
Original file line number Diff line number Diff line change
@@ -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 { }
}
Loading