Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Pass in genericParamProvider #555

Closed
wants to merge 1 commit into from

Conversation

CreateAndInject
Copy link
Contributor

No description provided.

Copy link
Contributor

@wtfsck wtfsck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you explain what this PR improves?

src/DotNet/Importer.cs Show resolved Hide resolved
@CreateAndInject
Copy link
Contributor Author

CreateAndInject commented Apr 28, 2024

Could you explain what this PR improves?

Some code depends on IMethod.FullName:
if (method.FullName == "System.Void System.Collections.Generic.ICollection`1::Add(T)")
This check will be failed when there's no onwer, since FullName will be System.Void System.Collections.Generic.ICollection`1::Add(!0) without owner.

@wwh1004
Copy link
Contributor

wwh1004 commented Apr 30, 2024

If uses dnlib to import a TypeSig from one ModuleDefMD to another ModuleDefMD, we shouldn't assume the imported TypeSig is in the same gp context. Even in the same ModuleDefMD, just importing a TypeSig from one type to another, the gp context is indeterminate.
This pr will result in not being able to get a TypeSig with an empty gp context (gp context is read-only, GenericSig cannot be modified after it has been created).

@CreateAndInject
Copy link
Contributor Author

If uses dnlib to import a TypeSig from one ModuleDefMD to another ModuleDefMD, we shouldn't assume the imported TypeSig is in the same gp context. Even in the same ModuleDefMD, just importing a TypeSig from one type to another, the gp context is indeterminate.

I think GenericParamContext is designed as:

new Importer(module, new GenericParamContext(member1context)).Import(member1);
new Importer(module, new GenericParamContext(member2context)).Import(member2);
...

rather than use:

module.Import(member1);
module.Import(member2);
...

In this PR, preparing different Importers/GenericParamContexts are unnecessary in some cases, just module.Import(...)

This pr will result in not being able to get a TypeSig with an empty gp context (gp context is read-only, GenericSig cannot be modified after it has been created).

I don't understand, can you explain more?

@wwh1004
Copy link
Contributor

wwh1004 commented Apr 30, 2024

gp context is a parameter designed for display. You can leave it empty.

@CreateAndInject
Copy link
Contributor Author

gp context is a parameter designed for display. You can leave it empty.

Yes, that's why I check if (gpContext.Type is null &&, I only resolve type/method when user doesn't support gpContext

@wwh1004
Copy link
Contributor

wwh1004 commented Apr 30, 2024

Almost all existing code does not pass gp context, even de4dot, ConfuserEx. When the gp context is empty, it cannot be assumed that the gp context of TypeSig is consistent with the original one.
Suppose someone uses dnlib to import the TypeSig of module A into module B, and then disposes the ModuleDefMD of A. Then there will be a disposed object in the imported TypeSig in module B. So everyone must use the Importer's constructor with gp context overload. This will change everyone's usage habits.

@CreateAndInject
Copy link
Contributor Author

When the gp context is empty, it cannot be assumed that the gp context of TypeSig is consistent with the original one.

If the gp context is empty, everything is null, what do you mean consistent with? Whether we use an empty gp context or not, there's no difference.

Suppose someone uses dnlib to import the TypeSig of module A into module B, and then disposes the ModuleDefMD of A. Then there will be a disposed object in the imported TypeSig in module B.

Yes, otherwise we can't get the correct FullName when our code calls generic members defined in other assembly, eg: T System.Activator::CreateInstance<T>()
This also happen in .NET Core, can you fix #453 so that dnlib can resolve .NET Core assembly.

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

Successfully merging this pull request may close these issues.

3 participants