Fix dataflow analysis for GetNestedType
https://github.com/dotnet/linker/pull/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 provide reflectability of interface methods.
Opening as draft, we'll want to also adjust marking appropriately.
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
Note regarding the new-api-needs-documentation label:
This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas See info in area-owners.md if you want to be subscribed.
This pr currently has 3 parts:
- Fix dataflow analysis to reflect what we do when marking things for DAMT.NestedTypes: we actually also keep members, so don't treat that as DAMT.None
- Take advantage of this in EventSource. This should be a separate pr but i wanted measurements. It's a 0.3% size saving on webapiaot. I'll take it.
- Adjust marking to not to keep .All, but just all members (afaik the only difference is that .All will also make members on implemented interfaces reflection-visible, potentially causing a ton of damage if it's a popular generic interface)
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?
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.
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.Interfacesfor example?
EventSource wouldn't need the interfaces. I chose this based on the discussion in https://github.com/dotnet/linker/issues/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 .Interfaces, so probably doesn't hurt to keep it. I like that this at least leaves open the option to stop marking members of interfaces.
/ba-g deadlettered wasm tests