Two places in the core select a MapTo overload by filtering GetMethods() and taking the first
match. Type.GetMethods() does not guarantee order, so which overload is selected is not defined by
the code.
Current behaviour. src/Mapsicle/Mapsicle.cs:998, inside the nested-object branch of
Map<TDestination>(this object?, TDestination):
var mapMethod = typeof(Mapper).GetMethods()
.First(m => m.Name == "MapTo" && m.GetParameters().Length == 1 && m.GetGenericArguments().Length == 1)
.MakeGenericMethod(targetType);
and the identical expression at src/Mapsicle/Mapsicle.cs:1319 inside
Mapper.CreatePropertyBinding.
Three public overloads satisfy that predicate:
T MapTo[T](System.Object)
System.Collections.Generic.List`1[T] MapTo[T](System.Collections.IEnumerable)
T MapTo[T](System.Collections.Generic.IDictionary`2[System.String,System.Object])
On .NET 8 today the runtime happens to return MapTo[T](Object) first, which is the intended one, so
this works. If the order changed, MakeGenericMethod would produce a method whose parameter type no
longer matches the expression being passed and Expression.Call would throw at delegate-build time,
or, for the IDictionary overload, the where T : new() constraint would throw
ArgumentException from MakeGenericMethod for any destination type without a public parameterless
constructor.
This is the same bug that was fixed for Queryable.Select in 1.2.3. The fix there was to select by
signature rather than order, and src/Mapsicle.EntityFramework/QueryableExtensions.cs:43 still
carries the comment explaining why. The core was not given the same treatment. Note that
src/Mapsicle/Mapsicle.cs:591 already does it correctly:
var mapMethod = typeof(Mapper).GetMethod("MapTo", new[] { typeof(object) })!.MakeGenericMethod(targetType);
What should change. Replace both GetMethods().First(...) calls with the explicit form used at
line 591. GetMethod(name, Type[]) matches on parameter types, so it is unambiguous.
Two things to be careful about. GetMethod(string, Type[]) cannot bind an open generic method by its
generic parameter types, which is why the working call at line 591 works (the parameter is
object, not T) and is the reason a copy-paste to a different overload would not. And the two call
sites pass differently typed expressions: line 1002 passes propExp (the source property type)
while line 592 passes Expression.Convert(propExp, typeof(object)). Keep whichever conversion each
site already has; changing it is a separate behaviour change.
How you know you succeeded. Two things, and the second matters more than the first:
- The existing suite stays green.
tests/Mapsicle.Tests/ComplexMappingTests.cs and
NullHandlingTests.cs already exercise nested mapping through both call sites.
- Add a guard test that fails if a future overload is added that would have collided:
[Fact]
public void MapTo_ObjectOverload_IsSelectableBySignature()
{
var method = typeof(Mapper).GetMethod("MapTo", new[] { typeof(object) });
Assert.NotNull(method);
Assert.Single(method!.GetGenericArguments());
}
That test fails today only if the overload set changes, which is exactly when someone needs to be
told. Making the ordering itself fail on demand is not practical, so the honest framing for the PR
description is: this removes an undefined behaviour, and the existing nested-mapping tests prove the
selected overload did not change.
Two places in the core select a
MapTooverload by filteringGetMethods()and taking the firstmatch.
Type.GetMethods()does not guarantee order, so which overload is selected is not defined bythe code.
Current behaviour.
src/Mapsicle/Mapsicle.cs:998, inside the nested-object branch ofMap<TDestination>(this object?, TDestination):and the identical expression at
src/Mapsicle/Mapsicle.cs:1319insideMapper.CreatePropertyBinding.Three public overloads satisfy that predicate:
On .NET 8 today the runtime happens to return
MapTo[T](Object)first, which is the intended one, sothis works. If the order changed,
MakeGenericMethodwould produce a method whose parameter type nolonger matches the expression being passed and
Expression.Callwould throw at delegate-build time,or, for the
IDictionaryoverload, thewhere T : new()constraint would throwArgumentExceptionfromMakeGenericMethodfor any destination type without a public parameterlessconstructor.
This is the same bug that was fixed for
Queryable.Selectin 1.2.3. The fix there was to select bysignature rather than order, and
src/Mapsicle.EntityFramework/QueryableExtensions.cs:43stillcarries the comment explaining why. The core was not given the same treatment. Note that
src/Mapsicle/Mapsicle.cs:591already does it correctly:What should change. Replace both
GetMethods().First(...)calls with the explicit form used atline 591.
GetMethod(name, Type[])matches on parameter types, so it is unambiguous.Two things to be careful about.
GetMethod(string, Type[])cannot bind an open generic method by itsgeneric parameter types, which is why the working call at line 591 works (the parameter is
object, notT) and is the reason a copy-paste to a different overload would not. And the two callsites pass differently typed expressions: line 1002 passes
propExp(the source property type)while line 592 passes
Expression.Convert(propExp, typeof(object)). Keep whichever conversion eachsite already has; changing it is a separate behaviour change.
How you know you succeeded. Two things, and the second matters more than the first:
tests/Mapsicle.Tests/ComplexMappingTests.csandNullHandlingTests.csalready exercise nested mapping through both call sites.That test fails today only if the overload set changes, which is exactly when someone needs to be
told. Making the ordering itself fail on demand is not practical, so the honest framing for the PR
description is: this removes an undefined behaviour, and the existing nested-mapping tests prove the
selected overload did not change.