Fix the [CommandProperty] field probe: readonly means not assignable - #190
Merged
Merged
Conversation
mgravell
added a commit
that referenced
this pull request
Aug 18, 2026
The member-exists check returned true for a public *readonly* field and false for a mutable one - inverted. It never fired in anger because real ADO.NET command types expose these knobs (BindByName etc) as properties, which were checked correctly; but a [CommandProperty] naming a mutable public field was wrongly rejected with DAP033, and naming a readonly one passed validation and then emitted an assignment that cannot compile (CS0191) in the consumer's build. Deliberately excluded from the capture-model rework (which was byte-identical by contract) and fixed separately here. The new tests fail without the fix - the mutable/readonly field cases both flip.
mgravell
force-pushed
the
phase2-model-plans
branch
from
August 18, 2026 15:37
8a18d5d to
dcc6e49
Compare
mgravell
force-pushed
the
fix-commandproperty-readonly-field
branch
from
August 18, 2026 15:37
db7c9a6 to
ae1fe89
Compare
mgravell
added a commit
that referenced
this pull request
Aug 18, 2026
* Phase 2, increment 3c-ii (result side): ResultType projected to RowPlan SuccessSourceState.ResultType (ITypeSymbol) becomes RowPlan: everything row-factory emission reads - member db-names/types/reader-methods, constructor/factory choice, deferred-construction facts, the inbuilt- helper decision, query-column mapping - projected at parse. RowReaderState de-dupes on the plan's structural equality instead of the old symbol+columns key, and AppendReader/GetRowParser/Execute<T> consume plan strings. One cached symbol remains in the whole model: ParameterType. Byte-identical: goldens unchanged (net10/net48), harness hash equal (f8d3a61f). * Phase 2, increment 3c-ii (parameter side): the last cached symbol is gone SuccessSourceState.ParameterType (ITypeSymbol) becomes ParamPlan: everything command-factory emission reads - member db-types/sizes/ directions with the Add-mode sizing decisions precomputed, DbString and cancellation facts, the anonymous-type shape witness (built at parse), the multi-exec element plan and cast - projected at parse time. CommandFactoryState de-dupes on plan equality (the systemObject fallback becomes a plan built once per Generate), and the whole cached model is now plain data. The states also gain structural equality, which the node-level incremental cache needs (reference equality re-ran downstream on every edit even with plain fields); ModelShapeTests now covers the SourceState family and requires equality on nested model types instead of skipping them. Byte-identical: goldens unchanged (net10/net48), harness hash equal (f8d3a61f). * Phase 2, increment 4: the Compilation no longer feeds the output step The interceptor generator's Generate combined the raw CompilationProvider, re-running the whole output step on every keystroke. Its compilation uses are now projected into an equatable InterceptorEnvironment via Select: AllowUnsafe, assembly name, has-InterceptsLocationAttribute, the DbCommand special-types sweep (pre-filtered to the providers needing per-command setup), the module-level [CommandFactory<T>] resolution, and the system-object fallback plan for CommandFactoryState. The analyzer-bridge path builds the same environment from its CompilationAnalysisContext. GenerateState.GetInterceptorFilePath (the last Compilation hanger-on) is gone - its projection moved to parse in 3a. This completes the phase-2 exit criteria: plain equatable cached model, no CompilationProvider into the output step, shape test in CI, output byte-identical (goldens net10/net48; harness hash f8d3a61f throughout). * Prove the cache: incremental-caching tests, both directions Three cases, each asserting on the driver's tracked output steps: - an edit in an unrelated file leaves every output step Cached/Unchanged (on the old pipeline this failed by construction: the raw Compilation fed the output step, so any edit re-ran it); - an edit in the *same file* below the call-site also leaves the output cached - Parse re-runs and produces fresh state instances, so this one specifically needs the states' structural equality; - a real edit (a new bindable member on the row type) re-runs the output and changes the generated text. Note the SQL literal is deliberately not used for this: the SQL flows through as a runtime argument, so changing it does not change the generated shape. Also asserts the generated text is byte-identical across cached re-runs. * Fix the [CommandProperty] field probe: readonly means not assignable (#190) The member-exists check returned true for a public *readonly* field and false for a mutable one - inverted. It never fired in anger because real ADO.NET command types expose these knobs (BindByName etc) as properties, which were checked correctly; but a [CommandProperty] naming a mutable public field was wrongly rejected with DAP033, and naming a readonly one passed validation and then emitted an assignment that cannot compile (CS0191) in the consumer's build. Deliberately excluded from the capture-model rework (which was byte-identical by contract) and fixed separately here. The new tests fail without the fix - the mutable/readonly field cases both flip.
mgravell
added a commit
that referenced
this pull request
Aug 19, 2026
* Start the Dapper/Dapper.AOT parity accounting under notes/ The goal on record: enable Dapper.AOT in the Dapper test suite, announce types via attributes, and have it swallow everything - AOT-clean. - parity.md: the feature table, with impact/complexity per gap (several 'gaps' score zero because the concept doesn't exist under AOT, e.g. the ref-emit plan cache) - tokens.md: @ids expansion, {=literal}, ?foo? pseudo-positional, param filtering - type-vs-generic.md: the announced-types design space for Type-based APIs - test-suite-audit.md: the Dapper tests as acceptance corpus, sequenced * GetTypeDeserializer is valid API, not cache plumbing With announced types it's the same dispatch map (boxed materializer), and its generic strengthening already exists as GetRowParser<T>. The real hole is the write side: CreateParamInfoGenerator has no generic counterpart - recorded the GetParameterBinder<T> proposal, and the question of blessing CommandFactory<T>/RowFactory<T> as the supported surface. ReadChar and friends are plain AOT-safe statics, nothing to do. * Scope the accounting to the public API and observable behavior PublicAPI.Shipped.txt is the checklist; the contract is what reaches the provider and what comes back, never Dapper's internals. Cuts both ways: the dynamic row needs behavioral fidelity only (the type is internal), while the public infrastructure statics ARE in scope because extenders call them. * Record the decision: internals-asserting tests get adjusted, not maintained * New work item: warn (new DAP id) on use of the has-no-meaning APIs Plan-cache surface, CommandFlags.NoCache, possibly ConnectionStringComparer: supported-and-meaningless under AOT, which is a different statement to DAP001's unsupported-but-meaningful. Warning, not error - the code runs. * Measurement caveat: build-time DAP counts are an upper bound Some failure modes are silent until executed - handled means intercepted, not correct. Only the DB-backed test run catches silent divergence. * First harness baseline: 'handled 396 of 396' alongside 96 compile errors Two root-cause generator bugs (array-of-anonymous parameter emits the display string and wrecks the parse; inaccessible row types are emitted rather than refused), plus two scorecard honesty problems (the denominator excludes unattempted APIs; handled does not mean compiles) and a zero- analyzer-diagnostics anomaly to re-check once the compile is clean. * Generator audit: the capture model snapshots Roslyn nodes; fix first Both generators' cached SourceState hold IMethodSymbol/ITypeSymbol/ Location (MemberMap even holds an IOperation), and the pipeline combines the raw CompilationProvider into the source output - so it behaves as a full-recompute generator with a memory leak. Recorded as a sequencing gate ahead of the gap-closing features, with the fix shape that worked for protobuf-net (plain equatable model, span-based locations, separate diagnostics branch, shape-enforcing test). * Record the agreed plan: gap table, then generator model, then features The line that resolves the phase-1/2 tension: nothing that adds parse-time state lands before the model rework completes; refusals and scorecard fixes are allowed ahead of it, which is what lets phase 1 see. * Round 2 numbers, and log the modern-interceptor-syntax work item * Round 3: the suite compiles with AOT enabled (4 fix PRs + 2 severity downgrades) * Scoreboard: all three TFM legs compile; local SQL Server available * Work item: [UnsafeAccessor] may lift the accessibility refusals (net8+) * Round 4: the honest scorecard says 53%, not 100% * Harvest the skip breakdown; flag the DAP016 corpus-shape decision * Round 5: first behavioral run - 84 failures, every one compiled clean * Note that aot-harness is deliberately local-only * Phase 2 log: approach and increments * Phase 2 log: increment 1 done * Phase 2 log: 3a done * Phase 2 log: 3b done * Phase 2 log: 3c-i done; two cached symbols remain * Phase 2 log: result-side plan done; one symbol left * Phase 2 log: cached model fully plain; only increment 4 remains * Phase 2 log: complete - PRs #187 + #188 * Phase 2 log: caching tests landed * Phase 2 log: readonly-field quirk fixed (#190) * Round 6: DAP051 + restructure takes interception to 68.1% * Round 6b: 612/760 behavioral; failures track interception growth honestly * DynamicParameters design: delegate to the bag; needs one small Dapper API * Round 7: DynamicParameters at 73.5%; First-pipeline drain divergence found * Round 7b: 612/762; every failure class maps to a planned feature * Interceptor-syntax migration: soft-target requirement recorded * Tokens: runtime-SQL design - per-factory memoized role scan * Record the feature-detection rule (DAP052) in the design note * Round 8: CommandBehavior parity fix (PR #196) * Round 8b: 616/762, suite loop 17s * Round 9: list expansion lands (PR #197), 638/762 * Round 10: custom parameters + the two bugs they uncovered, 658/793 * Round 11: dynamic-record fidelity, 672/793 * Sync with main; point parity rows at their open PRs The table lands on main via #186; from here each feature PR flips its own cells, so the table and the merge history cannot drift apart. Rows with an open PR say so, and the flip to a settled status is that PR's job.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
[CommandProperty]member-exists probe returned true for a public readonly field and false for a mutable one — inverted. It never fired in anger because real ADO.NET command types expose these knobs (BindByName,InitialLONGFetchSize) as properties, which were checked correctly; but a[CommandProperty]naming a mutable public field was wrongly rejected with DAP033, and naming a readonly one passed validation and then emitted an assignment that cannot compile (CS0191) in the consumer's build.I kept this out of the capture-model rework deliberately (#187/#188 are byte-identical by contract; this is a behavior change) and stacked it here on #188, where the probe now lives (
CommandProperty.Create). The new theory covers mutable/readonly fields, settable/get-only properties, static, and missing members; the two field cases fail without the fix. Full suite green (net10.0/net48).