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

Getting "Method reference is used with definition return type / parameter." / Due to TypeForwardedTo type #359

Closed
deniszykov opened this issue Apr 5, 2024 · 22 comments

Comments

@deniszykov
Copy link
Contributor

Issue:

I'm getting following warning message:

WARNING: Method reference is used with definition return type / parameter. Indicates a likely invalid set of assemblies, consider one of the following
WARNING:  - Remove the assembly defining Microsoft.Extensions.DependencyInjection.IServiceCollection from the merge
WARNING:  - Add assembly defining Microsoft.Extensions.DependencyInjection.IServiceCollection Microsoft.Extensions.Logging.ILoggingBuilder::get_Services() to the merge

Because ReferenceFixator is failing to map declaring type of ILoggingBuilder::get_Services() method in ReferenceFixator.cs: 436

TypeReference declaringType = Fix(method.DeclaringType);

and it is failing because TypeReference.Scope for method.DeclaringType is Microsoft.Extensions.Logging while type is declared in Microsoft.Extensions.Logging.Abstractions and being forwarded:

// Microsoft.Extensions.Logging, Version=8.0.0.0, Culture=neutral, PublicKeyToken=adb9793829ddae60
[assembly: TypeForwardedTo(typeof(ILoggingBuilder))]

Steps to reproduce:

  1. Take these assemblies
  2. Merge them
/target:exe  "/lib:%UserProfile%\.nuget\packages\microsoft.netcore.app.ref\6.0.0\ref\net6.0" "/out:Test.dll" "Microsoft.Extensions.Logging.Configuration.dll" "Microsoft.Extensions.DependencyInjection.Abstractions.dll" "Microsoft.Extensions.DependencyInjection.dll"  "Microsoft.Extensions.Logging.Abstractions.dll"

libs.zip

Expected:

Type forwarding is respected. Probably type is registered twice in MappingHandler with both scopes when it is forwarded.

@deniszykov
Copy link
Contributor Author

deniszykov commented Apr 8, 2024

I tried to investigate further and trail gone cold in RepackImporter.cs:50 at

Import(ExportedType type, Collection<ExportedType> col, ModuleDefinition module)

Type forwards are processed there, but IMHO incorrectly.

@KirillOsenkov
Copy link
Collaborator

The exported type ILoggingBuilder is being skipped here:

image

I'm guessing we erase all memory of it being a forwarded type, we still need to remember that it was a forwarded type even though it's being rewritten

@KirillOsenkov
Copy link
Collaborator

Assembly reference graph for reference:
image

@KirillOsenkov
Copy link
Collaborator

we can remove Microsoft.Extensions.Logging.Configuration from tests to minimize the repro

@KirillOsenkov
Copy link
Collaborator

if I remove that continue statement, I no longer get the warning about ILoggingBuilder

@KirillOsenkov
Copy link
Collaborator

Actually I think this may not be a bug at all. I have enhanced to warning to specify more details:

image

When rewriting the body of this method we encounter the AddOptions() extension method call:
image

and that method is defined in Microsoft.Extensions.Options:
image

So the warning is technically correct, we should either Remove the assembly defining Microsoft.Extensions.DependencyInjection.IServiceCollection from the merge (Microsoft.Extensions.DependencyInjection.Abstractions.dll)

or

add Microsoft.Extensions.Options.

When I add Microsoft.Extensions.Options.dll, the warnings go away.

@deniszykov
Copy link
Contributor Author

With better diagnostic message I have found and fixed most on these warning, but not all. One is left:

Add assembly defining <>f__AnonymousType4`2<System.String,System.Collections.Generic.Dictionary`2<System.String,Serilog.Settings.Configuration.IConfigurationArgumentValue>> Serilog.Settings.Configuration.ConfigurationReader/<>c::<GetMethodCalls>b__20_1
(Microsoft.Extensions.Configuration.IConfigurationSection) to the merge: <MY_PROJECT_ASSEMBLY_NAME>

of course <MY_PROJECT_ASSEMBLY_NAME> is included in merge. I will report back later.

@deniszykov
Copy link
Contributor Author

deniszykov commented Apr 9, 2024

Unfortunately I can't just isolate this case as a small set of assemblies. But I found that if I move this code a little lower, the problem disappears:

image

It is unable match/map method to MethodDefinition def because return types are not matching.

image

They are not matching due to rename of anonymous type <>f__AnonymousType4'2 to <18c72155-01ef-4bc4-9d93-2a702bceba97><>f__AnonymousType4'2

image

Both assemblies (maybe even more that 2) have <>f__AnonymousType4'2 in empty namespace.

@KirillOsenkov
Copy link
Collaborator

can you send me your full list of assemblies so I can debug too?

@deniszykov
Copy link
Contributor Author

deniszykov commented Apr 9, 2024

I can't send you all my libraries because it is private project. But I'm trying to create minimal repro:
And I'm stuck with strange merge behavior:

I have 2 types:
ClassLibrary1 -> [root namespace].MyGenericType<T1, T2>
ClassLibrary2 -> [root namespace].MyGenericType<T1, T2>

both have default .ctr and different fields.

After merge I get one merged type with only ONE constructor. My thoughts it is breaking behavior.

image

merge_on_type.zip

I'm still working on minimal repro.

@deniszykov
Copy link
Contributor Author

deniszykov commented Apr 9, 2024

Also I have found that /internalize rename/mangle public class and keep name of internalized. I think it should be opposite.

/target:exe /internalize /lib:<user>\.nuget\packages\microsoft.netcore.app.ref\6.0.0\ref\net6.0 /out:Test.dll ClassLibrary1.dll ClassLibrary2.dll

image
libs_internalize.zip

@deniszykov
Copy link
Contributor Author

It's taking too much time to reproduce these circumstances.
I'm gladly present debugging session for you @KirillOsenkov in skype(deniszykov87)/discord (490096546608578560)/telegram(deniszykov) during evenings or mornings in CET.

@AlexeyZarubin
Copy link

AlexeyZarubin commented Jun 3, 2024

@deniszykov @KirillOsenkov Hi guys.

I met the same issue with TypeForwardingTo and Microsoft.Extensions.Logging library.
Are there any plans to fix this bug or maybe there is a workaround?

@KirillOsenkov
Copy link
Collaborator

A small self-contained repro would help here

@AlexeyZarubin
Copy link

A small self-contained repro would help here

Ok, let me work on it.

@AlexeyZarubin
Copy link

AlexeyZarubin commented Jun 3, 2024

A small self-contained repro would help here

Here it is, please follow the readme:
https://github.com/AlexeyZarubin/IL-repack-issue-demo/tree/main

@KirillOsenkov Thanks for your help!

@AlexeyZarubin
Copy link

@KirillOsenkov Did you have a chance to test the issue?

@KirillOsenkov
Copy link
Collaborator

I hope I'll have time this weekend

@AlexeyZarubin
Copy link

Hi @KirillOsenkov. Any updates?

@KirillOsenkov
Copy link
Collaborator

Please give the latest commit a try (I haven't published a new version of the NuGet package yet)

@KirillOsenkov
Copy link
Collaborator

@AlexeyZarubin
Copy link

This is a great fix, thank you for your help!

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

No branches or pull requests

3 participants