Duplicate dependency field analyzer

Robust.Analyzers.Tests/DuplicateDependencyAnalyzerTest.cs

@@ -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"));
+    }
+}

Robust.Analyzers/DuplicateDependencyAnalyzer.cs

@@ -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));
+                    }
+                }
+            }
+        }
+    }
+}

Robust.Roslyn.Shared/Diagnostics.cs

@@ -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.");

Robust.Shared/GameObjects/Systems/SharedUserInterfaceSystem.cs

@@ -23,7 +23,6 @@
     [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!;
@@ -223,7 +222,7 @@
 
         if (ent.Comp.ClientOpenInterfaces.TryGetValue(key, out var cBui))
         {
             if (cBui.DeferredClose)
                 _queuedCloses.Add(cBui);
             else
             {
@@ -390,7 +389,7 @@
                 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.
@@ -404,7 +403,7 @@
 
             var bui = ent.Comp.ClientOpenInterfaces[key];
 
             if (bui.DeferredClose)
                 _queuedCloses.Add(bui);
             else
             {
@@ -455,7 +454,7 @@
         // Existing BUI just keep it.
         if (entity.Comp.ClientOpenInterfaces.TryGetValue(key, out var existing))
         {
             if (existing.DeferredClose)
                 _queuedCloses.Remove(existing);
 
             return;

Robust.Shared/Physics/Systems/SharedPhysicsSystem.Island.cs

@@ -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;

Robust.Shared/Physics/Systems/SharedPhysicsSystem.cs

@@ -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!;
@@ -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)
@@ -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);
         }
 

Robust.UnitTesting/Shared/IoC/IoCManager_Test.cs

@@ -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()
         {