From f0ed3537ee0c9302fae76ac31da7286a5eea2da4 Mon Sep 17 00:00:00 2001 From: Pieter-Jan Briers Date: Sat, 28 Sep 2024 07:35:18 +0200 Subject: [PATCH] Duplicate dependency field analyzer (#5463) * 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 --- .../DuplicateDependencyAnalyzerTest.cs | 63 +++++++++ .../DuplicateDependencyAnalyzer.cs | 126 ++++++++++++++++++ Robust.Roslyn.Shared/Diagnostics.cs | 1 + .../Systems/SharedUserInterfaceSystem.cs | 3 +- .../Systems/SharedPhysicsSystem.Island.cs | 28 ++-- .../Physics/Systems/SharedPhysicsSystem.cs | 11 +- .../Shared/IoC/IoCManager_Test.cs | 2 + 7 files changed, 212 insertions(+), 22 deletions(-) create mode 100644 Robust.Analyzers.Tests/DuplicateDependencyAnalyzerTest.cs create mode 100644 Robust.Analyzers/DuplicateDependencyAnalyzer.cs diff --git a/Robust.Analyzers.Tests/DuplicateDependencyAnalyzerTest.cs b/Robust.Analyzers.Tests/DuplicateDependencyAnalyzerTest.cs new file mode 100644 index 000000000..94d801cd4 --- /dev/null +++ b/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; + +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() + { + 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")); + } +} diff --git a/Robust.Analyzers/DuplicateDependencyAnalyzer.cs b/Robust.Analyzers/DuplicateDependencyAnalyzer.cs new file mode 100644 index 000000000..5fdfbe3e9 --- /dev/null +++ b/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 + +/// +/// Analyzer that detects duplicate [Dependency] fields inside a single type. +/// +[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 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> _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)); + } + } + } + } + } +} diff --git a/Robust.Roslyn.Shared/Diagnostics.cs b/Robust.Roslyn.Shared/Diagnostics.cs index ec0664fbb..4b35d44d7 100644 --- a/Robust.Roslyn.Shared/Diagnostics.cs +++ b/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."); diff --git a/Robust.Shared/GameObjects/Systems/SharedUserInterfaceSystem.cs b/Robust.Shared/GameObjects/Systems/SharedUserInterfaceSystem.cs index 0305cbb37..f08e55200 100644 --- a/Robust.Shared/GameObjects/Systems/SharedUserInterfaceSystem.cs +++ b/Robust.Shared/GameObjects/Systems/SharedUserInterfaceSystem.cs @@ -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!; @@ -390,7 +389,7 @@ public abstract class SharedUserInterfaceSystem : EntitySystem ent.Comp.States.Remove(key); } - var attachedEnt = _player.LocalEntity; + var attachedEnt = Player.LocalEntity; var clientBuis = new ValueList(ent.Comp.ClientOpenInterfaces.Keys); // Check if the UI is open by us, otherwise dispose of it. diff --git a/Robust.Shared/Physics/Systems/SharedPhysicsSystem.Island.cs b/Robust.Shared/Physics/Systems/SharedPhysicsSystem.Island.cs index b084b6945..b3fbc3b34 100644 --- a/Robust.Shared/Physics/Systems/SharedPhysicsSystem.Island.cs +++ b/Robust.Shared/Physics/Systems/SharedPhysicsSystem.Island.cs @@ -198,20 +198,20 @@ public abstract partial class SharedPhysicsSystem 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; diff --git a/Robust.Shared/Physics/Systems/SharedPhysicsSystem.cs b/Robust.Shared/Physics/Systems/SharedPhysicsSystem.cs index 7ee351b79..721af0a36 100644 --- a/Robust.Shared/Physics/Systems/SharedPhysicsSystem.cs +++ b/Robust.Shared/Physics/Systems/SharedPhysicsSystem.cs @@ -43,7 +43,6 @@ namespace Robust.Shared.Physics.Systems 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 @@ namespace Robust.Shared.Physics.Systems 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 @@ namespace Robust.Shared.Physics.Systems 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); } diff --git a/Robust.UnitTesting/Shared/IoC/IoCManager_Test.cs b/Robust.UnitTesting/Shared/IoC/IoCManager_Test.cs index 3450b1572..1811962f9 100644 --- a/Robust.UnitTesting/Shared/IoC/IoCManager_Test.cs +++ b/Robust.UnitTesting/Shared/IoC/IoCManager_Test.cs @@ -258,6 +258,7 @@ namespace Robust.UnitTesting.Shared.IoC 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!; @@ -265,6 +266,7 @@ namespace Robust.UnitTesting.Shared.IoC [Dependency] public TestFieldInjection mydifferentself = default!; #pragma warning restore 649 +#pragma warning restore RA0032 public override void Test() {