Use DynamicDependencyAttribute

[sbomer]

Fixes #37475. I'm still waiting for dependency flow (dotnet/linker#1262 allows keeping members by name like PreserveDependency did), but opening this for review in the meantime.

[MichalStrehovsky]

Should we use this as an opportunity to clean these up?

• There's a lot of annotations that shouldn't be needed because the reflection pattern is within the method and the linker can analyze that.
• Some annotations should have a TODO mentioning Reflection patterns can not resolve assemblies outside of the reference closure linker#943 (annotation can be deleted once that's fixed)
• Some could be converted to DynamicallyAccessedMembers, which is a preferable annotation because it works with the dataflow analysis.

[marek-safar]

There's a lot of annotations that shouldn't be needed because the reflection pattern is within the method and the linker can analyze that.

Yes, let's remove them

DynamicallyAccessedMembers, which is a preferable annotation because it works with the dataflow analysis.

I disagree at BCL level we should be explicit where possible

[MichalStrehovsky]

DynamicallyAccessedMembers, which is a preferable annotation because it works with the dataflow analysis.

I disagree at BCL level we should be explicit where possible

I meant places like the GetHelperType method in this pull request: if you annotate the return parameter with DynamicallyAccessedMembers, it both ensures we keep the constructor, and the place that passes the result of GetHelperType to Activator stops generating warning. If we keep DynamicDependency, we need to add a suppression to the Activator call here:

runtime/src/libraries/System.Linq.Expressions/src/System/Linq/Expressions/Interpreter/CallInstruction.cs

Line 239 in b8339a8

return (CallInstruction)Activator.CreateInstance(GetHelperType(info, arrTypes), info)!;

[vitek-karas]

I would probably postpone converting DynamicDependency to data flow annotations into a different PR/issue. I think it's a good idea to do that where it makes sense, since it's the cleaner approach which works better as the code evolves.

For this conversion itself, there are basically 3 cases:

• DynamicDependency into a different assembly - we need to keep these, regardless of the reflection pattern. This is basically a workaround to the problem that linker can't add assemblies into the closure in Type.GetType and so on. So the attribute in this case mainly acts as the thing which pulls in the assembly.
• DynamicDependency to things within the same assembly which there are no reflection patterns to access - like the stuff on System.String for example. We also need to keep these, as there's no other way to handle this.
• DynamicDependency on things within the same assembly, where linker will figure out the pattern. We should remove these. Linker will issue a warning if it can't figure out the pattern. In such case adding DynamicDependency will actually not remove the warning, we would also have to add UncoditionalSuppressMessage.

[eerhardt]

There's a lot of annotations that shouldn't be needed because the reflection pattern is within the method and the linker can analyze that.

Yes, let's remove them

The plan was to be explicit with the attributes until we can turn on the linker analysis, and have a test that covers the dependency is kept. Once we have linker analysis enabled (and it breaking the build if this gets broken) and a test that covers these reflection usages, I'd prefer we keep being explicit.

[sbomer]

For now I have added comments about the ones that should be converted to DynamicallyAccessedMembers and those that are needed to work around dotnet/linker#943. I'll leave removing the unnecessary ones for another PR when we turn on analysis warnings and maybe add tests. This change shows the ones we should be able to remove, and #37837 tracks the remaining work.

[sbomer]

I've moved this attribute closer to the internalcall that depends on it. Does that make sense?

[eerhardt]

Not for you to fix here, but reading this message, it sounds like we should move these to a “library build” xml descriptor file instead. That way we don’t need to hack around this with ILKeepDepsAttributes=false. I’ll open an issue for this on Monday.

[eerhardt]

LGTM

[sbomer]

The remaining failures look like #37946.