Skip to content

Say something at the mute overloads (DAP057), and open 1.1 - #220

Merged
mgravell merged 2 commits into
mainfrom
diagnose-mute-overloads
Sep 11, 2026
Merged

mgravell merged 2 commits into
mainfrom
diagnose-mute-overloads

Conversation

@mgravell

@mgravell mgravell commented Sep 11, 2026

Copy link
Copy Markdown
Member

Gap #0 from parity.md — the cheapest safety win on the list, and the one that most deserves to be in a release.

The problem

34 public overloads told the consumer nothing at all. They work under JIT — the call-site just stays on vanilla Dapper — and fail after a native-AOT publish, with no build-time signal anywhere. That is the shape behind issues #112, #158 and #165.

Every one is CommandDefinition-shaped, because the analyzer only inspects call-sites carrying SQL as a string argument and these hide it inside the struct. Two groups, two answers:

  • 21 are operations we do support, in a spelling we cannot read → new DAP057, reported from the generator (the analyzer never sees them), naming the method and pointing at the overload that takes the SQL directly;
  • 12 are unsupported in any spelling — ExecuteReader, multi-map, QueryMultipleDAP001 from the generator, the same id their visible siblings already get from the analyzer, so both spellings behave alike.

Severity follows PublishAot

Leaving a call-site on vanilla Dapper is a missed optimization under JIT and a latent publish-time crash under native AOT. Same fact, very different stakes — so DAP057 reports at the severity that matches:

JIT project:                  info DAP057: 'Execute' passes its SQL inside a CommandDefinition…
PublishAot=true:           warning DAP057: 'Execute' passes its SQL inside a CommandDefinition…

The SDK sets EnableAotAnalyzer when PublishAot is on and it is compiler-visible by default, so this needs no build props of our own — PublishAot itself is not compiler-visible, which is why that property stands in. Proved end to end against the packed package: a net10.0 project reports info at default verbosity, and the same project with PublishAot=true reports warning.

The point: an upgrade does not bury a JIT project in new warnings (and cannot break anyone's TreatWarningsAsErrors), while an AOT project is told about every call-site that will not survive publishing.

Numbers

Surface report — gains an unsupported: CommandDefinition bucket and collapses unsupported API (undiagnosed), since nothing is undiagnosed any more. Silent skips 34 → 1. The survivor is a reporting artifact rather than a gap: GetRowParser<T>(concreteType) does different things depending on whether a given call passes the Type, which a symbol-level report cannot say (the bound #214 documented). Call-sites there get DAP056 or are handled, correctly.

Corpus (round 15 rig, net10.0, local SQL Server):

before after
skipped silently 25 0
refused with diagnostics 203 228
handled 432 of 736 432 of 736
suite 729 / 800 729 / 800

Behaviour is untouched — this adds diagnostics and nothing else. Unit suites: 380/380 on net8.0 and net10.0.

Also here: opening 1.1

version.json moves to 1.1 with versionHeightOffset: -1, so the first build is 1.1.0 rather than 1.1.1. Confirmed by packing: Dapper.AOT.1.1.0-g.nupkg.

DAP001 follows PublishAot too (second commit)

Originally left out as a separate decision; folded in, because leaving it made the PR incoherent — an AOT project would be warned about Execute(CommandDefinition) but not about QueryMultiple, which fails identically after publish.

It turned out to matter more than tidiness. Info-severity analyzer diagnostics do not surface in a normal dotnet build — only in IDEs and at raised verbosity. So DAP001 was effectively being emitted into a void, and the surface report's unsupported API (diagnosed) bucket was true on paper and silent in practice. Proof — the same three call-sites, before and after the promotion:

Program.cs(11,58): warning DAP001   <- QueryMultiple(CommandDefinition)   [generator]
Program.cs(13,70): warning DAP001   <- QueryMultiple("...")               [analyzer]
Program.cs(15,60): warning DAP001   <- Query<int,int,int>("proc", ...)    [analyzer]

As Info, none of those three appeared at all. So this is what makes the whole unsupported surface visible to the people it will hurt, rather than just the CommandDefinition slice.

Blast radius, Dapper suite built with the AOT flag set (MSBuild double-lists, so halve): 152 DAP001 + 50 DAP057 — about 76 and 25 real call-sites, every one of which fails after publish. A JIT consumer sees no change whatsoever.

12 lines across three files: the TargetsNativeAot plumbing already existed, the analyzer just needed an AnalyzerOptions overload, and DAP001 has only two report sites. No golden churn — the test harness sets no MSBuild properties, so goldens stay info.

Verification

Unit suites 380/380 on net8.0 and net10.0. Corpus unchanged at 729 / 800 with both commits in — this PR adds diagnostics and changes no behaviour.

34 public overloads told the consumer nothing at all. They work under JIT - the
call-site just stays on vanilla Dapper - and fail after a native-AOT publish, with
no build-time signal anywhere. That is the shape behind issues #112, #158 and #165.

Every one of them is CommandDefinition-shaped, because the analyzer only inspects
call-sites carrying SQL as a string argument and these hide it inside the struct.
Two groups, two answers:

- 21 are operations we *do* support, in a spelling we cannot read: new **DAP057**,
  reported from the generator (the analyzer cannot see them), naming the method and
  pointing at the overload that takes the SQL directly;
- 12 are APIs unsupported in any spelling - ExecuteReader, multi-map, QueryMultiple.
  Those now get **DAP001 from the generator**, the same id their visible siblings
  already get from the analyzer, so both spellings behave alike.

**Severity follows PublishAot.** Leaving a call-site on vanilla Dapper is a missed
optimization under JIT and a latent publish-time crash under native AOT, so DAP057
is info in the first case and a warning in the second. The SDK sets EnableAotAnalyzer
when PublishAot is on and it is compiler-visible by default, so this needs no build
props of our own. Proved end to end: a net10.0 project reports info at default
verbosity, and the same project with PublishAot=true reports warning. That way an
upgrade does not bury a JIT project in new warnings, while an AOT project is told
about every call-site that will not survive publishing.

The surface report gains an `unsupported: CommandDefinition` bucket and collapses
the undiagnosed-unsupported one, since nothing is undiagnosed any more. Silent skips
go **34 -> 1**, and the remaining one is a reporting artifact rather than a gap:
GetRowParser<T>(concreteType) does different things depending on whether a call
passes the Type, which a symbol-level report cannot say.

Corpus (round 15 rig, net10.0): silent skips **25 -> 0**, refused-with-diagnostics
203 -> 228, handled unchanged at 432 of 736, and the suite still passes 729 of 800 -
this adds diagnostics and changes no behaviour.

version.json opens 1.1 with versionHeightOffset -1, so the first build is 1.1.0
rather than 1.1.1; confirmed by packing (Dapper.AOT.1.1.0-g.nupkg).
DAP057 warned under native AOT while DAP001 stayed info, so an AOT project was told
about Execute(CommandDefinition) but not about QueryMultiple - both of which fail
identically after publish. Same argument, same treatment: the analyzer gains an
AnalyzerOptions overload of TargetsNativeAot, and both DAP001 report sites (analyzer
for the visible spellings, generator for the ones it cannot see) promote to warning
when the consumer is publishing native AOT.

This turns out to matter more than tidiness. **Info-severity analyzer diagnostics do
not surface in a normal `dotnet build`** - only in IDEs and at raised verbosity - so
DAP001 was effectively being emitted into a void, and the surface report's
"unsupported API (diagnosed)" bucket was true on paper and silent in practice. The
promotion is what makes the unsupported surface visible to the people it will hurt;
without it DAP057 would be the only thing an AOT publisher ever saw.

Measured on the Dapper suite built with the AOT flag set (MSBuild double-lists, so
halve these): 152 DAP001 + 50 DAP057, i.e. ~76 and ~25 real call-sites, every one of
which would fail after publish. A JIT consumer sees no change at all.

12 lines across three files, because the TargetsNativeAot plumbing already existed
and DAP001 has only two report sites. No golden churn - the test harness sets no
MSBuild properties, so goldens stay info. Unit suites 380/380 on net8.0 and net10.0;
corpus unchanged at 729 of 800.
@mgravell
mgravell merged commit 1b1ae46 into main Sep 11, 2026
2 checks passed
@mgravell
mgravell deleted the diagnose-mute-overloads branch September 11, 2026 13:37
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.
mgravell added a commit that referenced this pull request Sep 11, 2026
The new net48 CI step caught this on its first real outing, which is exactly what it
was added for: CommandDefinitionOverloads.output.netfx.txt still claimed "2 skipped
silently" and carried no DAP057, because #220 was developed on Linux and only the
.output.* goldens can be regenerated there - the .output.netfx.* twins need an actual
net48 run.

Swept the rest rather than fixing just the one that failed: comparing every
.output.txt against its .output.netfx.txt, this is the *only* pair whose diagnostic
ids differ, and the only one whose scorecard buckets disagree. The other scorecard
differences are legitimate - netfx genuinely has fewer call-sites (15 of 15 vs 17 of
17, and so on), and DateOnly.net6 is gated off netfx entirely.

The generated-code goldens are untouched: #220 added diagnostics, not code, and the
handled count is 1 of 3 on both sides either way.
mgravell added a commit that referenced this pull request Sep 11, 2026
…CI (#222)

* Target 1.1.0, fix the release-tag pattern, and report the version in CI

Three things, all in service of cutting 1.1.0 cleanly.

**Land on 1.1.0.** versionHeightOffset goes -1 -> -3. The offset is a fixed shift,
not a pin: height counts commits since the `version` property changed, and two more
have landed since (#221, and this one), so -1 would compute 1.1.1. Editing
versionHeightOffset does *not* reset the height - verified, height stays 3 - so the
offset has to absorb the drift. -3 puts this commit at exactly 1.1.0, which is what
main will compute once this squash-merges (one commit on top of 78f0fc7 either way).

**Fix the release-tag pattern**, which has never worked. publicReleaseRefSpec requires
`^refs/tags/v\d+\.\d+`, but every tag this repo has ever cut is unprefixed - 1.0.52,
1.0.48, 1.0.45 and so on - so the regex cannot match any of them. Checking out the
real 1.0.52 tag and asking nbgv gives `1.0.52-g7a36975e31`, PublicRelease False: a
tag-triggered build has always produced a -g suffixed version, and the packages that
shipped must have come from main builds instead. Relaxed to `v?`, so both spellings
work; at an unprefixed 1.1.0 tag nbgv now reports a clean 1.1.0, PublicRelease True.
Same fix, same reason, as StackExchange.Redis 611e478.

**Report the computed version in CI**, modelled on StackExchange.Redis's CI.yml: a
step that writes the NuGetPackageVersion to the job summary and the log, placed before
restore/build so it stays legible when a later step fails. Cutting a release then means
reading that line off a green main build and tagging with exactly it.

* Add release.yml: verify the tag, then publish both packages

There was no release workflow at all - dotnet.yml pushes to MyGet on main, and
nothing publishes to nuget.org, so every release to date must have been pushed by
hand. That is also why the broken tag pattern went unnoticed.

Triggered by a published GitHub Release, with workflow_dispatch as a dry run that
does everything except the tag check and the push, so the pipeline can be proven
without cutting a release.

The guard is the point: nbgv computes the version from version.json plus commit
height, so the tag name does not set it and a mistyped tag would otherwise ship a
package that disagrees with its release. The step compares the two and fails loudly
instead. A leading "v" is stripped, matching the v? pattern this PR also fixes.

Publishes **both** packages: Dapper.AOT and Dapper.Advisor are both on nuget.org at
1.0.52, and both set GeneratePackageOnBuild for Release, so a Release build produces
the pair and the collect step gathers them with the same glob dotnet.yml already
uses. They are uploaded as a run artifact before the push, so a failed push does not
cost the build.

Auth is Trusted Publishing (OIDC) - no long-lived key in the repo. That needs setup
outside this file, recorded in the header comment: a GitHub environment named
"release", a NUGET_USER secret, and a trusted-publishing policy on nuget.org for
*each* of the two packages.

No further versionHeightOffset delta: the repo squash-merges, so this lands as one
commit on main regardless of how many are on the branch, and -3 still computes 1.1.0.

* Drop the MyGet push, and give the README badges

release.yml now publishes to nuget.org on a tagged release, so the MyGet push on
every main build is both redundant and a second, unversioned place for packages to
appear. Removed, along with the Pack and Purge steps that existed only to feed it -
packaging is still exercised on every build, because both src projects set
GeneratePackageOnBuild for Release, so a broken nuspec still fails CI.

That also retires the MYGETAPIKEY secret; nothing references it now.

Badges: there were none at all, so this adds rather than updates - build status, and
current nuget.org versions for both published packages. Worth having now that a
release actually lands on nuget.org by a route anyone can see.

* Fix the stale netfx golden #220 left behind

The new net48 CI step caught this on its first real outing, which is exactly what it
was added for: CommandDefinitionOverloads.output.netfx.txt still claimed "2 skipped
silently" and carried no DAP057, because #220 was developed on Linux and only the
.output.* goldens can be regenerated there - the .output.netfx.* twins need an actual
net48 run.

Swept the rest rather than fixing just the one that failed: comparing every
.output.txt against its .output.netfx.txt, this is the *only* pair whose diagnostic
ids differ, and the only one whose scorecard buckets disagree. The other scorecard
differences are legitimate - netfx genuinely has fewer call-sites (15 of 15 vs 17 of
17, and so on), and DateOnly.net6 is gated off netfx entirely.

The generated-code goldens are untouched: #220 added diagnostics, not code, and the
handled count is 1 of 3 on both sides either way.

* Say which ref the computed version belongs to

The first real run printed `1.1.4-g90538b49e3` on this PR, which is correct and
misleading at once: a PR builds refs/pull/N/merge, whose height includes every branch
commit plus the merge, so it is not the number that will ship. Squash-merged onto
main the same work computes 1.1.0.

Since the whole point of this step is "read this, tag with it", that gap is a live
mis-tag waiting to happen. On main the summary now says to tag with exactly that
value; anywhere else it says, in the summary itself, that the number is not the one
that would ship and to read it off a main run instead. The log line carries the ref
either way.

release.yml's guard would catch the resulting mismatch, but not hitting it beats
being caught by it.
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