gluck / il-repack

Open-source alternative to ILMerge
Apache License 2.0
1.16k stars 214 forks source link

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

Closed deniszykov closed 2 months ago

deniszykov commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Assembly reference graph for reference: image

KirillOsenkov commented 5 months ago

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

KirillOsenkov commented 5 months ago

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

KirillOsenkov commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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

deniszykov commented 5 months ago

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 commented 5 months ago

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 commented 4 months ago

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 commented 3 months ago

@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 commented 3 months ago

A small self-contained repro would help here

AlexeyZarubin commented 3 months ago

A small self-contained repro would help here

Ok, let me work on it.

AlexeyZarubin commented 3 months ago

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 commented 3 months ago

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

KirillOsenkov commented 3 months ago

I hope I'll have time this weekend

AlexeyZarubin commented 2 months ago

Hi @KirillOsenkov. Any updates?

KirillOsenkov commented 2 months ago

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

KirillOsenkov commented 2 months ago

Published https://www.nuget.org/packages/ILRepack/2.0.34 https://www.nuget.org/packages/ILRepack.Lib/2.0.34

AlexeyZarubin commented 2 months ago

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