Skip to content
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

Improve Importer TryToUseTypeDefs #513

Merged
merged 1 commit into from
Jul 18, 2023
Merged

Conversation

CreateAndInject
Copy link
Contributor

This PR resolve following issues:

  1. Fix inconsistent type like this which for method/field:
    Improve Importer TryToUseMethodDefs and TryToUseFieldDefs options 2 #443 8ed982f

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

src/DotNet/Importer.cs Outdated Show resolved Hide resolved
@ElektroKill
Copy link
Contributor

@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
Copy link
Contributor Author

CreateAndInject commented Jul 12, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

wtfsck commented Jul 15, 2023

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

@CreateAndInject
Copy link
Contributor Author

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

@wtfsck wtfsck merged commit 32ad9d1 into 0xd4d:master Jul 18, 2023
2 checks passed
@wtfsck
Copy link
Contributor

wtfsck commented Jul 18, 2023

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants