dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.26k stars 4.73k forks source link

Treat code kept from descriptors or command line as accessed for reflection #103932

Open sbomer opened 2 years ago

sbomer commented 2 years ago

@MichalStrehovsky in https://github.com/dotnet/linker/pull/2792#issuecomment-1137977247:

I'm wondering whether the following ways to keep an entire type should be treated equivalently in analysis:

  1. void Foo([DAM(DAMK.All) Type x) called with typeof(SomeTypeThatHasClosureThatDoesReflection)
  2. [DynamicDependency(DAMK.All, typeof(SomeTypeThatHasClosureThatDoesReflection))]
  3. ILLinkTrim.Descriptors.xml saying to keep the type in entirety
  4. Command line argument to illink saying to keep the entire assembly that contains the type

All of the above tell analysis that someone is going to access potentially everything on this type without analysis seeing it. They may use reflection invoke. They may use hosting APIs. But the analysis has no visibility into the values set/passed. It keeps the members so that someone can do arbitrary things with them at runtime.

@sbomer in https://github.com/dotnet/linker/pull/2792#issuecomment-1138012830:

I like the idea to treat descriptors/arguments as keeping things for reflection. If someone is going out of their way to preserve things we should assume the worst. This would also encourage people to turn on more aggressive trimming (since they might otherwise get more warnings from unused parts of the rooted assemblies).

@MichalStrehovsky in https://github.com/dotnet/linker/pull/2792#issuecomment-1138043846:

We should guide people towards:

  • If you [DAM] anything in the assembly, you should also set IsTrimmable=true
  • Never set TrimMode to anything but link

Then it should be less of a nuisance.

@vitek-karas in https://github.com/dotnet/linker/pull/2792#issuecomment-1139679207:

I do agree with descriptors and similar to trigger reflection dependency. We should fix that currently. That said I know that there were some cases in the past where we decided we should NOT warn on RUC if the reference is coming from a descriptor. Maybe we should revisit that again.

As for "copy" assemblies, maybe those should not warn, but it's not a big deal I think. We should have single-warn on by default and so typically one should get a single warning from the entire assembly.

dotnet-policy-service[bot] commented 4 months ago

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas See info in area-owners.md if you want to be subscribed.