Enhance NestedTypes in DynamicallyAccessedMemberTypes #2133
Conversation
|
Should this really keep everything on the nested types? I would not expect |
|
I was primarily going with the discussion on the issue for the fix from @MichalStrehovsky and the view that there are not wide usage of I looked into this change since I was curious if this change would have an impact on the library size due to The trimmer is not able to reason on what is needed for internal types that are decorated with |
vitek-karas
left a comment
There was a problem hiding this comment.
Assuming we agree on the behavior (as per the discussion on the issue) the change looks good.
This should be followed by changing the intrinsic behavior of GetNestedType which currently only propagates the All annotation to the return value if the source type was annotated with All. With this change, it can now propagate All always. In fact we may want to change the runtime and add the annotation on the return value directly.
MichalStrehovsky
left a comment
There was a problem hiding this comment.
Looks good modulo Vitek's comment about the GetNestedType intrinsic.
Sven is right that we might be able to not mark non-public nested types in the deeper levels, but I'm not sure if it will result in savings in practice and it looks like it would complicate the implementation quite a bit.
class Foo
{
public class PublicNestedAndReflected
{
protected class ProtectedNestedButNotReflected { }
}
}If we ask for all public nested types on Foo, we might be able to get rid of ProtectedNestedButNotReflected. But I think it's very unlikely that a fully-preserved PublicNestedType would have no references to ProtectedNestedButNotReflected (not many other classes have access to this nested class since it's not public - chances are pretty high we would end up keeping it anyway).
| [Kept] | ||
| public static class PublicNestedType | ||
| { | ||
| // PublicNestedType should be kept but linker won't mark anything besides the declaration |
There was a problem hiding this comment.
Nit: We should delete these comments in our testing given that now the link will mark more than the declaration
… members (dotnet/linker#2133) Commit migrated from dotnet/linker@a80487e
dotnet/linker#2133 adjusted rooting logic to keep `.All` on all nested types preserved with PublicNestedTypes/NonPublicNestedTypes. The PR resolved the bug and the analysis work that was identified in the review as necessary never happened. So we just uselessly preserve members. This adds dataflow analysis to propagate this. Since we never had the dataflow analysis, I'm exposing this not as .All, but a subset of .All that doesn't do the worst part - doesn't require reflectability of interface methods.
dotnet/linker#2133 adjusted rooting logic to keep `.All` on all nested types preserved with PublicNestedTypes/NonPublicNestedTypes. The PR resolved the bug and the analysis work that was identified in the review as necessary never happened. So we just uselessly preserve members. This adds dataflow analysis to propagate this. Since we never had the dataflow analysis, I'm exposing this not as .All, but a subset of .All that doesn't do the worst part - doesn't require reflectability of interface methods.
dotnet/linker#2133 adjusted rooting logic to keep `.All` on all nested types preserved with PublicNestedTypes/NonPublicNestedTypes. The PR resolved the bug and the analysis work that was identified in the review as necessary never happened. So we just uselessly preserve members. This adds dataflow analysis to propagate this. Since we never had the dataflow analysis, I'm exposing this not as .All, but a subset of .All that doesn't do the worst part - doesn't require reflectability of interface methods.
We don't do much for nested type options in
DynamicallyAccessedMemberTypes, forcing the much broader scopeAllto be used if we want to preserve members inside nested types. This fix is to keep all the members for nested types.This impacts
EventSourcesince the type is tagged currently withAlland there is a related discussion around size. Unfortunately,EventSourceneeds to preserve its members as well and there does not seem to be much size saving onEventSourcewith this change.Fixes #1174