Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Epic - Make WinForms trim compatible #4649

Open
3 of 13 tasks
agocke opened this issue Mar 5, 2021 · 14 comments
Open
3 of 13 tasks

Epic - Make WinForms trim compatible #4649

agocke opened this issue Mar 5, 2021 · 14 comments
Assignees
Labels
area-ILLinker/AOT area-Trimming Epic Groups multiple user stories. Can be grouped under a theme. Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@agocke
Copy link
Member

agocke commented Mar 5, 2021

The TrimTest project tracks a simple WinForms application for trimming purposes. This project generates 2535 trim warnings -
Default_Trimmed_Warnings.txt.

Below issues track the work related to getting the warnings addressed.

Linked to this epic

@RussKie RussKie added area-Infrastructure waiting-review This item is waiting on review by one or more members of team labels Mar 10, 2021
@RussKie RussKie added this to the 6.0 milestone Mar 10, 2021
@RussKie RussKie removed the waiting-review This item is waiting on review by one or more members of team label Mar 11, 2021
@RussKie
Copy link
Member

RussKie commented Mar 11, 2021

From an offline chat with @agocke:

(These) warnings effectively represent reflection that the toolchain can't figure out. In most cases the warning is probably about a use of a particular reflection method and will tell you that the System.Type you're operating on isn't marked with the same requirements as the reflection method requires. After finding the source of the System.Type you have two options:

  1. The type is statically known, like typeof or even a string literal with the full name
  2. The type is retrieved dynamically, like from an arbitrary object.GetType() or from a name from the user

In case (1), this is likely just an annotation problem. You need to use the DynamicallyAccessedMembers attribute to walk back through the code path, and mark the type as requiring the specific capabilities. When you get back to the original typeof call or whatever, the toolchain will be able to figure out what it needs to preserve on the type.

In case (2), this is fundamentally not linker safe. There's no guarantee that the thing you're reflecting against won't be trimmed, and the member won't be gone at runtime. For that it's usually recommended that you use RequiresUnreferencedCodeAttribute on the method. It silences the method warnings, but produces a warning for anyone calling that method. You can keep doing this until you bubble it up to the public APIs, where users will get a warning that the method is not linker safe.

Useful docs:

@weltkante
Copy link
Contributor

weltkante commented Mar 11, 2021

I find no mention of COM interop in the links, the warnings about COM interop not being supported are worrying (IL2050: xxx declares a parameter with COM marshalling. Correctness of COM interop cannot be guaranteed after trimming. Interfaces and interface members might be removed.) since this is fundamental functionality to communicate with Windows (WinForms without clipboard, drag'n'drop, RichEdit, accessibility etc. would not be the same)

Furthermore the text mentions trimming of COM interfaces yet only reports P/Invoke signatures. That has me worried that ComImport is not recognized by the linker and also gets broken (otherwise the comment about trimming interface members makes not much sense). Trimming members of ComImport interfaces is definitiely not allowed (changes method order which is fatal for IUnknown based interfaces) and I'd expect this to be easily recognizable (just check the attribute) so I'm confused what the warning means in the first place.

I really hope these are just false positives and COM interop is supported, or at least can be supported by custom annotations, otherwise I don't think WinForms can be made compatible.

@agocke agocke changed the title Determine linker compatibility for .NET 6 WinForms is not trim compatible May 1, 2021
@merriemcgaw merriemcgaw modified the milestones: 6.0, 7.0 Jun 11, 2021
@merriemcgaw merriemcgaw added the Epic Groups multiple user stories. Can be grouped under a theme. label Dec 4, 2021
@merriemcgaw
Copy link
Member

Adding this to our dotnet/planning/issues/27 theme

@merriemcgaw merriemcgaw changed the title WinForms is not trim compatible Epic - WinForms is not trim compatible Dec 4, 2021
@OliaG OliaG changed the title Epic - WinForms is not trim compatible Epic - Make WinForms trim compatible Dec 6, 2021
@merriemcgaw merriemcgaw added Priority 2 Priority:2 Work that is important, but not critical for the release and removed Priority 2 labels Jan 19, 2022
kant2002 added a commit to kant2002/winforms that referenced this issue Feb 23, 2022
ghost pushed a commit that referenced this issue Mar 30, 2022
@kant2002
Copy link
Contributor

kant2002 commented Apr 4, 2022

I did try trim recent version of WinForms and what are the log which I receive 640 rows.
with_design.txt

Most issues which I see is from Design namespace which for most application does not make sense. Also I do not think that in practice that would make sense to target that namespace without additional work on Designer which happens in private.

So I remove from log these warnings and receive smaller file of 260 rows.
without_design.txt

I look again, and next target for questionable warnings would be Converters which is mostly used during design-time. Also DefaultValueAttribute somewhat irrelevant for "trimming-app".

So I would like to ask questions

  • How that Design namespace issues can be hidden from trim warnings?
  • Can whole issue be splitted in something more manageable. For example I see "design" "typeconverters" "binding", "resources" areas which can be worked separately. Please correct me if I wrong here. Since this issue looks so big, nobody want to bother, but if split issue on something smaller, it would be easier to handle.
  • Can issue to be reframed, and instead of "WinForms not linker friendly" clear messages where WinForms has hard time with ILlink to be articulated?

@RussKie
Copy link
Member

RussKie commented Apr 7, 2022

Most issues which I see is from Design namespace which for most application does not make sense. Also, I do not think that in practice that would make sense to target that namespace without additional work on Designer which happens in private.

The designer codebase has its own copies of several runtime types (for various reasons). Most of the types that are present in this codebase can and are used in runtime scenarios (see https://github.com/dotnet/winforms/issues?q=label%3A%22area%3A+designer+support%22+sort%3Acreated-desc), e.g., editors and type converters are invoked via the PropertyGrid control. Some other types from the "designer" namespace can be used in general-purpose designers, such as a report viewer control (see #4908 for more info).

@RussKie
Copy link
Member

RussKie commented Apr 7, 2022

  • Can whole issue be splitted in something more manageable. For example I see "design" "typeconverters" "binding", "resources" areas which can be worked separately. Please correct me if I wrong here. Since this issue looks so big, nobody want to bother, but if split issue on something smaller, it would be easier to handle.

Right now this epic isn't high on the team's backlog, and you're paving the road :) (and we are very appreciative of your work, thank you). Which means you can split and dice it any way you wish.

with_design.txt contains only the following warnings: IL2050, IL2094, IL2093, IL2092, IL2046, IL2026, IL2072, IL2075, IL2067, IL2057, IL2062, IL2070, IL2111, IL2087. without_design.txt contains all the same list without IL2087.
Maybe the work can be split by addressing each warning type?

@kant2002
Copy link
Contributor

kant2002 commented Apr 7, 2022

with_design.txt contains only the following warnings: IL2050, IL2094, IL2093, IL2092, IL2046, IL2026, IL2072, IL2075, IL2067, IL2057, IL2062, IL2070, IL2111, IL2087. without_design.txt contains all the same list without IL2087.

That's interesting observation. I did not notice that.

Maybe the work can be split by addressing each warning type?

I would like (for now) discuss what's in principle should be done. Kind of meta-discussion. I would like to get rid of undocumented _SuppressWinFormsTrimError property introduced in https://github.com/dotnet/sdk/pull/19409/files . That cannot happens until team decide that earlier issues no longer a problem. I have only vague understanding why this block was introduced. If only reasons stated here https://github.com/dotnet/sdk/pull/16892/files then with ComWrappers work ongoing, that block can be removed and resurrect warning for example. If something else, then would be good if it is spelled out explicitly.

My understanding is that System.Windows.Forms.Design assembly provide supplementary code for WinForms designer. They provide different kind of actions which can happens with control, hints, etc. That's important part of experience for WinForms, but that code mostly run in Designer app. So trimming block does not applied to that code IMO.

e.g., editors and type converters are invoked via the PropertyGrid control.

Yes. That's kind of problems which I would like to surface in my attempt to assess what's blocking this issue. I completely forgot about UITypeEditor and friends which is used a lot inside PropertyGrid and lives in Design namespace.

However we need to port the general purpose designer infrastructure – i.e. API that enable building "a designer that the user can embed in their application". For example, a user might have a business application that contains a report designer feature, which can use our "general designer" framework.

From where I am now, I cannot assess, is this kind of applications can prevent removal of _SuppressWinFormsTrimError or not. I looking for smallest possible amount of work, so I would like to probe what you consider acceptable, and what not. If by a chance I can do not annotate Design namespace, I would prefer not to.

@jkotas
Copy link
Member

jkotas commented Apr 7, 2022

How that Design namespace issues can be hidden from trim warnings?

Make sure that the code from Design namespace is not getting rooted and that it gets trimmed in a typical WinForms apps. Code that gets trimmed won't produce warnings.

https://docs.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming is a general guidance for how to prepare libraries for trimming.

ghost pushed a commit that referenced this issue Jun 15, 2022
- IL2050 requires more ComWrappers work, don't want touch that
- IL2026 is `RequiresUnreferencedCodeAttribute` which require a bit more
brainpower.  Better keep that for later when more code would be annotated.

Jab at #4649
@JeremyKuhne
Copy link
Member

Comment on tooling: #7256 (comment)

@mikola-akbal
Copy link

Please, make Windows Forms compatible with trimming.

@mikola-akbal
Copy link

mikola-akbal commented Jul 25, 2023

Can you estimate when Native AOT will support Windows Forms? Time or version of .NET.

@KlausLoeffelmann
Copy link
Member

KlausLoeffelmann commented Jan 26, 2024

@merriemcgaw, just to get this Epic on your radar for planning.
I think due to the scope, it's not a good candidate for tracking it your projects, but I just wanted to ping you that we're maintaining this.

Also @elachlan FYI (I am sure, he wants to be in the loop).

@KlausLoeffelmann
Copy link
Member

@mikola-akbal: Just as an update: we have started work in the runtime a while ago, and I am starting to investigate Binding from the runtime and the Out-Of-Proc Designer side.

@JOULTICOA
Copy link

JOULTICOA commented Apr 29, 2024

Bless all of you working on this. If you've been in the weeds its easy to forget how revolutionary (and massive) this is.

Q: If this doesn't quite make it completely for 9 STS, will that mean that it won't make it into 10 LTS? or will it probably end up in 10 LTS 90% complete regardless?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-ILLinker/AOT area-Trimming Epic Groups multiple user stories. Can be grouped under a theme. Priority:2 Work that is important, but not critical for the release
Projects
Status: No status
Development

No branches or pull requests