Add WinForms Analyzer and CodeFixes Infrastructure for C# and Visual Basic #11515

KlausLoeffelmann · 2024-06-10T06:09:36Z

This PR introduces/extends the WinForms Analyzer infrastructure for C# and Visual Basic.
We will be introducing for .NET 9 a first set of Analyzers and Code Fixes, specifically for WinForms applications. The Analyzers will help developers to identify common issues in their WinForms applications and the Code Fixes will help to automatically fix these issues.

The PR includes the following changes:

Refactor existing Analyzers in preparation to add VB Projects both for Analyzers and Code Fixes.
Introduce Analyzers/CodeFixes for missing CodeDOM serialization configuration both for C# and VB.
Bump up Roslyn/Analyzer NuGet package versions.
Fix formatting in resource files and inconsistencies in unshipped files.
Setup remaining analyzer projects.
Finalize analyzer configuration.

Microsoft Reviewers: Open in CodeFlow

codecov · 2024-06-11T07:22:50Z

Codecov Report

Attention: Patch coverage is 97.09677% with 9 lines in your changes missing coverage. Please review.

Project coverage is 74.84430%. Comparing base (c3cb9b3) to head (3cf1630).
Report is 1 commits behind head on main.

Additional details and impacted files

@@                 Coverage Diff                 @@
##                main      #11515         +/-   ##
===================================================
+ Coverage   74.78710%   74.84430%   +0.05720%     
===================================================
  Files           3021        3012          -9     
  Lines         629289      629269         -20     
  Branches       46689       46692          +3     
===================================================
+ Hits          470627      470972        +345     
+ Misses        155301      154936        -365     
  Partials        3361        3361

Flag	Coverage Δ
Debug	`74.84430% <97.09677%> (+0.05720%)`	⬆️
integration	`18.03813% <ø> (+0.07912%)`	⬆️
production	`47.83718% <94.91525%> (+0.09464%)`	⬆️
test	`97.02004% <100.00000%> (+0.03443%)`	⬆️
unit	`44.86531% <94.91525%> (+0.00807%)`	⬆️

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

RussKie · 2024-06-18T11:47:33Z

eng/packageContent.targets

-      <AnalyzerTargetLanguage Condition="$(_AnalyzerTargetLanguage.Contains('.CSharp', StringComparison.OrdinalIgnoreCase))">/cs</AnalyzerTargetLanguage>
-      <AnalyzerTargetLanguage Condition="$(_AnalyzerTargetLanguage.Contains('.VisualBasic', StringComparison.OrdinalIgnoreCase))">/vb</AnalyzerTargetLanguage>
+      <AnalyzerTargetLanguage Condition="$(_AnalyzerTargetLanguage.Contains('.CSharp', StringComparison.OrdinalIgnoreCase))">/CSharp</AnalyzerTargetLanguage>
+      <AnalyzerTargetLanguage Condition="$(_AnalyzerTargetLanguage.Contains('.VisualBasic', StringComparison.OrdinalIgnoreCase))">/VisualBasic</AnalyzerTargetLanguage>


❗ cs and vb are established SDK folder conventions

CS conflicts with the Czeck resource folder.

Yes, it does. And I was as surprised and voiced my concerns about it, but that's the established convention. This is why Windows Forms and Windows Desktop analyzers are placed as they are:
dotnet/windowsdesktop#1693

It's been several years but I believe I talked with @ericstj about this (as he designed the inbox analyzer experience). I suggest chatting with Eric first.

The convention predates inbox analyzers. It's the same story for NuGet analyzers. https://learn.microsoft.com/en-us/nuget/guides/analyzers-conventions#analyzers-path-format

The actual placement of the files for the inbox analyzers is actually not critical to their inclusion. The inclusion is determined by the FrameworkList.xml - but we chose to keep the same convention for placement in the targeting pack, just as we choose the same convention for the reference assemblies in the targeting pack (ref/tfm).

IOW - you could probably put them in different paths if you want, but it's done this way for consistency with the publicly documented thing.

Thank you @ericstj! No idea how a mere mortal can find these links. Gotta work more on my bing-fu and google-fu :)

The FrameworkList.xml is something I patched, when I was testing the "results", and so far it worked fine with the changed folders. But if it is an agreed upon convention, I can change it back, no problem. The thing is, I was REALLY searching for it, and - convention yes or no - CSharp and VisualBasic just would help so much more for discoverability.

Also, I have yet to test this branch by building a dedicated SDK based on the package of this branch, and see, if everything is ending up where it belongs. What would be the general approach to do that? Clone the SDK repo and patch the NuGet sources?

Are you talking about testing your changes and getting them deployed to the Windows Desktop SDK?

For that (IIRC, it's been few years) I remember creating experimental branches in both dotnet/winforms and dotnet/wpf from which I'd publish to BAR (you'll need to create private subscriptions in darc), and see the changes go all the way to dotnet/windowsdesktop. This flow allows you to verify that the transport packages contain all the assemblies (and anything else) you expect.

Changing it back to VB and CS for consistency.

src/System.Windows.Forms.Analyzers.CSharp/src/AnalyzerReleases.Unshipped.md

...arp/src/System/Windows/Forms/CSharp/Generators/ApplicationConfiguration/ProjectFileReader.cs

...trolPropertySerializationConfiguration/ControlPropertySerializationDiagnosticAnalyzerTest.cs

...rp/tests/UnitTests/System/Windows/Forms/Generators/ApplicationConfigurationGeneratorTests.cs

KlausLoeffelmann · 2024-06-19T06:54:41Z

@RussKie: I would very much appreciate if you - in the future - respect my ask to NOT start reviewing a draft PR, especially when I point out that it is not ready for review. I appreciate your valuable engagement, but if I am in the process to completely restructure this, (but still want to see at the end of the day, if pipelines pass), premature reviews generate too much useless noise comments, half of which become redundant, anyway.

UPDATE: No longer Draft.

lonitra

Would it make sense to include the other code fixes like generating ShouldSerializeX or DefaultValue?

src/System.Windows.Forms.Analyzers.CSharp/src/Resources/SR.resx

lonitra · 2024-06-25T22:28:00Z

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs

+
+        // Is the read/write and at least internal?
+        if (propertySymbol.SetMethod is null
+            || propertySymbol.DeclaredAccessibility < Accessibility.Internal)


Why do we include internal?

A UserContrtol or a Form can serialize Control properties which are scoped to the same assembly.

Could you detail how to reproduce that? I had tried creating a UserControl with an internal property type that would be binary serialized into the resx e.g. PointF, and I didn't see it appear in the resx.

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs

...singPropertySerializationConfiguration/ControlPropertySerializationDiagnosticAnalyzerTest.cs

...ixes/AddDesignerSerializationVisibility/AddDesignerSerializationVisibilityCodeFixProvider.cs

lonitra · 2024-07-15T20:24:38Z

src/Microsoft.VisualBasic.Forms/src/Microsoft.VisualBasic.Forms.vbproj

@@ -9,7 +9,7 @@
    <AssemblyName>Microsoft.VisualBasic.Forms</AssemblyName>
    <Deterministic>true</Deterministic>
    <RootNamespace></RootNamespace>
-    <LangVersion>15.0</LangVersion>
+    <LangVersion>15.5</LangVersion>


Any reason why we are moving to 15.5 specifically and not just using latest?

Also
Option Explicit On
Option Strict On
Option Infer Off

That is what is specified in most files, that should be centralizing here.

Why would we do Option Infer Off?
I don't like that at all. Let's please do not introduce that as standard, unless there are really good reasons for that I would be missing?!

...indows/Forms/CSharp/Generators/ApplicationConfiguration/ApplicationConfigurationGenerator.cs

...ixes/AddDesignerSerializationVisibility/AddDesignerSerializationVisibilityCodeFixProvider.cs

lonitra · 2024-07-15T20:34:11Z

...ixes/AddDesignerSerializationVisibility/AddDesignerSerializationVisibilityCodeFixProvider.vb

+                    Function(al) al.Attributes).Any(
+                        Function(a) a.Name.ToString() = DesignerSerializationVisibilityAttributeName) Then
+
+                Debug.Assert(False, "The attribute should not be there.")


Suggested change

Debug.Assert(False, "The attribute should not be there.")

Debug.Fail("The attribute should not be there.")

...stem.Windows.Forms.Analyzers/src/System/Windows/Forms/Analyzers/Diagnostics/DiagnosticIDs.cs

...s.Forms.Analyzers/tests/UnitTests/System/Windows/Forms/Analyzers/AppManifestAnalyzerTests.cs

src/System.Windows.Forms.Analyzers/src/System/Windows/Forms/Analyzers/IsExternalInit.cs

…gnostics.

lonitra

🚀

To fix new error 'WFO1000' from the analyzers added in dotnet/winforms#11515

* Update to RC1 versions * Update maui branding to rc1 * Update Versions.props * Update SDK dependency name * Add [DefaultValue] attribute to properties of WinForms BlazorWebView To fix new error 'WFO1000' from the analyzers added in dotnet/winforms#11515 * Update rc1 versions Update versions Update sdk and runtime rc1 Update versions Update sdk , ios and android versions Update Versions.props More update iOS and android Update versions Update iOS Update packages aspnet and sdk Update sdk , ios and aspnet * Update versions * Update sdk * Update Versions.props --------- Co-authored-by: Eilon Lipton <[email protected]>

KlausLoeffelmann self-assigned this Jun 10, 2024

KlausLoeffelmann added this to the .NET 9.0 milestone Jun 10, 2024

dotnet-policy-service bot added the draft draft PR label Jun 10, 2024

KlausLoeffelmann added the Analyzer (Roslyn) A Roslyn Analyzer is either needed for the context, needs to be scope extended or fixed. label Jun 10, 2024