Skip to content

Fix IL2093: annotate GetFieldType override to match DbDataReader base#242

Closed
buvinghausen wants to merge 2 commits into
MarkPflug:mainfrom
buvinghausen:aot_read_hotfix
Closed

Fix IL2093: annotate GetFieldType override to match DbDataReader base#242
buvinghausen wants to merge 2 commits into
MarkPflug:mainfrom
buvinghausen:aot_read_hotfix

Conversation

@buvinghausen

Copy link
Copy Markdown

Summary

DbDataReader.GetFieldType(int) declares its return value with:

[return: DynamicallyAccessedMembers(
    DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.PublicFields)]
public abstract Type GetFieldType(int ordinal);

ExcelDataReader's sealed override didn't repeat this annotation, which trips IL2093 ("return value doesn't match the annotations of the overridden method") in any AOT/trimmed consumer that dispatches GetFieldType polymorphically through DbDataReader — regardless of whether this library itself ever uses GetFieldType reflectively. Virtual-dispatch trim analysis is conservative about the override's contract, not just its body.

This PR adds the matching [return: DynamicallyAccessedMembers(...)] attribute to ExcelDataReader.GetFieldType, closing the annotation gap and getting the read path one step closer to full AOT-compatibility.

Changes

  • ExcelDataReader.cs — added the DynamicallyAccessedMembers return annotation on GetFieldType, matching DbDataReader's declaration exactly (PublicProperties | PublicFields).
  • DynamicallyAccessedMembersAttribute.cs (new) — a #if !NET5_0_OR_GREATER polyfill of the attribute/enum, needed because this package still multi-targets netstandard2.0/netstandard2.1, where neither type exists in the reference assemblies. It's compile-time only — no runtime or analyzer behavior on those older TFMs, since trim analysis doesn't run against them anyway.

Verification

  • Builds clean (0 warnings/errors) across all three TFMs: net6.0, netstandard2.1, netstandard2.0.
  • Confirmed via IL disassembly that the emitted net6.0 metadata for GetFieldType's return annotation is byte-for-byte identical to DbDataReader.GetFieldType's (DynamicallyAccessedMemberTypes.PublicProperties | PublicFields, 0x0220).

Note for reviewers

This repo doesn't currently target net8.0+ or set IsAotCompatible/IsTrimmable, so IL2093 won't reproduce in this repo's own build/CI even without the fix — it only surfaces in a downstream consumer that opts into trim/AOT analysis and calls through the base DbDataReader type. Flagging so the lack of a local repro doesn't read as "unnecessary change."

@MarkPflug

Copy link
Copy Markdown
Owner

I have a question about this. I'm not clear what the polyfill accomplishes. If someone was going to use this with AOT, then they'd already be targeting net6.0, no? To me, it feels like the application of the attribute on GetFieldType should be conditional, since it doesn't accomplish anything on other frameworks? Or am I misunderstanding something. I'm not that familiar with AOT.

@buvinghausen

Copy link
Copy Markdown
Author

Good catch, both of these — and the trimmer had more to say than I expected once I dug in. Fixed all three, here's the reasoning for each:

#1 (GetFieldType annotation / IL2093) — you're right that it doesn't accomplish anything on the older TFMs. DynamicallyAccessedMembersAttribute has been a real BCL type since .NET 5, so on net6.0 this is the genuine framework attribute — no polyfill involved. On netstandard2.0/netstandard2.1 the polyfill existed only to make that one annotation compile; it's never inspected by anything, because trim/AOT analysis only ever runs against the TFM-specific asset a consumer actually resolves, and an AOT app targeting net7/net8 resolves the net6.0 asset (nearest compatible TFM), never the netstandard ones. So: wrapped the annotation in #if NET5_0_OR_GREATER and deleted the polyfill file outright.

#2 (SchemaTable.Add / IL2067) — annotated the type parameter with [DynamicallyAccessedMembers(PublicProperties | PublicFields)] (guarded the same way) so it matches DataColumn's constructor. Trivial as expected, but it surfaced a second-order issue: annotating that parameter makes every call site responsible for satisfying the requirement, and Add(table, SchemaTableColumn.DataType, typeof(Type)) doesn't — System.Type itself has a TypeInitializer property that's annotated, so the trimmer can't fully verify the chain and reports a new diagnostic, IL2111, rather than assuming it's fine. That call only needs typeof(Type) preserved as an opaque metadata value (it's never reflected over), so I added a scoped UnconditionalSuppressMessage for IL2111 on GetSchemaTable rather than letting it propagate further.

#3 (GetFieldType's body / IL2073) — this one can't actually be threaded through ExcelColumn.DataType the way it might look at first: I decompiled the net6.0 reference assembly and DbColumn.DataType is a plain concrete property, not virtual, so there's no override point to hang an annotation on even if we wanted to. Since the actual values here are either our own primitive types or a caller-supplied schema Type (whose trim-safety is inherently the caller's responsibility, same as it is for DbColumn.DataType generally), I added a scoped UnconditionalSuppressMessage for IL2073 directly on GetFieldType, with a comment explaining why.

Verified by temporarily forcing EnableTrimAnalyzer/EnableAotAnalyzer on a net8.0 build of the library: IL2093, IL2067, IL2073, and the follow-on IL2111 are all clean now. Regular multi-target build (net6.0/netstandard2.1/netstandard2.0) is unaffected — 0 warnings.

One more thing surfaced along the way, unrelated to this thread and left untouched: FieldAccessor.cs and XlsxDataWriter+FieldWriter.cs both use MakeGenericMethod/MakeGenericType (IL3050/IL2070) — genuine reflection-based codegen, not an annotation gap, so not something a suppression can responsibly paper over. Happy to open a separate issue for that if you want to track it.

@buvinghausen

Copy link
Copy Markdown
Author

@MarkPflug going to go ahead and close this one out since I'm assuming you'd rather have the CI verified version for future guardrails but this will solve all of my issues which is I love using your library to parse XLSX files but we distribute the program to partners so I need AOT support so that they can run it on any hardware/os and not have to have .NET installed as a prereq.

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.

2 participants