Fixes #3094 - #3097
Fixes #3094#3097
Conversation
The offender string's member half came from a regex asking for an identifier, a parenthesised anything and an opening brace, with no containment test — a description of most C# statements rather than of a member declaration. 48 of the 160 T-SQL literals the scan reads were labelled with a bare keyword or with nothing, and several more with a local variable or the type being constructed. Member ranges are now derived from declaration-head match offsets over walked source, and a literal is attributed to the innermost member whose range contains it. The declaration head requires an access modifier, which is what excludes every shape that misattributed: a statement keyword and a local variable can carry neither. Detection is untouched. The offender line also carries the line number, which is the half of the label that cannot be wrong.
The offender-label check refused the statement keywords that were actually observed. A resolver that starts returning a modifier or a built-in type name is the same defect one token over, and readonly is the measured instance: dropping the tuple-type test makes a tuple-typed field resolve to it. The whole reserved set is refused instead. DeclaredName also carried a whitespace test on the parameter list and a name-end move onto a generic argument list's closing bracket. Dumping all 20,308 declarations in the scanned trees with and without them diffs to nothing — they were a pair that existed to cancel each other out, and no mutation of either could red a test. Every branch left is red-proofed. Two arranged cases added for the shapes this turns on: a tuple-typed field, and a generic member whose constraint clause sits between the parameter list and the body.
A member count and a declaration count in a comment are frozen enumerations: both change on the next commit and nothing holds them true, which is the mechanism the fixture table's content anchors exist to avoid. The measurement each one supported is stated without the numeral, and the one figure kept is the pre-fix 48-of-160, which describes a resolver that no longer exists and so cannot go stale.
The line assertion compared the reported line against the resolved member's own line range — but the member was selected by containing that exact offset and LineOf is monotonic in it, so the comparison could not disagree with what it validated. #3089 named this shape on the same kind of scan. It now reads the file as lines, finds the anchor's own line, and requires the reported line not to be past it: a different derivation of the same answer, which reds when the newline count is broken.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed. This PR is scoped entirely to Left one inline finding: Everything else I traced through (the access-modifier-anchored |
The doc comment claimed the keyword arm and the unknown arm leave no third thing the resolver can return. They do: a plausible user-defined identifier that is not the enclosing member passes both. Of the four misattributions the fixture table was built from, only `if` was a keyword — a BCL type name and two locals are all identifier-shaped, so three of the four are shapes that assertion cannot see. They are pinned at five sites by exact comparison and nowhere else. The consequence is that widening DeclarationHead's modifier list is guarded by a comment and five fixture rows rather than by a corpus-scale test, because a re-admitted local produces an identifier-shaped label. Both comments now state that bound and point at each other. No detector added. The available one asserts the label appears among the map's names, and EnclosingMember returns a name taken from that map, so it cannot disagree with what it validates.
|
Reviewed. This is entirely a test-tooling change ( Left two inline comments:
Otherwise this is a very thoroughly self-vetted change (the mutation-tested arms (a)–(o) in the PR description cover the shapes I'd otherwise have flagged — accessor exclusion, generic constraints, tuple-typed fields, brace-in-literal, over/under-extension at EOF) and the regex/scanning logic checks out for the cases it explicitly claims to handle. |
DeclarationEnd returned at the closing brace of the first top-level
brace group. On an auto-property that group is the accessor list, so
`public string Q { get; set; } = "SELECT …";` ended before its own
initializer and a literal there was contained by nothing — labelled
<unknown>, the failure this change exists to remove.
Neither arm of ShapeOf can see it. A range that stops short of its own
member is still closed and still well under NextStart, so it reads as
WholeMember. That is why this is handled rather than documented as a
bound: it is the one truncation with no detector.
Measured across the scanned trees, 1,614 member ranges were short by
their initializer. No T-SQL sits in one today, so no label changed; all
160 attributions and every range shape are unchanged, and no member
becomes over-extended.
MemberMapOf in the scan is also lazy now, matching the sibling test —
it was walking every file including the ones with no T-SQL in them.
|
Reviewed. This is a test-only change ( Correctness of the new resolver — traced One real finding, left as an inline comment: No security, injection, or performance concerns — this is offline static analysis over source text, no SQL execution, no untrusted input. |
StoreSqlClockDisciplineTests carried a byte-identical copy of the pre-fix regex, and its own doc comment said what it was for: "the waiver key and the failure message". It feeds the resolved name into Waived.Contains(file + ":" + member), so a wrong name there is a wrong DECISION, not a wrong message — it can miss a legitimate waiver, or collide with an unrelated waived key and swallow a real bare-clock finding. Fixing the message-only copy and filing that one is shipping the inert half. The resolver moves to CSharpMemberMap and both pins use it, the treatment CSharpSourceWalker got in #2913 and RepoFile in #3090. Correcting the copy in place instead would leave two implementations that have already been shown to diverge. Measured before moving: 21 of the 140 SQL literals in the store tree change label — if, using, while, Select, NpgsqlCommand — and none of them is the one waived key, which resolves to RefreshSql under both. A resolver that stops resolving now reds that suite by name, which is how the wiring is shown to be live rather than merely compiled.
|
Reviewed the diff ( Lite/Darling parity: No concern. This convention-guard machinery has no counterpart in Correctness: Traced Security / performance: No concern — pure static analysis over local repo source at test time, no external input, no new I/O beyond reading files already on disk. Regexes are simple/bounded and One cosmetic nit posted inline (comment indentation). Otherwise looks solid — nice that the fixture table asserts its own |
|
Reviewed this PR (test-only change: Scope check: No shipped Lite/Darling app code changed — this only touches the shared test-tooling in Verification performed:
Nothing here touches T-SQL, so the CONTRIBUTING.md T-SQL style rules (AS aliases, |
1 participant
Fixes #3094.
EnclosingMemberbuilt the member half of every offender string from a regex whose second alternative asked for an identifier, a parenthesised anything and an opening brace, with no containment test anywhere. 48 of the 160 T-SQL literals the scan reads were labelled with a bare C# keyword or with nothing at all — 29if, 11catch, 7foreach, 1<unknown>— plus 3 with a type being constructed and 7 with a local variable.Detection is unchanged: the same literals are read, the same rules fire, the offender count is identical. The offender list is the guard's whole output at the moment it reds, and it was sending readers to a type name, a local variable, or the word
if.What replaces it
Member ranges are derived from declaration-head match offsets over walked source (
CSharpSourceWalker.StripCommentsAndStrings, #2913/#3052 — not a hand-rolled mask), each carrying the next declaration's start as a bound that does not come from the brace walk. A literal is attributed to the innermost member whose range contains it.The access modifier at the start of a line is the whole discriminator, and it is what excludes every shape that misattributed rather than blacklisting them one at a time: a statement keyword cannot carry one and neither can a local variable. What it therefore cannot read is stated on the field rather than discovered later — an accessor, an attribute argument, a member with no access modifier — and each of those resolves to
<unknown>, whichEveryTsqlLiteralInTheCorpus_IsAttributedToADeclaredMemberreds on by name. Widening the modifier list to admit a bareconstorstaticwould re-admit the local-variable case, so the resolution fails toward the worse label: an honest<unknown>a reader escalates, not a plausible name a reader trusts.The offender line now also carries the line number, which is the half of the label that cannot be wrong — a newline count over character-aligned text — so a reader still lands on the literal if attribution ever fails. It is cross-checked by a second derivation:
LineOfcounts newlines in a span, the pin reads the file as lines and finds the anchor's own line, and requires the reported line not to be past it. Asserting instead that the line falls inside the resolved member's range — which the first draft did — is a tautology: the member is selected by containing that exact offset andLineOfis monotonic in it, so the comparison cannot disagree with what it validates. #3089 named this shape on the same kind of scan, and it was worth catching in review of my own diff rather than in review of the PR.The fixture table
Located by a distinctive anchor substring, never an offset. All five are live source that changes most weeks; a pinned integer would red for a reason unrelated to attribution, and the next reader would repair it by updating the number — which silently retargets the pin at whatever literal now sits there. The anchor is content, so it travels with the literal, and the test requires it to match exactly one literal in the file and to still be in the population.
Each row also carries what the predecessor said and whether that was right, and the table asserts its own rows against that: a row claiming to fix a misattribution whose expectation equals the old answer pins nothing, and a row claiming the old answer was right while expecting a different one is a contradiction. Both directions.
Darling/PerformanceMonitor.Darling.Analysis/PgPlanFetcher.cs:184SqlConnectionStringBuilderFetchPlanBySqlHandleAsyncnew SqlConnectionStringBuilder(connectionString) {. The label named something that is not a member of the file at allLite/Services/LocalDataService.FinOps.Recommendations.cs:34sqlGetAgReplicaRoleAsyncconst string sql. Wrong as soon as a method holds two queries, which three other sites in this same file areLite/Services/LocalDataService.QueryStore.cs:845ifFetchQueryStorePlanAsyncif (quotedDbName == null) {PerformanceMonitor.Collectors/IndexObjectStatsCollector.cs:127optimizeForSequentialKeyBuildPerDatabaseStatsBodyPerformanceMonitor.Collectors/ServerPropertiesCollector.cs:60QueryTextQueryTextTwo corrections to the brief this was worked from, both measured rather than argued.
IndexObjectStatsCollector'soptimizeForSequentialKeyis a local in the same member, not a name from a different one — the resolver's own reproduction of the shipped string is what settles it. AndIndexObjectStatsCollector.cs:437reportsifas well (BuildEnumerationQuery), while.FinOps.Recommendations.csreportsifat two more sites andcatchat two others, all five of themGetRecommendationsAsync— so the file-level picture is worse than any single row: seven T-SQL literals, four labelled with a keyword.What turns five rows into the population
EveryTsqlLiteralInTheCorpus_IsAttributedToADeclaredMemberasks the same question of all 160 sites. A per-site table can never say what that says, because the sites it does not name are exactly where the next one will be — and on the tree as it shipped this is the assertion that carried the finding.Its keyword arm is the check that shares no code with the resolver:
if,catchandforeachare not member names in any C# program, so a label that is one is wrong without anything having to agree about how members are found. A second derivation of "what are this file's members" would have been free to make the same mistake. It refuses the whole reserved set rather than the statement keywords actually observed, because a resolver returning a modifier or a built-in type name is the same defect one token over — andreadonlyis the measured instance, not a hypothetical: dropping the tuple-type test makes a tuple-typed field resolve to it, and a list of statement keywords would have let that through.What these two arms bracket is keyword-or-nothing, not wrong-or-nothing, and that is the stated limitation of this assertion. A label that is a plausible user-defined identifier but not the enclosing member passes both arms in silence — and that is the majority shape here, not an edge case: of the four misattributions the fixture table was built from, only
ifwas a keyword.SqlConnectionStringBuilder(a BCL type),optimizeForSequentialKeyandsql(locals) are all identifier-shaped, so three of the four are shapes the corpus test cannot see. They are pinned by exact comparison at five sites and nowhere else; a regression to that shape anywhere in the other ~155 sites is caught only if it happens to land on one of the five.The consequence worth naming, because it turns a comment into the only guard: widening
DeclarationHead's modifier list is fixture-guarded only. That field's own note explains that admitting a bareconstorstaticwould re-admit the local-variable case — and a re-admitted local produces an identifier-shaped label, which is exactly what the corpus assertion is blind to. Both doc comments now state that bound and cross-reference each other, so someone arriving to widen the list reads it from either side.No detector was added for the gap, deliberately. The available one — assert the label appears among
map.Declarations' names — cannot fail:EnclosingMemberreturnsDeclaredRange.Nametaken from that very map, so the check and the thing it validates are the same value read twice. That is #3089's tautology one artifact over, and a pin that cannot fail is worse than a limitation that is written down, because it converts the gap into false confidence. Closing it for real needs a second independent derivation of "which member is this offset in", which is a larger change than the message this PR fixes.This paragraph is the amendment the review gate asked for, and it was a fair catch: the first draft asserted completeness the test does not have, in a file whose entire subject is labels that read as more trustworthy than they are. Measuring it moved the figure the wrong way — the report said two of four, and it is three.
TheMemberScan_ReadsEveryDeclarationWholecovers the direction attribution is silent about. A truncated body stops containing the literals below the cut, so they resolve to<unknown>and are loud; an over-extended one keeps containing them and the next member's, so it hands out a confident wrong answer that nothing downstream can see. Its floors are derived per tree and per anchor file, not counted — an exact declaration total would restate the size of the tree and go stale on the next commit, which is the thing the fixture table's anchors exist to avoid.Red-proof, per case
Darling.Testsisnet10.0-windowswithUseWPFand cannot run on macOS, so the realTsqlConventionGuardTests.csandCSharpSourceWalker.cswere compiled into a throwawaynet10.0xunit v3 host namedDarling.Tests, staged in the gitignored projectbin/— inside the repo, becauseRepoRoot()andSourceFiles()are[CallerFilePath]-anchored and a stage outside the tree cannot findPerformanceMonitor.sln.Darling/Darling.Tests/is excluded from the sweep's own glob, so the staged copies are not scanned. Every variant is a fresh staged copy; the worktree was never mutated, so no restore step could destroy anything. The.csprojis passed todotnet buildexplicitly,Build succeededis asserted separately from running, and a run producing noTEST EXECUTION SUMMARYis discarded rather than counted.Each fixture row is red-proofed on its own, by reverting the helper to the pre-fix regex and trimming the table to that one row — so the row is shown to fail for its own reason rather than behind whichever row happens to be evaluated first:
PgPlanFetcher.csFetchPlanBySqlHandleAsyncSqlConnectionStringBuilderLocalDataService.FinOps.Recommendations.csGetAgReplicaRoleAsyncsqlLocalDataService.QueryStore.csFetchQueryStorePlanAsyncifIndexObjectStatsCollector.csBuildPerDatabaseStatsBodyoptimizeForSequentialKeyServerPropertiesCollector.csQueryTextTheOffenderLabel_…is absent from the FAIL list,Failed: 4where every other row gives 5That last line is the non-regression evidence, from both sides. The row passes on the fixed code, and it passes under revert as well, so the site the predecessor already got right is untouched — and it is the only one of the five for which that is true.
The whole-file variants, each naming exactly the assertion it should:
TheOffenderLabel_…,EveryTsqlLiteralInTheCorpus_…,TheMemberScan_ReadsEveryDeclarationWhole,TheResolver_AttributesByScope_…,TheMemberScan_ReadsABodyWhoseLiteralHoldsABrace_…— 548 of 160 T-SQL literals are labelled with a C# statement keyword or with nothing, then the list. This is #3094's central measurement, reproduced by the pinconst/static/readonlyadmitted)TheOffenderLabel_…,TheResolver_…,TheMemberScan_ReadsEveryDeclarationWholeAlertContextBuilders.cs:55 BuildBlockingContext runs to line 107, past the declaration at line 66TheMemberScan_ReadsABodyWhoseLiteralHoldsABrace_…onlyTheMemberScan_ReadsEveryDeclarationWhole,TheMemberScan_ReadsABodyWhoseLiteralHoldsABrace_…FactRiskDisclosure.cs:285 QuoteNameTheMemberScan_ReadsEveryDeclarationWholeArchiveService.cs:40 IsArchiving runs to line 44, past the declaration at line 43TheMemberScan_IsBoundedByTheNextDeclaration_…Expected: OverExtendedExpected: UnterminatedDeclaredNameTheResolver_AttributesByScope_…TheResolver_AttributesByScope_…</>type resolves to the wrong tokenTheResolver_AttributesByScope_…LineOf's newline count doubledTheOffenderLabel_…reports line 367, but the anchor is on line 187 of the file as read by lines— the line cross-check, which the tautology it replaced could not have caughtTheResolver_AttributesByScope_…Auto's literal resolves to<unknown>EnclosingMemberalways returns<unknown>NoStoreSqlComparesANaiveTimestampToABareClock+ 4DarlingModuleMap.cs:<unknown> — collection_time >= now() - interval '2 days'. The sibling suite's waiver decision rides the shared resolver, shown rather than assumedcmp-identical to the committed file andgit statuswas emptyBoth discard paths were exercised rather than assumed, since a variant that comes back green having never compiled must not be counted: a deliberately wrong anchor reports
ANCHOR FAILUREand stops before building, and a deliberately uncompilable edit reportsBUILD FAILED — DISCARDand never runs. Every mutation asserts its anchor matches exactly once.They then earned it on a real accident, which is worth recording. Renaming a local from
checked_toexercisedin this file staled two anchors in the per-row mutation script. The runner correctly reportedANCHOR FAILUREand thenBUILD FAILED — DISCARD— but the shell wrapper collecting the results defaulted a missing failure message to the stringGREEN, so the first re-run printed five rows of "green" for five variants that never ran. The row-level red-proof above was regenerated after making the reporter require aTotal:line before it will call anything a pass. A harness that can print a pass it did not observe is the same defect as a pin that cannot fail, one level out, and it very nearly went into this description as a result.(c) is the honest one, and it changes what this PR claims
Containment is not what fixed the measured sites. Measured over the corpus: asking for the innermost containing member and asking for the nearest member declaration above the offset give the same answer at all 160 T-SQL sites, so dropping containment reds only the arranged control. What produced the 48 wrong labels was the pattern the predecessor matched with, and
DeclarationHeadis where that is fixed. Containment is hardening for the one direction the declaration regex cannot cover — a literal inside no member at all resolves to<unknown>rather than borrowing the name of whichever member sits above it. That is its status, and the code comment says so rather than claiming more.(d) is the one that earned the walker on measurement rather than principle
FactRiskDisclosure.QuoteNameis:Read raw, the
[and]inside those literals unbalance the bracket depth the declaration scan tracks, so it never sees a depth-zero;, runs to the end of the file and reports the body as unterminated. Six more members do the same. Prior art's arranged case was a brace in a literal; this is a bracket, it is on the shipped tree today, and it is not hypothetical.Two dead branches removed, which took measuring rather than reasoning
An earlier draft of
DeclaredNamealso required the(to follow the name with only whitespace between, and moved the name's end onto the>that closed a generic argument list so a generic method would still qualify. Neither could be made to red a test. Dumping all 20,308 declarations in the scanned trees with and without them and diffing gives zero differing lines — every name, every file. They were a pair that existed only to cancel each other out. Removed, and every branch left in that function is red-proofed by (l), (m) or (n).Verified, and not
dotnet build Darling/Darling.Tests/Darling.Tests.csproj -p:EnableWindowsTargeting=true: 0 errors, 50 warnings, all pre-existing xunit-analyzer nags in other files and unchanged in count. The eight pre-existing[Fact]s in this file ran green in the harness against the real tree at every step, which is the evidence that detection is untouched — this file goes 8 → 14.The harness is not the CI job.
Darling.Testsexecutes only on the Windowsbuildjob, and no WPF window was opened. Every claim here is about source as parsed.Review findings, both fixed
claude[bot]reviewed with empty review bodies and the substance in five inline comments (three of them the same finding, restated across two passes). Every finding was verified against the code rather than taken on report, and all are acted on. Replies on the threads themselves are pending Erik's go-ahead — the standing rule is that the first PR comment of a session is asked for first, and this session could not ask.1.
DeclarationEndtruncated a member's range before a trailing initializer — correct, and bigger than reported.The scan returned at the closing brace of the first top-level brace group. On an auto-property that group is the accessor list, so the range ended before the initializer, and a literal there was contained by nothing — labelled
<unknown>, which is the failure this PR exists to remove. Reproduced before fixing:Autoresolved withEndat the accessor list's}, the literal four characters later, andEnclosingMemberreturned<unknown>.Neither arm of
ShapeOfcan see it, and that is what made it worth fixing rather than documenting: a range that stops short of its own member is still closed (notUnterminated) and still well underNextStart(notOverExtended), so it reads asWholeMember. It is the one truncation with no detector — precisely the gap the two arms were introduced to close, in a shape they cannot reach.The report said the shape exists but no default is T-SQL today. Measured, the latent population is much larger than "a shape none of the fixtures exercise": 1,614 member ranges in the scanned trees were short by their initializer. Now that the scan carries on past an accessor list when — and only when — the next non-whitespace character is
=, all 1,614 end at their own terminating;. Nothing else moved: all 160 attributions are unchanged, no member becomes over-extended, and every range shape is stillWholeMember.AutoinTheResolver_AttributesByScope_…is the pin, red-proofed by variant (p).An
=is the only thing that can legally follow the brace inside the same declaration — after a method body's}the next non-whitespace is}, a modifier, or[— so the continuation cannot run into the following member.2. The same broken resolver in a sibling file — correct, and it is the one that was making a decision.
StoreSqlClockDisciplineTests.cscarried a byte-identical copy of the pre-fix regex, and its own doc comment said what for: "for the waiver key and the failure message". It feeds the resolved name intoWaived.Contains(file + ":" + member).That makes a wrong name there a wrong decision, not a wrong message. Three directions, and the third is the one that costs: it can report under the wrong member, it can miss a legitimate waiver and go red on waived code, or it can collide with an unrelated waived key and silently swallow a real bare-clock finding. My change fixed the message-only copy; leaving the deciding one and filing it is shipping the inert half of a defect, so it is fixed here.
The resolver now lives in
Darling/Darling.Tests/CSharpMemberMap.csand both pins use it — the treatmentCSharpSourceWalkergot in #2913 andRepoFilein #3090. Correcting the copy in place would leave two implementations of a thing that has already been shown to diverge, which is what those two extractions exist to stop.Measured before moving anything: 21 of the 140 SQL literals in the store tree change label —
if,using,while,Select(a LINQ call) andNpgsqlCommand(a constructed type) — and none of them is the one waived key.DarlingModuleMap.cs:RefreshSqlresolves toRefreshSqlunder both resolvers, because it is apublic const stringfield, the shape the old regex got right. So the extraction changes 21 labels for the better and no waiver decision at all.The wiring is shown to be live rather than merely compiled, which is #2213's lesson: variant (q) makes
EnclosingMemberalways return<unknown>, andNoStoreSqlComparesANaiveTimestampToABareClockreds withDarlingModuleMap.cs:<unknown> — collection_time >= now() - interval '2 days'. The waiver misses, the waived finding surfaces, and that suite fails by name. A green build proves nothing about a seam; that does.3. The eager
MemberMapOfin the scan — correct, and my own inconsistency.NoTsqlStatementViolatesACoveredConventionbuilt the map for every file in every scanned tree, including the great majority with no T-SQL in them, while the sibling test added in the same PR did it lazily. Nowmembers ??= MemberMapOf(text)at the point a finding needs it, which is the sibling's shape and the one I should have written.Out of scope, deliberately
<unknown>. Walking back over[…]blocks is ten lines, and nothing in the corpus would exercise it — a bound stated and left loud is worth more than machinery no test can distinguish, which is the lesson (h) above taught the hard way.McpQueryTools.GetTopQueriesByCpuis the near miss: its parameter attribute is inside the declaration and resolves fine; its method attribute would not, and is not in the population.PgPanelTabOwnershipTestsandPgRegistryPanelPlacementTestscarry narrower,LoadPg…Async-specific versions (Fixes #3088 #3089 declined the same extraction hours ago and noted the two had already diverged). They are message-only and neither drives a decision, which is what separates them fromStoreSqlClockDisciplineTestsabove — that one is fixed here precisely because it decides. Folding the Pg pair intoCSharpMemberMapis worth doing as its own change, against both at once, rather than reaching into files whose inline review anchors are current.ThePopulation_IsTsqlStatements_AndNothingElseadmits prose beginning with "If".McpQueryTools.cs:20is a[Description]reading "If true, only return queries whose cached plan has EVER run at DOP > 1…", which opens with a T-SQL statement keyword and names a DMV, so it satisfies both halves of the population rule. Harmless — it violates no covered convention — and filed as a side finding rather than fixed here, because narrowing the population is a scope decision for that test and not for this one.CHANGELOG
Not edited here — every lane appends to the same
[Unreleased]block and a per-PR edit conflicts with whichever sibling merges first. Entry text for the coordinator: