Skip to content

Feat: Per-file incremental generator output - #5

Merged
DJGosnell merged 6 commits into
masterfrom
feat/per-file-incremental-output
Mar 14, 2026
Merged

Feat: Per-file incremental generator output#5
DJGosnell merged 6 commits into
masterfrom
feat/per-file-incremental-output

Conversation

@DJGosnell

@DJGosnell DJGosnell commented Mar 14, 2026

Copy link
Copy Markdown
Member

Summary

  • Closes Feat: Per-File Incremental Generator Output #3
  • Restructures both Phase 1 (entity/context) and Phase 2 (interceptors) of QuarryGenerator so that a change in a single source file only regenerates the output files relevant to that file
  • Groups interceptors by execution chain instead of by invocation kind for more readable output
  • Output filenames use human-readable sanitized source paths instead of opaque hex hashes for easy identification

Reason for Change

Currently, .Collect() on all usage sites funnels into a single Execute() call that regenerates everything. Any change to any source file triggers full regeneration of all interceptor files — O(n) work for O(1) changes.

Impact

Phase 0 — Model value equality: All 25+ pipeline model types now implement IEquatable<T> with structural comparison. Added EquatableArray<T>, EquatableDictionary<TKey,TValue>, and EqualityHelpers for collection equality. Roslyn types (SyntaxNode, Location) are excluded from equality since they use reference identity.

Phase 1 — Per-context entity generation: Entity/context/metadata generation is registered directly on contextDeclarations (per-value, no Collect()). Each context is independently cached — changing one context no longer regenerates files for other contexts. Cross-context diagnostics (duplicate TypeMapping) use a separate collected pipeline.

Phase 2 — Per-file interceptor output: The interceptor pipeline uses SelectMany(GroupByFileAndProcess) to fan collected data into per-file FileInterceptorGroup objects, each independently cached by Roslyn. Output filenames use a sanitized source path tag: {Context}.Interceptors.{src_Models_User}.g.cs. Diagnostics are carried through the pipeline as DiagnosticInfo objects and reported in the output callback.

Chain-based grouping: Interceptors are now grouped by execution chain (#region Chain: ExecuteFetchAllAsync at line 42) instead of by InterceptorKind (#region Where Interceptors), so related clause and execution sites appear together.

Human-readable file tags: Replaced SHA256-based hex hashes with sanitized path tags (e.g. src_Models_User instead of a1b2c3d4). This makes it trivial to find the generated interceptor file for any given source file. The FileHasher utility strips extensions, drive letters, and leading slashes, then replaces path separators with underscores.

Migration Steps

  • Output filenames change from {Context}.Interceptors.g.cs to {Context}.Interceptors.{file_tag}.g.cs — this is transparent to consumers since interceptor files use file scoped classes
  • No API changes — all changes are internal to the generator

Performance Considerations

  • GroupByFileAndProcess still runs on every change (enrichment + chain analysis), but this is relatively cheap
  • The expensive string-building in InterceptorCodeGenerator only runs for files whose FileInterceptorGroup equality check fails
  • Projects with many source files using Quarry will see proportionally fewer regenerations

Security Considerations

  • No security impact — changes are internal to the compile-time source generator

Breaking Changes

  • Consumer-facing: None — interceptor files are file scoped and invisible to consumers
  • Internal: Test assertions updated for new output filename patterns and chain-based region names

🤖 Generated with Claude Code

DJGosnell and others added 3 commits March 13, 2026 21:59
Implement IEquatable<T> on all model types that flow through the
incremental generator pipeline. This is the foundation for per-file
incremental output — Roslyn's caching only works when types have
structural equality.

- Add EquatableArray<T> and EquatableDictionary<TKey,TValue> wrappers
- Add EqualityHelpers with sequence/dictionary comparison utilities
- Add HashCode polyfill for netstandard2.0 compatibility
- Implement IEquatable<T> on 25+ model types (bottom-up from leaves)
- Exclude Roslyn types (SyntaxNode, Location) from equality
- Add SyntacticExpression.DeepEquals for expression tree comparison

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restructure Initialize() to register entity/context/metadata generation
directly on contextDeclarations (per-value, no Collect). Each context is
now independently cached — changing one context no longer regenerates
entity/metadata files for other contexts.

- Extract GenerateEntityAndContextCode() for per-context output
- Extract CheckDuplicateTypeMappings() for cross-context diagnostics
- Simplify Execute() to only handle Phase 2 interceptor generation

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restructure the interceptor pipeline to emit one output file per
(context, source file) pair instead of one file per context. Roslyn
caches each file independently — changing one source file only
regenerates its interceptor output, not all interceptors.

Pipeline: SelectMany(GroupByFileAndProcess) → RegisterSourceOutput(EmitFileInterceptors)

- Add DiagnosticLocation, DiagnosticInfo, FileInterceptorGroup types
- Add FileHasher for stable output filenames
- Replace Execute()/GenerateInterceptors() with GroupByFileAndProcess()
  that enriches sites, analyzes chains, collects diagnostics, and fans
  out into per-file FileInterceptorGroup objects
- Add EmitFileInterceptors() that reports deferred diagnostics and
  delegates to InterceptorCodeGenerator per group
- Output filenames: {Context}.Interceptors.{hash}.g.cs
- Group interceptors by chain instead of by InterceptorKind for more
  readable output (related clause + execution sites together)
- Update all tests for new stableFileHash parameter and chain grouping

Closes #3

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@DJGosnell DJGosnell left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review: Per-File Incremental Generator Output

Overview

Restructures QuarryGenerator so a change in one source file only regenerates interceptor output for that file. Adds value equality to 25+ model types, splits entity generation into per-context output, and implements a SelectMany-based per-file pipeline with chain-based interceptor grouping.

Correctness

  1. FileInterceptorGroup.fileSites filter readability — The filter s.IsAnalyzable || !fileChainMemberIds.Contains(s.UniqueId) is a double-negative that's hard to parse. Since non-analyzable non-chain-members were already filtered out in allSitesForGeneration, s.IsAnalyzable alone would be equivalent and clearer.

  2. Orphaned diagnostic location fallback — When no SyntaxTree is available for orphaned diagnostics, Location.None is used. This is acceptable since it only happens for files with exclusively non-analyzable sites (rare), but worth documenting.

  3. EquatableArray<T> and EquatableDictionary<TKey,TValue> are created but unused — These types were created in Phase 0 but no model type uses them — all models use IReadOnlyList<T> with manual EqualityHelpers calls. Consider removing or adding a comment explaining they're infrastructure for future use.

Performance

  1. O(n×m) chain-to-site matching in InterceptorCodeGenerator — The chain grouping uses allSitesForGeneration.FirstOrDefault(s => s.UniqueId == clause.Site.UniqueId) inside nested loops. For large files this is O(chains × clauses × sites). Build a dictionary lookup instead:

    var siteByUniqueId = allSitesForGeneration.ToDictionary(s => s.UniqueId);
  2. EquatableArray.GetHashCode hashes all elements — For large arrays this runs on every equality check. Consider caching the hash or limiting to first N elements.

Style

  1. Diagnostic IDs as magic strings"QRY001", "QRY014", etc. are duplicated between DiagnosticInfo construction sites and GetDescriptorById. Consider using the descriptors' .Id property or defining constants.

Test Coverage

  1. Tests updated appropriately — All 8 test files updated for the new stableFileHash parameter and region naming.

  2. Missing tests — No dedicated tests for:

    • EquatableArray<T> / EquatableDictionary<K,V> equality semantics
    • FileHasher.ComputeStableHash determinism
    • EqualityHelpers edge cases
    • Incremental caching behavior (verifying cache hits/misses via GeneratorDriver tracked steps)

Recommendations

Priority Item
P1 Fix O(n×m) lookup in chain grouping (#4)
P2 Add unit tests for EqualityHelpers, FileHasher, and key model equality (#8)
P3 Remove unused EquatableArray/EquatableDictionary or document as future infra (#3)
P3 Replace magic diagnostic ID strings with descriptor references (#6)

Verdict

The architecture is sound — SelectMany fan-out is the standard Roslyn pattern for per-file incremental output. Equality implementations are thorough and correctly exclude Roslyn reference-equality types. Chain-based grouping is a nice readability improvement. Main concerns are missing test coverage for infrastructure types and the O(n×m) lookup.

DJGosnell and others added 3 commits March 13, 2026 22:59
Build a dictionary for chain-to-site matching instead of linear scans,
and use DiagnosticDescriptors.*.Id instead of hardcoded "QRY0xx" strings.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nd incremental caching

Covers EqualityHelpers edge cases (null, empty, reference equality),
FileHasher determinism and path normalization, EquatableArray/Dictionary
equality and collection semantics, and GeneratorDriver tracked-step
verification for per-file incremental output caching.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ames

Output files now use sanitized source paths (e.g. src_Models_User) instead
of hex hashes (e.g. a1b2c3d4), making it easy to find generated code for
a specific source file.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@DJGosnell
DJGosnell merged commit 70c7077 into master Mar 14, 2026
1 check passed
@DJGosnell
DJGosnell deleted the feat/per-file-incremental-output branch March 20, 2026 03:54
DJGosnell added a commit that referenced this pull request Apr 6, 2026
…ed diagnostics

Closes the pass #2 remediation work that began in WIP commit 61a0ee5. The
source-side fixes from that commit are unchanged; this commit adds the
diagnostic and coverage tests that pass #2 identified as gaps and folds in a
classification correction.

Source remediation (carried over from 61a0ee5; CI verified at 243034d):
- CteNameHelpers.ExtractShortName consolidated from two divergent local helpers
  in ChainAnalyzer and TransitionBodyEmitter. Strips both `global::` and
  namespace prefixes so global-namespace DTOs no longer trigger a silent
  captured-param drop (Correctness #1).
- QRY080 CteInnerChainNotAnalyzable + QRY081 FromCteWithoutWith dedicated
  diagnostic descriptors. ChainAnalyzer reports via `diagnostics?.Add(...)`
  instead of routing user-input errors through QRY900 InternalError
  (Codebase Consistency #4).
- ProjectionAnalyzer.BuildColumnInfoFromTypeSymbol EntityRef FK detection
  tightened to verify `ContainingNamespace == Quarry` and unwrap `Nullable<T>`
  so `EntityRef<X,Y>?` still resolves as a foreign key (Correctness #2/#3).
- CallSiteBinder single-context dialect fallback when ContextClassName is null
  AND AllContexts.Length == 1 (Correctness #4).
- IsCteInnerChain detection in FileEmitter now scans all clause sites instead
  of inspecting only ClauseSites[0] (Codebase Consistency #5).
- Documented cteInnerResults span-key uniqueness invariant and the multi-CTE
  first-match limitation in EmitCteDefinition (Correctness #5/#6, #206 tracks
  the multi-CTE work).
- TransitionBodyEmitter switched to short-name `Dictionary<,>?` parameter via
  `using System.Collections.Generic` (Codebase Consistency #2).

Tests added (this commit):
- Cte_With_NonInlineInnerArgument_EmitsQRY080: passes a field-reference inner
  argument to With<T>() so DetectCteInnerChain cannot classify it; asserts
  QRY080 fires and QRY900 does not (Test Quality #1).
- Cte_FromCte_WithoutPrecedingWith_EmitsQRY081: FromCte<T>() with no preceding
  With<T>(); asserts QRY081 fires and QRY900 does not (Test Quality #1).
- Cte_With_GlobalNamespaceDto_StripsGlobalPrefix: two-source compilation with
  a `global::GlobalOrderDto`; asserts the generated SQL constant uses the bare
  `GlobalOrderDto` name (verbatim-quoted form) and that the captured-param
  copy `Pn = __inner.P0` is emitted. Regression for the Correctness #1 silent
  failure mode.
- Cte_FromCte_AllColumns: identity FromCte<Order>().Select(o => o) across all
  4 dialects, vs the existing tuple-projection tests (Plan Compliance #1).

Test reclassification:
- Cte_FromCte_CapturedParam was marked Test Quality #2 (A) for "re-execute the
  SAME prepared chain after mutation". On investigation, PreparedQuery in this
  codebase is a SNAPSHOT at chain construction — the generated `Where_xxx`
  interceptor extracts the captured variable into the carrier P0 field at the
  `.Where()` call, before `Prepare()` runs, and there is no Bind/SetParameter
  API to re-execute the same instance with a new value. The lt2 pattern is
  the correct way to test "different captured value" semantics. The test is
  unchanged in behavior but the comment now documents the snapshot semantics.
  Reclassified A→D in review.md.

Test totals: 2782 main + 103 analyzer + 79 migration = 2964 passing.
Manifests regenerated to include the new Cte_FromCte_AllColumns chain.
DJGosnell added a commit that referenced this pull request Apr 6, 2026
…ed diagnostics

Closes the pass #2 remediation work that began in WIP commit 61a0ee5. The
source-side fixes from that commit are unchanged; this commit adds the
diagnostic and coverage tests that pass #2 identified as gaps and folds in a
classification correction.

Source remediation (carried over from 61a0ee5; CI verified at 243034d):
- CteNameHelpers.ExtractShortName consolidated from two divergent local helpers
  in ChainAnalyzer and TransitionBodyEmitter. Strips both `global::` and
  namespace prefixes so global-namespace DTOs no longer trigger a silent
  captured-param drop (Correctness #1).
- QRY080 CteInnerChainNotAnalyzable + QRY081 FromCteWithoutWith dedicated
  diagnostic descriptors. ChainAnalyzer reports via `diagnostics?.Add(...)`
  instead of routing user-input errors through QRY900 InternalError
  (Codebase Consistency #4).
- ProjectionAnalyzer.BuildColumnInfoFromTypeSymbol EntityRef FK detection
  tightened to verify `ContainingNamespace == Quarry` and unwrap `Nullable<T>`
  so `EntityRef<X,Y>?` still resolves as a foreign key (Correctness #2/#3).
- CallSiteBinder single-context dialect fallback when ContextClassName is null
  AND AllContexts.Length == 1 (Correctness #4).
- IsCteInnerChain detection in FileEmitter now scans all clause sites instead
  of inspecting only ClauseSites[0] (Codebase Consistency #5).
- Documented cteInnerResults span-key uniqueness invariant and the multi-CTE
  first-match limitation in EmitCteDefinition (Correctness #5/#6, #206 tracks
  the multi-CTE work).
- TransitionBodyEmitter switched to short-name `Dictionary<,>?` parameter via
  `using System.Collections.Generic` (Codebase Consistency #2).

Tests added (this commit):
- Cte_With_NonInlineInnerArgument_EmitsQRY080: passes a field-reference inner
  argument to With<T>() so DetectCteInnerChain cannot classify it; asserts
  QRY080 fires and QRY900 does not (Test Quality #1).
- Cte_FromCte_WithoutPrecedingWith_EmitsQRY081: FromCte<T>() with no preceding
  With<T>(); asserts QRY081 fires and QRY900 does not (Test Quality #1).
- Cte_With_GlobalNamespaceDto_StripsGlobalPrefix: two-source compilation with
  a `global::GlobalOrderDto`; asserts the generated SQL constant uses the bare
  `GlobalOrderDto` name (verbatim-quoted form) and that the captured-param
  copy `Pn = __inner.P0` is emitted. Regression for the Correctness #1 silent
  failure mode.
- Cte_FromCte_AllColumns: identity FromCte<Order>().Select(o => o) across all
  4 dialects, vs the existing tuple-projection tests (Plan Compliance #1).

Test reclassification:
- Cte_FromCte_CapturedParam was marked Test Quality #2 (A) for "re-execute the
  SAME prepared chain after mutation". On investigation, PreparedQuery in this
  codebase is a SNAPSHOT at chain construction — the generated `Where_xxx`
  interceptor extracts the captured variable into the carrier P0 field at the
  `.Where()` call, before `Prepare()` runs, and there is no Bind/SetParameter
  API to re-execute the same instance with a new value. The lt2 pattern is
  the correct way to test "different captured value" semantics. The test is
  unchanged in behavior but the comment now documents the snapshot semantics.
  Reclassified A→D in review.md.

Test totals: 2782 main + 103 analyzer + 79 migration = 2964 passing.
Manifests regenerated to include the new Cte_FromCte_AllColumns chain.
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Resume bookkeeping only — no code changes. Closes the review classification
loop by filling Action Taken for B items #3/#5/#6/#7 with references to
commit be224dd; adds session log entry covering the resume.
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Address review findings from _sessions/256-fix-sql-raw-select-projection/review.md:

- Fail-loud fallback (#3): AnalyzeProjectedExpression returns null for a
  methodName=="Raw" whose GetAggregateInfo returned null, preventing the
  generic type-info fallback from producing an empty-column ProjectedColumn
  that would regress the original #256 bug. AnalyzeInvocation emits a
  specific CreateFailed message for the single-column path.

- Captured-var typing (#4, #8, #16): RenderRawArgToCanonical tries
  ResolveScalarArgSql first for simple scalar args (identifiers, literals,
  captured members). This consults the semantic model for authoritative CLR
  types rather than the SqlExprParser default of "object", so a captured
  DateTime landed in ParameterInfo as "object" becomes "DateTime".

- Dialect-aware booleans (#5): FormatLiteralForProjection emits canonical
  {@boolt}/{@boolf} placeholders rather than per-dialect bool text.
  QuoteSqlExpression resolves the placeholder to TRUE/FALSE on PostgreSQL
  and 1/0 elsewhere. Because projection analysis runs at a fixed discovery
  dialect, emitting dialect-rendered text at analysis time would embed the
  wrong value in the cached SqlExpression.

- String-concat guard (#6): RenderRawArgNode's Add walker bails to null if
  either operand is a string/char typed literal or captured; the projection
  fails loudly rather than emitting "a + b" which is invalid on MySQL/SqlServer.
  Users who want string concat should write it in the template text.

- Validation refactor (#7): Replaced the throwaway RawCallExpr shell with a
  dedicated IsRawTemplateValid helper. Transient shell is now isolated to
  one helper with clear scope.

- T="object" guard (#21): TryExtractSqlRawTypeArg returns null rather than
  "object" when T is unresolvable. BuildSqlRawInfo then fails the projection
  loudly instead of letting ChainAnalyzer.TryResolveAggregateTypeFromSql
  misinfer a CLR type from SUM/MIN/MAX substrings in the user's template.

- Tests: Add Select_SqlRaw_BooleanLiteralArg_DialectAware (#5, #13) and
  Select_SqlRaw_CapturedVariable_TypeInferredFromSemanticModel (#4, #12, #14).

Review classifications + Action Taken recorded in
_sessions/256-fix-sql-raw-select-projection/review.md §Classifications.
All 3253 tests pass (3242 baseline + 11 Sql.Raw projection tests).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Review items addressed inline:
- #3 (B, Correctness): `CheckRowEntityMaterializability` now also rejects
  abstract classes and interfaces. CS0144 would otherwise fire against the
  generated `new T()` with no indication of which row type was at fault.
- #5 (B, Tests): adds a nested-row test driving the struct-reader fallback
  by using an expression SELECT list, exercising `SanitizeForIdentifier`
  and the FQN-in-`IRowReader<T>` path.
- #6 (B, Tests): strengthens the namespace-level-row regression to assert
  that `using TestApp.Rows;` IS emitted.
- #7 (B, Tests): adds a QRY044 test where `build_property.InterceptorsNamespaces`
  is entirely absent (null), confirming the diagnostic still fires.
- Plus QRY043 tests for abstract and interface row types.

Test totals: 3256 passing (+4 from Phase 5 green baseline).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Resume bookkeeping only — no code changes. Closes the review classification
loop by filling Action Taken for B items #3/#5/#6/#7 with references to
commit be224dd; adds session log entry covering the resume.
DJGosnell added a commit that referenced this pull request Apr 23, 2026
* Support Sql.Raw<T> in single-entity Select projections (#256)

Previously, Sql.Raw<T> used inside a Select tuple/DTO/object-init projection
silently rendered as an empty string literal in the generated SQL
(e.g., SELECT "OrderId", "" FROM "orders") because ProjectionAnalyzer fell
through to the generic fallback where columnName="" and the raw C# source
was placed in SqlExpression then later stripped.

Add a Raw case to GetAggregateInfo that parses each template argument via
SqlExprParser, walks the resulting SqlExpr tree with a projection-aware
renderer, and substitutes {0}/{1}/... template placeholders with canonical
{ColumnName} identifier placeholders (dialect-resolved later), @__proj{N}
parameter placeholders for captured runtime vars, and inline literals for
compile-time constants. Binary ops, unary ops, function calls, IS NULL, IN,
and LIKE expressions render inline so args like u.Price * 2 are supported.

The generic T type argument on Sql.Raw<T> determines the column CLR type;
template/arg count mismatches fail via RawCallExpr.Validate().

Covers tuple, DTO, object-initializer, and single-column projection forms
for single-entity Select (joined projections follow in a separate commit).

Task #1 of 3 in #256 workflow.

* Support Sql.Raw<T> in joined and single-column Select projections (#256)

Add a Raw case to GetJoinedAggregateInfo that resolves column references
to canonical {alias}.{ColumnName} placeholders using the per-parameter
lookup. The shared BuildSqlRawInfo helper and the SqlExpr tree walker from
Phase 1 are reused via a column-resolver delegate — only the column
resolution differs between single-entity and joined contexts.

Single-column projections (e.g., .Select(u => Sql.Raw<string>(...)))
already route through GetAggregateInfo via AnalyzeInvocation, so no
additional change is needed; joined single-column does the same via
AnalyzeJoinedInvocation → ResolveJoinedAggregate → GetJoinedAggregateInfo.

Add sanity tests for joined-tuple and single-column forms; the existing
column-reference sanity test from Phase 1 covers single-entity tuple form.

Task #2 of 3 in #256 workflow.

* Cross-dialect test coverage for Sql.Raw<T> in Select projections (#256)

Add 6 tests to complement the sanity tests from Phases 1+2, covering the
full surface of the fix:

- Multiple column references — Sql.Raw<string>("coalesce({0}, {1})", ...)
- Captured variable — verifies @__proj{N} local placeholders are remapped
  to dialect-specific parameter placeholders (@p0 / $1 / ?)
- Literal parameter — compile-time constant inlines as SQL literal
- No placeholders — template with no {N} substitutions passes through
- DTO initializer — new UserSummaryDto { ... = Sql.Raw<string>(...) }
- Binary op arg — u.UserId * 10 exercises the IR-based arg tree walker

All 9 tests (3 from Phases 1+2 + 6 added here) assert SQL output across
SQLite, PostgreSQL, MySQL, and SqlServer.

Task #3 of 3 in #256 workflow.

* Remediate review findings for Sql.Raw<T> Select projection (#256)

Address review findings from _sessions/256-fix-sql-raw-select-projection/review.md:

- Fail-loud fallback (#3): AnalyzeProjectedExpression returns null for a
  methodName=="Raw" whose GetAggregateInfo returned null, preventing the
  generic type-info fallback from producing an empty-column ProjectedColumn
  that would regress the original #256 bug. AnalyzeInvocation emits a
  specific CreateFailed message for the single-column path.

- Captured-var typing (#4, #8, #16): RenderRawArgToCanonical tries
  ResolveScalarArgSql first for simple scalar args (identifiers, literals,
  captured members). This consults the semantic model for authoritative CLR
  types rather than the SqlExprParser default of "object", so a captured
  DateTime landed in ParameterInfo as "object" becomes "DateTime".

- Dialect-aware booleans (#5): FormatLiteralForProjection emits canonical
  {@boolt}/{@boolf} placeholders rather than per-dialect bool text.
  QuoteSqlExpression resolves the placeholder to TRUE/FALSE on PostgreSQL
  and 1/0 elsewhere. Because projection analysis runs at a fixed discovery
  dialect, emitting dialect-rendered text at analysis time would embed the
  wrong value in the cached SqlExpression.

- String-concat guard (#6): RenderRawArgNode's Add walker bails to null if
  either operand is a string/char typed literal or captured; the projection
  fails loudly rather than emitting "a + b" which is invalid on MySQL/SqlServer.
  Users who want string concat should write it in the template text.

- Validation refactor (#7): Replaced the throwaway RawCallExpr shell with a
  dedicated IsRawTemplateValid helper. Transient shell is now isolated to
  one helper with clear scope.

- T="object" guard (#21): TryExtractSqlRawTypeArg returns null rather than
  "object" when T is unresolvable. BuildSqlRawInfo then fails the projection
  loudly instead of letting ChainAnalyzer.TryResolveAggregateTypeFromSql
  misinfer a CLR type from SUM/MIN/MAX substrings in the user's template.

- Tests: Add Select_SqlRaw_BooleanLiteralArg_DialectAware (#5, #13) and
  Select_SqlRaw_CapturedVariable_TypeInferredFromSemanticModel (#4, #12, #14).

Review classifications + Action Taken recorded in
_sessions/256-fix-sql-raw-select-projection/review.md §Classifications.
All 3253 tests pass (3242 baseline + 11 Sql.Raw projection tests).

* Record PR #262 in workflow state (#256)

* Back-step REMEDIATE → REVIEW for session 2 re-analysis (#256)

User declined to finalize PR #262 and requested a fresh full
review. Archived prior review.md to review-session1.md; workflow
phase reset to REVIEW.

* Make Sql.Raw projection walker fail loudly on unsupported args (#256)

Review session 2 findings #4, #8.

The projection-arg walker used to emit `SqlRawExpr.SqlText` verbatim
when it reached a node the parser could not translate. Since
`SqlExprParser` falls through to `new SqlRawExpr(expression.ToString())`
for C# ternaries, unknown invocations, postfix unary (other than `!`),
array-creation without initializer, element access, and interpolated
strings, any of those inside `Sql.Raw<T>(template, arg)` would leak the
C# source text — e.g. `/* unsupported: C# ternary expression */` or
`u.Foo(x)` — into the generated SQL. This reproduces the exact class of
silent-wrong-SQL bug this PR fixes. The walker now returns null for
`SqlRawExpr`, forcing the caller to bail out.

The string-concat Add guard now also checks `ColumnRefExpr` operands
via a new `isStringColumn` delegate threaded alongside the existing
column resolver. Previously only direct string literals and captured
string locals were caught, so `Sql.Raw<string>("{0}", u.FirstName + u.LastName)`
would still emit `("FirstName" + "LastName")` — invalid on MySQL and
SqlServer. The guard now bails on any string-typed operand: literal,
captured, or column.

Both `RenderRawArgToCanonical` and `RenderRawArgToCanonicalJoined`
receive an `isStringColumn` delegate that closes over the column
lookup for the current context; `IsStringColumnRef` and
`IsJoinedStringColumnRef` implement the two contexts.

* Propagate IsStaticField and ExpressionPath for Sql.Raw captured vars (#256)

Review session 2 findings #2, #9, #17.

`AddCapturedAsProjectionParameter` now mirrors the ParameterInfo
construction in `SqlExprClauseTranslator.ExtractParametersCore`
(SqlExprClauseTranslator.cs:88-92) so captured-variable metadata
propagates consistently across the Where-path and Select-projection
paths:

- `ExpressionPath = captured.ExpressionPath` — needed by the parameter
  binder to emit direct-path navigation code for deep captures like
  `obj.Inner.Field`.
- `IsStaticCapture = captured.IsStaticField` — controls whether
  UnsafeAccessor uses StaticField kind (null target) vs Field kind
  (func.Target).

Latent today: the walker path is only reached for captured variables
that are NOT direct identifiers or parameter-prefixed member accesses
(the fast-path in RenderRawArgToCanonical delegates those to
ResolveScalarArgSql). Remaining walker captures are rare (function-call
args, operator arms) and SqlExprParser never sets IsStaticField=true in
the current code. The fix aligns the projection path with the canonical
pattern so future parser changes or more complex Raw arg shapes don't
silently drop capture metadata.

* Tighten bool-literal detection and operator-table fallback (#256)

Review session 2 findings #6, #7.

`FormatLiteralForProjection` previously treated `"TRUE"`, `"true"`, and
`"1"` as truthy, but `SqlExprParser.ParseLiteral` only ever emits
`"TRUE"` or `"FALSE"` for bool literals (SqlExprParser.cs:270-274). The
extra checks were dead and added cognitive load — now only `"TRUE"`
maps to `{@boolt}`, everything else to `{@boolf}`.

`GetRawBinaryOperator` used to return `"?"` for any unmapped
`SqlBinaryOperator`, which is the MySQL parameter placeholder and
would parse into unrelated runtime behavior — the exact silent-
wrong-SQL failure mode this PR fixes. It now throws
`ArgumentOutOfRangeException`, so any future operator addition that
forgets to update the table will fail the build rather than emit
garbage SQL.

* Emit QRY029 for Sql.Raw template errors in Select projections (#256)

Review session 2 finding #1.

Where-path `Sql.Raw` calls surface template errors (placeholder/
argument count mismatch) as QRY029 compile-time errors via
`PipelineOrchestrator.CollectTranslatedDiagnostics`, because the raw
site's `Expression` is a `RawCallExpr` that runs `Validate()`. Select-
projection `Sql.Raw` calls were silent — `ProjectionAnalyzer.
BuildSqlRawInfo` validated via a transient `RawCallExpr` shell and, on
failure, discarded both the shell and the error, returning `(null, null)`.
The projection then failed analysis and the chain degraded to runtime
build, leaving no user-visible diagnostic.

`GetRawTemplateValidationError` now returns the `Validate()` message
(the same string the Where path shows). `BuildSqlRawInfo` records the
message via a thread-static accumulator modeled on `PipelineErrorBag`.
Public entry points in `ProjectionAnalyzer` drain the accumulator and
attach the messages to the returned `ProjectionInfo` via a new init-
only `SqlRawValidationErrors` property. `PipelineOrchestrator` emits
QRY029 per entry, scoped to the Select call's location.

Scope note: QRY029 is attached to the Select call location rather
than the individual `Sql.Raw` call site — less precise than the
Where path, but sufficient to locate the bad template in the lambda.

Added `ProjectionFailureReason.SqlRawValidationError` for future callers
that want to distinguish this degradation class; current pipeline
reads from `SqlRawValidationErrors` directly.

* Test remediation coverage for Sql.Raw Select projection (#256)

Review session 2 findings #11, #12, #14.

UsageSiteDiscoveryTests.cs (#11):
Three new QRY029 tests for projection-path Sql.Raw:
- Too many arguments
- Too few arguments
- Non-sequential placeholders ({0}, {2} skipping {1})
All assert QRY029 fires and the message matches the RawCallExpr.Validate
text, mirroring the existing Where-path tests.

CrossDialectMiscTests.cs (#12, #14):
- Select_SqlRaw_CapturedVariable_TypeInferredFromSemanticModel now also
  asserts Name, Value, IsCollection, IsEnum, and executes the query at
  runtime to verify the captured DateTime round-trips through the
  parameter binder (shallow-test remediation).
- Select_SqlRaw_Joined_MultipleArgs — joined projection with three
  args from a literal, u.UserName, and o.Status. Exercises
  ResolveJoinedColumnRefToPlaceholder for two distinct lambda params.
- Select_SqlRaw_Joined_WithCapturedVariable — joined projection with a
  captured int threshold in the Raw arg. Exercises
  IsScalarArgCandidateJoined and the fast-path delegation in the
  joined context.

Manifest regeneration reflects the three new cross-dialect queries.

* Share binary-operator table between SqlExprRenderer and Sql.Raw walker (#256)

Review session 2 finding #16 (scoped).

The Sql.Raw projection walker used to carry its own copy of the
SqlBinaryOperator → text table (`GetRawBinaryOperator`). Any future
operator addition would require updating both it and
`SqlExprRenderer.GetSqlOperator`, and any dialect-specific fix in the
renderer (e.g., the string-concat TODO at SqlExprRenderer.cs:290)
would bypass the walker. `GetSqlOperator` is now `internal` and
called directly from the walker.

The renderer's fallback was also tightened to throw
`ArgumentOutOfRangeException` rather than emit `"?"` — matching the
walker's stricter contract and preventing an unmapped operator from
silently rendering as the MySQL parameter placeholder.

Full consolidation of the remaining shape-level duplication
(IN/LIKE/IS NULL/function-call/unary emission) is out of scope: those
render paths in SqlExprRenderer invoke dialect-sensitive recursion
that the canonical walker must not enter. Tracked as a follow-up in
the walker's docstring: to share the rest, SqlExprRenderer would need
a canonical-projection output mode with delegate hooks for column
resolution, captured-value accumulation, and literal rendering.

* Record Action Taken for session 2 review findings (#256)

Populates the review.md Classifications table with a concise description
of how each A/B finding was addressed (or why D findings were left
alone). Session 2 remediation complete — ready to finalize.

* Save PR #262 body draft alongside session artifacts (#256)

* Update workflow.md for session 2 REMEDIATE completion (#256)

* chore: remove session artifacts before merge
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Review items addressed inline:
- #3 (B, Correctness): `CheckRowEntityMaterializability` now also rejects
  abstract classes and interfaces. CS0144 would otherwise fire against the
  generated `new T()` with no indication of which row type was at fault.
- #5 (B, Tests): adds a nested-row test driving the struct-reader fallback
  by using an expression SELECT list, exercising `SanitizeForIdentifier`
  and the FQN-in-`IRowReader<T>` path.
- #6 (B, Tests): strengthens the namespace-level-row regression to assert
  that `using TestApp.Rows;` IS emitted.
- #7 (B, Tests): adds a QRY044 test where `build_property.InterceptorsNamespaces`
  is entirely absent (null), confirming the diagnostic still fires.
- Plus QRY043 tests for abstract and interface row types.

Test totals: 3256 passing (+4 from Phase 5 green baseline).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Resume bookkeeping only — no code changes. Closes the review classification
loop by filling Action Taken for B items #3/#5/#6/#7 with references to
commit be224dd; adds session log entry covering the resume.
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Review items addressed inline:
- #3 (B, Correctness): `CheckRowEntityMaterializability` now also rejects
  abstract classes and interfaces. CS0144 would otherwise fire against the
  generated `new T()` with no indication of which row type was at fault.
- #5 (B, Tests): adds a nested-row test driving the struct-reader fallback
  by using an expression SELECT list, exercising `SanitizeForIdentifier`
  and the FQN-in-`IRowReader<T>` path.
- #6 (B, Tests): strengthens the namespace-level-row regression to assert
  that `using TestApp.Rows;` IS emitted.
- #7 (B, Tests): adds a QRY044 test where `build_property.InterceptorsNamespaces`
  is entirely absent (null), confirming the diagnostic still fires.
- Plus QRY043 tests for abstract and interface row types.

Test totals: 3256 passing (+4 from Phase 5 green baseline).
DJGosnell added a commit that referenced this pull request Apr 23, 2026
Resume bookkeeping only — no code changes. Closes the review classification
loop by filling Action Taken for B items #3/#5/#6/#7 with references to
commit be224dd; adds session log entry covering the resume.
DJGosnell added a commit that referenced this pull request Apr 23, 2026
* feat(generator): QRY043 diagnostic for un-materializable RawSqlAsync row types

Surface the real reason RawSqlAsync<T> row types fail to compile when T is a
positional record or has init-only properties. Previously authors saw cryptic
CS7036/CS8852 errors against generated code; now they see QRY043 naming their
type and the specific shape violation.

Detection runs in DisplayClassEnricher where the ITypeSymbol is already resolved
via the supplemental compilation. Emission is suppressed for affected sites so
QRY043 is the only error reported.

* feat(generator): support nested row-entity types in RawSqlAsync interceptors

Nested row types (row record declared inside an enclosing class) previously broke
the generator: it emitted `using <EnclosingType>;` which the compiler rejects
with CS0138. Fix tracks the nesting state on RawSqlTypeInfo and emits the
`global::`-prefixed fully qualified type name in generated bodies for nested
types so references resolve without a using directive. Namespace-level row
types still use their short name + using (unchanged codegen).

* feat(packaging): ship Quarry.targets auto-registering Quarry.Generated

Adds build/Quarry.targets to the Quarry NuGet package. It appends
Quarry.Generated to <InterceptorsNamespaces> so consumers no longer hit CS9137
for the Quarry-internal namespace they can't reasonably discover. Consumers
still add their own QuarryContext namespace — Phase 4's QRY044 analyzer
surfaces that gap at authoring time.

Also exposes InterceptorsNamespaces as a CompilerVisibleProperty so that
analyzer can read it from AnalyzerConfigOptions.

* feat(analyzers): QRY044 warns when QuarryContext namespace is missing from InterceptorsNamespaces

Surfaces the CS9137 project-setup gap at authoring time with the exact
<InterceptorsNamespaces> line to paste into the .csproj. Warning severity:
the build would fail with CS9137 anyway, so this is an earlier signal, not a
new error. Context classes in the global namespace are ignored because
Quarry.Generated (auto-registered by the shipped targets file) already
covers that path.

Descriptive diagnostic only — no CodeFixProvider, since the fix target is
the .csproj rather than a source document and standard Roslyn code fixes
can't reliably modify project files.

* docs: document row-entity shape requirements and interceptor opt-in behavior

Updates llm.md and Quarry.Generator/{README.md,llm.md} with:
- Revised InterceptorsNamespaces guidance: Quarry.Generated is now
  auto-registered by the shipped targets file, so the doc only prompts for
  the consumer's context namespace.
- New Row entity shape note under Raw SQL describing the parameterless ctor
  + public get/set property requirements, with QRY043 and the chain-query
  workaround for immutable shapes.
- Nested row types are explicitly called out as supported.
- QRY043 and QRY044 added to the diagnostic inventory tables.

* fix(generator): remediate review findings for #259

Review items addressed inline:
- #3 (B, Correctness): `CheckRowEntityMaterializability` now also rejects
  abstract classes and interfaces. CS0144 would otherwise fire against the
  generated `new T()` with no indication of which row type was at fault.
- #5 (B, Tests): adds a nested-row test driving the struct-reader fallback
  by using an expression SELECT list, exercising `SanitizeForIdentifier`
  and the FQN-in-`IRowReader<T>` path.
- #6 (B, Tests): strengthens the namespace-level-row regression to assert
  that `using TestApp.Rows;` IS emitted.
- #7 (B, Tests): adds a QRY044 test where `build_property.InterceptorsNamespaces`
  is entirely absent (null), confirming the diagnostic still fires.
- Plus QRY043 tests for abstract and interface row types.

Test totals: 3256 passing (+4 from Phase 5 green baseline).

* chore: record PR #260 in workflow.md + add pr-body.md session artifact

* docs: include abstract class and interface rejection in QRY043 docs

The REMEDIATE-phase extension of CheckRowEntityMaterializability added two
additional rejection cases (abstract classes and interfaces) that weren't
reflected in llm.md, src/Quarry.Generator/README.md, or
src/Quarry.Generator/llm.md. Brings the guide text and diagnostic inventory
tables in line with the actual QRY043 behavior.

* chore(session): record session 2 resume and populate review Action Taken

Resume bookkeeping only — no code changes. Closes the review classification
loop by filling Action Taken for B items #3/#5/#6/#7 with references to
commit be224dd; adds session log entry covering the resume.

* chore(session): back-step REMEDIATE -> REVIEW for re-analysis

* fix: remediate second-round review findings (A/#4, B/#1/#7/#8/#9)

Addresses review.md classifications from session 2:
- A/#4 Correctness: extend HasQuarryContextAttributeSyntactic with an
  AliasQualifiedNameSyntax branch so [global::QuarryContextAttribute]
  and extern-alias attribute forms pass the syntactic pre-filter.
- B/#1 Plan: drop RawSqlTypeInfo.FullyQualifiedResultTypeName — written
  but never read; ResultTypeName already carries the final display form
  (FQN for nested, short for not).
- B/#7 Tests: add alias-qualified attribute QRY044 coverage.
- B/#8 Tests: add struct-with-init-only-property QRY043 coverage.
- B/#9 Tests: add mixed RawSqlScalarAsync<int> + failing RawSqlAsync<T>
  test exercising the scalar branch in PipelineOrchestrator + FileEmitter.

3259 tests pass (+3 new).

* docs(pr): update PR body for second remediation round (A/#4, B/#1/#7/#8/#9)

* chore(session): log rebase on origin/master (4daf62d)

* chore(session): log second rebase on origin/master (e4354a4)

* chore(session): log third rebase on origin/master (95c4bbb + d2a6b1e)

* chore: remove session artifacts before merge
DJGosnell added a commit that referenced this pull request Apr 25, 2026
#266)

* Add Testcontainers helper + Npgsql parameter-binding regression probe (#258)

Phase 1 of the redux fix. Adds Testcontainers.PostgreSql 4.* to
Quarry.Tests and a lazy, process-wide PostgreSqlContainer helper so every
integration test routes through a single container instance.

Moves the DESIGN-phase empirical probe into NpgsqlParameterBindingTests
as long-term regression documentation. Five tests (A–E) encode against a
real Npgsql 10 + PG 17 container which ParameterName/SQL configurations
Npgsql accepts, proving that:
 - @pn SQL + @pn name works (rewrite path),
 - $N SQL + @pn name fails (the v0.3.0 state — original #258),
 - $N SQL + $N name fails (the v0.3.1/0.3.2 state — PR #261),
 - $N SQL + empty/unset name works (native positional binding, the
   configuration Phases 2–3 will make Quarry emit on PostgreSQL).

* Return empty ParameterName for PostgreSQL in SqlFormatting (#258)

Phase 2 of the redux fix. SqlFormatting.GetParameterName now returns
string.Empty on PostgreSQL instead of "$N+1". This is what makes
Npgsql 10 use native positional binding against the $N placeholders that
FormatParameter already emits — the only configuration the Phase 1
probe proved works on Npgsql 10.

Also fixes MigrationRunner.AddParameter and Quarry.Tool's
MigrateCommands.AddParameter, both of which route DbParameter.ParameterName
through GetParameterName — this is what closes the original #258 bug
observed in v0.3.0 and re-surfaced in v0.3.1 / v0.3.2.

Tests:
 - DialectTests.GetParameterName_ReturnsNameForDbParameter: PostgreSQL
   TestCases flip from "$1"/"$6" to ""/"".
 - DialectTests.GetParameterName_MatchesFormatParameter_ForNamedDialects:
   PostgreSQL dropped — it is no longer a name-binding dialect from
   Quarry's perspective; the new invariant is "PG ParameterName is empty".
 - DialectTests.GetParameterName_IsAlwaysEmpty_ForPostgreSQL: new
   regression guard.

* Emit empty ParameterName for PostgreSQL in generator (#258)

Phase 3 of the redux fix. The three generator sites that assign
DbParameter.ParameterName now route PostgreSQL through the empty-string
literal, matching what SqlFormatting.GetParameterName returns for PG at
runtime:

 - CarrierEmitter.FormatParamName         → ""  (compile-time constant)
 - CarrierEmitter.EmitParamNameExpr       → "\"\""  (C# literal "")
 - TerminalBodyEmitter batch-insert       → "\"\""
 - TerminalEmitHelpers diag param-name    → "\"\""

The SQL-text side is untouched: FormatParameter still emits $N on PG,
TerminalEmitHelpers.EmitCollectionPartsPopulation still builds $N string
arrays for IN-clause expansion, and BatchInsertSqlBuilder still uses $N.

This makes every generated entity-insert, batch-insert, WHERE-clause,
ORDER-BY, LIMIT/OFFSET, and diag parameter path on PG ship DbParameter
instances with empty ParameterName — the configuration the Phase 1 probe
proved works on Npgsql 10.

CarrierGenerationTests regression guards flipped:
 - EntityInsert on PG: asserts `__pN.ParameterName = ""` (both empty and
   does-not-match $N or @pn patterns for defense in depth).
 - BatchInsert on PG: asserts `__p.ParameterName = ""` (not .Dollar,
   not .AtP).

ParameterNames.Dollar remains in Quarry.Internal — still used to build
$N SQL text for IN-clause placeholder arrays.

* Upgrade QueryTestHarness.Pg to real Npgsql connection (#258)

Phase 4 of the redux fix. QueryTestHarness.Pg now attaches to a real
NpgsqlConnection against the shared Testcontainers PG 17 container
instead of MockDbConnection. My and Ss stay on the mock.

Isolation strategy is transactional by default:
 - PostgresTestContainer.EnsureBaselineAsync() creates the shared
   quarry_test schema, DDL, and seed data exactly once per test process.
 - Each CreateAsync() opens a fresh pooled NpgsqlConnection,
   SET search_path TO quarry_test, BEGIN.
 - DisposeAsync() ROLLBACKs, closing the connection back to the pool.
 - Near-zero per-test overhead (full suite: +2s for 2990 tests).

Tests that need their own schema — migration runner tests,
transaction-behavior tests, anything that issues its own BEGIN/COMMIT —
pass useOwnPgSchema: true to CreateAsync(). That path calls
CreateOwnedSchemaAsync which creates a test_<guid> schema with the
full DDL + seed, sets search_path, and DROP SCHEMA CASCADEs on dispose.

PG DDL port of the SQLite harness:
 - Primary keys use GENERATED BY DEFAULT AS IDENTITY (seed with explicit
   IDs works; subsequent auto-INSERTs auto-generate).
 - REAL → DOUBLE PRECISION (PG's REAL is single-precision, insufficient
   for the money columns).
 - Identifiers stay double-quoted (case-sensitive, matching the source).
 - DiscountedPrice computed column translates 1:1 to PG 12+ syntax.
 - CREATE VIEW "Order" for test-harness table-name aliasing.
 - IDENTITY sequences are advanced past max(seeded_id) after seed so
   first auto-INSERT doesn't collide.

Full Quarry.Tests suite: 2990/2990 passing (unchanged from pre-upgrade
count; the harness change is transparent to existing diagnostic-path
tests, and no test in the current suite depends on Pg mock-capture
behavior that the real connection would break).

* Add focused PG integration tests + fix MigrationRunner DateTime type (#258)

Phases 5–7 combined. Adds the four focused integration tests that drive
every generator + runtime code path PR #261 touched, all against the real
Npgsql 10 + PG 17 container:

 - Quarry.Tests/Integration/PostgresIntegrationTests.cs:
   - EntityInsert_OnPostgreSQL_ExecutesSuccessfully
     (CarrierEmitter.EmitCarrierInsertTerminal)
   - InsertBatch_OnPostgreSQL_ExecutesSuccessfully
     (TerminalBodyEmitter batch path)
   - WhereInCollection_OnPostgreSQL_ExecutesSuccessfully
     (TerminalEmitHelpers.EmitCollectionPartsPopulation)
 - Quarry.Tests/Migration/PostgresMigrationRunnerTests.cs:
   - RunAsync_InsertsHistoryRow_OnPostgreSQL — closes the original #258
     scenario end-to-end. Uses a per-test fresh PG schema because
     MigrationRunner issues its own BEGIN/COMMIT, incompatible with
     QueryTestHarness's outer transactional rollback.

These tests surfaced a second pre-existing PG bug in MigrationRunner
that was hiding behind the parameter-binding bug: InsertHistoryRowAsync
passed `DateTime.UtcNow.ToString("o")` (a string) for the `applied_at`
and `started_at` parameters. SQLite's TEXT column tolerated this, but
PG's `TIMESTAMP NOT NULL` column rejected it with
`42804: column "applied_at" is of type timestamp without time zone but
expression is of type text`. Fix: bind the DateTime directly and let
Npgsql/Microsoft.Data.Sqlite choose the correct wire type. SQLite
continues to pass (Microsoft.Data.Sqlite serialises DateTime to TEXT
transparently).

Phases 6 and 7 absorbed into this commit:
 - Phase 6 (helper dedup) is unnecessary: MigrationRunner lives in the
   Quarry package, not Quarry.Migration, so the migration-runner test
   belongs in Quarry.Tests alongside the existing SQLite one. No
   cross-project helper linking needed.
 - Phase 7 (cross-dialect PG triage) produced no work items: the full
   Quarry.Tests run after the harness upgrade passes with only the
   single MigrationRunner DateTime bug surfaced above. All 3312 tests
   green across Quarry.Tests / Quarry.Migration.Tests / Analyzers.Tests.

Note on IQueryBuilder<T> entity-terminal signature mismatch: the
Addresses/Warehouses reads use an explicit `.Select(x => x.Field)`
projection because the entity-terminal fallback has an unrelated
interceptor signature mismatch (CS9144) that is out of scope for this
fix. Using projected terminals keeps the parameter-binding coverage
clean without fighting that unrelated issue.

* Tighten RawSqlAsync docs around Npgsql binding modes (#258)

Phase 8 of the redux fix. PR #261 left comments in QuarryContext.cs
that asserted "Npgsql 10 strict binding requires the placeholder and
the name to agree" — a framing that's not quite right and that led the
rest of PR #261 astray. The empirical probe in NpgsqlParameterBindingTests
shows that Npgsql switches between named- and positional-binding modes
based on whether any DbParameter has a ParameterName set, not based
on what CommandText contains.

RawSqlAsync keeps its @pn + @pn pairing. Users write @pn placeholders;
the runtime binds ParameterName = @pn. On PostgreSQL Npgsql rewrites
the @pn placeholders to native positional — the same code path that
works today. The refreshed XML doc + anchor comment now explain why
mixing conventions (@pn in SQL + empty name, or $N in SQL + @pn name)
would flip Npgsql into the wrong mode, and why the chain-API path
(which emits $N + empty name) is a different contract that Quarry
controls end-to-end.

* Address review findings (#258)

REMEDIATE phase. Nine review findings classified A fixed together:

#4 (critical) — CarrierEmitter.cs:690 collection-parameter expansion
now emits `__pc.ParameterName = ""` on PostgreSQL. Pre-fix it reused
the `__colNParts` array (which holds `$N` strings for SQL text) as
ParameterName, flipping Npgsql back into named-lookup mode — the exact
v0.3.1/0.3.2 "C" failure configuration for any `.Contains(collection)`
where the collection is not a compile-time constant. SQLite / SqlServer
paths continue to use `__colNParts[__bi]` as the name (they bind by
name).

#5 (medium) — QueryTestHarness.CreateAsync wraps all setup in
try/catch and unwinds partial state on throw: rolls back the PG tx,
drops the owned schema if created, disposes Sqlite/Npgsql/Mock
connections. Previously a transient mid-setup exception leaked all
three connections until process exit.

#6 (low) — PostgresMigrationRunnerTests.TearDown now logs the
failure message to TestContext.Out instead of swallowing silently,
so accumulating orphan schemas are diagnosable without masking the
test result.

#11 (high) — WhereInCollection_OnPostgreSQL_ExecutesSuccessfully now
sources the array from a helper method so SqlExprAnnotator's
constant-inlining pass cannot fold it to literal SQL. Verified by
inspecting the generated interceptor: the `__col0Len` runtime loop
IS emitted, and this is the test that would have caught #4.

#13 (medium) — Added CarrierGeneration_WhereInCollection_*
generator-level regression guards for PG (empty ParameterName)
and SQLite (preserves the __colNParts[__bi] assignment).

#16 (low) — PostgresIntegrationTests changed from `internal class`
to `public class` to match the other two new PG test fixtures.

#20 (info) — PostgresTestContainer.EnsureBaselineAsync is now
safe across concurrent test processes sharing one container: probes
whether the baseline tables exist before doing any DDL, and gates the
critical section with a PostgreSQL advisory lock. No drop-and-recreate.

#25 (medium) — PostgresTestContainer.GetContainerAsync now catches
Docker-unavailable exceptions (heuristic on type-name / message for
"Docker", "Testcontainers", "daemon", "named pipe"), caches the
failure reason, and calls Assert.Ignore with a clear "Install Docker
to run the Quarry test suite" message. Developers without Docker see
a clean Ignored result for every PG-backed test instead of cascading
exceptions.

Full suite: 2996 Quarry.Tests + 201 Quarry.Migration.Tests + 117
Quarry.Analyzers.Tests = 3314 passing.

Rebuild requirement (#21) is surfaced in the PR body — not a code
change.

* Record PR #266 in session log

* Record PR #266 in workflow

* [WIP] Phase 9 bootstrap: OrderBy Pg mirror + NUMERIC DDL bug identified

Suspending mid-Phase-9 for handoff. Full state in
_sessions/258-fix-npgsql-parameter-naming-redux/handoff.md.

Working tree:
 - CrossDialectOrderByTests.cs: 4 tests mirrored with pg execution
 - PostgresTestContainer.cs: doc comment updated to describe NUMERIC(18,2)
   for decimal columns, but the DDL still emits DOUBLE PRECISION (the bug)
 - workflow.md: status=suspended, phase=IMPLEMENT, phase-9 session log entry

Verified failure:
  CrossDialectOrderByTests.OrderBy_Joined_RightTableColumn on Pg:
  'Reading as System.Decimal is not supported for fields having
  DataTypeName double precision'

Next session should (a) fix decimal columns to NUMERIC(18, 2), (b) re-run
OrderBy tests green, (c) roll the pattern across the other ~25 files
smallest-first per handoff.md.

PR #266 is not affected by this WIP — it is rebased + CI-green on an
earlier commit and remains mergeable today.

* Fix decimal column DDL in PG test harness (NUMERIC vs DOUBLE PRECISION) (#258)

PostgresTestContainer.CreateSchemaObjectsAsync emitted DOUBLE PRECISION
for every decimal-backed column (orders.Total, order_items.UnitPrice/
LineTotal, accounts.Balance/credit_limit, products.Price/DiscountedPrice)
because the SQLite source schema used REAL for all non-integer numerics.

PG's DOUBLE PRECISION is IEEE 754 binary float (= .NET double); Npgsql
refuses GetDecimal on a double-precision column ("Reading as 'System.Decimal'
is not supported for fields having DataTypeName 'double precision'").

Production DDL is unaffected: Quarry/Migration/SqlTypeMapper.cs already
maps decimal -> numeric(p,s) on PostgreSQL. Bug was contained to the test
harness port from SQLite.

Unblocks Phase 9 Pg-execute mirror in CrossDialectOrderByTests.OrderBy_
Joined_RightTableColumn (decimal tuple assertions) and any later test that
reads a decimal column on Pg.

Tests: 2996 / 2996 (Quarry.Tests) - no regressions.

* Phase 9: mirror Pg execution across all CrossDialect tests (#258)

Lite-only `await lt.ExecuteXxxAsync()` blocks now mirrored with the
parallel `await pg.ExecuteXxxAsync()` block applying the same assertions
to the Pg result, across 22 CrossDialect*Tests files in src/Quarry.Tests/
SqlOutput (~233 mirror blocks added). This closes the test-coverage gap
that hid PR #261's Npgsql parameter-naming regression: cross-dialect
tests previously only verified SQL-string shape on Pg, never executed
through the real provider.

Test-harness DDL alignment with `Quarry.Migration.SqlTypeMapper`
(`PostgresTestContainer.CreateSchemaObjectsAsync`):
- Col<bool>          INTEGER     -> BOOLEAN          (+seed 1/0 -> TRUE/FALSE)
- Col<DateTime>      TEXT        -> TIMESTAMP
- Col<DateTimeOffset>TEXT        -> TIMESTAMPTZ
- FOREIGN KEYs from SQLite source dropped: SQLite does not enforce FKs
  by default; replicating them in PG breaks delete-tests that legitimately
  depend on no FK enforcement.

Three PG-vs-Lite divergences triaged inline:
- `Select_Distinct`: pgResults sorted by UserId before assertions; PG
  does not guarantee insertion-order return without ORDER BY.
- `Join_Distinct_OrderBy_Limit`: Pg execution intentionally not mirrored;
  PG rejects DISTINCT with ORDER BY on a non-projected column (42P10),
  which SQLite tolerates. SQL-text assertion still verifies generator
  output is identical across dialects.
- `Cte_TwoChainedWiths_DistinctDtos_CapturedParams`: workaround for an
  unrelated Quarry source-generator bug in chained-With dispatch
  (variable renamed `orderCutoff` -> `cutoff` so the closure shape
  matches Chain_3's expected fields). Bug to be filed as a follow-up
  issue at REMEDIATE; out of scope for #258.

`quarry-manifest.postgresql.md` regenerated by the source generator to
reflect the expanded Pg execution surface.

Tests: 2996 + 201 + 117 = 3314, all green.

* Reference follow-up issues #267 and #268 in Phase 9 triage comments

Issues filed:
- #267 — Generator emits non-portable SELECT DISTINCT + ORDER BY on PG (42P10).
  Skipped Pg execution in Join_Distinct_OrderBy_Limit; comment now points to
  the tracking issue and notes the re-enable condition.
- #268 — Source generator chained-With dispatch resolves wrong closure-field
  extractor by structural shape. Comment in Cte_TwoChainedWiths_DistinctDtos
  _CapturedParams now points to #268 and notes that "cutoff" should be
  reverted to "orderCutoff" once the generator is fixed.

workflow.md decision log updated with the issue numbers.

* Add #267/#268 references to pr-body.md (PR #266 body already pushed)

* Archive issue bodies for #269 (MySQL) and #270 (SQL Server)

* Add row-order sort helper + apply at 11 Pg-execute sites (REMEDIATE A/B)

Phase 9 review surfaced a latent flake hazard: 11 cross-dialect tests
asserted on `pgResults[N]` indexed positions without an explicit
`ORDER BY` in the chain. PG does not guarantee row order from a base
scan or join without ORDER BY; the suite passes today only because PG
happens to return small-table sequential-scan results in heap order.
A planner change (statistics, parallel scan, hash join chosen for a
CTE) would surface as flake in CI.

Fix:
- New `src/Quarry.Tests/PgRowOrderExtensions.cs` exposes
  `Task<List<T>>.SortedByAsync(keySelector)`. Materialises and sorts
  the result before assertion. Centralises the rationale comment so
  future test authors get the explanation in one place.
- Helper is an extension on Task<List<T>> (not PreparedQuery<T>) so
  the Quarry chain analyzer (QRY036) still sees `.ExecuteFetchAllAsync()`
  as the literal terminal at the end of the prepared chain.

Sites updated (11):
- CrossDialectSelectTests.Select_Distinct (replaces inline `.OrderBy(...).ToList()`)
- CrossDialectNavigationJoinTests.NavigationJoin_Where_ExecutesCorrectly
- CrossDialectNavigationJoinTests.NavigationJoin_GroupBy_Navigation
- CrossDialectNavigationJoinTests.NavigationJoin_DeepChain_ExecutesCorrectly
  (sorts by (ProductName, UserName) since the projection has no ID
  column; pg-expected values updated to alphabetic-tuple order;
  lite-expected values stay in insertion order — same asymmetric
  pattern as Select_Distinct)
- CrossDialectCteTests (7 sites):
  - Cte_FromCte_CapturedParam
  - Cte_FromCte_AllColumns
  - Cte_TwoChainedWiths_DistinctDtos_CapturedParams
  - Cte_ThreeChainedWiths_AllUsedDownstream
  - Cte_TwoChainedWiths_FirstEmptySecondCaptured_CapturedParam
  - Cte_FromCte_DedicatedDto

Closes review findings #4 (medium, A) and #13 (low, B). Tests:
2996/2996 green.

* Update pr-body.md with REMEDIATE pass + dual-review summary

* chore: remove session artifacts before merge
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feat: Per-File Incremental Generator Output

1 participant