Explore WinForms feature switch coverage in System.Windows.Forms project #10866
Conversation
LakshanF
requested review from
sbomer,
JeremyKuhne,
KlausLoeffelmann,
vitek-karas and
lonitra
| @@ -2398,6 +2399,7 @@ private void CreateInstance() | |||
| return null; | |||
| } | |||
|
|
|||
| [RequiresUnreferencedCode("Calls System.ComponentModel.TypeDescriptor.GetProperties(Object)")] | |||
There was a problem hiding this comment.
This only comes into play when running in the designer (when you actually have a Site for the Component). I'm not sure how we would deal with this in a way that would give meaningful feedback. It isn't an expected runtime scenario.
I think this is one big bucket of things- anything that touches the Site.
There was a problem hiding this comment.
I would actually expect annotating is the right answer here. Basically, we need to wall off the designer code from the app code, right?
So a mechanical way to do that is to annotate everything that uses the designer as RUC. If that code is successfully removed from all trimming code paths, the RUC warning won't fire because the code won't be called.
But this approach also helps catch bugs because it ensures that if a reference is ever added in the future, it will warn.
| @@ -30,6 +30,7 @@ public override void RegisterEvents(Com2PropertyDescriptor[]? properties) | |||
| /// Here is where we handle IVsPerPropertyBrowsing.GetLocalizedPropertyInfo and IVsPerPropertyBrowsing. | |||
| /// Hide properties such as IPerPropertyBrowsing, IProvidePropertyBuilder, etc. | |||
| /// </summary> | |||
| [RequiresUnreferencedCode("Calls System.Windows.Forms.ComponentModel.Com2Interop.Com2IManagedPerPropertyBrowsingHandler.GetComponentAttributes(IVSMDPerPropertyBrowsing*, Int32)")] | |||
There was a problem hiding this comment.
This class is highly unlikely to come up. It is a VS specific support interface.
|
There is a big swath of design-time only issues here. |
|
Would it make sense to approach this from a different angle - build a blank WinForms app (with WPF removed) and start with addressing the warnings there until we get into a clean state for the blank app? This diff is rather overwhelming and there's a lot of advanced scenarios such as ActiveX that we maybe don't need to solve right now. The diff will only get bigger. (Also the unnecessary whitespace diffs in enum formatting are not helping.) Maybe this is not the case for the other reviewers, but I don't have much here to comment on because it touches so many things and I don't see myself drilling into the implementation and static callgraph of every method this touches to understand how it's used. I just have the generic guideline of "please please don't suppress warnings". There's a good chance the suppression will be wrong even with all the expertise we have Cc'd on the review. I'm filing an issue on a wrong suppression (#10868) I found in existing code while clicking around this PR. |
|
Agreed with Michal on the "start with an app" approach. I do think the eventual goal should be to annotate everything, so we can get static validation that it's all being properly removed, but there's just a lot of code in WinForms and starting with the template app makes sense |
This is a draft PR to solicit feedback and not meant to be merged.
Remove
nowarnfor IL2026 and explore the impact on trimming. The RUC warnings are currently at leaf level and making WinForms assemblies trim compatible require blubbling these warning up to public API level or suppressing the warnings. Bubbling up the warnings from the leaf level to public API root level will encompass a fairly large graph of API nodes, and likely make WinForms unusable with trimming. Suppressing warnings, especially at the leaf level can reduce the size of this graph but can be potentially very dangerous if not done correctly and require careful analysis to ensure that the trimmed WinForms surface area work correctly. The current plan is to use a feature switch to guard against trim problematic features in a trimmed WinForms application.This PR explores the space of suppression so as to understand the WinForms feature switch coverage and get feedback from domain experts on problematic areas for trimming to further expand the feature switch coverage. Current identified areas where a feature switch will restrict trimmed WinForms are:
The commits in this PR show the impacted APIs from the leaf level bubbling up the call graph. It doesn't show the full graph since the goal is to identify the suppression points closer to the leaf level APIs. These commits also show suppressions applied at leaf level (those no longer need to be bubbled up). Current suppressions are not done as we envision how we do this in the final product. For example, the suppression is applied at method level instead of the exact location callsite. This approach was adopted to expedite the feedback.
These commits show base classes and interfaces that could cause trimming concerns since derived classes call RUC methods. This can be seen via the IL2046 warning. This might mean that the feature guard could restrict usage on these types for a trimmed WinForms application. Some of these impacted types are;
Microsoft Reviewers: Open in CodeFlow