Native AOT Compatibility Checks - #79
Merged
Merged
Conversation
Records the triage decision for the 68 IL2026/IL3050 warnings that appear once IsAotCompatible is enabled: route the reflection-based JsonSerializer calls through JsonTypeInfo, following the existing write-path pattern, rather than annotating the public entry points. Closes #78 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
The ILC publish gate stays, but it covers the Minimal API write path only; the in-process resolver-composition tests carry the proof for the CloudEvents write and HTTP read paths. The sample keeps its narrowed trim-warning suppression so the gate can report warnings from this library. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
Resolver composition cannot synthesize the closed wrapper contracts, so combining a shipped library context with the consumer's would still force consumers to declare CloudEventsEnvelopeForWriting<T> and the HttpRead* payloads themselves. Take the other route from the review: the write path calls the existing public Utf8JsonWriter extension directly instead of detouring through JsonSerializer, and the read paths build their wrapper contracts from the library's own converters via JsonMetadataServices.CreateValueInfo. The consumer context then only ever declares the result value type. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
TrimmerSingleWarn=false expands Serilog into individual diagnostics, and Release TreatWarningsAsErrors turns IL2067 into a failing publish. Keep single-warn mode and demote only the IL2104 package rollup: the SDK applies single-warn to package references while project references keep reporting in detail, so an injected regression in Light.PortableResults still fails the publish with file and line. Also records why the CI step must not pass PublishAot on the command line. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
GetTypeInfo throws NotSupportedException for an unknown type rather than returning null, so a null check or failed-cast check after it is unreachable for the only case that occurs in practice. That makes the promised actionable exception impossible and leaves three existing guards as dead code. Prescribe TryGetTypeInfo with explicit translation, require a negative test per site, and fold the repair of the existing guards into this work so the broken pattern is not copied further. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
Building the wrapper contract from the library's own converter bypasses converters supplied through JsonSerializerOptions, which existing read tests depend on. Resolve from the options first and fall back only when the configured resolver cannot supply a contract. The options must have their resolver materialized before that check: the library's default options never assign a TypeInfoResolver, so TryGetTypeInfo reports false on them and every reflection-backed caller would otherwise lose its replacement converters. Also corrects the direct write call to pass SerializerOptions and the count of suppressed converter factories. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
Restores three points lost in the condensation: the existing read tests that pin converter precedence and would fail if the library contract came first, the release-timing argument for doing this before 0.7.0, and the to-be-confirmed status of non-generic results needing no consumer registration. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMMdhU9uijRHcmmuynn2Ve
Signed-off-by: Kenny Pflug <kenny.pflug@live.de>
WriteLibraryValue invoked the resolved converter directly, which does not flush the writer, while the resolver path went through JsonSerializer, which does. The public WriteCloudEvent overloads taking a Utf8JsonWriter therefore left the event buffered whenever the envelope contract came from a library-owned converter: whether a caller saw output before flushing or disposing the writer depended on how they had configured their resolver. Contracts and converters were also only cached against read-only options. Invoking a converter directly never freezes them, so the CloudEvents write path re-ran TryGetTypeInfo and the full converter scan on every write and never populated the cache. Freezing before caching - after the converter is resolved, so that a failed lookup does not freeze the caller's options - makes the cache effective and keeps a converter registered later from invalidating it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
The guards in LightResult and LightActionResult pointed at AddDefaultLightResultsHttpWriteJsonConverters, which was renamed to AddDefaultPortableResultsHttpWriteJsonConverters. The stale name was harmless while the guards were unreachable; repairing them is what makes it reach users. They also offered converter registration as the only remedy, which helps just the reflection-backed case. A source-generated resolver cannot supply the closed HttpResultForWriting contract at all - which is what makes these guards reachable in the first place - so the message now also asks for the type to be declared in the JsonSerializerContext. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
….Json FindRegisteredConverter claimed to mirror how JsonSerializerOptions pick a converter. It follows their list order, but it skips an entry whose CanConvert claims the type while it cannot supply a JsonConverter<TLibraryType>, where JsonSerializerOptions would select that entry and then throw. The skip is intended - it keeps such an entry from making a library-owned type unwritable and unreadable - so the comment now states it instead of implying equivalence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
The publish was the last step of build-and-test, so coverage-comment - which needs that job - waited for a slow native compilation it has nothing to do with. Moving it into an independent job lets it run in parallel with the tests and returns the coverage comment to its previous latency. The job is deliberately not gated on build-and-test: it restores and builds what it needs on its own, so a dependency would only serialize two unrelated pieces of work. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
The tests that assert exact GC.GetAllocatedBytesForCurrentThread() deltas were flaky. The counter is per-thread, so concurrent tests cannot add to it directly, but a GC triggered by any other thread retires and refills the measuring thread's allocation context, and that accounting boundary shifts the observed value by up to the size of a context. Every deviation stayed below 8 KB, and one landed in the baseline rather than the measured value, which is why a one-sided tolerance would not have covered it. A tolerance was the alternative. It would have to be two-sided and roughly 8 KB wide, which on the span test asserting zero allocations would let a regression of hundreds of bytes per iteration through unnoticed. Scheduling the four affected classes into one collection with DisableParallelization removes the cause instead. Measured in Debug, where the suite reproduces this most readily: four failures in twelve runs before, none in twenty-four runs after, for roughly three percent of the runtime. Release did not reproduce it in thirty-three runs, nor in eighteen under CPU contention, so this mainly affected Debug and Stryker runs rather than the Release CI gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
Four differences between the plan and the implementation: the fourth unreachable guard in ErrorsExtensions, which the plan did not list; the DynamicallyAccessedMembers annotation that Light.PortableResults.Validation needed although the plan recorded it as clean; the publish gate becoming its own job, because the plan asked both for a step in build-and-test and for no coverage job to depend on it; and step 3 of the contract resolution freezing the options, without which the caching the plan required never took effect. Adds the zero-based sequence to the plan file now that the issue has more than one document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
Step 3 of the contract resolution calls MakeReadOnly so that the contract it caches cannot be invalidated by a converter registered afterwards. The README described the caching but not the freeze, leaving a caller to discover it as an InvalidOperationException from a late Converters.Add. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VsEiLdTSnUPsGV1UFBxnrg
9 tasks
Minimum allowed line rate is |
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.
Closes #78