Fix dataflow analysis for GetNestedType #109814
Conversation
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-issue-labeler
bot
added
needs-area-label
|
Note regarding the |
1 similar comment
|
Note regarding the |
MichalStrehovsky
added
linkable-framework
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas |
|
This pr currently has 3 parts:
The third one is running into some weird test failures, I'm not sure we want to go there. Doesn't look to help much extra for webapiaot size so I'd scope it out, we can always revisit. Anyone have opinion? |
Scoping it out for now sounds like a good plan to me. |
There was a problem hiding this comment.
Implementation LGTM, thanks!
I'm exposing this not as .All, but a subset of .All that doesn't do the worst part
Curious how you determined that this was sufficient - is it based on the EventSource usage? Would it be possible to leave out DAMT.Interfaces for example?
EventSource wouldn't need the interfaces. I chose this based on the discussion in dotnet/linker#1174 - if one wants to keep a nested type - they probably want to do something with it, so we should better allow all things and .Interfaces is one of the members on DAMT. I'm indifferent on the .Interfaces - interface list is not a member. We just added it to DAMT because we needed it. If you'd like to first try without .Interfaces, I can make that change. We can always add that later based on feedback, I don't think that one would be breaking in any way. |
|
I don't feel strongly about it either way. I was just curious about the reasoning - I don't think there will be much impact for |
|
/ba-g deadlettered wasm tests |
This takes advantage of dotnet#109814. This is in theory a breaking change in case someone took advantage of our annotation and is doing their own reflection on `EventSource` descendants. Someone else reflecting on EventSource is problematic however. We placed around various suppressions due to our own annotations like this: https://github.com/dotnet/runtime/blob/0d62887a30553b8177dc90f9e39559be0e6c7707/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L414-L424 It means that if someone else is reflection-accessing these, they would not get a trimming warning. Hopefully, nobody does that. This PR also assumes that nobody does that (and the PR should therefore not be breaking in practice). We annotate the class for our purposes, but someone else could be taking advantage of that in their own code.
This takes advantage of #109814. This is in theory a breaking change in case someone took advantage of our annotation and is doing their own reflection on `EventSource` descendants. Someone else reflecting on EventSource is problematic however. We placed around various suppressions due to our own annotations like this: https://github.com/dotnet/runtime/blob/0d62887a30553b8177dc90f9e39559be0e6c7707/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L414-L424 It means that if someone else is reflection-accessing these, they would not get a trimming warning. Hopefully, nobody does that. This PR also assumes that nobody does that (and the PR should therefore not be breaking in practice). We annotate the class for our purposes, but someone else could be taking advantage of that in their own code.
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.
…10001) This takes advantage of dotnet#109814. This is in theory a breaking change in case someone took advantage of our annotation and is doing their own reflection on `EventSource` descendants. Someone else reflecting on EventSource is problematic however. We placed around various suppressions due to our own annotations like this: https://github.com/dotnet/runtime/blob/0d62887a30553b8177dc90f9e39559be0e6c7707/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Tracing/EventSource.cs#L414-L424 It means that if someone else is reflection-accessing these, they would not get a trimming warning. Hopefully, nobody does that. This PR also assumes that nobody does that (and the PR should therefore not be breaking in practice). We annotate the class for our purposes, but someone else could be taking advantage of that in their own code.
dotnet/linker#2133 adjusted rooting logic to keep
.Allon 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 provide reflectability of interface methods.
Opening as draft, we'll want to also adjust marking appropriately.