Skip to content

Two typeof(Mapper).GetMethods().First(...) calls depend on reflection ordering #4

Description

@arnelirobles

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:

  1. The existing suite stays green. tests/Mapsicle.Tests/ComplexMappingTests.cs and
    NullHandlingTests.cs already exercise nested mapping through both call sites.
  2. 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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinggood first issueGood for newcomers

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions