CLR enum underlying type mapping by NinoFloris · Pull Request #6581 · npgsql/npgsql · GitHub
Skip to content

CLR enum underlying type mapping - #6581

Open
NinoFloris wants to merge 11 commits into
npgsql:mainfrom
NinoFloris:enum-underlying-type-mapping
Open

NinoFloris wants to merge 11 commits into
npgsql:mainfrom
NinoFloris:enum-underlying-type-mapping

Conversation

@NinoFloris

Copy link
Copy Markdown
Member

Solves half of #3303 and fixes #5591.

This brings NativeAOT compatible *unmapped* CLR enum support for reads and writes. Once merged we begin to map (by default, inferred) to the closest pg type based on the enum's underlying type:

  • int, uint => int4
  • long, ulong => int8
  • byte, sbyte, short, ushort => int2

We don't support any primitive conversions like we have for raw primitives. We can leave those until people actually ask for them. This means if users want to map an int enum into an int8 (and so on) that we do not yet support this.

Arrays are supported but not fully, see: dotnet/runtime#127249. We could consider adding these mappings on CoreCLR only. So far I've kept the functionality symmetrical, as we try to do with most mappings.

A lot of the recent work was required to land this feature:

  • Cleaning up the provider/concrete split that proliferated 'AsObject' logic all over the codebase.
  • Getting all non-generic converter dispatching to use the centralized PgConverter.Read (and so on) methods.
  • Rationalizing all PgTypeInfo variance logic to be able to confidently add the enum <-> underlying exemptions.

This PR just really threads the needle through CLR and NativeAOT quirks. The enum <-> underlying "reduced-type equivalence" is one big, weird, exception in the type system: array covariance carries it through, though generic instantiation stays invariant. Given our infra is generic it requires full manual mapping to faithfully resurface the relationship, rather than simply relying on the (allocating) boxing/unboxing helpers to do it for us.

Once past the IL stage NativeAOT brings its own problems. Its aggressive trimming does not carve out any real exceptions for these historical equivalences beyond the absolute simplest cases (nevertheless working with enums through their underlying type is the recommended way). Then just as I was rounding off the work I noticed a classic bloat issue, one that has surfaced a few times before in our NativeAOT journey. In short, there are fundamental limitations in the current IL scanner, and we are still waiting for a more capable one (dotnet/runtime#83021).

More concretely: we cannot make unmapped enum reading and writing *allocation free* on NativeAOT without incurring massive code bloat. The essential issue is that the scanner cannot clean up code blocks that were found dead or under vacuous conditionals after scanning completed. This applies to typeof(T).IsX branches, isinst branches over types that didn't get instantiated, and so forth. All of that knowledge requires the whole program view the scanner itself is building (and potentially the generic specialization view that codegen has) which all happens after it has decided the methods and types must be kept.

If we do not fortify our typeof(T).IsEnum check with RuntimeFeature.IsDynamicCodeSupported we would root all that enum dispatching logic for every T that flows through, and as they are dispatching stubs, that's basically everything. We can't use any of our usual tricks here - like putting the code behind virtual dispatch - because there are no actual rooted instantations over the user's TEnum. The entire point was that we didn't need to know about those types in Npgsql. Finally, on the more exotic end there is no 'unsafe but guaranteed' equivalence of virtual table layout between T = enum and its underlying T = int either. Which means we cannot 'safely' do Unsafe.As<PgConverter<IntEnum>(PgConverter<int>) and be guaranteed their virtual slots line up such that we end up in the virtual method we intended to call. As a result NativeAOT support is a bit worse than I would have wanted, but it sits on the edge of what's reasonably possible.

Suffice to say this ended up being an even more hilariously painful feature to land than I initially assumed, but it's type safe, trim safe, and complete with as much compositional support as the runtime provides guarantees for.

Copilot AI review requested due to automatic review settings May 19, 2026 11:28
@NinoFloris
NinoFloris requested review from roji and vonzshik as code owners May 19, 2026 11:28

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR introduces default CLR enum support by mapping enums to PostgreSQL integer types based on the enum’s underlying type (including array support), while also refactoring/moving PostgreSQL enum (label) mapping tests into a dedicated suite.

Changes:

  • Add enum-underlying type mapping via a new EnumUnderlyingConverter<T> and resolver support in AdoTypeInfoResolverFactory.
  • Extend converter dispatch and array conversion to support enum↔underlying “reduced-type equivalence” with trimming/NativeAOT considerations.
  • Restructure and expand tests: move PG-enum label mapping tests into PgEnumTests and add enum-underlying tests in EnumTests.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/Npgsql.Tests/Types/PgEnumTests.cs New test suite covering PostgreSQL enum (label) mapping/unmapping and unmapped PG enum behavior.
test/Npgsql.Tests/Types/EnumTests.cs Replaced prior PG-enum tests with tests for enum-underlying integer mappings.
test/Npgsql.Tests/GlobalTypeMapperTests.cs Updated expectations around enum mapping behavior (DbType/type inference assertions).
test/Npgsql.Tests/BugTests.cs Adjusted composite test to avoid accidental enum fallback mapping by changing a PG field type.
src/Npgsql/TypeMapping/UserTypeMapper.cs Switched mapped PG enum converter from EnumConverter<> to PgEnumConverter<>.
src/Npgsql/Internal/ResolverFactories/UnmappedTypeInfoResolverFactory.cs Renamed resolver classes and updated converter activation to PgEnumConverter<>.
src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs Added CLR enum underlying-type typeinfo resolution and enum-array support.
src/Npgsql/Internal/PgTypeInfo.cs Allowed enum requested types over underlying converters and adjusted SupportsRead/Write defaults for this case.
src/Npgsql/Internal/PgConverter.cs Added enum-underlying dispatch paths for Read/Write/Bind/IsDbNull gated by RuntimeFeature.IsDynamicCodeSupported.
src/Npgsql/Internal/PgBufferedConverter.cs Unsealed ReadAsObject/WriteAsObject overrides to allow specialized behavior in derived converters.
src/Npgsql/Internal/Converters/PgEnumConverter.cs Renamed enum label converter type from EnumConverter<> to PgEnumConverter<>.
src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs New converter intended to read/write enums via their underlying integer PG wire formats.
src/Npgsql/Internal/Converters/ArrayConverter.cs Added “effective array type” creation support to enable enum-array materialization over underlying element converters.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs
Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs
Comment thread src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs Outdated
Comment thread test/Npgsql.Tests/Types/EnumTests.cs
Comment thread src/Npgsql/Internal/Converters/ArrayConverter.cs Outdated

@vonzshik vonzshik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just one moment

Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs
Copilot AI review requested due to automatic review settings May 23, 2026 15:48
@NinoFloris
NinoFloris force-pushed the enum-underlying-type-mapping branch from 1f3121c to 5ba7640 Compare May 23, 2026 15:48
@NinoFloris
NinoFloris requested a review from vonzshik May 23, 2026 15:55

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.

Comment thread src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs
Comment thread src/Npgsql/Internal/Converters/ArrayConverter.cs Outdated
Comment thread src/Npgsql/Internal/ResolverFactories/UnmappedTypeInfoResolverFactory.cs Outdated
Comment thread src/Npgsql/Internal/PgConverter.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs
Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs
Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Comment thread src/Npgsql/Internal/Converters/EnumUnderlyingConverter.cs
Copilot AI balanced review requested due to automatic review settings October 5, 2026 17:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Default enum inference is missing and public support-direction validation can be bypassed.

Review effort: Balanced
Findings: None

Resolved since last review (4)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Explicit support overrides bypass converter validation

src/​Npgsql/​Internal/​PgTypeInfo.cs:421

Explicit supportsReading: true/supportsWriting: true now writes the backing fields directly, bypassing the existing init-property guards. Because ResolveType also newly accepts enum/underlying pairs before checking the converter marker, a public PgConcreteTypeInfo.Create caller can pair an ordinary underlying converter with an enum requestedType and force both directions on, defeating the stated IEnumUnderlyingConverter restriction and causing invalid typed dispatch. Keep the marker-derived defaults here, but apply explicit overrides through the validated properties as before.

Medium severity Unmapped enums fail default PostgreSQL type inference

src/​Npgsql/​Internal/​ResolverFactories/​AdoTypeInfoResolverFactory.cs:59

This makes the new mapping explicit-only: resolving an enum with no DataTypeName returns null (and the array resolver is likewise gated on a non-null name), so value-only enum parameters still fail inference. That contradicts the PR contract that unmapped enums are inferred by default to int2/int4/int8. Please resolve the null-name case to the canonical type while preserving higher-priority PostgreSQL-enum mappings, and update these tests from DataTypeInference.Nothing accordingly.

Copilot AI balanced review requested due to automatic review settings October 5, 2026 18:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

NativeAOT guard ordering defeats trimming protection, and default inference contradicts the stated behavior.

Review effort: Balanced
Findings: 1 Medium severity · 2 Low severity

Open (3)

Comment thread src/Npgsql/Internal/ResolverFactories/AdoTypeInfoResolverFactory.cs
Comment thread src/Npgsql/Internal/PgConverter.cs Outdated
Comment thread src/Npgsql/Internal/PgTypeInfo.cs Outdated
@NinoFloris
NinoFloris force-pushed the enum-underlying-type-mapping branch from 2ae75bd to 740e716 Compare October 5, 2026 18:48
Copilot AI balanced review requested due to automatic review settings October 5, 2026 18:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

NativeAOT and composite enum scenarios central to the change lack dedicated regression coverage.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
Resolved since last review (3)

Comment thread src/Npgsql/Internal/PgConverter.cs
Comment thread test/Npgsql.Tests/BugTests.cs
Cover npgsql#5591 with a short-backed enum and its array inside a mapped composite.
Copilot AI balanced review requested due to automatic review settings October 5, 2026 19:12

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The claimed issue #5591 fix lacks coverage for its reported direct short[] composite scenario.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)
Previously missed (1)

In code that hasn't changed since last review

Low severity Remove unused flattening helper or route callers through it

src/​Npgsql/​Util/​IterationIndices.cs:120

This helper has no callers, so it is not currently a “single owner” of the flattening formula and only adds an unused API surface. The linked runtime proposal is for span-based Array.GetValue/SetValue, not an Array.GetFlattenedIndex API. Remove this method until there is a consumer, or update the implementation to actually centralize existing flattening logic through it.

Comment thread test/Npgsql.Tests/Types/CompositeTests.cs

This branch has not been deployed

No deployments
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.

Lack a default Type Mapping for short array in composite to PostgreSQL smallint[]

3 participants