-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Bubble up formatter trim warnings #11023
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11023 +/- ##
===================================================
+ Coverage 73.22354% 73.34303% +0.11948%
===================================================
Files 3096 3094 -2
Lines 633620 632450 -1170
Branches 47336 46945 -391
===================================================
- Hits 463959 463858 -101
+ Misses 166112 165110 -1002
+ Partials 3549 3482 -67
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -263,6 +268,7 @@ internal static class Formatter | |||
/// - Uses TypeConverters or IConvertible where appropriate | |||
/// - Throws a FormatException if no suitable conversion can be found | |||
/// </summary> | |||
[RequiresUnreferencedCode(ComponentModelTrimIncompatibilityMessage)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code path hit at runtime by the test app? I don't know whether we should bubble up warnings, or carve out a trim-compatible path through this method. If we bubble it up, I'd like to understand where we are going to cut off the annotations so that the test app doesn't hit warnings due to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ParseObjectInternal
path is not hit at runtime. Added feature guards on the call stack path that reach the binding boundary. FormatObjectInternal
is hit at runtime by the runtime app and bubbled up its RUC to publish methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just had two suggestions for making messages a bit clearer - otherwise fine!
src/System.Windows.Forms/src/System/Windows/Forms/Internal/Formatter.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Klaus Löffelmann <9663150+KlausLoeffelmann@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bubbling up trim warnings to the public surface or to the binding feature boundary area seen by the TrimTest project. This is the first iteration of getting the
TrimTest
project warnings towards zero as discussed here. Binding feature boundary is guarded by a feature check and added a RUC toBindingContext
setter so as to generate warnings when using binding in WinForms trimming. The intention is applications likeTrimTest
that doesn't use binding will not generate warnings but applications that do use binding will get warnings at build time (when they useBindingContext
). The runtime behavior will be a throw for trimmed WinForms applications if they use binding.Microsoft Reviewers: Open in CodeFlow