Skip to content

Commit 9dd3f9d

Browse files
committed
Added SSS002 analyzer flagging readonly fields in non-public types whose declared type is more abstract than the immediately-assigned initializer, and applied the rule across SqlType / Collation (concrete-type singletons; helper-method extraction where switch-expression inference needed widening).
1 parent dd4ba5f commit 9dd3f9d

7 files changed

Lines changed: 410 additions & 91 deletions

File tree

CLAUDE.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ When adding error coverage: add a factory; never construct directly. The number
9898
## Conventions that fail builds
9999

100100
- **SSS001** (custom analyzer): non-public types may not have auto-properties or trivial wrappers over same-type fields. Expose the field directly: `public readonly T Foo = expr;`. Overrides, abstracts, statics, explicit-interface members are exempt. Lives in `SqlServerSimulator.Analyzers/`.
101+
- **SSS002** (custom analyzer): a `readonly` field in a non-public-API type whose declared type is a strict supertype of its immediately-assigned initializer should be declared as the concrete type. Same-assembly callers gain no API-stability benefit from the abstraction; the concrete declaration exposes more members directly and avoids virtual dispatch. Public types are exempt; value-typed initializers (boxing) are exempt; const fields and fields without initializers don't apply. After applying the rule, switch / conditional expressions that previously inferred the (now-shed) base type may need an explicit base-type annotation — extract to a helper method with an explicit return type (`SqlType.ResolveSimpleKeyword`), declare the destination variable explicitly (`SqlType resultType = ... ? ...`), or cast a `null` arm.
101102
- **MSTEST0049**: async tests must thread `TestContext.CancellationToken`. Pattern: `public TestContext TestContext { get; set; } = null!;` plus a helper that uses `this.TestContext.CancellationToken`.
102103
- **MSTEST0037**: prefer `Assert.IsEmpty(values)` over `Assert.AreEqual(0, values.Count)`; use typed asserts over generic `Assert.AreEqual` on known types.
103104
- **AssemblyHooks**: each test project has `AssemblyHooks.cs` with `static [TestClass]` hosting `[AssemblyInitialize]`. The analyzer-tests warm-up specifically prevents Roslyn cache contention under parallel execution (~3x slowdown without it).
Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
using Microsoft.CodeAnalysis.CSharp.Testing;
2+
using Microsoft.CodeAnalysis.Testing;
3+
4+
namespace SqlServerSimulator.Analyzers;
5+
6+
[TestClass]
7+
public sealed class WidenedFieldTypeAnalyzerTests
8+
{
9+
public TestContext TestContext { get; set; } = null!;
10+
11+
private Task RunAsync(string source) =>
12+
new CSharpAnalyzerTest<WidenedFieldTypeAnalyzer, DefaultVerifier>
13+
{ TestCode = source }.RunAsync(this.TestContext.CancellationToken);
14+
15+
[TestMethod]
16+
public Task BaseClassDeclared_DerivedAssigned_OnInternalType_Reports() =>
17+
RunAsync("""
18+
internal abstract class Base { }
19+
internal sealed class Derived : Base { }
20+
internal sealed class Holder
21+
{
22+
public static readonly Base {|SSS002:Singleton|} = new Derived();
23+
}
24+
""");
25+
26+
[TestMethod]
27+
public Task InterfaceDeclared_ConcreteAssigned_OnInternalType_Reports() =>
28+
RunAsync("""
29+
using System.Collections.Generic;
30+
internal sealed class Holder
31+
{
32+
private static readonly IReadOnlyList<int> {|SSS002:items|} = new List<int>();
33+
}
34+
""");
35+
36+
[TestMethod]
37+
public Task SameTypeDeclaredAndAssigned_DoesNotReport() =>
38+
RunAsync("""
39+
internal sealed class Derived
40+
{
41+
public static readonly Derived Instance = new Derived();
42+
}
43+
""");
44+
45+
[TestMethod]
46+
public Task PublicType_DoesNotReport() =>
47+
RunAsync("""
48+
public abstract class Base { }
49+
public sealed class Derived : Base { }
50+
public sealed class Holder
51+
{
52+
public static readonly Base Singleton = new Derived();
53+
}
54+
""");
55+
56+
[TestMethod]
57+
public Task PublicTypeNestedInInternal_TreatedAsNonPublic_Reports() =>
58+
RunAsync("""
59+
internal sealed class Outer
60+
{
61+
public sealed class Holder
62+
{
63+
public static readonly object {|SSS002:Singleton|} = new System.Text.StringBuilder();
64+
}
65+
}
66+
""");
67+
68+
[TestMethod]
69+
public Task NoInitializer_DoesNotReport() =>
70+
RunAsync("""
71+
internal sealed class Holder
72+
{
73+
public static readonly object Singleton;
74+
static Holder() { Singleton = new System.Text.StringBuilder(); }
75+
}
76+
""");
77+
78+
[TestMethod]
79+
public Task NotReadonly_DoesNotReport() =>
80+
RunAsync("""
81+
internal abstract class Base { }
82+
internal sealed class Derived : Base { }
83+
internal sealed class Holder
84+
{
85+
public static Base Mutable = new Derived();
86+
}
87+
""");
88+
89+
[TestMethod]
90+
public Task ConstField_DoesNotReport() =>
91+
RunAsync("""
92+
internal sealed class Holder
93+
{
94+
public const int N = 42;
95+
}
96+
""");
97+
98+
[TestMethod]
99+
public Task ValueTypeBoxedIntoObject_DoesNotReport() =>
100+
RunAsync("""
101+
internal sealed class Holder
102+
{
103+
public static readonly object Boxed = 42;
104+
}
105+
""");
106+
107+
[TestMethod]
108+
public Task FactoryReturningSameType_DoesNotReport() =>
109+
RunAsync("""
110+
internal sealed class Holder
111+
{
112+
private static System.Text.StringBuilder Make() => new();
113+
public static readonly System.Text.StringBuilder Builder = Make();
114+
}
115+
""");
116+
117+
[TestMethod]
118+
public Task NullInitializer_DoesNotReport() =>
119+
RunAsync("""
120+
internal sealed class Holder
121+
{
122+
public static readonly object? Maybe = null;
123+
}
124+
""");
125+
126+
[TestMethod]
127+
public Task InstanceField_OnInternalType_Reports() =>
128+
RunAsync("""
129+
internal abstract class Base { }
130+
internal sealed class Derived : Base { }
131+
internal sealed class Holder
132+
{
133+
public readonly Base {|SSS002:Singleton|} = new Derived();
134+
}
135+
""");
136+
137+
[TestMethod]
138+
public Task TwoFieldsInOneDeclaration_BothInitializedWithDerived_BothReport() =>
139+
RunAsync("""
140+
internal abstract class Base { }
141+
internal sealed class Derived : Base { }
142+
internal sealed class Holder
143+
{
144+
public static readonly Base {|SSS002:A|} = new Derived(), {|SSS002:B|} = new Derived();
145+
}
146+
""");
147+
}
Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
using System.Collections.Immutable;
2+
using System.Linq;
3+
using Microsoft.CodeAnalysis;
4+
using Microsoft.CodeAnalysis.CSharp;
5+
using Microsoft.CodeAnalysis.CSharp.Syntax;
6+
using Microsoft.CodeAnalysis.Diagnostics;
7+
8+
namespace SqlServerSimulator.Analyzers;
9+
10+
/// <summary>
11+
/// Flags <c>readonly</c> fields in non-public-API types whose declared type
12+
/// is a strict supertype (base class or implemented interface) of the
13+
/// immediately-assigned initializer's static type.
14+
/// </summary>
15+
/// <remarks>
16+
/// <para>
17+
/// In a non-public type, no API-stability boundary justifies hiding the
18+
/// concrete shape of an immediately-bound singleton. Declaring the field as
19+
/// the abstract type forces every same-assembly read to go through virtual
20+
/// dispatch (and stops the IDE from offering members the concrete type
21+
/// adds). Declaring the field as the concrete type lets callers see
22+
/// non-virtual / non-overridden members directly and shrinks the call-site
23+
/// indirection.
24+
/// </para>
25+
/// <para>
26+
/// The rule fires only when an immediate initializer pins the runtime type
27+
/// at compile time. Fields without initializers, fields whose initializer
28+
/// already matches the declared type, and fields whose initializer is a
29+
/// value type (treating the declared abstract type as a deliberate boxing
30+
/// boundary) are exempt. Public types are also exempt: the declared type
31+
/// there is the documented API contract.
32+
/// </para>
33+
/// </remarks>
34+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
35+
public sealed class WidenedFieldTypeAnalyzer : DiagnosticAnalyzer
36+
{
37+
private static readonly DiagnosticDescriptor Rule = new(
38+
id: "SSS002",
39+
title: "Field declared as an abstract type but initialized with a more specific value",
40+
messageFormat: "Field '{0}' is declared as '{1}' but its initializer produces '{2}'; in non-public type '{3}', declare the field as '{2}' to expose the concrete shape directly",
41+
category: "Design",
42+
defaultSeverity: DiagnosticSeverity.Warning,
43+
isEnabledByDefault: true,
44+
description: "A readonly field whose declared type is a base class or interface of its immediate initializer's runtime type provides no API-stability benefit when the containing type isn't part of the public API. Declare the field as the concrete type so same-assembly callers see the specific members and avoid virtual dispatch.");
45+
46+
/// <inheritdoc/>
47+
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule];
48+
49+
/// <inheritdoc/>
50+
public override void Initialize(AnalysisContext context)
51+
{
52+
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
53+
context.EnableConcurrentExecution();
54+
context.RegisterSyntaxNodeAction(AnalyzeFieldDeclaration, SyntaxKind.FieldDeclaration);
55+
}
56+
57+
private static void AnalyzeFieldDeclaration(SyntaxNodeAnalysisContext context)
58+
{
59+
var fieldDecl = (FieldDeclarationSyntax)context.Node;
60+
61+
// Const fields are restricted by the compiler to compile-time
62+
// primitives that must match the declared type exactly — there's no
63+
// widening to flag. readonly is the gate; non-readonly fields can
64+
// legitimately be reassigned to less-specific values later.
65+
if (fieldDecl.Modifiers.Any(SyntaxKind.ConstKeyword))
66+
return;
67+
if (!fieldDecl.Modifiers.Any(SyntaxKind.ReadOnlyKeyword))
68+
return;
69+
70+
foreach (var declarator in fieldDecl.Declaration.Variables)
71+
{
72+
if (declarator.Initializer is not { Value: { } initializerExpression })
73+
continue;
74+
75+
if (context.SemanticModel.GetDeclaredSymbol(declarator, context.CancellationToken) is not IFieldSymbol field)
76+
continue;
77+
78+
var containingType = field.ContainingType;
79+
if (containingType is null || IsEffectivelyPublic(containingType))
80+
continue;
81+
82+
if (field.Type is not INamedTypeSymbol declaredType)
83+
continue;
84+
85+
var initializerType = context.SemanticModel.GetTypeInfo(initializerExpression, context.CancellationToken).Type;
86+
if (initializerType is not INamedTypeSymbol assignedType)
87+
continue;
88+
89+
// Same type → no widening; nothing to flag. (Reference equality
90+
// via SymbolEqualityComparer is the right check; nominal name
91+
// matches won't appear here because the semantic model returns
92+
// the same canonical symbol on both sides.)
93+
if (SymbolEqualityComparer.Default.Equals(declaredType, assignedType))
94+
continue;
95+
96+
// Value-typed initializer with a reference-typed field is a
97+
// deliberate boxing boundary — declaring the field as the value
98+
// type would change semantics. Leave it alone.
99+
if (assignedType.IsValueType)
100+
continue;
101+
102+
// Confirm the declared type is actually a supertype of the
103+
// assigned type. (Defensive: the code compiled, so this should
104+
// always hold for a non-equal pair, but null and tuple types
105+
// can sneak through TypeInfo.)
106+
if (!IsAssignableTo(assignedType, declaredType))
107+
continue;
108+
109+
var location = declarator.Identifier.GetLocation();
110+
var diagnostic = Diagnostic.Create(
111+
Rule,
112+
location,
113+
field.Name,
114+
declaredType.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat),
115+
assignedType.ToDisplayString(SymbolDisplayFormat.MinimallyQualifiedFormat),
116+
containingType.Name);
117+
context.ReportDiagnostic(diagnostic);
118+
}
119+
}
120+
121+
/// <summary>
122+
/// True when <paramref name="source"/> is a strict subtype of
123+
/// <paramref name="target"/> — either through the base-class chain or
124+
/// because <paramref name="target"/> is one of <paramref name="source"/>'s
125+
/// implemented interfaces.
126+
/// </summary>
127+
private static bool IsAssignableTo(INamedTypeSymbol source, INamedTypeSymbol target)
128+
{
129+
for (var current = source.BaseType; current is not null; current = current.BaseType)
130+
{
131+
if (SymbolEqualityComparer.Default.Equals(current, target))
132+
return true;
133+
}
134+
foreach (var iface in source.AllInterfaces)
135+
{
136+
if (SymbolEqualityComparer.Default.Equals(iface, target))
137+
return true;
138+
}
139+
return false;
140+
}
141+
142+
/// <summary>
143+
/// True iff <paramref name="type"/> and every containing type up to the
144+
/// namespace are <c>public</c>. Mirrors the
145+
/// <see cref="WrapperPropertyAnalyzer"/> exemption.
146+
/// </summary>
147+
private static bool IsEffectivelyPublic(INamedTypeSymbol type) =>
148+
type.DeclaredAccessibility == Accessibility.Public
149+
&& (type.ContainingType is null || IsEffectivelyPublic(type.ContainingType));
150+
}

SqlServerSimulator.Tests.Internal/Storage/MixedTypeRowTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ public void Bit_RoundTrips(bool value)
5757
[TestMethod]
5858
public void EachType_NullRoundTrips()
5959
{
60-
foreach (var type in new[] { SqlType.Int32, SqlType.BigInt, SqlType.SmallInt, SqlType.TinyInt, SqlType.Bit })
60+
foreach (var type in new SqlType[] { SqlType.Int32, SqlType.BigInt, SqlType.SmallInt, SqlType.TinyInt, SqlType.Bit })
6161
{
6262
var decoded = RowDecoder.DecodeRow([type], RowEncoder.EncodeRow([type], [SqlValue.Null(type)]));
6363
IsTrue(decoded[0].IsNull);

SqlServerSimulator/Collation.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,15 +11,15 @@ private protected Collation()
1111

1212
public abstract string Name { get; }
1313

14-
internal static readonly Collation Default = new SQL_Latin1_General_CP1_CI_AS();
14+
internal static readonly SQL_Latin1_General_CP1_CI_AS Default = new();
1515

1616
public abstract int Compare(string? x, string? y);
1717

1818
public virtual bool Equals(string? x, string? y) => this.Compare(x, y) == 0;
1919

2020
public abstract int GetHashCode(string obj);
2121

22-
private sealed class SQL_Latin1_General_CP1_CI_AS : Collation
22+
internal sealed class SQL_Latin1_General_CP1_CI_AS : Collation
2323
{
2424
public override string Name => "SQL_Latin1_General_CP1_CI_AS";
2525

SqlServerSimulator/Parser/Expressions/TwoSidedExpression.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -279,7 +279,7 @@ private static decimal MoneyOrIntegerToDecimal(SqlValue v) =>
279279
private protected static SqlValue ApproximateArithmetic(SqlValue left, SqlValue right, char op)
280280
{
281281
var resultIsReal = left.Type == SqlType.Real && right.Type == SqlType.Real;
282-
var resultType = resultIsReal ? SqlType.Real : SqlType.Float;
282+
SqlType resultType = resultIsReal ? SqlType.Real : SqlType.Float;
283283
if (left.IsNull || right.IsNull)
284284
return SqlValue.Null(resultType);
285285

0 commit comments

Comments
 (0)