You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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<>.
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.
Unmapped enums fail default PostgreSQL type inference
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.
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.
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
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.
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:
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:
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.IsDynamicCodeSupportedwe 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' doUnsafe.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.