-
Notifications
You must be signed in to change notification settings - Fork 228
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Ambiguity in NpgsqlArrayValueConverter Causes Test Failures on Mono Runtime #3395
Comments
Thanks for this, but isn't this something that needs to be fixed on the mono side? Libraries shouldn't start having to change code to accomodate mono discrepancies, no? |
@roji , Yes you are right. This is just a work around. A better fix will be given in Mono. |
Description
The following test cases fail in the test/EFCore.PG.Tests/ suite when run on the Mono runtime:
NpgsqlArrayValueConverterTest.Can_convert_number_arrays_to_enum_arrays_object
NpgsqlArrayValueConverterTest.Can_convert_number_arrays_to_enum_arrays
NpgsqlArrayValueConverterTest.Can_convert_enum_arrays_to_number_arrays
NpgsqlArrayValueConverterTest.Can_convert_enum_arrays_to_number_arrays_object
fails on the Mono runtime, even though it passes on CoreCLR. This issue arises due to differences in how the Mono runtime and CoreCLR handle Type.GetInterfaces() for arrays or collections.
A similar issue has been reported in the .NET Runtime repository:
Mono ves_icall_RuntimeType_GetInterfaces generates an extra Implemented Interface (#108964).
Referencing this issue, Mono generates additional generic interfaces that lead to ambiguity in certain scenarios.
Reproducible Example
Program.cs
Analysis
Behavior on CoreCLR
The output of
GetInterfaces()
forBeatles[]
on CoreCLR:Behavior on Mono
The output of
GetInterfaces()
forBeatles[]
on Mono:The additional
System.Collections.Generic.IEnumerable<int>
interface in Mono leads to ambiguity in identifying the correct element type forTryGetElementType
.Expected Output
The correct element type for the Beatles[] array should be typeof(Beatles).
Actual Output on Mono
Ambiguity arises when TryGetElementType internally retrieves interfaces. Mono includes:
IEnumerable<int>
IEnumerable<Beatles>
The function cannot resolve which interface is relevant, causing it to fail.
Root Cause
The
Type.GetInterfaces()
API (via Mono'sves_icall_RuntimeType_GetInterfaces
) generates an extra generic interface for arrays (e.g.,IEnumerable<int>
), which is not present in CoreCLR. This discrepancy causesTryGetElementType
to fail due to ambiguous matches.Observed Behaviour [src/Shared/SharedTypeExtensions.cs]
here
This logic:
Issue
: On Mono, where extra interfaces exist (e.g., IEnumerable), this logic discards the valid interface (IEnumerable), leading to incorrect results.Proposed Fix
Justification for the Fix
The updated implementation resolves ambiguity for arrays more reliably by:
Narrowing Down to Compatible Interfaces:
var arrayCompatible = types.FirstOrDefault(t => t.GenericTypeArguments.Contains(elementType));
This checks each interface in types and selects the first interface whose generic arguments contain the array's element type (e.g., Beatles for Beatles[]).
Prioritizing Array Compatibility: If a compatible interface is found (e.g.,
IEnumerable<Beatles>
), it is returned immediately. This ensures the method prefers interfaces that match the array's actual element type.Fallback to First Interface: If no specific array-compatible interface is found, the method defaults to the first interface in types. While not ideal, this ensures the method still functions and prevents errors in cases where no clear match exists.
Example
For the type Beatles[], Type.GetInterfaces() on Mono returns:
IEnumerable<int>
IEnumerable<Beatles>
The updated method:
IEnumerable<int>
: No match (int ≠ Beatles).IEnumerable<Beatles>
: Match found!IEnumerable<Beatles>
as the resolved type.This fix resolves test failures specific to Mono while maintaining compatibility across CoreCLR. This change ensures consistency in behavior and resolves ambiguity introduced by Mono's interface resolution logic.
@uweigand @giritrivedi @saitama951
The text was updated successfully, but these errors were encountered: