Repository navigation
test(arch): ratchet test types that parse C# without binding symbols - #1189
Conversation
|
Review round 1 (Codex
Contrived (PR note): The floor and stale-row checks behaved correctly. |
…for the exemption
…d compilation no longer exempts
|
Reply to review round 1. The head is now
The historical proof is now in the PR body. #584, #872, #1043 and #1056 each add a type the ratchet flags. #1012 and #1013 add none: their scanners already queried a semantic model, and their bypasses came from comparing symbol names as strings, which this ratchet does not judge. Deleting the Local run: |
|
Review round 2 (Codex All round 1 repros are closed. New gaps:
Cost: the guard runs in 18–32 s locally. The Application CI job is about 11 s (11–13%) slower than recent Under the review-loop rule this is a stop point: two rounds whose findings were all in the guard's own detection logic (a control for a control). The maintainer decides whether to keep going. |
|
Reply to review round 2. The head is now
Local run on |
Closes #1184
Guard tests that parse C# by spelling keep missing aliases, generic arity, nested types,
global usingand#if !DEBUG. Reviewers found these bypasses in 30 merged PRs (#1010, #1013, #1033, #1056 and others). This PR stops new ones from being added.What it checks.
SpellingScannerRatchetTestscompiles every trackedtests/**/*.csfile, one compilation per test project. The compilation includes the SDK implicit usings and each project's MSBuild<Using>items. It binds every invocation and method-group argument and keeps two facts per outermost type:Parse*orCreatemethod onCSharpSyntaxTreeorSyntaxFactory, or one of our own helpers that returns aSyntaxTreeor anySyntaxNode(such asCompatibilityExceptionScanner.Parse). A call that does not bind but is spelledParseText,ParseSyntaxTreeorParseCompilationUnitalso counts, so a missing import fails closed.SemanticModel: a member call, or an extension method whose receiver is the model, such asGetSymbolInfoorGetDeclaredSymbol. It does not matter where the model came from or whether the variable is typedvar.The two facts are recorded separately, so a call that does both (a
SemanticModelextension that returns the node it found) keeps the type exempt. A type that parses but never binds fails with:Pinned list. The 11 types that already work this way are listed by full name, each with a one-line reason. They may change freely. A listed type that stops parsing by spelling (moved to an analyzer, deleted, or now binding) fails until its row is deleted, so the list only shrinks. No row can be removed today: all 11 still parse without binding. Moving them is out of scope (#1184).
Creating a compilation does not count as binding. Neither does asking it for a model. Only a query on the model does. A discarded
_ = CSharpCompilation.Create(...)therefore no longer exempts a type (review round 1, note 5). None of the types already exempt in the real tree depended on the looser rule.Proof against past PRs. A throwaway harness ran the same detection over
tests/at each PR's squash commit and at its parent, reading the files and MSBuild items withgit show:deaa260eTenantBypass.GuardScanner8f9e6728Architecture.ModuleLedgerScanner(#1078 later moved it to bound symbols)7ce95cf46e3f5845b998b072TenantBypass.FarmDirectoryCallerTests20198660Architecture.SourcePreprocessorTests#1012 and #1013 are honest misses.
InsightsReadOnlyTestsandCompatibilityExceptionScanneralready queried a semantic model. Their bypasses (alias normalisation, generic arity) came from comparing symbol names as strings, which this ratchet does not judge.On the current tree, deleting the
GuardScannerrow (#584's scanner) turns the guard red:Restoring the row makes it green again (3 of 3 passed).
Review round 1. Each finding has a fixture row, and each row was red before its fix commit:
<Using>hidCSharpSyntaxTreeProjectUsing,Unresolved,MsBuildUsingItems_BecomeGlobalUsingsa53d326f. The reviewer's<Using>plus file mutation on the real tree is also red, namingReviewProjectUsing.CompatibilityExceptionScanner.ParseReuse,ReuseNode,Group(method group)907c5088. The reviewer'sReviewReuseParserfile on the real tree is also red.CSharpCompilationparameter exempted a typeCompilationParameterb10f2fe6VarBinding,ExtensionBinding040dd669Discarded,Helper(model requested, never queried)924461aeReview round 2. The maintainer chose to fix two findings and note one.
SyntaxNodeescapedReuseBasede4a9941Finder,SemanticFind05d9bbae. With the fix reverted, the guard namesSemanticFind.<Using>items are read as raw XMLLimits.
InsightsReadOnlyTestsis one such type.<Using>items are read from the XML ofDirectory.Build.props,tests/Directory.Build.propsand each test csproj, not evaluated. Conditions, other imports,Remove, and metadata written as child elements or asStatic="True"are not followed. A missed alias can hide a parser, and a stale one can make an unrelatedParseTextread as a C# parse. Reading the SDK-generated global-usings file would fix this; it needs a build output the guard does not have today.Cost. The guard binds every call in 388 files, with binding run per tree in parallel. In CI the Application job took 110 s at
e05701d8, against 99 s on recent main.Tests run locally:
dotnet test tests/Cluckwork.Application.Tests --filter FullyQualifiedName~SpellingScannerRatchetTests(3 passed, at05d9bbae).