winforms Clean up most of the analyzer info

This turns a number of the code analyzers into warnings and fixes them. Most of the outstanding messages are fixed.

It wasn't particularly easy to break this up as the analyzers are pretty squidgy about reporting what is outstanding. Everything from this point should be much more focused.

We have significant weirdness with collection initialization syntax. It is ambiguous for most of our collection types and AddRange. Behavior varies between debug and release. Hopefully adding params of span overloads will address the ambiguity issue so that auto-fixes will actually create code that builds.

Microsoft Reviewers: Open in CodeFlow

Mar 14 '24 02:03 JeremyKuhne

Apologies to other pending changes. Ping me with any questions on conflict resolution. I think this is the last of the massive changes.

Mar 14 '24 02:03 JeremyKuhne

Codecov Report

Attention: Patch coverage is 50.70165% with 808 lines in your changes are missing coverage. Please review.

Project coverage is 73.19756%. Comparing base (b1b2c54) to head (0630b69).

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11050         +/-   ##
===================================================
+ Coverage   73.18970%   73.19756%   +0.00785%     
===================================================
  Files           3097        3097                 
  Lines         633749      633890        +141     
  Branches       47368       47348         -20     
===================================================
+ Hits          463839      463992        +153     
+ Misses        166358      166347         -11     
+ Partials        3552        3551          -1

Flag	Coverage Δ
Debug	`73.19756% <50.70165%> (+0.00785%)`	:arrow_up:
integration	`18.42950% <5.80524%> (-0.00864%)`	:arrow_down:
production	`46.68463% <26.63551%> (-0.00537%)`	:arrow_down:
test	`94.97091% <95.95782%> (+0.00433%)`	:arrow_up:
unit	`43.59333% <24.76636%> (+0.02533%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Mar 14 '24 03:03 codecov[bot]

A lot of it is style Using imports and just function calls vs Function calls the include part or all of the name space. I prefer long imports with short statements, VB has a mixture of both (even in same file) so you get a lot of suggestions. Reviewer seem to have different opinions and sometimes prefer VB import aliases. Since VB is already verbose I prefer things that make lines shorter and more readable.Also I have split editor.config with specific preferences for VB and C#, Klaus prefers longer more explicit statements for example. Dim x as Integer = 6‘ first is preferred for VB but C# is opposite Dim y as BooleanIf x = 3 then y = TrueElse y FalseEnd If‘ Vs thisDim y as Boolean = x = 3His reasoning as I understand it is most people reading VB code will not understand some of the VB shortcuts and compiler generates same code. Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Mar 13, 2024, at 5:15 PM, Jeremy Kuhne @.***> wrote: Apologies to other pending changes. Ping me with any questions on conflict resolution.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you are subscribed to this thread.Message ID: @.***>

Mar 14 '24 03:03 paul1956

This is why, you write tests before this kind of automated change. Automated code fixes in Roslyn are very good but not perfect as several people have found out but usually they just cause errors vs silently do the wrong thing. Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Mar 13, 2024, at 8:07 PM, Tanya Solyanik @.***> wrote: @Tanya-Solyanik approved this pull request.

I scrolled through this change, but I hope that code fixes work correctly, as I don't trust my eyes anymore.

In src/System.Windows.Forms.Design/src/System/ComponentModel/Design/ArrayEditor.cs:

@@ -44,7 +44,7 @@ protected override object[] GetItems(object editValue) /// protected override object SetItems(object editValue, object[] value) {

   if (editValue is not null && !(editValue is Array))

   if (editValue is not null and not Array)

This could be simplified as (editValue is not Array)

In src/System.Windows.Forms.Design/src/System/ComponentModel/Design/CollectionEditor.CollectionEditorCollectionForm.cs:

@@ -997,9 +993,7 @@ private void UpButton_Click(object? sender, EventArgs e) { SuspendEnabledUpdates(); int ti = _listBox.TopIndex;

           object itemMove = _listBox.Items[index];

           _listBox.Items[index] = _listBox.Items[index - 1];

           _listBox.Items[index - 1] = itemMove;

           (_listBox.Items[index - 1], _listBox.Items[index]) = (_listBox.Items[index], _listBox.Items[index - 1]);

What rule is this?Or was this a manual change?

In src/System.Windows.Forms.Design/src/System/Windows/Forms/Design/CommandSet.cs:

                           if (componentNames is not null && idx < componentNames.Length)

                           // If we couldn't find a property for this event, or of the property is read only, then

```
                           // abort.
```
```
                           //
```

remove this line

In src/System.Windows.Forms.Primitives/src/System/Windows/Forms/Internals/ThemingScope.cs:

@@ -35,7 +35,7 @@ public ThemingScope(bool useVisualStyles) } }

public void Dispose()

public readonly void Dispose()

This is weird... readonly void?

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

Mar 14 '24 06:03 paul1956

@paul1956 We can iterate on VB style with @KlausLoeffelmann.

While there is some risk to making changes of this magnitude the benefits to the long-term health of the codebase are significant. We had over 6000 "info" messages in the repo (probably more than 10K before I fixed a number of others). It was difficult to see whether or not you were introducing additional ones when modifying/adding code. It also had a significant impact on the perf in VS. This will also reduce contribution overhead as we won't have to spend as much time manually pointing out style guidelines. :)

Mar 14 '24 17:03 JeremyKuhne

I am a huge proponent of getting to 0 “info” messages, when I managed software development in the stone age it was a requirement for every project.I have written many code fix’s beyond what Roslyn offers like forcing VB Event names to match event handlers, which rename doesn’t do and fix accidentally deletion of handles clause when designer removed them (basically mirror fixes).I was just pointing out they have been know to break code as was pointed out to me by @kirsan31 when I tried to make DataVisualization “info” message free.Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Mar 14, 2024, at 8:17 AM, Jeremy Kuhne @.***> wrote: @paul1956 We can iterate on VB style with @KlausLoeffelmann. While there is some risk to making changes of this magnitude the benefits to the long-term health of the codebase are significant. We had over 6000 "info" messages in the repo (probably more than 10K before I fixed a number of others). It was difficult to see whether or not you were introducing additional ones when modifying/adding code. It also had a significant impact on the perf in VS. This will also reduce contribution overhead as we won't have to spend as much time manually pointing out style guidelines. :)

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

Mar 14 '24 18:03 paul1956