Skip to content

Commit

Permalink
Duplicate dependency field analyzer (#5463)
Browse files Browse the repository at this point in the history
* Duplicate dependency field analyzer

Detects cases of duplicate [Dependency] fields in a type. We apparently have 27 of these across RT + SS14.

* Fix duplicate dependencies in Robust
  • Loading branch information
PJB3005 authored Sep 28, 2024
1 parent 74e7e61 commit f0ed353
Show file tree
Hide file tree
Showing 7 changed files with 212 additions and 22 deletions.
63 changes: 63 additions & 0 deletions Robust.Analyzers.Tests/DuplicateDependencyAnalyzerTest.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CSharp.Testing;
using Microsoft.CodeAnalysis.Testing;
using Microsoft.CodeAnalysis.Testing.Verifiers;
using NUnit.Framework;
using VerifyCS =
Microsoft.CodeAnalysis.CSharp.Testing.NUnit.AnalyzerVerifier<Robust.Analyzers.DuplicateDependencyAnalyzer>;

namespace Robust.Analyzers.Tests;

[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)]
[TestFixture]
[TestOf(typeof(DuplicateDependencyAnalyzer))]
public sealed class DuplicateDependencyAnalyzerTest
{
private static Task Verifier(string code, params DiagnosticResult[] expected)
{
var test = new CSharpAnalyzerTest<DuplicateDependencyAnalyzer, NUnitVerifier>()
{
TestState =
{
Sources = { code }
},
};

TestHelper.AddEmbeddedSources(
test.TestState,
"Robust.Shared.IoC.DependencyAttribute.cs"
);

// ExpectedDiagnostics cannot be set, so we need to AddRange here...
test.TestState.ExpectedDiagnostics.AddRange(expected);

return test.RunAsync();
}

[Test]
public async Task Test()
{
const string code = """
using Robust.Shared.IoC;

public sealed class Foo
{
[Dependency]
private object? Field;

[Dependency]
private object? Field2;

[Dependency]
private string? DifferentField;

private string? NonDependency1;
private string? NonDependency2;
}
""";

await Verifier(code,
// /0/Test0.cs(9,21): warning RA0032: Another [Dependency] field of type 'object?' already exists in this type as field 'Field'
VerifyCS.Diagnostic().WithSpan(9, 21, 9, 27).WithArguments("object?", "Field"));
}
}
126 changes: 126 additions & 0 deletions Robust.Analyzers/DuplicateDependencyAnalyzer.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
using System.Collections.Immutable;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using Robust.Roslyn.Shared;

namespace Robust.Analyzers;

#nullable enable

/// <summary>
/// Analyzer that detects duplicate <c>[Dependency]</c> fields inside a single type.
/// </summary>
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class DuplicateDependencyAnalyzer : DiagnosticAnalyzer
{
private const string DependencyAttributeType = "Robust.Shared.IoC.DependencyAttribute";

private static readonly DiagnosticDescriptor Rule = new(
Diagnostics.IdDuplicateDependency,
"Duplicate dependency field",
"Another [Dependency] field of type '{0}' already exists in this type with field '{1}'",
"Usage",
DiagnosticSeverity.Warning,
true);

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

public override void Initialize(AnalysisContext context)
{
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
context.EnableConcurrentExecution();
context.RegisterCompilationStartAction(compilationContext =>
{
var dependencyAttributeType = compilationContext.Compilation.GetTypeByMetadataName(DependencyAttributeType);
if (dependencyAttributeType == null)
return;
compilationContext.RegisterSymbolStartAction(symbolContext =>
{
var typeSymbol = (INamedTypeSymbol)symbolContext.Symbol;
// Only deal with non-static classes, doesn't make sense to have dependencies in anything else.
if (typeSymbol.TypeKind != TypeKind.Class || typeSymbol.IsStatic)
return;
var state = new AnalyzerState(dependencyAttributeType);
symbolContext.RegisterSyntaxNodeAction(state.AnalyzeField, SyntaxKind.FieldDeclaration);
symbolContext.RegisterSymbolEndAction(state.End);
},
SymbolKind.NamedType);
});
}

private sealed class AnalyzerState(INamedTypeSymbol dependencyAttributeType)
{
private readonly Dictionary<ITypeSymbol, List<IFieldSymbol>> _dependencyFields = new(SymbolEqualityComparer.Default);

public void AnalyzeField(SyntaxNodeAnalysisContext context)
{
var field = (FieldDeclarationSyntax)context.Node;
if (field.AttributeLists.Count == 0)
return;

if (context.ContainingSymbol is not IFieldSymbol fieldSymbol)
return;

// Can't have [Dependency]s for non-reference types.
if (!fieldSymbol.Type.IsReferenceType)
return;

if (!IsDependency(context.ContainingSymbol))
return;

lock (_dependencyFields)
{
if (!_dependencyFields.TryGetValue(fieldSymbol.Type, out var dependencyFields))
{
dependencyFields = [];
_dependencyFields.Add(fieldSymbol.Type, dependencyFields);
}

dependencyFields.Add(fieldSymbol);
}
}

private bool IsDependency(ISymbol symbol)
{
foreach (var attributeData in symbol.GetAttributes())
{
if (SymbolEqualityComparer.Default.Equals(attributeData.AttributeClass, dependencyAttributeType))
return true;
}

return false;
}

public void End(SymbolAnalysisContext context)
{
lock (_dependencyFields)
{
foreach (var pair in _dependencyFields)
{
var fieldType = pair.Key;
var fields = pair.Value;
if (fields.Count <= 1)
continue;

// Sort so we can have deterministic order to skip reporting for a single field.
// Whichever sorts first doesn't get reported.
fields.Sort(static (a, b) => string.Compare(a.Name, b.Name, StringComparison.Ordinal));

// Start at index 1 to skip first field.
var firstField = fields[0];
for (var i = 1; i < fields.Count; i++)
{
var field = fields[i];

context.ReportDiagnostic(
Diagnostic.Create(Rule, field.Locations[0], fieldType.ToDisplayString(), firstField.Name));
}
}
}
}
}
}
1 change: 1 addition & 0 deletions Robust.Roslyn.Shared/Diagnostics.cs
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ public static class Diagnostics
public const string IdDataFieldNoVVReadWrite = "RA0029";
public const string IdUseNonGenericVariant = "RA0030";
public const string IdPreferOtherType = "RA0031";
public const string IdDuplicateDependency = "RA0032";

public static SuppressionDescriptor MeansImplicitAssignment =>
new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ public abstract class SharedUserInterfaceSystem : EntitySystem
[Dependency] private readonly IGameTiming _timing = default!;
[Dependency] private readonly INetManager _netManager = default!;
[Dependency] private readonly IParallelManager _parallel = default!;
[Dependency] private readonly ISharedPlayerManager _player = default!;
[Dependency] protected readonly IPrototypeManager ProtoManager = default!;
[Dependency] private readonly IReflectionManager _reflection = default!;
[Dependency] protected readonly ISharedPlayerManager Player = default!;
Expand Down Expand Up @@ -390,7 +389,7 @@ private void OnUserInterfaceHandleState(Entity<UserInterfaceComponent> ent, ref
ent.Comp.States.Remove(key);
}

var attachedEnt = _player.LocalEntity;
var attachedEnt = Player.LocalEntity;
var clientBuis = new ValueList<Enum>(ent.Comp.ClientOpenInterfaces.Keys);

// Check if the UI is open by us, otherwise dispose of it.
Expand Down
28 changes: 14 additions & 14 deletions Robust.Shared/Physics/Systems/SharedPhysicsSystem.Island.cs
Original file line number Diff line number Diff line change
Expand Up @@ -198,20 +198,20 @@ internal record struct IslandData(

private void InitializeIsland()
{
Subs.CVar(_configManager, CVars.NetTickrate, SetTickRate, true);
Subs.CVar(_configManager, CVars.WarmStarting, SetWarmStarting, true);
Subs.CVar(_configManager, CVars.MaxLinearCorrection, SetMaxLinearCorrection, true);
Subs.CVar(_configManager, CVars.MaxAngularCorrection, SetMaxAngularCorrection, true);
Subs.CVar(_configManager, CVars.VelocityIterations, SetVelocityIterations, true);
Subs.CVar(_configManager, CVars.PositionIterations, SetPositionIterations, true);
Subs.CVar(_configManager, CVars.MaxLinVelocity, SetMaxLinearVelocity, true);
Subs.CVar(_configManager, CVars.MaxAngVelocity, SetMaxAngularVelocity, true);
Subs.CVar(_configManager, CVars.SleepAllowed, SetSleepAllowed, true);
Subs.CVar(_configManager, CVars.AngularSleepTolerance, SetAngularToleranceSqr, true);
Subs.CVar(_configManager, CVars.LinearSleepTolerance, SetLinearToleranceSqr, true);
Subs.CVar(_configManager, CVars.TimeToSleep, SetTimeToSleep, true);
Subs.CVar(_configManager, CVars.VelocityThreshold, SetVelocityThreshold, true);
Subs.CVar(_configManager, CVars.Baumgarte, SetBaumgarte, true);
Subs.CVar(_cfg, CVars.NetTickrate, SetTickRate, true);
Subs.CVar(_cfg, CVars.WarmStarting, SetWarmStarting, true);
Subs.CVar(_cfg, CVars.MaxLinearCorrection, SetMaxLinearCorrection, true);
Subs.CVar(_cfg, CVars.MaxAngularCorrection, SetMaxAngularCorrection, true);
Subs.CVar(_cfg, CVars.VelocityIterations, SetVelocityIterations, true);
Subs.CVar(_cfg, CVars.PositionIterations, SetPositionIterations, true);
Subs.CVar(_cfg, CVars.MaxLinVelocity, SetMaxLinearVelocity, true);
Subs.CVar(_cfg, CVars.MaxAngVelocity, SetMaxAngularVelocity, true);
Subs.CVar(_cfg, CVars.SleepAllowed, SetSleepAllowed, true);
Subs.CVar(_cfg, CVars.AngularSleepTolerance, SetAngularToleranceSqr, true);
Subs.CVar(_cfg, CVars.LinearSleepTolerance, SetLinearToleranceSqr, true);
Subs.CVar(_cfg, CVars.TimeToSleep, SetTimeToSleep, true);
Subs.CVar(_cfg, CVars.VelocityThreshold, SetVelocityThreshold, true);
Subs.CVar(_cfg, CVars.Baumgarte, SetBaumgarte, true);
}

private void SetWarmStarting(bool value) => _warmStarting = value;
Expand Down
11 changes: 5 additions & 6 deletions Robust.Shared/Physics/Systems/SharedPhysicsSystem.cs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ public abstract partial class SharedPhysicsSystem : EntitySystem
Buckets = Histogram.ExponentialBuckets(0.000_001, 1.5, 25)
});

[Dependency] private readonly IConfigurationManager _configManager = default!;
[Dependency] private readonly IManifoldManager _manifoldManager = default!;
[Dependency] private readonly IMapManager _mapManager = default!;
[Dependency] private readonly IParallelManager _parallel = default!;
Expand Down Expand Up @@ -97,9 +96,9 @@ public override void Initialize()
InitializeIsland();
InitializeContacts();

Subs.CVar(_configManager, CVars.AutoClearForces, OnAutoClearChange);
Subs.CVar(_configManager, CVars.NetTickrate, UpdateSubsteps, true);
Subs.CVar(_configManager, CVars.TargetMinimumTickrate, UpdateSubsteps, true);
Subs.CVar(_cfg, CVars.AutoClearForces, OnAutoClearChange);
Subs.CVar(_cfg, CVars.NetTickrate, UpdateSubsteps, true);
Subs.CVar(_cfg, CVars.TargetMinimumTickrate, UpdateSubsteps, true);
}

private void OnPhysicsShutdown(EntityUid uid, PhysicsComponent component, ComponentShutdown args)
Expand Down Expand Up @@ -143,8 +142,8 @@ private void OnAutoClearChange(bool value)

private void UpdateSubsteps(int obj)
{
var targetMinTickrate = (float) _configManager.GetCVar(CVars.TargetMinimumTickrate);
var serverTickrate = (float) _configManager.GetCVar(CVars.NetTickrate);
var targetMinTickrate = (float) _cfg.GetCVar(CVars.TargetMinimumTickrate);
var serverTickrate = (float) _cfg.GetCVar(CVars.NetTickrate);
_substeps = (int)Math.Ceiling(targetMinTickrate / serverTickrate);
}

Expand Down
2 changes: 2 additions & 0 deletions Robust.UnitTesting/Shared/IoC/IoCManager_Test.cs
Original file line number Diff line number Diff line change
Expand Up @@ -258,13 +258,15 @@ public virtual void Test()

public sealed class TestFieldInjection : TestFieldInjectionParent
{
#pragma warning disable RA0032 // Duplicate [Dependency] field. I wrote this test 7 years idk if this makes sense.
[Dependency]
#pragma warning disable 649
private readonly TestFieldInjection myuniqueself = default!;

[Dependency]
public TestFieldInjection mydifferentself = default!;
#pragma warning restore 649
#pragma warning restore RA0032

public override void Test()
{
Expand Down

0 comments on commit f0ed353

Please sign in to comment.