0xd4d / dnlib

Reads and writes .NET assemblies and modules
MIT License
2.18k stars 587 forks source link

Improve Importer TryToUseTypeDefs #513

Closed CreateAndInject closed 1 year ago

CreateAndInject commented 1 year ago

This PR resolve following issues:

  1. Fix inconsistent type like this which for method/field:

    443 https://github.com/0xd4d/dnlib/commit/8ed982fc0760adb7597ffbf2f1cecfcd97d819a3

  2. TryToUseTypeDefs can work also without creating an AssemblyResolver (NullResolver by default)

ElektroKill commented 1 year ago

@CreateAndInject I think I came up with a better way to implement this.

Instead of CreateTypeRef we could create this method:

ITypeDefOrRef CreateTypeDefOrRef(Type type) {
    var tdr = mapper?.Map(type);

    if (tdr is TypeSpec)
        throw new InvalidOperationException();
    if (tdr is TypeDef td)
        return td;
    if (tdr is TypeRef tr)
        return TryResolve(tr);

    if (TryToUseTypeDefs && IsThisModule(type.Module) && module.ResolveToken(type.MetadataToken) is TypeDef td2)
        return td2;

    return TryResolve(CreateTypeRef2(type));
}

and also modify ImportMapper.Map(Type) to return ITypeDefOrRef to allow further customization of importing process from reflection! New ImportMapper.Map(Type):

/// <summary>
/// Overrides default behavior of <see cref="Importer.Import(Type)"/>
/// May be used to use reference assemblies for <see cref="Type"/> resolution, for example.
/// </summary>
/// <param name="source"><see cref="Type"/> to create <see cref="ITypeDefOrRef"/> for. <paramref name="source"/> is non-generic type or generic type without generic arguments.</param>
/// <returns><see cref="ITypeDefOrRef"/> or null to use default <see cref="Importer"/>'s type resolution</returns>
public virtual ITypeDefOrRef Map(Type source) => null;

What do you think?

CreateAndInject commented 1 year ago

I considered ITypeDefOrRef Map(Type source) yesterday also:

  1. The purpose of ImportMapper is remaping members, e.g we make a dynamic unpacker based on .NET Core but support both .NET Framework and .NET Core, when our unpacker load a .NET Framework assembly, all the references types in that assembly will be redirected to .NET Core, so we should use ImportMapper to remap back to .NET Framework. Seems no reason to remap members defined in current module
  2. If we chage the return type to ITypeDefOrRef, the user may make mistake: When they want to remap a type defined in another assembly, they may just load that assembly and resolve a TypeDef, that's incorrect. If the return type is TypeRef, people can't make this mistake
ElektroKill commented 1 year ago
1. The purpose of ImportMapper is remaping members, e.g we make a dynamic unpacker based on .NET Core but support both .NET Framework and .NET Core, when our unpacker load a .NET Framework assembly, all the references types in that assembly will be redirected to .NET Core, so we should use ImportMapper to remap back to .NET Framework. Seems no reason to remap members defined in current module

Remapping to a TypeDef can be beneficial for ImportMapper implementations used in member cloners/injectors where one might use typeof expression to easily locate members and wants to map them to already existing members in a different assembly. Importer and ImportMapper should not be solely constrained to use with DynamicMethodBodyReader.

2. If we chage the return type to `ITypeDefOrRef`, the user may make mistake:
   When they want to remap a type defined in another assembly, they may just load that assembly and resolve a TypeDef, that's incorrect. If the return type is `TypeRef`, people can't make this mistake

If the user is customizing the import procedure, they should be familiar with how type and member references work and where they are or are not applicable.

If we decide not to change ImportMapper, we can tweak my suggestion to be:

ITypeDefOrRef CreateTypeDefOrRef(Type type) {
    var tr = mapper?.Map(type);
    if (tr is not null)
        return TryResolve(tr);

    if (TryToUseTypeDefs && IsThisModule(type.Module) && module.ResolveToken(type.MetadataToken) is TypeDef td)
        return td;

    return TryResolve(CreateTypeRef2(type));
}

Using such an implementation over the current changes you implemented makes the code flow much clearer to the reader of the source code.

CreateAndInject commented 1 year ago

Don't know if ImportMapper is designed for remapping the same member in semantics but from different versons or frameworks, e.g: remap System.Convert mscorlib 4.0.0.0 to System.Convert mscorlib 2.0.0.0 or remap System.Convert System.Runtime.Extensions... to System.Convert netstandard... Let's see how @wtfsck decide

wtfsck commented 1 year ago

@CreateAndInject I don't use ImportMapper at all so let's see what people who actually use it think.

CreateAndInject commented 1 year ago

I send a force-pushed to make the changes clear.

wtfsck commented 1 year ago

Thanks!