Weld concatenated literals before scanning store SQL, and make the PostgreSQL parse-check population what the reader files declare - #3223
Conversation
…r a bare clock StoreSqlClockDisciplineTests scanned one string literal BODY at a time, so a predicate assembled across a C# concatenation was split for the scan: a bare now() on one side of a + with the naive collector column on the other is a comparison in neither body, and the suite reported clean on it. The scan now reads maximal runs of literals welded by concatenation, rendered as the string the runtime builds - nothing between a bare +, a single space where the glue carries an expression whose value is unknown, which is the substitution CSharpSourceWalker already makes for an interpolation hole. Measured on the corpus: 815 runs weld two or more literals and 8 are units no member body was SQL-shaped enough to reach the discriminator at all. Four of those 8 are the config_alert_log reads in DarlingAlertReader and ViewerDataService.AlertHistory - the site class this pin's own remarks name as its reason for existing. All four bind their bounds and are clean; none had ever been read. The offender count is unchanged at 1 (the arithmetic-waived DarlingModuleMap RefreshSql), so the change adds reach without adding a false positive. That holds with #3216's three composed TouchAndProbeSql sites in the tree, which this scan now welds and reads for the first time.
DarlingPgReadSqlParsesLiveTests discovered reads through f.IsLiteral, so a static readonly string composed by concatenation was outside the population: DarlingPgColumnStatsReader.CoverageEvidenceSql, a shipped read, had never been parse-checked. Discovery now reads const and static readonly alike. Its anti-vacuity floor was 10 against a doc claim of "twelve constants across nine readers" that had gone stale: measured, 28 reader types ship 49 reads, so the floor tolerated a 39-read drop. Replaced by three clauses that fail differently - a ratchet at the live population, a per-type clause (one reader ships 9 reads, so no slack total can see a type emptying), and a census of what the reader SOURCE declares, which is the only denominator from outside reflection and so the only one that sees a read leave reflection's reach while its declaration stays in the file. The census runs without a server, so it covers the Windows build job too. The old floor lived inside the DARLING_TEST_PG-gated test, which meant nothing checked the discovery on half the CI matrix.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Reviewed. Scope is entirely Darling.Tests source-scanning infrastructure ( Traced the new Left one inline comment on |
The census compared source-declared fields against reflection using Path.GetFileNameWithoutExtension as the type name, and its comment justified that with a 1-file-per-type invariant nothing asserted. CONTRIBUTING.md endorses partial classes for large files, so splitting DarlingPgFooReader.cs would key half its fields DarlingPgFooReader.Extra.SomeSql, match nothing, and fail LOUDLY for a field reflection and the parse check both cover. Reproduced: with DarlingPgTrendReader made partial and an ExtraProbeSql added in DarlingPgTrendReader.Extra.cs, file-stem keying reds naming DarlingPgTrendReader.Extra.ExtraProbeSql; keyed by declaring type the same split is green and the read count rises to 50. Attribution now runs through CSharpMemberMap, so it walks with CSharpSourceWalker like the rest of the source pins and picks the INNERMOST enclosing type - a reader file also declares its row types. A field that cannot be attributed asserts as an attribution failure rather than as an uncovered read, because reporting it as the latter sends the next reader to the wrong file.
|
Reviewed. This is test-only infrastructure inside I traced the two changed guards in detail ( One maintainability finding posted inline: Non-blocking observation: several other source-scanning pins still read |
The census resolved a field's declaring type with a private copy of EnclosingMember's containment scan, differing from it only by a DeclarationKind - a third copy of the shape #2913 consolidated from five and #3094 fixed a half-corrected copy of. It was not even a faithful copy. EnclosingMember escalates an unterminated body (End < 0) to Unknown rather than guess; the copy treated one as running to EOF and would have attributed a field to a type the brace walk had lost. Inert today (0 unattributed either way), and exactly the quiet divergence between two copies that #3094 was about. CSharpMemberMap now exposes EnclosingType beside EnclosingMember, both delegating to one private Enclosing(map, offset, kind), so the End < 0 judgement is made once. Proved behaviour-preserving for the six pins that already call EnclosingMember: the pre-refactor body and the delegated call agree at all 673,093 probed offsets across 2,229 files - every literal start and end, every declaration boundary, and a coarse sweep besides. DeclarationKind's doc said types are "excluded from attribution", which EnclosingType makes untrue; it now says excluded from MEMBER attribution.
|
Reviewed. This is test-only ( Lite/Darling parity: No gap. Security: None found — no new file/network/process boundaries, and the regex/reflection work operates only over the repo's own known-good source tree at test time. Correctness: Left one inline comment on Otherwise the welding logic ( 🤖 Generated with Claude Code |
…nter it The census matched `const string X =` anywhere in the stripped source. C# allows a LOCAL const string, reflection only ever sees static fields, so a local added to any reader method would sit in `missing` permanently - and the only way to quiet it would be putting a local into NotQueryFields, which is for fields holding no query rather than for what the pattern over-matched. The scan now walks the declarations CSharpMemberMap already found and asks which of them are static string fields, instead of pattern-matching positions of its own. DeclarationHead requires an access modifier, which a local cannot carry, so anchoring is what excludes them. Reproduced: with `const string UnrelatedLocal = "not a query";` added inside DarlingPgXminReader's read method, the free pattern reds naming DarlingPgXminReader.UnrelatedLocal; anchored to declarations the same local is clean. The population is unchanged either way - 52 declared, 49 discovered, the 3 remaining being exactly the NotQueryFields entries - so anchoring costs no reach.
ReviewScope: this PR only touches I traced the two new/changed algorithms by hand against the PR's own fixtures:
No correctness bugs, parity drift, or security issues found. Two very minor, non-blocking observations:
Nice work on the fixture coverage for the "must-not-weld" shapes — the two " |
1 participant
Two source-scanning guards each reported a clean pass over a population that had a real thing outside it. Neither weakness came from #3216; both are pre-existing, and both are the shape where the guard's own output is indistinguishable from success.
1.
StoreSqlClockDisciplineTestsscanned per literal BODY, so a concatenated predicate was split for the scanThe pin flags store SQL comparing a naive collector timestamp against a bare clock function, because PostgreSQL resolves the mixed comparison at the store session's TimeZone — a documented one-hour predicate spanning five under
America/New_York. It iteratedCSharpSourceWalker.StringLiteralBodiesone body at a time, so a statement assembled as@"… WHERE collection_time < " + Bound + @"now() - interval '1 hour'"put the column in one body and the clock in another. The comparison is in neither, and the second body is not even SQL-shaped, so it never reached the discriminator at all.The scan now reads maximal runs of literals welded by C#
+concatenation, rendered as the string the runtime builds: nothing between a bare+(which contributes nothing, so inserting a space would invent a token boundary in a statement merely split across source lines), and a single space where the glue carries an expression whose value is unknown — the substitutionStringLiteralBodiesalready makes for an interpolation hole, for the same reason.Reachability, measured, not argued. 815 runs in the corpus weld two or more literals, and 8 are units no member body was SQL-shaped enough to reach the discriminator. Four of those eight are the
config_alert_logreads inDarlingAlertReaderandViewerDataService.AlertHistory— the exact site class this pin's own remarks name as its reason for existing ("two of the sites this pin was written for are alert reads and an alert that returns no rows never fires"). All four bind their bounds and are clean. None had ever been read.No false positive. Offenders are 1 before and 1 after (the arithmetic-waived
DarlingModuleMap.RefreshSql). That holds with #3216's three composedTouchAndProbeSqlsites in the tree, which this scan now welds and reads for the first time — confirming they are inert rather than taking it on trust.The
LooksLikeSqlremarks that stated this limit are replaced rather than left to contradict the code.What is deliberately not welded, because welding it would invent a predicate the runtime never builds: separate statements, separate arguments or collection elements, separate blocks, and the two arms of a conditional. Two tests do that, and each is load-bearing on a shape the other clears:
+at both ends of the gap. 2,175 gaps have one; the two the alternative "gap contains a+" rule would additionally weld are real corpus shapes (DarlingMcpPgWraparoundTools'&&Rank(multi)>Rank(xid)?worst+), and one of them is the fixture for this clause.… + x; var b = y + …has a+at both ends and is two statements; 10 corpus gaps are that shape, inComposeCompilerandDarlingNetworkConfigEditor— files whose business is composing SQL, where a false weld could invent an offender.Depth is why the separator test is a walk and not a
Contains: a comma inside the glue's own call (+ DarlingToolExitCode.Diagnose(exitCode, exePath) +, four times here) is one concatenation, and a depth-blind rule splits it for no gain.Nesting is respected — a literal inside an interpolation hole welds only with its own siblings, and 144 bodies are nested that way.
The DDL column scrape reads the same welded units. Measured, that changes nothing today (57 names either way, because every rung's DDL is one literal); it is there so the two sites cannot disagree about what a literal is, which is how the split went unnoticed.
2.
DarlingPgReadSqlParsesLiveTestshad a floor of 10 against a population of 49, and one shipped read was outside itThis is the suite that
PREPAREs every shipped PostgreSQL read against a real server. The defect it exists for (#2554: an ambiguousLEFT JOINcolumn, 42702, throwing on every call for months while a dozen text assertions passed) is invisible to every other instrument, so a read that drops out of this population loses its only parse check.The population claim was wrong. Its doc said "twelve constants exist today across nine readers". Measured: 28 reader types ship 49 reads. A floor of 10 against 49 tolerated a 39-read drop — 79% of the population could leave while the guard reported a clean pass.
And one read was already outside. Discovery filtered on
f.IsLiteral, and astatic readonly stringcomposed by concatenation is not a literal.DarlingPgColumnStatsReader.CoverageEvidenceSql— a shipped read, executed at every column-stats coverage call, interpolating a status list and two collector thresholds — had never been parse-checked. Discovery now readsconstandstatic readonlyalike; that read passesPREPAREagainst PostgreSQL 17.11, verified rather than assumed.The single floor is replaced by three clauses that fail differently:
DarlingPgTrendReaderalone ships 9, so no total with slack can see one type emptying.const string/static readonly stringdeclared in the Storage project'sDarlingPg*Reader*.csfiles must be in the parse-checked population or in a named, per-siteNotQueryFieldsset. This is the only clause whose denominator comes from outside reflection, so it is the only one that sees a read leave reflection's reach while its declaration sits in the file — a type renamed off the pattern, a field moved, a field kind the filter does not read. A stale allowlist entry reds too.NotQueryFieldsholds three: two SQL fragments interpolated into a query, andDarlingPgTableBloatReader.StaleStatisticsChurnRatioSql, which is spelled like a query and holds"0.2"— the trap that makes any name-shaped rule useless here.The census runs without a server. The old floor lived inside the
DARLING_TEST_PG-gated test, so on the Windowsbuildjob nothing checked the discovery at all.Census keying: by declaring type, not file stem
The census originally keyed source-declared fields by
Path.GetFileNameWithoutExtension, and its comment justified that with a 1-file-per-type invariant nothing asserted.CONTRIBUTING.mdendorses partial classes for large files, so splittingDarlingPgFooReader.cswould key half its fieldsDarlingPgFooReader.Extra.SomeSql, match nothing, and fail loudly for a field that reflection and the parse check both cover — a false failure in a pin whose whole subject is guards that report the wrong thing.Reproduced both ways. With
DarlingPgTrendReadermade partial and anExtraProbeSqladded inDarlingPgTrendReader.Extra.cs:DarlingPgTrendReader.Extra.ExtraProbeSql"Attribution runs through
CSharpMemberMap, so it walks withCSharpSourceWalkerlike the other source pins, and takes the innermost enclosing type — a reader file also declares its row types, so nearest-declaration-above would get it wrong. A field that cannot be attributed asserts as an attribution failure rather than as an uncovered read, because reporting it as the latter sends the next reader to the wrong file.One containment scan, not a third copy
Resolving the declaring type was first done with a private copy of
CSharpMemberMap.EnclosingMember's containment scan, differing only by aDeclarationKind— a third copy of the shape #2913 consolidated from five and #3094 fixed a half-corrected copy of.It was also not a faithful copy.
EnclosingMemberescalates an unterminated body (End < 0) toUnknownrather than guess; the copy treated one as running to EOF and would have attributed a field to a type the brace walk had lost. Inert today (0 unattributed either way), and precisely the quiet divergence between two copies that #3094 was about.CSharpMemberMapnow exposesEnclosingTypebesideEnclosingMember, both delegating to one privateEnclosing(map, offset, kind), so theEnd < 0judgement is made once.DeclarationKind's doc claimed types are "excluded from attribution", whichEnclosingTypemakes untrue; it now says excluded from member attribution.Proved behaviour-preserving for the six pins that already call
EnclosingMember: the pre-refactor body and the delegated call agree at all 673,093 probed offsets across 2,229 files — every literal start and end, every declaration boundary, and a coarse sweep besides.EnclosingTyperesolves to a type at 633,459 of those offsets, so it is not degenerate.The census pattern is anchored to a declaration
It first matched
const string X =anywhere in the stripped source. C# allows a localconst string, and reflection only ever sees static fields — so a local added to any reader method would sit inmissingpermanently, and the only way to quiet it would be putting a local intoNotQueryFields, which is for fields that hold no query rather than for what the pattern over-matched.The scan now walks the declarations
CSharpMemberMapalready found and asks which of them are static string fields, instead of pattern-matching positions of its own.DeclarationHeadrequires an access modifier, which a local cannot carry, so anchoring is what excludes them.Reproduced with
const string UnrelatedLocal = "not a query";added insideDarlingPgXminReader's read method:DarlingPgXminReader.UnrelatedLocal"The population is unchanged either way — 52 declared, 49 discovered, the 3 remaining being exactly the
NotQueryFieldsentries — so anchoring costs no reach.Mutation evidence
Baseline on this branch, xUnit v3 in-process runner over both guard files against live PostgreSQL 17.11 (
timescale/timescaledb:latest-pg17,initdb -U darlingas CI does):Total: 5, Errors: 0, Failed: 0, Skipped: 0, Not Run: 0.+weakened to contains-a-++rendering invents a token boundaryconstonly (=devbefore this PR)DarlingPgColumnStatsReader.CoverageEvidenceSqlCoverageEvidenceSqlPREPAREd and not merely countedDarlingPgXminReader.PgXminHorizonSqlconst→ plainstatic)EnclosingTypefiltersMemberinstead ofTypeThree results are green by design, and each says something:
const string→static readonly string(the shape this PR was briefed on)Every mutation applied by byte-level replace with a
count == 1anchor, verified changed by sha256, rebuilt, run, restored, restore confirmed by sha256. Tree ended byte-identical to HEAD on all five touched files.Two mutations in an earlier pass did not compile (a type rename and a field deletion break their callers); both were replaced by shapes that isolate the same coverage loss and do compile, rather than reported as passes.
Three of my own controls were originally not load-bearing and the battery is what showed it: the separator test, the both-ends test and the nesting logic each had every fixture cleared by a different clause. The fixtures that now discriminate them are modelled on real corpus gaps, and the nesting case was rewritten as a composition assertion after the findings-based version turned out to be flagging pre-existing walker behaviour rather than anything the grouping did.
Scope
Test-only; no product code changes.
Darling.Testsbuilds on macOS withEnableWindowsTargetingand each commit builds independently. The suites themselves cannot run on macOS, so both guard files were compiled and executed through a scratchnet10.0xUnit project that links the real source files —[CallerFilePath]therefore resolves to the real tree and the scans read the real corpus. CI is the arbiter for the rest of the suite.