Skip to content

Commit bb9761f

Browse files
RyanLamanskyRyanLamansky
committed
Added the SSS003 analyzer flagging allocating string.ToUpperInvariant() / ToLowerInvariant() whose result governs a switch, and rewrote DatePartKind.Resolve to the span-form keyword dispatch the rest of the parser already uses.
1 parent 3d008c5 commit bb9761f

4 files changed

Lines changed: 332 additions & 18 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ The number lands in `Data["HelpLink.EvtID"]` for tests to assert. When adding er
117117

118118
- **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, and interface implementations (both explicit and implicit — a property whose name and signature satisfy a member of an implemented interface) are exempt; the interface contract dictates the property shape. Lives in `SqlServerSimulator.Analyzers/`.
119119
- **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.
120+
- **SSS003** (custom analyzer): `string.ToUpperInvariant()` / `string.ToLowerInvariant()` whose result is the *governing expression* of a `switch` statement or `switch` expression allocates a temporary string only to throw it away after dispatch. Use the `Span<char>` overload — `Span<char> buf = stackalloc char[s.Length]; s.AsSpan().ToUpperInvariant(buf)` — and switch on the resulting count (which lets the parser dispatch by length first; matches the established `Parser/Expression.cs:ResolveBuiltIn` and `Storage/SqlType.cs:GetByName` pattern). The rule is intentionally narrow: only the switch-governing case is flagged. Allocating uses where the upper/lower string itself is the function's returned value (e.g. SQL `UPPER`, GUID-to-string casts) don't trip — their result isn't a switch governing expression.
120121
- **MSTEST0049**: async tests must thread `TestContext.CancellationToken`. Pattern: `public TestContext TestContext { get; set; } = null!;` plus a helper that uses `this.TestContext.CancellationToken`.
121122
- **MSTEST0037**: prefer `Assert.IsEmpty(values)` over `Assert.AreEqual(0, values.Count)`; use typed asserts over generic `Assert.AreEqual` on known types.
122123
- **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).

‎SqlServerSimulator.Analyzers.Tests/TransientUpperLowerInvariantAnalyzerTests.cs‎

Lines changed: 140 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,140 @@
1+
using Microsoft.CodeAnalysis.CSharp.Testing;
2+
using Microsoft.CodeAnalysis.Testing;
3+
4+
namespace SqlServerSimulator.Analyzers;
5+
6+
[TestClass]
7+
public sealed class TransientUpperLowerInvariantAnalyzerTests
8+
{
9+
public TestContext TestContext { get; set; } = null!;
10+
11+
private Task RunAsync(string source) =>
12+
new CSharpAnalyzerTest<TransientUpperLowerInvariantAnalyzer, DefaultVerifier>
13+
{ TestCode = source }.RunAsync(this.TestContext.CancellationToken);
14+
15+
[TestMethod]
16+
public Task SwitchExpressionGovern_OnString_Reports() =>
17+
RunAsync("""
18+
internal static class Holder
19+
{
20+
public static int Resolve(string s) => s.{|SSS003:ToUpperInvariant|}() switch
21+
{
22+
"X" => 1,
23+
_ => 0,
24+
};
25+
}
26+
""");
27+
28+
[TestMethod]
29+
public Task SwitchStatementGovern_OnString_Reports() =>
30+
RunAsync("""
31+
internal static class Holder
32+
{
33+
public static int Resolve(string s)
34+
{
35+
switch (s.{|SSS003:ToLowerInvariant|}())
36+
{
37+
case "x": return 1;
38+
default: return 0;
39+
}
40+
}
41+
}
42+
""");
43+
44+
[TestMethod]
45+
public Task ParenthesizedSwitchGovern_StillReports() =>
46+
RunAsync("""
47+
internal static class Holder
48+
{
49+
public static int Resolve(string s) => (s.{|SSS003:ToUpperInvariant|}()) switch
50+
{
51+
"X" => 1,
52+
_ => 0,
53+
};
54+
}
55+
""");
56+
57+
[TestMethod]
58+
public Task SpanOverload_Allowed_DoesNotReport() =>
59+
RunAsync("""
60+
using System;
61+
internal static class Holder
62+
{
63+
public static int Resolve(string s)
64+
{
65+
Span<char> buf = stackalloc char[s.Length];
66+
return s.AsSpan().ToUpperInvariant(buf) switch
67+
{
68+
// The Span overload returns the count written; switch
69+
// on the length, then read the buffer separately.
70+
1 => 1,
71+
_ => 0,
72+
};
73+
}
74+
}
75+
""");
76+
77+
[TestMethod]
78+
public Task ResultUsedAsString_DoesNotReport() =>
79+
RunAsync("""
80+
internal static class Holder
81+
{
82+
// The upper-cased string IS the function's output — no transient
83+
// dispatch, no allocation to elide.
84+
public static string Up(string s) => s.ToUpperInvariant();
85+
}
86+
""");
87+
88+
[TestMethod]
89+
public Task ResultPipedThroughLength_DoesNotReport() =>
90+
RunAsync("""
91+
internal static class Holder
92+
{
93+
// Switch governs '.Length', not the upper-cased string itself.
94+
public static int Resolve(string s) => s.ToUpperInvariant().Length switch
95+
{
96+
0 => 1,
97+
_ => 0,
98+
};
99+
}
100+
""");
101+
102+
[TestMethod]
103+
public Task ResultStoredFirst_DoesNotReport() =>
104+
RunAsync("""
105+
internal static class Holder
106+
{
107+
// Storing in a local breaks the "transient" pattern — the rule
108+
// is conservative; only flag the direct-feed shape.
109+
public static int Resolve(string s)
110+
{
111+
var upper = s.ToUpperInvariant();
112+
return upper switch
113+
{
114+
"X" => 1,
115+
_ => 0,
116+
};
117+
}
118+
}
119+
""");
120+
121+
[TestMethod]
122+
public Task SpanReceiver_DoesNotReport() =>
123+
RunAsync("""
124+
using System;
125+
internal static class Holder
126+
{
127+
public static int Resolve(ReadOnlySpan<char> s)
128+
{
129+
Span<char> buf = stackalloc char[s.Length];
130+
// s.ToUpperInvariant(buf) is the Span overload — not the
131+
// string method we're flagging. Should not report.
132+
return s.ToUpperInvariant(buf) switch
133+
{
134+
1 => 1,
135+
_ => 0,
136+
};
137+
}
138+
}
139+
""");
140+
}

‎SqlServerSimulator.Analyzers/TransientUpperLowerInvariantAnalyzer.cs‎

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,101 @@
1+
using System.Collections.Immutable;
2+
using Microsoft.CodeAnalysis;
3+
using Microsoft.CodeAnalysis.CSharp;
4+
using Microsoft.CodeAnalysis.CSharp.Syntax;
5+
using Microsoft.CodeAnalysis.Diagnostics;
6+
7+
namespace SqlServerSimulator.Analyzers;
8+
9+
/// <summary>
10+
/// Flags <see cref="string.ToUpperInvariant"/> / <see cref="string.ToLowerInvariant"/>
11+
/// calls whose result is consumed directly by a <c>switch</c> expression /
12+
/// statement — the transient case where the allocation can be avoided by
13+
/// using the <c>ToUpperInvariant(Span&lt;char&gt;)</c> /
14+
/// <c>ToLowerInvariant(Span&lt;char&gt;)</c> overloads with a stackalloc
15+
/// destination and switching on the resulting <see cref="System.ReadOnlySpan{Char}"/>.
16+
/// </summary>
17+
/// <remarks>
18+
/// Pairs naturally with the parser's keyword-dispatch hot paths — see
19+
/// <c>Parser/Expression.cs:ResolveBuiltIn</c> and <c>Storage/SqlType.cs:GetByName</c>
20+
/// for the established Span pattern. Switch-arm pattern equality on
21+
/// <c>string</c> literals works the same against <see cref="System.ReadOnlySpan{Char}"/>
22+
/// since C# 11, so the call-site rewrite is mechanical:
23+
/// <code>
24+
/// // before: return s.ToUpperInvariant() switch { "X" =&gt; ..., _ =&gt; null };
25+
/// // after: Span&lt;char&gt; buf = stackalloc char[s.Length];
26+
/// // return s.AsSpan().ToUpperInvariant(buf) switch { "X" =&gt; ..., _ =&gt; null };
27+
/// // (or pre-validate length and emit a diagnostic for over-long input)
28+
/// </code>
29+
/// Allocating uses where the upper/lower string itself is the function's
30+
/// returned value (e.g. <c>UPPER</c>'s SQL semantics, GUID-to-string casts)
31+
/// don't trip the rule since their result isn't a switch governing expression.
32+
/// </remarks>
33+
[DiagnosticAnalyzer(LanguageNames.CSharp)]
34+
public sealed class TransientUpperLowerInvariantAnalyzer : DiagnosticAnalyzer
35+
{
36+
private static readonly DiagnosticDescriptor Rule = new(
37+
id: "SSS003",
38+
title: "Allocating ToUpperInvariant/ToLowerInvariant feeding a switch",
39+
messageFormat: "string.{0}() allocates a temporary string but the result is fed directly to a switch — use the Span<char> overload with a stackalloc destination",
40+
category: "Performance",
41+
defaultSeverity: DiagnosticSeverity.Warning,
42+
isEnabledByDefault: true,
43+
description: "string.ToUpperInvariant() and string.ToLowerInvariant() each return a new string. When the result is consumed only by a switch (expression or statement), the Span<char> overload writing into a stackalloc buffer does the same case-folding without allocation; switch arms accept ReadOnlySpan<char> against string-literal patterns identically since C# 11.");
44+
45+
/// <inheritdoc/>
46+
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => [Rule];
47+
48+
/// <inheritdoc/>
49+
public override void Initialize(AnalysisContext context)
50+
{
51+
context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
52+
context.EnableConcurrentExecution();
53+
context.RegisterSyntaxNodeAction(AnalyzeInvocation, SyntaxKind.InvocationExpression);
54+
}
55+
56+
private static void AnalyzeInvocation(SyntaxNodeAnalysisContext context)
57+
{
58+
var invocation = (InvocationExpressionSyntax)context.Node;
59+
60+
if (invocation.Expression is not MemberAccessExpressionSyntax member)
61+
return;
62+
63+
var methodName = member.Name.Identifier.ValueText;
64+
if (methodName is not "ToUpperInvariant" and not "ToLowerInvariant")
65+
return;
66+
67+
// The Span<char> overload takes one argument; the allocating one
68+
// takes none. Skip the already-allocation-free form.
69+
if (invocation.ArgumentList.Arguments.Count != 0)
70+
return;
71+
72+
// Receiver must be a System.String instance — not Span<char>, not a
73+
// user-defined extension target. The semantic model gives us the
74+
// canonical type symbol; SpecialType pins the BCL string identity.
75+
var receiverType = context.SemanticModel.GetTypeInfo(member.Expression, context.CancellationToken).Type;
76+
if (receiverType?.SpecialType != SpecialType.System_String)
77+
return;
78+
79+
if (!IsSwitchGoverningExpression(invocation))
80+
return;
81+
82+
context.ReportDiagnostic(Diagnostic.Create(Rule, member.Name.GetLocation(), methodName));
83+
}
84+
85+
/// <summary>
86+
/// True iff <paramref name="node"/> sits in the governing-expression slot
87+
/// of a <c>switch</c> statement or <c>switch</c> expression — the only
88+
/// position where the rewrite to <see cref="System.ReadOnlySpan{Char}"/>
89+
/// is mechanical. Walks through trivial syntactic wrappers (parentheses)
90+
/// but stops at any other parent kind, so e.g.
91+
/// <c>s.ToUpperInvariant().Length switch { ... }</c> is not flagged
92+
/// (the governing expression is the <c>.Length</c> access).
93+
/// </summary>
94+
private static bool IsSwitchGoverningExpression(SyntaxNode node)
95+
{
96+
var current = node.Parent;
97+
while (current is ParenthesizedExpressionSyntax parens)
98+
current = parens.Parent;
99+
return current is SwitchExpressionSyntax or SwitchStatementSyntax;
100+
}
101+
}

‎SqlServerSimulator/Parser/Expressions/DatePartKind.cs‎

Lines changed: 90 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -37,25 +37,97 @@ internal static class DatePartKinds
3737
public static DatePartKind ResolveOrThrow(string keyword) =>
3838
Resolve(keyword) ?? throw SimulatedSqlException.NotARecognizedDatepartOption(keyword);
3939

40-
private static DatePartKind? Resolve(string keyword) => keyword.ToUpperInvariant() switch
40+
/// <summary>
41+
/// Span-based keyword dispatch — matches the pattern used in
42+
/// <c>Parser/Expression.cs:ResolveBuiltIn</c> and
43+
/// <c>Storage/SqlType.cs:GetByName</c> so the parser stays
44+
/// allocation-free in its keyword-resolution hot paths.
45+
/// </summary>
46+
private static DatePartKind? Resolve(string keyword)
4147
{
42-
"YEAR" or "YY" or "YYYY" => DatePartKind.Year,
43-
"QUARTER" or "QQ" or "Q" => DatePartKind.Quarter,
44-
"MONTH" or "MM" or "M" => DatePartKind.Month,
45-
"DAYOFYEAR" or "DY" or "Y" => DatePartKind.DayOfYear,
46-
"DAY" or "DD" or "D" => DatePartKind.Day,
47-
"WEEK" or "WK" or "WW" => DatePartKind.Week,
48-
"ISO_WEEK" or "ISOWK" or "ISOWW" => DatePartKind.IsoWeek,
49-
"WEEKDAY" or "DW" => DatePartKind.Weekday,
50-
"HOUR" or "HH" => DatePartKind.Hour,
51-
"MINUTE" or "MI" or "N" => DatePartKind.Minute,
52-
"SECOND" or "SS" or "S" => DatePartKind.Second,
53-
"MILLISECOND" or "MS" => DatePartKind.Millisecond,
54-
"MICROSECOND" or "MCS" => DatePartKind.Microsecond,
55-
"NANOSECOND" or "NS" => DatePartKind.Nanosecond,
56-
"TZOFFSET" or "TZ" => DatePartKind.TzOffset,
57-
_ => null,
58-
};
48+
Span<char> upper = stackalloc char[keyword.Length];
49+
return keyword.AsSpan().ToUpperInvariant(upper) switch
50+
{
51+
1 => upper switch
52+
{
53+
"Q" => DatePartKind.Quarter,
54+
"M" => DatePartKind.Month,
55+
"Y" => DatePartKind.DayOfYear,
56+
"D" => DatePartKind.Day,
57+
"N" => DatePartKind.Minute,
58+
"S" => DatePartKind.Second,
59+
_ => null,
60+
},
61+
2 => upper switch
62+
{
63+
"YY" => DatePartKind.Year,
64+
"QQ" => DatePartKind.Quarter,
65+
"MM" => DatePartKind.Month,
66+
"DY" => DatePartKind.DayOfYear,
67+
"DD" => DatePartKind.Day,
68+
"WK" or "WW" => DatePartKind.Week,
69+
"DW" => DatePartKind.Weekday,
70+
"HH" => DatePartKind.Hour,
71+
"MI" => DatePartKind.Minute,
72+
"SS" => DatePartKind.Second,
73+
"MS" => DatePartKind.Millisecond,
74+
"NS" => DatePartKind.Nanosecond,
75+
"TZ" => DatePartKind.TzOffset,
76+
_ => null,
77+
},
78+
3 => upper switch
79+
{
80+
"DAY" => DatePartKind.Day,
81+
"MCS" => DatePartKind.Microsecond,
82+
_ => null,
83+
},
84+
4 => upper switch
85+
{
86+
"YEAR" => DatePartKind.Year,
87+
"YYYY" => DatePartKind.Year,
88+
"WEEK" => DatePartKind.Week,
89+
"HOUR" => DatePartKind.Hour,
90+
_ => null,
91+
},
92+
5 => upper switch
93+
{
94+
"MONTH" => DatePartKind.Month,
95+
"ISOWK" or "ISOWW" => DatePartKind.IsoWeek,
96+
_ => null,
97+
},
98+
6 => upper switch
99+
{
100+
"MINUTE" => DatePartKind.Minute,
101+
"SECOND" => DatePartKind.Second,
102+
_ => null,
103+
},
104+
7 => upper switch
105+
{
106+
"QUARTER" => DatePartKind.Quarter,
107+
"WEEKDAY" => DatePartKind.Weekday,
108+
_ => null,
109+
},
110+
8 => upper switch
111+
{
112+
"ISO_WEEK" => DatePartKind.IsoWeek,
113+
"TZOFFSET" => DatePartKind.TzOffset,
114+
_ => null,
115+
},
116+
9 => upper switch
117+
{
118+
"DAYOFYEAR" => DatePartKind.DayOfYear,
119+
_ => null,
120+
},
121+
11 => upper switch
122+
{
123+
"MILLISECOND" => DatePartKind.Millisecond,
124+
"MICROSECOND" => DatePartKind.Microsecond,
125+
"NANOSECOND" => DatePartKind.Nanosecond,
126+
_ => null,
127+
},
128+
_ => null,
129+
};
130+
}
59131

60132
private static bool IsTimePart(DatePartKind k) => k is DatePartKind.Hour
61133
or DatePartKind.Minute or DatePartKind.Second or DatePartKind.Millisecond

0 commit comments

Comments
 (0)