Repository navigation
Conversation
SerializationUtil scanned each type's methods for [OnDeserializing] and [OnDeserialized] via reflection. Under TrimMode=full that metadata is removed, so the callbacks never fired: CalendarObject.Initialize() was never called during deserialization, and the random GUID seeded by UniqueComponent's constructor survived as the first value of the UID group, silently replacing every parsed UID. Replace the scan with an internal IDeserializationCallbacks interface implemented by CalendarObject and CalendarDataType - the only two types that used the attributes. Removes two ConcurrentDictionary caches and a reflective invoke from the deserialization hot path. Retires IL2070 warnings at SerializationUtil.cs:43 and :63.
_objectType was unconditionally List<object> (MakeGenericType ignored _innerType), so the reflective construction and the cached Add MethodInfo were both unnecessary indirection. Replace with new List<object>() and a direct Add call. Retires IL2077 at GenericListSerializer.cs:41 and IL2080 at :63, and removes the 'FIXME: cache this' comment that motivated the field.
…erfaces ServiceProvider.SetService(object) reflected over GetInterfaces() to register an instance under every interface it implements, and RemoveService(Type) did the same on removal. Both are unavailable to the trimmer. Add SetService<TService>(TService) and RemoveService<TService>(), and use them for the five registrations in the SerializationContext constructor. Only four service types are ever retrieved (ISerializerFactory, DataTypeMapper, EncodingStack, IEncodingProvider), so explicit registration is sufficient. The reflective overloads remain for API compatibility, marked [Obsolete] and [RequiresUnreferencedCode]. Note that an unqualified SetService(someInstance) call now binds to the generic overload and registers only under the static type of the argument - callers relying on interface fan-out must name the service type explicitly. Adds a netstandard2.0 polyfill for RequiresUnreferencedCodeAttribute. Retires IL2075 at ServiceProvider.cs:51 and IL2070 at :64.
CopyComponentTests exercised 5 of 28 copyable types with field-equality assertions - thin cover for the clone-path rewrite in B4, where the failure mode is a wrong runtime type or a shallow copy that no field-equality test would notice. Add CopyAllTypesTests, which reflects over every concrete ICopyable type and asserts that a copy has the exact same runtime type, is a distinct instance, returns null for an unrelated T, and does not share its child collection. Plus a serialize -> copy -> serialize stability check over the whole embedded .ics corpus. The corpus check immediately found two real bugs, fixed here: - CalendarProperty.CopyFrom never copied Parameters, so a copied property silently lost VALUE, TZID, ENCODING and friends (20 fixtures affected). - CalendarObjectBase.Copy<T> used the public-constructor-only Activator overload, so copying a CalendarObject threw MissingMethodException. CalendarDataType.Copy<T> already passed nonPublic: true; match it. A third, deeper defect is documented and allowlisted rather than fixed: CalendarDataType.CopyFrom points the copy's parameter proxy at the source's collection, and Attachment/Attendee.CopyFrom then write empty FMTTYPE= / RSVP=FALSE parameters. The fix belongs with the object-model refactor. 7 fixtures are ignored with an explanatory reason.
Copy<T>() called Activator.CreateInstance(GetType()) on both copy roots, which the trimmer cannot see through. Introduce a protected virtual CreateNew() - a 'virtual constructor' - on CalendarObjectBase and CalendarDataType, and override it in all 28 concrete ICopyable types with a direct new. Every instantiation is now statically visible. CreateNew() is virtual, not abstract, so this stays an additive change: subclasses outside the library keep working via a base implementation that falls back to Activator. That fallback carries an UnconditionalSuppressMessage rather than [RequiresUnreferencedCode], because annotating a virtual member would force all 28 overrides to carry the annotation too (IL2046). A subclass relying on the fallback under NativeAOT fails loudly with MissingMethodException rather than silently. CopyAllTypesTests.CreateNewIsOverridden enforces that no library type regresses onto the fallback - a missing override works fine under the JIT and only breaks once published with trimming enabled. Adds a netstandard2.0 polyfill for UnconditionalSuppressMessageAttribute. Retires IL2072 at CalendarObjectBase.cs:26 and CalendarDataType.cs:162. 11 warnings -> 3.
…ializer factories CreateAndAssociate() instantiated TargetType via Activator. Every caller already declares TargetType as a concrete typeof() literal, so each serializer can create its target with a direct new. Add protected virtual CreateTargetInstance() to DataTypeSerializer and override it in the 13 serializers whose TargetType is an ICalendarDataType (the 11 that call CreateAndAssociate, plus FreeBusyEntrySerializer and UtcOffsetSerializer for consistency). The base returns null rather than being abstract, because DataTypeSerializer is also the base for serializers targeting string, int and enums, which never produced an instance here either. DataTypeSerializerFactoryTests enforces that every ICalendarDataType-targeting serializer overrides it and returns exactly its TargetType - FreeBusyEntrySerializer derives from PeriodSerializer and AttendeeSerializer from StringSerializer, so a missing override inherits the wrong type rather than failing outright. Retires IL2072 at DataTypeSerializer.cs:20. 3 warnings -> 2.
…lendar.Create GroupedValueList already declared 'where TItem : new()', so the Activator.CreateInstance call was redundant - replace it with new TItem(). Both instantiations (CalendarPropertyList with CalendarProperty, and ParameterList with CalendarParameter) have public parameterless constructors. Calendar.Create<T>() gains a new() constraint and constructs directly. This is technically source-breaking for a caller passing a component type without a public parameterless constructor, but every component in the library has one. The method also no longer throws ArgumentException on failure, since construction can no longer fail. Retires IL2087 at GroupedValueList.cs:32 and Calendar.cs:351. The library now builds with zero trim-analyzer warnings on net10.0, net8.0 and netstandard2.0 - down from 11.
Deserialize wrapped both the Decode call and Enum.Parse in a bare catch that fell through to returning the raw string. Any failure - including a decoding error - therefore produced a silently mistyped property value instead of an error. Scope the try to Enum.Parse alone and catch only ArgumentException, the legitimate 'unknown enum value in the wild' case. Everything else now propagates. EnumSerializer is kept rather than deleted: no DataTypeMapper mapping resolves to an enum (AlarmAction, TransparencyType and the *Status types are static string-constant classes), so it is unreachable on deserialize, but PropertySerializer calls SerializerFactory.Build(value.GetType()) on serialize, which reaches the IsEnum arm whenever a consumer stores an enum as a property value.
Set IsAotCompatible on Ical.Net for net8.0+, which implies IsTrimmable, EnableTrimAnalyzer, EnableAotAnalyzer and EnableSingleFileAnalyzer. The condition mirrors NodaTime's, excluding netstandard2.0. The library builds clean with it, so the analyzer is now a real gate on new reflection. The analyzer alone is not sufficient: this library compiled clean, ran clean and exited 0 while emitting a duplicated calendar event. Add Ical.Net.AotTests, a plain Exe with hand-rolled assertions (VSTest and NUnit3TestAdapter are not AOT-compatible) published with PublishAot and TrimMode=full, deliberately with no TrimmerRootAssembly and no ILLink descriptor - needing either is what this gate exists to catch. The method is differential rather than absolute: every fixture writes stable key = value lines, and CI runs the same fixtures under the JIT and under NativeAOT and diffs the transcripts. A divergence fails the build even when nobody wrote the matching assertion, which is how the UID corruption surfaced in the first place. Fixtures cover RRULE/BYSETPOS, EXDATE, RDATE, a DST boundary, VTIMEZONE round-trip, full serialize -> parse -> serialize stability, Copy on all 24 publicly constructible ICopyable types, deep-copy independence, and service resolution. RECURRENCE-ID override suppression asserts the parsed UID verbatim, not just the occurrence count - the corruption produced correct times with corrupted identities, so a count-only check catches it only by luck. Verified locally on win-x64: publishes with no ILC warnings, exits 0, and the 124-line transcript is byte-identical to the JIT run. CI matrix is ubuntu-latest + windows-latest, in a separate workflow from tests.yml (which pins ubuntu-22.04 for mono/net4x).
Enum.Parse, Enum.TryParse and Enum.IsDefined are reflection over enum metadata, and none of them was exercised by the gate. EnumSerializer in particular was not reachable from any existing fixture. Add an enum-parsing fixture covering all four sites: EnumSerializer's Enum.Parse(Type, string, bool) including the ignore-case and hyphen-stripping paths and the narrowed ArgumentException fallback, EnumSerializer.SerializeToString on a boxed enum, RecurrenceRule.Frequency via Enum.IsDefined(Type, object), and RecurrenceRuleSerializer via Enum.TryParse<FrequencyType>. These are safe because the trimmer never strips fields from an enum type - enum members are static literal fields, so an enum that survives at all survives whole. The BCL annotates none of these overloads; the only Enum API carrying [RequiresDynamicCode] is GetValues(Type), which has to build an array of the enum type at runtime and which the library does not use. Verified on win-x64: the native binary produces the same results as the JIT for all nine assertions.
…andling The AOT gate carried ten fixtures, most of which no longer test anything AOT-specific. Once Copy<T> and CreateAndAssociate construct their targets with a direct new, instantiation is compiler-verified and cannot fail under trimming, so fixtures like copy-every-type assert what the compiler already guarantees. Others (BYSETPOS evaluation, DST arithmetic, deep-copy semantics) test managed logic that behaves identically under the JIT and is covered far better by Ical.Net.Tests. Cut Ical.Net.AotTests to a single Program.cs covering the two things a native binary can still tell us: that the parsed UID survives trimming, and that a document round-trips through the whole property pipeline. Drop the JIT/AOT transcript diff and the bespoke harness with it. The gate now publishes and runs both net8.0 and net10.0 as a step in tests.yml rather than a separate workflow, so aot.yml is deleted. EnumSerializer: narrowing the bare catch to ArgumentException missed OverflowException, which Enum.Parse throws for a numeric string too large for the underlying type. That turned a malformed value into a thrown exception instead of the documented fall back to the raw string. Catch both; a decoding failure still propagates. Found by the new EnumSerializerTests, which also move the parse-fallback assertions out of the AOT binary and into the JIT suite where they belong. Comments across the branch are rewritten to describe the code as it stands rather than narrate what changed; that history belongs in the commit log and PR description.
Replaces the inline bash loop with a separate job in the same workflow, using a matrix over the two AOT-capable target frameworks. Each step is a single declarative command, and net8.0 and net10.0 now report as independent status checks that run in parallel with the test job rather than serially after it. Adds an explicit clang and zlib1g-dev install: ILC needs a native toolchain to link, and a missing zlib surfaces as an opaque link failure.
…Service SetService<TService>(TService) was added as an overload of SetService(object). Overload resolution then picked the generic for almost every call site, silently narrowing what an existing call registers, and suppressed the obsolete warning on the member it was meant to replace: SetService(new MyFactory()) -> generic, registers MyFactory only IFactory i = ...; SetService(i) -> generic, registers IFactory only object o = ...; SetService(o) -> non-generic, old behaviour Three outcomes decided by the static type of the expression, with the deprecation warning firing only in the case whose behaviour had not changed. Overriders of the virtual SetService(object) were also bypassed, since the SerializationContext constructor no longer reached it. Renaming the type-safe method to AddService<TService> removes the overload set entirely. SetService(object) keeps its previous behaviour for every existing call and now reliably emits its [Obsolete] warning, so callers get a working migration path instead of a silent change.
- SerializationContext registered its default services by calling the overridable AddService from its own constructor, so a derived type could intercept them before its own constructor ran. Registers directly against the provider instead. The same hazard existed before this branch, when the constructor called the virtual SetService. - Restored adjacency of the SetService and RemoveService overloads in ServiceProvider, SerializationContext and SerializerBase; inserting AddService<TService> between them had split each pair. - Removed a genuinely redundant null-forgiving operator in CalendarProperty.CopyFrom. - Ical.Net.AotTests kept its failure count in a local captured by a local function, which defeated flow analysis and made the exit-code expression look constant. Moved the tally and the assertions into a static Smoke class, and replaced every null-forgiving operator with Load/Serialize helpers that throw on null - a null there is a fixture bug, not a result.
Top-level statements put the Smoke helper class in the global namespace. Declare it in Ical.Net.AotTests instead.
|
Merged
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.
Make Ical.Net trimming- and NativeAOT-compatible
Replaces the five
Activator.CreateInstancesites and two reflective metadata scans with virtual factories and direct dispatch. Zero trim-analyzer warnings on net10.0, net8.0 and netstandard2.0 — with no ILLink descriptor, noTrimmerRootAssemblyand no source generator, so the compiler guarantees correctness rather than a hand-maintained metadata file.The problem
Published with
PublishAotandTrimMode=full, the library did not crash. It compiled clean, ran clean, exited 0 — and emitted a wrong calendar:SerializationUtilfound[OnDeserializing]handlers by scanningType.GetMethods(). Trimming removes that metadata, so the callback never fires and the random GUID seeded byUniqueComponent's constructor survives as the parsedUID.RECURRENCE-IDmatching is keyed on(Uid, Instant), andUidalso drivesEquals,GetHashCodeandCompareTo— so the damage reaches any consumer-side deduplication or database key. Worth fixing regardless of AOT: it is silent data corruption, and the fix deletes code and removes a reflective invoke from the deserialization hot path.The fix
Deserialization callbacks — the attribute scan becomes an internal
IDeserializationCallbacksinterface on the two types that used the attributes. Safe to drop the attributes because nothing here is BCL-serializable (no[Serializable],ISerializableorDataContractin the library).Cloning —
Copy<T>()now calls aprotected virtual CreateNew(), overridden in each of the 28 concreteICopyabletypes with a directnew:Virtual rather than abstract, so external subclasses keep compiling on an
Activatorfallback. All 28 matter:Copy<T>recurses through the whole object graph, andEventSerializerreaches it for events carrying bothDURATIONandDTEND.Data-type serializers —
CreateAndAssociate()instantiatedTargetTypereflectively; each serializer now overridesCreateTargetInstance()with a directnew. Every caller already declaredTargetTypeas atypeof()literal.Service registration —
SetService(object)reflected overGetInterfaces().AddService<TService>/RemoveService<TService>state the type explicitly; only four service types are ever retrieved, so nothing needs the fan-out. Deliberately not an overload ofSetService— as one it would win resolution at most call sites, silently narrowing what an existing call registers while suppressing the obsolete warning.Collections —
GenericListSerializerbuiltList<object>reflectively while ignoring its inner type;GroupedValueListalready hadwhere TItem : new();Calendar.Create<T>()gains anew()constraint.EnumSerializer— its barecatchturned any failure, including a decoding error, into a silently mistyped value. Thetrynow wraps only the parse. The remainingEnum.Parse/TryParse/IsDefinedcalls are AOT-safe and unannotated by the BCL; the one unsafe API,Enum.GetValues(Type), is not used.API impact
Existing code compiles and behaves as before. Nothing is removed.
AddService<TService>/RemoveService<TService>addedSetService(object),RemoveService(Type)marked[Obsolete]CreateNew()added asprotected virtualCalendar.Create<T>()gainswhere T : new()CI
IsAotCompatibleis enabled for net8.0 and above, turning on the trim, AOT and single-file analyzers so the build fails on newly introduced reflection. netstandard2.0 is excluded.The analyzer alone is not enough — the library was analyzer-clean while emitting the calendar above.
tests.ymlgains anaotmatrix job over net8.0 and net10.0 that publishes a native binary withTrimMode=fulland runs it, asserting on parsed values rather than the exit code, with noTrimmerRootAssemblyand no descriptor. It is a plain console project because NUnit has no AOT runner, and because rooting a test assembly would preserve exactly the metadata whose absence causes the bug above — the gate would pass while the library was broken.Test-side: blanket
Copy<T>coverage over all 28 copyable types, enforcement that no type regresses onto the reflection fallback, the same for serializer factories, and a serialize → copy → serialize check across the embedded.icscorpus.Known issue, pre-existing — #976
That corpus check surfaced a copy defect this PR does not fix, filed as #976:
CalendarDataType.CopyFrompoints the copy's parameter proxy at the source's collection rather than copying it, so a copy and its original share one parameter collection — settingFormatTypeon a copied attachment mutates the event it was copied from. It reproduces onmainwith no trimming involved.This PR fixes one adjacent symptom, because the copy tests could not otherwise go green:
CalendarProperty.CopyFromnever copiedParameters, so properties with a non-ICalendarDataTypevalue (URL→Uri,X-properties →string) silently lost theirs. The aliasing itself, and the emptyFMTTYPE=/RSVP=FALSEparameters that appear once parameters are genuinely copied, are left to #976 — the fix is object-model surgery that belongs with the v6 refactor. Seven corpus fixtures are allowlisted with an explanatory reason, so the defect stays visible and any new divergence still fails.Notes for review