Skip to content

Fix QRY: InsertBatch and ToDiagnostics do not compile for consumers (#334) - #340

Open
DJGosnell wants to merge 11 commits into
masterfrom
334-insertbatch-internal-type
Open

DJGosnell wants to merge 11 commits into
masterfrom
334-insertbatch-internal-type

Conversation

@DJGosnell

Copy link
Copy Markdown
Member

Summary

InsertBatch(...) did not compile in any project outside this repository. Its generated interceptor
calls Quarry.Internal.BatchInsertSqlBuilder.Build(...), and that type was internal, so consumers
got error CS0122: 'BatchInsertSqlBuilder' is inaccessible due to its protection level in the
generated *.Interceptors.*.g.cs.

Issue #334 also asked for a recurrence guard: "a second instance would fail the same way." There
was one, and it was larger — see below.

Reason for Change

A generated interceptor is compiled into the consumer's assembly, so every Quarry type, method
and constructor it names is part of the public API contract whether or not it is documented as such.
Nothing enforced that.

The reason it went unnoticed is structural: all seven InternalsVisibleTo grants in
src/Quarry/Quarry.csproj cover every project in the solution — Quarry.Tests, Quarry.Benchmarks,
Quarry.Sample.WebApp, Quarry.Sample.Aot. No in-repo build has ever compiled generated
interceptors the way a consumer does.
The synthetic non-friend CSharpCompilation inside
Generation/InterceptorBindingGuardTests.cs (added by #314) is the only thing in the repository that
can observe this class of defect at all.

Impact

Two consumer-facing fixes, both of which made a documented feature unusable from NuGet:

Defect Symptom for a consumer Fix
Quarry.Internal.BatchInsertSqlBuilder was internal InsertBatch(...) — CS0122 type is now public
QueryDiagnostics' only constructor was internal ToDiagnostics() on any chain shape — CS1729 constructor is now public

The second was found by this PR's own guard on the first ToDiagnostics shape added to the matrix,
and is the wider of the two: llm.md documents ToDiagnostics() as available on every builder type
and "the primary tool for asserting generated SQL in tests". Three emitter sites construct it
(TerminalEmitHelpers.cs:615 — the general path every non-batch chain uses,
CarrierEmitter.cs:1095, TerminalBodyEmitter.cs:519).

It surfaced as CS1729, not CS0122, which is worth knowing: when a type's only constructor is
internal it is not an overload candidate at all outside a friend assembly, so the compiler reports
the arity rather than the protection level.

Fixing it here rather than deferring was an explicit decision (recorded in workflow.md): splitting
it out would have meant pinning a known-broken headline API out of the very guard being built.

Plan items implemented as specified

  • Promote BatchInsertSqlBuilder to public + [EditorBrowsable(Never)], matching the existing
    convention for the emitted surface (OpId, QueryExecutor, QueryLog, ParameterLog). No
    generator change — the emitted name was already correct, just unreachable.
  • Unpin InsertBatch interceptors reference internal BatchInsertSqlBuilder — CS0122 for any consumer outside InternalsVisibleTo #334 — the two InsertBatch shapes return to the clean-binding matrix and
    KnownBug_Issue334_BatchInsert_ReferencesInternalType is deleted.
  • A dedicated accessibility assertion, checked before the catch-all so a regression reads as
    "the emitter named a type consumers cannot reach" rather than "the fixture does not compile".
  • Broaden the matrix from 16 shapes on single-table chains to 33, across every emitter family:
    joins (inner/left), aggregates, correlated EXISTS subqueries, set operations, collection IN
    (both the IReadOnlyList and IEnumerable<T> arms), conditional masks, Prepare()/ToDiagnostics,
    window functions, CTEs and raw SQL.
  • Document the invariant in llm-testing.md and src/Quarry.Generator/llm.md.

Deviations from plan implemented

  • CTEs need no QuarryContext<TSelf>. The plan budgeted a separate context source and a refactor
    of Run on that premise. llm.md scopes the generic base to typed post-With accessors;
    FromCte<T>() works on the plain non-generic QuarryContext. Both were dropped as unnecessary.
  • RawSqlNonQueryAsync is never intercepted — only RawSqlAsync and RawSqlScalarAsync have an
    InterceptorKind. It emits nothing into the consumer's assembly, so there is no surface to guard
    and no shape for it. (llm.md lists all three together, which makes the asymmetry easy to miss.)
  • Raw-SQL interceptors are emitted into Quarry.Generated, not the context's namespace, so the
    fixture's InterceptorsNamespaces needed it. This matches an ordinary consumer more closely, not
    less — Quarry's shipped build targets (src/Quarry/build/Quarry.targets) register exactly it.
  • The window shape ships as Sql.RowNumber rather than the planned Sql.Rank with
    PartitionBy/descending; those two clauses remain unexercised.

Gaps in original plan implemented

  • Shape_StillReachesItsEmitter. Compiling clean and emitting an interceptor does not prove a
    shape reached the emitter it was added for. This earned itself on its first run: the collection
    shape as first written emitted none of the collection helpers while passing the binding matrix
    green. EveryShape_HasAnEmissionExpectation enforces that every shape declares what it must emit,
    so the coverage cannot silently rot.
  • AccessibilityGuard_DetectsAnInaccessibleType. The matrix is meaningful only if this
    compilation genuinely lacks friend access; if that quietly stopped being true every shape would
    keep passing while guarding nothing. The probe uses ScalarConverter, which is internal by design
    and stays that way. The guard was also verified by hand — temporarily reverting
    BatchInsertSqlBuilder to internal and watching both shapes fail with the new message.
  • Narrowed CS1729 classification. Classified as accessibility only when the quoted name is a
    type Quarry declares, so a genuine emitter arity bug — a defect class this matrix exists to
    catch — is still reported as one. Covered by tests including the negative case.
  • Multi-terminal shapes. Prepared_MultiTerminal's ToDiagnostics() half was compiled but never
    probed; Shape.AdditionalTerminals fixes that.
  • Insert(...).ToDiagnostics() — the third QueryDiagnostics construction site — had no shape.
  • Argument validation on both newly public entry points (see Security).

Migration Steps

None. All changes are strictly widening; no consumer action required. Consumers who previously could
not compile InsertBatch or ToDiagnostics() will find they now do.

Performance Considerations

None. No change to any emitted code path or runtime algorithm — the emitters were already producing
the correct calls. MaxParameterCount moves from const to static readonly, replacing a compile-time
inline with a static field read on a path that already builds a SQL string.

Security Considerations

Widening visibility grants consumers no capability they lacked — anything reachable through
BatchInsertSqlBuilder is reachable through RawSqlAsync already. Both newly public entry points
now validate their arguments rather than assuming a generated caller: Build null-checks
sqlPrefix, rejects non-positive entityCount/columnsPerRow, and computes the parameter product
in 64-bit so it cannot wrap negative past the MaxParameterCount ceiling; the QueryDiagnostics
constructor null-checks its three required arguments.

Breaking Changes

  • Consumer-facing — none. Every visibility change is additive: no removals, no signature changes,
    no behavioural change to any existing member.
  • Internal — MaxParameterCount is public static readonly rather than public const,
    deliberately: a public const is inlined into consumer assemblies at their compile time, which
    would freeze a value its own documentation calls "a conservative default". The QueryDiagnostics
    constructor is now frozen public API; its <remarks> states it is not supported and warns against
    binding to the signature, since new diagnostics fields are appended as optional parameters.

Testing

Quarry.Tests 3561 passed / 0 failed; Quarry.Migration.Tests 201 passed / 0 failed. Baseline
before this branch was 3501 / 201, both green. Manifest goldens unchanged throughout.

Generated interceptors for InsertBatch chains emit an unconditional call to
Quarry.Internal.BatchInsertSqlBuilder.Build(...) at TerminalBodyEmitter.cs:518
and :559, but the type was internal to the Quarry assembly. Any consumer
outside Quarry's InternalsVisibleTo list therefore failed to compile with
CS0122 in the generated *.Interceptors.*.g.cs — the feature did not work at
all for ordinary consumers.

Promote the type to public with [EditorBrowsable(Never)], matching the existing
convention for the emitted runtime surface (OpId, QueryExecutor, QueryLog,
ParameterLog). MaxParameterCount goes public alongside it since the now-public
Build documents that ceiling. No generator change is needed — the emitted name
was already correct, just unreachable.

Every project in this repo holds a friend grant, so no in-repo build modelled an
ordinary consumer and nothing caught this. Return the two InsertBatch shapes to
the clean-binding matrix in InterceptorBindingGuardTests, whose synthetic
CSharpCompilation is deliberately not a friend assembly, and drop the
KnownBug_Issue334 pin.

Refs #334
AssertBindsCleanly previously caught CS0122 only through its catch-all
"fixture does not compile cleanly" assertion, which reports the symptom rather
than the cause. Check the accessibility diagnostics explicitly and first, so a
regression reads as "the emitter named a type consumers cannot reach" and says
what to do about it.

CS0122 is the #334 call-site case; CS0050/CS0051/CS0053/CS0060 cover the same
defect surfacing through an emitted member's own signature.

Add AccessibilityGuard_DetectsAnInaccessibleType as a negative control. The
whole matrix is only meaningful if this compilation genuinely lacks friend
access to Quarry; if that silently stopped being true, every shape would keep
passing while guarding nothing — the exact blind spot that let #334 ship. The
probe references Quarry.Internal.ScalarConverter, which is internal by design
(called only from QueryExecutor, never emitted) and so stays a valid control.
That split is also why this cannot be a namespace convention: Quarry.Internal
holds both the public emitted surface and internal runtime-private helpers.

Extract CompileNonFriend so the control shares the matrix's exact references and
assembly name instead of a parallel setup that could drift.

Verified by temporarily reverting BatchInsertSqlBuilder to internal and watching
both BatchInsert shapes fail with the new message.

Refs #334
Adding a ToDiagnostics shape to the non-friend guard matrix showed that
ToDiagnostics() does not compile for any consumer, on any chain shape:

  error CS1729: 'QueryDiagnostics' does not contain a constructor that
                takes 23 arguments

QueryDiagnostics' only constructor was internal. Three emitter sites construct
it — TerminalEmitHelpers.cs:615 (the general path every non-batch chain uses),
CarrierEmitter.cs:1095, and TerminalBodyEmitter.cs:519 (batch insert) — so a
documented headline API was unusable outside Quarry's InternalsVisibleTo list.

Same root cause as the BatchInsertSqlBuilder defect, but it surfaced as CS1729
rather than CS0122: an inaccessible constructor with no accessible overload is
not a candidate at all, so the compiler reports the arity rather than the
protection level. Every sibling type the same emitted code constructs —
DiagnosticParameter, ClauseDiagnostic, SqlVariantDiagnostic,
ProjectionColumnDiagnostic, JoinDiagnostic, CollectionSqlCache — already has a
public constructor, so this restores consistency rather than widening the API.

Add both uncovered ToDiagnostics shapes: Projected_ToDiagnostics for the general
path and BatchInsert_ToDiagnostics for the batch path that the original #334 pin
never reached.

CS1729 is deliberately kept out of AccessibilityDiagnosticIds, since it usually
does mean a genuine emitter arity bug and mislabelling those would blunt the
matrix. The catch-all assertion instead notes that a CS1729 naming a Quarry type
may mean an internal constructor.

Refs #334
The matrix reached only single-table chains, so JoinBodyEmitter and the
GroupBy/Having assembly paths had no non-friend compilation coverage at all — an
internal type emitted on any of them would have shipped exactly the way #334 did.

Add OrderSchema and an Orders() accessor to the fixture's shared source, with FK
and navigation declarations mirroring Samples/OrderSchema.cs, then add four
shapes: inner join, left join (which additionally emits IsDBNull guards for the
nullable side), GroupBy/Having aggregate, and a correlated EXISTS subquery off a
Many<T> navigation.

Adding a second entity to the shared context regenerates output for every
pre-existing shape too; all 39 stay green.

Refs #334
…them

Add shapes for the emitter paths that call the remaining Quarry.Internal
helpers: set operations, collection IN (CollectionSqlCache, ParameterNames),
branched clauses (ThrowHelper.UnenumeratedMask), multi-terminal PreparedQuery,
and a window function in a projection.

Also add Shape_StillReachesItsRuntimeHelper. AssertBindsCleanly only proves a
shape compiles and that some interceptor was emitted for its terminal — it
cannot tell whether the shape exercised the emitter path it was added for. A
chain that silently stopped being analyzable would keep passing while guarding
nothing, which is indistinguishable from real coverage in a green run.

That test earned itself on its first run: the collection shape as first written
(an array literal with an entity terminal) emitted none of the collection
helpers despite passing the binding matrix. Rewriting it to the form
CollectionParameterCollisionTests uses fixed two of three, and the third turned
out to need a distinct shape — CarrierEmitter emits CollectionHelper.Materialize
only for collections typed IEnumerable<T>, since an IReadOnlyList is used
directly. Both arms are now covered.

Refs #334
RawSqlBodyEmitter is a separate emission path from the chain emitters, with its
own reader strategies, and had no non-friend compilation coverage.

Two things the fixture had to learn:

Raw-SQL interceptors are emitted into Quarry.Generated rather than the context's
namespace, so the fixture's InterceptorsNamespaces feature rejected them with
CS9137. Adding that namespace matches an ordinary consumer more closely, not
less — Quarry's shipped build targets register exactly it.

RawSqlNonQueryAsync is not intercepted at all: only RawSqlAsync and
RawSqlScalarAsync have an InterceptorKind, and the non-query overload is a plain
public method on QuarryContext. It emits nothing into the consumer's assembly,
so there is no emitted surface to guard and no shape for it. llm.md lists all
three together, which makes the asymmetry easy to miss.

Refs #334
The plan called for a dedicated QuarryContext<TSelf> context source and a
refactor of Run to support it, on the premise that CTEs require the generic
base. They do not: llm.md scopes that requirement to typed post-With accessors,
and FromCte<T>() works on the plain non-generic QuarryContext — Quarry.Tests'
own TestDbContext derives from it and CrossDialectCteTests drives CTEs through
exactly that. The shape goes onto the existing shared context and Run is
unchanged.

Refs #334
Record the rule the preceding commits enforce: generated interceptors compile
into the consumer's assembly, so every Quarry type, method and constructor they
name is public API whether or not it is documented as such — and no ordinary
build in this repo can catch a violation, because every project is a friend
assembly.

Both docs now name the two failure signatures, since the second is genuinely
unintuitive: CS0122 means an internal type was named, while CS1729 "does not
contain a constructor that takes N arguments" means that type's only constructor
is internal and therefore not a candidate at all — not an emitter arity bug.

Also record the maintenance rule that makes the matrix hold its value: add a
shape when adding an emitter path, and a RuntimeHelperExpectations entry with
it, or the shape can stop exercising its emitter and still pass green.

Refs #334
All 13 review findings addressed (10A/3B).

Runtime, following from making these members public:
- Build validates sqlPrefix, rejects non-positive entityCount/columnsPerRow,
  and computes the parameter product as long so it cannot wrap past the ceiling
- QueryDiagnostics' constructor validates its three required arguments; a null
  parameters list previously flowed into non-nullable properties
- MaxParameterCount is static readonly rather than const — a public const is
  inlined into consumer assemblies, freezing a value its own doc calls a
  conservative default
- The constructor carries the same "not supported API, may change without
  notice" disclaimer BatchInsertSqlBuilder already had, and warns against
  binding to a 23-parameter signature that grows

Guard matrix:
- Insert(...).ToDiagnostics() was the third QueryDiagnostics construction site
  and the only one still uncovered; it now has a shape
- Shapes may declare AdditionalTerminals, so multi-terminal shapes have every
  terminal probed. Prepared_MultiTerminal's ToDiagnostics half was compiled but
  never checked — the very path the internal ctor broke
- RuntimeHelperExpectations becomes ShapeEmissionExpectations, 9 entries to 33,
  covering every shape. Emitters with no distinctive helper pin an SQL or
  interceptor-header fragment instead, which makes the rule one every shape can
  follow. EveryShape_HasAnEmissionExpectation enforces it
- CS1729 is now classified as accessibility when the quoted name is a type
  Quarry declares — the form an internal constructor takes — while a CS1729
  naming a consumer type stays an arity bug. Replaces the hand-diagnosis hint
  with a tested classifier

Docs: llm.md no longer claims all seven emitted-surface types carry
[EditorBrowsable(Never)]; four predate the convention and are plain public.
Release notes staged for both fixes.

Refs #334

This branch has not been deployed

No deployments
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.

InsertBatch interceptors reference internal BatchInsertSqlBuilder — CS0122 for any consumer outside InternalsVisibleTo

1 participant