Skip to content

Commit 4e2ea19

Browse files
RyanLamanskyRyanLamansky
committed
Add SSS004 analyzer: flag if/else-if and if-{return/throw} chains with repeated '<sameScrutinee> is <SameType> { <SameProperty>: ... }' conditions, which collapse to a single switch with one isinst + one property read instead of N of each; refactors two ASC/DESC chains in Selection / WindowExpression that the rule found.
1 parent 6fd3f63 commit 4e2ea19

5 files changed

Lines changed: 647 additions & 14 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ The number lands in `Data["HelpLink.EvtID"]` for tests to assert. When adding er
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.
120120
- **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.
121+
- **SSS004** (custom analyzer): two or more `if` / `else if` branches (or a run of consecutive `if (cond) { return/throw; }` early-exit statements) whose conditions all have the shape `<sameScrutinee> is <SameType> { <SameProperty>: ... }` should be a single `switch`. The C# compiler emits one `isinst` and one property/field read per arm in the `if`-chain form; the equivalent `switch` fuses both — one `isinst` + one `ldfld` for the whole dispatch, with each arm reduced to a `ldloc` + `beq`/`bne` against the cached discriminant (verified via `ilspycmd -il`). Token-dispatch hot paths in this codebase use this pattern pervasively. The rule restricts the scrutinee to syntactically simple chains (locals, parameters, `this`, dotted member access) to avoid silently changing semantics for side-effecting expressions; method calls / indexers / casts in the scrutinee position skip the diagnostic. Property-pattern designations (`is T { P: x } v`) and multiple subpatterns (`is T { P: a, Q: b }`) are also skipped — the rewrite isn't mechanical for those.
121122
- **MSTEST0049**: async tests must thread `TestContext.CancellationToken`. Pattern: `public TestContext TestContext { get; set; } = null!;` plus a helper that uses `this.TestContext.CancellationToken`.
122123
- **MSTEST0037**: prefer `Assert.IsEmpty(values)` over `Assert.AreEqual(0, values.Count)`; use typed asserts over generic `Assert.AreEqual` on known types.
123124
- **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/PropertyPatternChainAnalyzerTests.cs‎

Lines changed: 353 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,353 @@
1+
using Microsoft.CodeAnalysis.CSharp.Testing;
2+
using Microsoft.CodeAnalysis.Testing;
3+
4+
namespace SqlServerSimulator.Analyzers;
5+
6+
[TestClass]
7+
public sealed class PropertyPatternChainAnalyzerTests
8+
{
9+
public TestContext TestContext { get; set; } = null!;
10+
11+
private Task RunAsync(string source) =>
12+
new CSharpAnalyzerTest<PropertyPatternChainAnalyzer, DefaultVerifier>
13+
{ TestCode = source }.RunAsync(this.TestContext.CancellationToken);
14+
15+
private const string Preamble = """
16+
internal enum Tag { A, B, C }
17+
internal abstract class Token { }
18+
internal sealed class Keyword : Token { public Tag Kind; }
19+
internal sealed class Op : Token { public char Ch; }
20+
""";
21+
22+
[TestMethod]
23+
public Task TwoArmIfElseIf_SameScrutineeTypeProperty_Reports() =>
24+
RunAsync(Preamble + """
25+
26+
internal static class Holder
27+
{
28+
public static int Resolve(Token t)
29+
{
30+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
31+
return 1;
32+
else if (t is Keyword { Kind: Tag.B })
33+
return 2;
34+
return 0;
35+
}
36+
}
37+
""");
38+
39+
[TestMethod]
40+
public Task ThreeArmIfElseIf_Reports() =>
41+
RunAsync(Preamble + """
42+
43+
internal static class Holder
44+
{
45+
public static int Resolve(Token t)
46+
{
47+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
48+
return 1;
49+
else if (t is Keyword { Kind: Tag.B })
50+
return 2;
51+
else if (t is Keyword { Kind: Tag.C })
52+
return 3;
53+
return 0;
54+
}
55+
}
56+
""");
57+
58+
[TestMethod]
59+
public Task IfElseIfWithFinalElse_StillReports() =>
60+
RunAsync(Preamble + """
61+
62+
internal static class Holder
63+
{
64+
public static int Resolve(Token t)
65+
{
66+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
67+
return 1;
68+
else if (t is Keyword { Kind: Tag.B })
69+
return 2;
70+
else
71+
return 0;
72+
}
73+
}
74+
""");
75+
76+
[TestMethod]
77+
public Task TwoConsecutiveIfReturn_Reports() =>
78+
RunAsync(Preamble + """
79+
80+
internal static class Holder
81+
{
82+
public static int Resolve(Token t)
83+
{
84+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
85+
return 1;
86+
if (t is Keyword { Kind: Tag.B })
87+
return 2;
88+
return 0;
89+
}
90+
}
91+
""");
92+
93+
[TestMethod]
94+
public Task IfThrow_Reports() =>
95+
RunAsync("""
96+
using System;
97+
""" + Preamble + """
98+
99+
internal static class Holder
100+
{
101+
public static int Resolve(Token t)
102+
{
103+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
104+
throw new InvalidOperationException();
105+
if (t is Keyword { Kind: Tag.B })
106+
throw new InvalidOperationException();
107+
return 0;
108+
}
109+
}
110+
""");
111+
112+
[TestMethod]
113+
public Task SingleArm_DoesNotReport() =>
114+
RunAsync(Preamble + """
115+
116+
internal static class Holder
117+
{
118+
public static int Resolve(Token t)
119+
{
120+
if (t is Keyword { Kind: Tag.A })
121+
return 1;
122+
return 0;
123+
}
124+
}
125+
""");
126+
127+
[TestMethod]
128+
public Task DifferentTypes_DoesNotReport() =>
129+
RunAsync(Preamble + """
130+
131+
internal static class Holder
132+
{
133+
public static int Resolve(Token t)
134+
{
135+
if (t is Keyword { Kind: Tag.A })
136+
return 1;
137+
else if (t is Op { Ch: ',' })
138+
return 2;
139+
return 0;
140+
}
141+
}
142+
""");
143+
144+
[TestMethod]
145+
public Task DifferentProperty_DoesNotReport() =>
146+
RunAsync(Preamble + """
147+
148+
internal sealed class Multi : Token { public Tag Kind; public Tag Other; }
149+
internal static class Holder
150+
{
151+
public static int Resolve(Token t)
152+
{
153+
if (t is Multi { Kind: Tag.A })
154+
return 1;
155+
else if (t is Multi { Other: Tag.B })
156+
return 2;
157+
return 0;
158+
}
159+
}
160+
""");
161+
162+
[TestMethod]
163+
public Task DifferentScrutinee_DoesNotReport() =>
164+
RunAsync(Preamble + """
165+
166+
internal static class Holder
167+
{
168+
public static int Resolve(Token a, Token b)
169+
{
170+
if (a is Keyword { Kind: Tag.A })
171+
return 1;
172+
else if (b is Keyword { Kind: Tag.B })
173+
return 2;
174+
return 0;
175+
}
176+
}
177+
""");
178+
179+
[TestMethod]
180+
public Task ScrutineeWithMethodCall_DoesNotReport() =>
181+
RunAsync(Preamble + """
182+
183+
internal static class Holder
184+
{
185+
public static Token Next() => null!;
186+
public static int Resolve()
187+
{
188+
// The 'Next()' call is side-effecting; collapsing to a switch
189+
// would call it once, not twice. Conservative: don't flag.
190+
if (Next() is Keyword { Kind: Tag.A })
191+
return 1;
192+
else if (Next() is Keyword { Kind: Tag.B })
193+
return 2;
194+
return 0;
195+
}
196+
}
197+
""");
198+
199+
[TestMethod]
200+
public Task ScrutineeWithIndexer_DoesNotReport() =>
201+
RunAsync(Preamble + """
202+
203+
internal static class Holder
204+
{
205+
public static int Resolve(Token[] arr)
206+
{
207+
if (arr[0] is Keyword { Kind: Tag.A })
208+
return 1;
209+
else if (arr[0] is Keyword { Kind: Tag.B })
210+
return 2;
211+
return 0;
212+
}
213+
}
214+
""");
215+
216+
[TestMethod]
217+
public Task DesignationOnPattern_DoesNotReport() =>
218+
RunAsync(Preamble + """
219+
220+
internal static class Holder
221+
{
222+
public static int Resolve(Token t)
223+
{
224+
// 'k' designations would force the rewrite to rename across
225+
// arms; the analyzer skips this shape.
226+
if (t is Keyword { Kind: Tag.A } k1)
227+
return k1.GetHashCode();
228+
else if (t is Keyword { Kind: Tag.B } k2)
229+
return k2.GetHashCode();
230+
return 0;
231+
}
232+
}
233+
""");
234+
235+
[TestMethod]
236+
public Task MultipleSubpatterns_DoesNotReport() =>
237+
RunAsync(Preamble + """
238+
239+
internal sealed class Multi : Token { public Tag Kind; public char Ch; }
240+
internal static class Holder
241+
{
242+
public static int Resolve(Token t)
243+
{
244+
if (t is Multi { Kind: Tag.A, Ch: ',' })
245+
return 1;
246+
else if (t is Multi { Kind: Tag.B, Ch: ';' })
247+
return 2;
248+
return 0;
249+
}
250+
}
251+
""");
252+
253+
[TestMethod]
254+
public Task IfReturnFollowedByNonExitingIf_DoesNotChain() =>
255+
RunAsync(Preamble + """
256+
257+
internal static class Holder
258+
{
259+
public static int Resolve(Token t)
260+
{
261+
int x = 0;
262+
if (t is Keyword { Kind: Tag.A })
263+
return 1;
264+
if (t is Keyword { Kind: Tag.B })
265+
x = 2; // doesn't exit; chain breaks here
266+
return x;
267+
}
268+
}
269+
""");
270+
271+
[TestMethod]
272+
public Task DottedScrutinee_StillReports() =>
273+
RunAsync(Preamble + """
274+
275+
internal sealed class Wrapper { public Token Token = null!; }
276+
internal static class Holder
277+
{
278+
public static int Resolve(Wrapper w)
279+
{
280+
{|SSS004:if|} (w.Token is Keyword { Kind: Tag.A })
281+
return 1;
282+
else if (w.Token is Keyword { Kind: Tag.B })
283+
return 2;
284+
return 0;
285+
}
286+
}
287+
""");
288+
289+
[TestMethod]
290+
public Task OrPatternInPropertyMatch_StillReports() =>
291+
RunAsync(Preamble + """
292+
293+
internal static class Holder
294+
{
295+
public static int Resolve(Token t)
296+
{
297+
{|SSS004:if|} (t is Keyword { Kind: Tag.A or Tag.B })
298+
return 1;
299+
else if (t is Keyword { Kind: Tag.C })
300+
return 2;
301+
return 0;
302+
}
303+
}
304+
""");
305+
306+
[TestMethod]
307+
public Task ChainInsideSwitchSection_Reports() =>
308+
RunAsync(Preamble + """
309+
310+
internal static class Holder
311+
{
312+
public static int Resolve(int code, Token t)
313+
{
314+
switch (code)
315+
{
316+
case 1:
317+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
318+
return 1;
319+
if (t is Keyword { Kind: Tag.B })
320+
return 2;
321+
return 0;
322+
default:
323+
return -1;
324+
}
325+
}
326+
}
327+
""");
328+
329+
[TestMethod]
330+
public Task NestedIfChainHeads_BothReport() =>
331+
RunAsync(Preamble + """
332+
333+
internal static class Holder
334+
{
335+
public static int Resolve(Token t, Token u)
336+
{
337+
{|SSS004:if|} (t is Keyword { Kind: Tag.A })
338+
{
339+
{|SSS004:if|} (u is Keyword { Kind: Tag.A })
340+
return 1;
341+
else if (u is Keyword { Kind: Tag.B })
342+
return 2;
343+
return 3;
344+
}
345+
else if (t is Keyword { Kind: Tag.B })
346+
{
347+
return 4;
348+
}
349+
return 0;
350+
}
351+
}
352+
""");
353+
}

0 commit comments

Comments
 (0)