Skip to content

Type-based APIs become an explicit non-goal, and say so (DAP056) - #214

Merged
mgravell merged 1 commit into
mainfrom
type-apis-nongoal
Sep 11, 2026
Merged

mgravell merged 1 commit into
mainfrom
type-apis-nongoal

Conversation

@mgravell

@mgravell mgravell commented Aug 26, 2026

Copy link
Copy Markdown
Member

Rebased onto main (2026-09-11): the parity-notes commit this used to carry landed separately as #213, so it is dropped here and only the DAP056 commit remains. The parity rows are unchanged in intent — they now build on #213's "cite the generated report" framing rather than restating it.

Also picked up from #216, which landed in between: the two new TypeBasedApis goldens were written on Windows and carried \ in their [InterceptsLocation(...)]; they are normalized to / like the rest, so the fixture passes off Windows too.

Dapper's Type-argument overloads pick the row type at execution time, which is the one thing compile-time generation cannot follow. Supporting them means a build-time registry of candidate types plus runtime dispatch keyed on Type — a lookup on the hot path, an open world for the trimmer, and dispatch ILC cannot resolve unless every announced type is rooted. That is the shape #206 was closed over; rebuilding it for a lower-value feature would be incoherent.

It costs no behaviour. These already generated nothing — a five-call probe handled 0 of 5. What changes is what we say: DAP056 now fires at the call-site and names the generic overload, replacing either silence or a confusing DAP009 about an unexpected type parameter.

Detection is call-site sensitive, deliberately

GetRowParser<T>(concreteType: null) — the default, and the common case — is perfectly supportable; only a call that actually passes a Type defers the decision. So the check looks at the arguments, not the symbol. GetRowParser<T>() stays supported, and the fixture pins both halves:

handled 2 of 7 enabled call-sites (0 unsupported API, 5 refused with diagnostics, 0 skipped silently)

GetRowParser(concreteType) goes too

It is the one row where "use the generic form" is not available advice, because the choice is data-dependent — so docs/rules/DAP056.md gives the pattern that is actually correct under AOT: a switch over GetRowParser<T>() per candidate type. More code, and strictly better, because it roots exactly the types you use where a registry roots everything registered.

Reported from the generator, not the analyzer

Most of these overloads carry no sql string, so the analyzer never sees them (the reason 7 of them were in the silent bucket). SkippedSourceState gains a reason enum — plain data, so the cached model stays equatable and ModelShapeTests stays happy — and Generate emits from it.

Knock-on effects

  • the surface report grows a non-goal: Type-based bucket for overloads whose Type parameter is required, so silent drops 27 → 22 and undiagnosed-unsupported 13 → 12; the mute total goes 40 → 34;
  • GetRowParser's existing fixture goes from 1 silent skip to 0;
  • the optional-Type overload stays in its previous bucket, because a symbol cannot say what a call will pass — the report's stated bound;
  • parity.md marks the affected rows 🚫 with the decision recorded, and its "announced types" item shrinks to cover only untyped parameters: it is no longer a design that unlocks several rows.

Suites green on net8.0 (369) and net48 (362); solution builds clean.

Decided 2026-08-26. Dapper's Type-argument overloads pick the row type at
execution time, which is the one thing compile-time generation cannot follow.
Supporting them means a build-time registry of candidate types plus runtime
dispatch keyed on Type - a lookup on the hot path, an open world for the
trimmer, and dispatch ILC cannot resolve unless every announced type is rooted.
That is the shape #206 was closed over; rebuilding it for a lower-value feature
would be incoherent.

Costs no behaviour: these already generated nothing (a five-call probe handled
0 of 5). What changes is what we say. DAP056 fires at the call-site and names
the generic overload, replacing either silence or a confusing DAP009 about an
unexpected 'type' parameter.

Detection is call-site sensitive, not symbol-level, because
GetRowParser<T>(concreteType: null) - the default, and the common case - is
perfectly supportable. Only a call that actually passes a Type defers the
decision. GetRowParser<T>() stays supported and the fixture pins both halves.

GetRowParser(concreteType) goes too, on Marc's call. It is the one row where
"use the generic form" is not available advice - the choice is data-dependent -
so the rule doc gives the pattern that is actually correct under AOT: a switch
over GetRowParser<T>() per candidate, which roots exactly the types used rather
than everything registered.

Reported from the generator rather than the analyzer: most of these overloads
carry no `sql` string, so the analyzer never sees them. SkippedSourceState
gains a reason enum (plain data, so the cached model stays equatable) and
Generate emits from it.

Knock-on: the surface report grows a 'non-goal: Type-based' bucket for overloads
whose Type parameter is required, so the silent count drops 27 -> 22 and
undiagnosed-unsupported 13 -> 12. GetRowParser's fixture goes from 1 silent skip
to 0. The optional-Type overload stays where it was, since the symbol cannot
say what a call will pass.
@mgravell
mgravell merged commit d275906 into main Sep 11, 2026
2 checks passed
mgravell added a commit that referenced this pull request Sep 11, 2026
`state-of-play.md` is the "read first after a break" page and was the most wrong
thing in the repo: it still listed #206/#207/#208 as in flight with a landing order
to follow, and knew nothing of #209-#216. It now says what is true - nothing of ours
in flight, the four open PRs are all external and all awaiting a decision.

It also absorbs the account of **why #206 was closed**, which was sitting unmerged on
the `typehandler-registration-note` branch and existed nowhere in main: gating stopped
the bridge emitting for consumers who did not opt in, but the read-side check still
sat in `RowFactory.GetValue<T>` - the type-flexible arm of every mapped member - so
everyone paid for a feature almost nobody would enable. The emission-time fix that
was considered and rejected is recorded with it, since that is the part most likely
to be re-proposed.

Two things promoted, because they gate real work and were buried:

- the behavioural harness is local-only and does **not** exist on this machine (no
  `aot-harness` branch, no SQL Server, Windows-shaped repack recipe), so no phase-3
  round can be closed and 677/793 cannot be re-measured until it is rebuilt;
- net48 is unverified since #216 and #214, both of which changed interceptor goldens.

Corrections: #117 and #162 are closed, not awaiting a decision; the `type-handler`
branch harvest was filed as "before #208 settles", and #208 has settled; diagnostic
ids as actually shipped (DAP053-056, next free DAP057).

`parity.md`: the `CommandDefinition` row still said "27 overloads, every one skipped
silently" - #214 moved the `Type`+`CommandDefinition` combinations out, so it is 21
of the 22 in that bucket. Cites the report rather than restating a number, which is
what #213 was for.

Also: `typehandlers-design.md` never existed in main - it was a file on the closed
#206 branch - leaving dangling links in three notes. Pointed at the note that did
land, `typehandler-registration.md`.
@mgravell
mgravell deleted the type-apis-nongoal branch September 11, 2026 13:34
mgravell added a commit that referenced this pull request Sep 11, 2026
The job already runs on windows-latest and *builds* net48, so a compile break there
would be caught - but the only test steps are net8.0 and net10.0. That gap matters
specifically because the `*.output.netfx.*` interceptor goldens are compared at
**test** time: a netfx golden can be wrong while CI stays green, which is exactly the
state #214, #216 and #220 have left main in (all three touched those files, and none
of them could be exercised on Linux).

One more step, same job, same image, same filter.
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.

1 participant