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

Linker is warning for Assembly.GetType for hard-coded type #1884

Open
krwq opened this issue Mar 11, 2021 · 8 comments
Open

Linker is warning for Assembly.GetType for hard-coded type #1884

krwq opened this issue Mar 11, 2021 · 8 comments
Labels

Comments

@krwq
Copy link
Member

krwq commented Mar 11, 2021

In dotnet/runtime we have https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/DynamicDebuggerProxy.cs#L420

private static readonly Type ComObjectType = typeof(object).Assembly.GetType("System.__ComObject");

with a single usage of that field https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/DynamicDebuggerProxy.cs#L445 :

else if (obj != null && ComObjectType.IsAssignableFrom(obj.GetType()))

it seems that linker should be able to figure out what to do here but it warns instead:

D:\src\runtime\src\libraries\Microsoft.CSharp\src\Microsoft\CSharp\RuntimeBinder\DynamicDebuggerProxy.cs(420,9): Trim analysis warning IL2026: Microsoft.CSharp.RuntimeBinder.DynamicMetaObjectProviderDebugView..cctor(): Using method 'System.Reflection.Assembly.GetType(String)' which has 'RequiresUnreferencedCodeAttribute' can break functionality when trimming application code. Types might be removed. [d:\src\runtime\src\libraries\src.proj]

cc: @eerhardt

@MichalStrehovsky
Copy link
Member

Can you change this to:

private static readonly Type ComObjectType = Type.GetType("System.__ComObject, System.Runtime");

It will be equivalent and illink models that API.

Modeling assembly was scoped out for now because this is the only situation where it would work (both the assembly and the type in question need to be visible).

Type.GetType can also work if the string comes from elsewhere so there's better payoff in implementing that one as intrinsic.

@marek-safar
Copy link
Contributor

Should we tweak the error message or add a special version to include Type.GetType suggestion?

@eerhardt
Copy link
Member

Roughly, the same pattern is found here:

https://github.com/dotnet/runtime/blob/e0bbfa0f555cf5b9d8b8749e1a91a78924c9d927/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/TypeDescriptor.cs#L2924-L2926

public ComNativeDescriptorProxy()
{
    Assembly assembly = Assembly.Load("System.Windows.Forms");
    Type realComNativeDescriptor = assembly.GetType("System.Windows.Forms.ComponentModel.Com2Interop.ComNativeDescriptor", throwOnError: true);
    _comNativeDescriptor = (TypeDescriptionProvider)Activator.CreateInstance(realComNativeDescriptor);
}

Albeit, this is pointing to an Assembly and Type that is unknown when analyzing the runtimepack.

@eerhardt
Copy link
Member

See dotnet/corefx#36496 (comment) for why it was written this way, and not

Type.GetType("System.Windows.Forms.ComponentModel.Com2Interop.ComNativeDescriptor, System.Windows.Forms");

This is purely because it is easier to determine what part failed - the assembly or type discovery. If and when we get better fusion logging, the one line makes sense for sure.

@AaronRobinsonMSFT - is it OK if I change ComNativeDescriptorProxy to use Type.GetType with an assembly qualified name to help the linker here? Or should I just suppress the linker warnings?

(If I change it, I'd like to test the change to make sure it works - can you tell me how to test it?)

@MichalStrehovsky
Copy link
Member

This is purely because it is easier to determine what part failed - the assembly or type discovery. If and when we get better fusion logging, the one line makes sense for sure.

I'm not sure I understand the motivation:

Assembly.Load("Dont");
Type.GetType("Foo, Dont", throwOnError: true);

Either of these will end up with a FileNotFoundException saying Dont cannot be found. What does the two liner help with?

@vitek-karas
Copy link
Member

We now have a very detailed tracing of assembly loading failures: https://docs.microsoft.com/en-us/dotnet/core/dependency-loading/collect-details

@eerhardt
Copy link
Member

What does the two liner help with?

My understanding is the scenario where the assembly can be found, but the Type can't be found.

But even in the 1-liner case you get different exception types between the 2 error cases: FileNotFoundException vs. TypeLoadException

@AaronRobinsonMSFT
Copy link
Member

@eerhardt I've tagged the appropriate owners for the scenario in the referenced PR. It looks fine to me and I think it should be okay. I've forwarded you an email about this area and how it impact WinForms.

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

No branches or pull requests

6 participants