From 72d893dec53d550bf7e881ab3945b381e4729729 Mon Sep 17 00:00:00 2001 From: Pieter-Jan Briers Date: Sat, 19 Apr 2025 09:29:17 +0200 Subject: [PATCH] Add "obsolete inheritance" analyzer (#5858) This allows us to make it obsolete to *inherit* from a class, and only that. Intended so people stop inheriting UI controls for no good reason. Fixes #5856 --- .../ObsoleteInheritanceAnalyzerTest.cs | 86 +++++++++++++++++++ .../Robust.Analyzers.Tests.csproj | 1 + .../ObsoleteInheritanceAnalyzer.cs | 75 ++++++++++++++++ Robust.Client/UserInterface/UIConstants.cs | 7 ++ Robust.Roslyn.Shared/Diagnostics.cs | 2 + .../Analyzers/ObsoleteInheritanceAttribute.cs | 28 ++++++ Robust.Shared/Analyzers/VirtualAttribute.cs | 1 + 7 files changed, 200 insertions(+) create mode 100644 Robust.Analyzers.Tests/ObsoleteInheritanceAnalyzerTest.cs create mode 100644 Robust.Analyzers/ObsoleteInheritanceAnalyzer.cs create mode 100644 Robust.Client/UserInterface/UIConstants.cs create mode 100644 Robust.Shared/Analyzers/ObsoleteInheritanceAttribute.cs diff --git a/Robust.Analyzers.Tests/ObsoleteInheritanceAnalyzerTest.cs b/Robust.Analyzers.Tests/ObsoleteInheritanceAnalyzerTest.cs new file mode 100644 index 000000000..a32dbe7ef --- /dev/null +++ b/Robust.Analyzers.Tests/ObsoleteInheritanceAnalyzerTest.cs @@ -0,0 +1,86 @@ +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CSharp.Testing; +using Microsoft.CodeAnalysis.Testing; +using NUnit.Framework; +using VerifyCS = + Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier; + +namespace Robust.Analyzers.Tests; + +/// +/// Analyzer that implements [ObsoleteInheritance] checking, to give obsoletion warnings for inheriting types +/// that should never have been virtual. +/// +[Parallelizable(ParallelScope.All | ParallelScope.Fixtures)] +[TestFixture] +public sealed class ObsoleteInheritanceAnalyzerTest +{ + private static Task Verifier(string code, params DiagnosticResult[] expected) + { + var test = new CSharpAnalyzerTest() + { + TestState = + { + Sources = { code }, + }, + }; + + TestHelper.AddEmbeddedSources( + test.TestState, + "Robust.Shared.Analyzers.ObsoleteInheritanceAttribute.cs" + ); + + // ExpectedDiagnostics cannot be set, so we need to AddRange here... + test.TestState.ExpectedDiagnostics.AddRange(expected); + + return test.RunAsync(); + } + + [Test] + public async Task TestBasic() + { + const string code = """ + using Robust.Shared.Analyzers; + + [ObsoleteInheritance] + public class Base; + + public class NotAllowed : Base; + """; + + await Verifier(code, + // /0/Test0.cs(6,14): warning RA0034: Type 'NotAllowed' inherits from 'Base', which has obsoleted inheriting from itself + VerifyCS.Diagnostic(ObsoleteInheritanceAnalyzer.Rule).WithSpan(6, 14, 6, 24).WithArguments("NotAllowed", "Base") + ); + } + + [Test] + public async Task TestMessage() + { + const string code = """ + using Robust.Shared.Analyzers; + + [ObsoleteInheritance("Sus")] + public class Base; + + public class NotAllowed : Base; + """; + + await Verifier(code, + // /0/Test0.cs(6,14): warning RA0034: Type 'NotAllowed' inherits from 'Base', which has obsoleted inheriting from itself: "Sus" + VerifyCS.Diagnostic(ObsoleteInheritanceAnalyzer.RuleWithMessage).WithSpan(6, 14, 6, 24).WithArguments("NotAllowed", "Base", "Sus") + ); + } + + [Test] + public async Task TestNormal() + { + const string code = """ + public class Base; + + public class AllowedAllowed : Base; + """; + + await Verifier(code); + } +} diff --git a/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj b/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj index 4f37fd397..f3d2d52ea 100644 --- a/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj +++ b/Robust.Analyzers.Tests/Robust.Analyzers.Tests.csproj @@ -14,6 +14,7 @@ + diff --git a/Robust.Analyzers/ObsoleteInheritanceAnalyzer.cs b/Robust.Analyzers/ObsoleteInheritanceAnalyzer.cs new file mode 100644 index 000000000..3ae91e84c --- /dev/null +++ b/Robust.Analyzers/ObsoleteInheritanceAnalyzer.cs @@ -0,0 +1,75 @@ +#nullable enable +using System.Collections.Immutable; +using Microsoft.CodeAnalysis; +using Microsoft.CodeAnalysis.Diagnostics; +using Robust.Roslyn.Shared; + +namespace Robust.Analyzers; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class ObsoleteInheritanceAnalyzer : DiagnosticAnalyzer +{ + private const string Attribute = "Robust.Shared.Analyzers.ObsoleteInheritanceAttribute"; + + public static readonly DiagnosticDescriptor Rule = new( + Diagnostics.IdObsoleteInheritance, + "Parent type has obsoleted inheritance", + "Type '{0}' inherits from '{1}', which has obsoleted inheriting from itself", + "Usage", + DiagnosticSeverity.Warning, + true); + + public static readonly DiagnosticDescriptor RuleWithMessage = new( + Diagnostics.IdObsoleteInheritanceWithMessage, + "Parent type has obsoleted inheritance", + "Type '{0}' inherits from '{1}', which has obsoleted inheriting from itself: \"{2}\"", + "Usage", + DiagnosticSeverity.Warning, + true); + + public override ImmutableArray SupportedDiagnostics => [Rule, RuleWithMessage]; + + public override void Initialize(AnalysisContext context) + { + context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None); + context.EnableConcurrentExecution(); + context.RegisterSymbolAction(CheckClass, SymbolKind.NamedType); + } + + private static void CheckClass(SymbolAnalysisContext context) + { + if (context.Symbol is not INamedTypeSymbol typeSymbol) + return; + + if (typeSymbol.IsValueType || typeSymbol.BaseType is not { } baseType) + return; + + if (!AttributeHelper.HasAttribute(baseType, Attribute, out var data)) + return; + + var location = context.Symbol.Locations[0]; + + if (GetMessageFromAttributeData(data) is { } message) + { + context.ReportDiagnostic(Diagnostic.Create( + RuleWithMessage, + location, + [typeSymbol.Name, baseType.Name, message])); + } + else + { + context.ReportDiagnostic(Diagnostic.Create( + Rule, + location, + [typeSymbol.Name, baseType.Name])); + } + } + + private static string? GetMessageFromAttributeData(AttributeData data) + { + if (data.ConstructorArguments is not [var message, ..]) + return null; + + return message.Value as string; + } +} diff --git a/Robust.Client/UserInterface/UIConstants.cs b/Robust.Client/UserInterface/UIConstants.cs new file mode 100644 index 000000000..b839383cd --- /dev/null +++ b/Robust.Client/UserInterface/UIConstants.cs @@ -0,0 +1,7 @@ +namespace Robust.Client.UserInterface; + +internal static class UIConstants +{ + public const string ObsoleteInheritanceMessage = + "Do not inherit from standard UI controls, compose via nesting instead"; +} diff --git a/Robust.Roslyn.Shared/Diagnostics.cs b/Robust.Roslyn.Shared/Diagnostics.cs index 91bac5a1e..534fbd439 100644 --- a/Robust.Roslyn.Shared/Diagnostics.cs +++ b/Robust.Roslyn.Shared/Diagnostics.cs @@ -37,6 +37,8 @@ public static class Diagnostics public const string IdPreferOtherType = "RA0031"; public const string IdDuplicateDependency = "RA0032"; public const string IdForbidLiteral = "RA0033"; + public const string IdObsoleteInheritance = "RA0034"; + public const string IdObsoleteInheritanceWithMessage = "RA0035"; public static SuppressionDescriptor MeansImplicitAssignment => new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned."); diff --git a/Robust.Shared/Analyzers/ObsoleteInheritanceAttribute.cs b/Robust.Shared/Analyzers/ObsoleteInheritanceAttribute.cs new file mode 100644 index 000000000..9cfc05f43 --- /dev/null +++ b/Robust.Shared/Analyzers/ObsoleteInheritanceAttribute.cs @@ -0,0 +1,28 @@ +using System; + +namespace Robust.Shared.Analyzers; + +/// +/// Indicates that the ability to inherit this type is obsolete, and attempting to do so should give a warning. +/// +/// +/// This is useful to gracefully deal with types that should never have had . +/// +/// +[AttributeUsage(AttributeTargets.Class, Inherited = false)] +public sealed class ObsoleteInheritanceAttribute : Attribute +{ + /// + /// An optional message provided alongside this obsoletion. + /// + public string? Message { get; } + + public ObsoleteInheritanceAttribute() + { + } + + public ObsoleteInheritanceAttribute(string message) + { + Message = message; + } +} diff --git a/Robust.Shared/Analyzers/VirtualAttribute.cs b/Robust.Shared/Analyzers/VirtualAttribute.cs index 849c4103b..fc0324c44 100644 --- a/Robust.Shared/Analyzers/VirtualAttribute.cs +++ b/Robust.Shared/Analyzers/VirtualAttribute.cs @@ -9,6 +9,7 @@ namespace Robust.Shared.Analyzers; /// Robust uses analyzers to prevent accidental usage of non-sealed classes: /// a class must be either marked [Virtual], abstract, or sealed. /// +/// [AttributeUsage(AttributeTargets.Class, Inherited = false)] public sealed class VirtualAttribute : Attribute {