[release/9.0] Fixing Analyzer issues, servicing, Part 2: Actual Fixes #13182

KlausLoeffelmann · 2025-03-25T00:01:47Z

Part 2 of servicing Analyzer fixes back to release/9.0.

This PR contains the actual fixes along with the added tests based on the back ported infrastructure from .NET 10.

See Part 1, Changing the Infrastructure, here:
#13072

Microsoft Reviewers: Open in CodeFlow

Fixes WFO1000 Analyzer: False positives on private properties without InitializeComponent method #13205
Fixes WFO1000 Analyzer: False positives on static properties of components #13206
Fixes WFO1000 Analyzer: False positives on overridden properties of standard control descendants #13207
Fixes WFO1000 Analyzer: False positives on properties of interfaces derived from IComponent #13208
Fixes WFO2001: Analyzer doesn't trigger the 'wrong async overload' warning with implicit member access. #13237
Fixes [Tracking] .NET 9 Analyzers Related Issues for Servicing #13160

Please note, that these fixes have been done and checked, so the respective other language applied, both in C# and Visual Basic, and then Unit Tests had been added to cover those scenarios.

From here, once these are OK'ed, we will back-port them (again, if you will, because the originally came from .NET 10 main), to .NET 10, and the new Clipboard Analyzer's tests will then be updated to integrate into the new test infra.

And after that, they can go into main/net10.

…_ IComponent.

codecov · 2025-03-31T00:54:00Z

Codecov Report

Attention: Patch coverage is 95.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 74.79265%. Comparing base (44f90e2) to head (4436879).
Report is 20 commits behind head on release/9.0.

Additional details and impacted files

@@                  Coverage Diff                  @@
##           release/9.0      #13182         +/-   ##
=====================================================
- Coverage     74.83821%   74.79265%   -0.04557%     
=====================================================
  Files             3022        2997         -25     
  Lines           630460      629261       -1199     
  Branches         46797       46742         -55     
=====================================================
- Hits            471825      470641       -1184     
- Misses          155247      155258         +11     
+ Partials          3388        3362         -26

Flag	Coverage Δ
Debug	`74.79265% <95.00000%> (+4.24119%)`	⬆️
integration	`18.00682% <ø> (-0.00378%)`	⬇️
production	`47.78797% <ø> (-0.07921%)`	⬇️
test	`96.99183% <95.00000%> (-0.04999%)`	⬇️
unit	`44.79785% <ø> (-0.08361%)`	⬇️

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ionConfiguration. * Addresses 13205, 13206, 13207, 13208.

…sts.

Tanya-Solyanik · 2025-04-04T21:34:35Z

...System.Windows.Forms/tests/IntegrationTests/WinformsControlsTest/WinformsControlsTest.csproj

@@ -12,6 +12,7 @@
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <EnableUnsafeBinaryFormatterSerialization>true</EnableUnsafeBinaryFormatterSerialization>
    <GenerateResourceWarnOnBinaryFormatterUse>false</GenerateResourceWarnOnBinaryFormatterUse>
+    <IsTestUtilityProject>true</IsTestUtilityProject>


Why is this property needed? Do we not set shipping/notshipping properties based on the Test suffix?

Tanya-Solyanik · 2025-04-17T04:02:39Z

src/System.Windows.Forms.Analyzers/common/tests/Microsoft.WinForms/NetVersion.cs

-    Net8_0,
-    Net9_0
+    Net6_0 = 0x00000006,
+    Net7_0 = 0x00000007,


I'm concerned that eventually & will go out of support and will not be installed on the CI machines. Can we test only against 9 in the servicing branches?

Tanya-Solyanik · 2025-04-17T04:05:41Z

...zers/common/tests/Microsoft.WinForms/ReferenceAssemblyGenerator.WinFormsReferencesFactory.cs

+
+using Microsoft.CodeAnalysis.Testing;
+
+// File-Cherry-Picked and modified a bit from Tanya Solyanik's Commit ec6a9f8,


This is not needed, we put notes like this when code is shared with other repos, and might go out of sync. In he same repo we can always look up history in the main branch

Tanya-Solyanik · 2025-04-17T05:11:41Z

...zers/common/tests/Microsoft.WinForms/ReferenceAssemblyGenerator.WinFormsReferencesFactory.cs

+    public static class WinFormsReferencesFactory
+    {
+        private const string RefPackageName = "Microsoft.NETCore.App.Ref";
+        private const string PrivatePackagePath = "artifacts\\packages\\Debug\\NonShipping\\Microsoft.Private.Winforms.9.0.3-dev.nupkg";


I don't see how this works -
it assumes someone had built Debug and it assumes specific version...

Wait... we shouldn't run against the current binary in the artifacts folder in the release branch, everything is already shipped...

Tanya-Solyanik · 2025-04-17T05:18:53Z

...AvoidPassingTaskWithoutCancellationToken/AvoidPassingTaskWithoutCancellationTokenAnalyzer.cs

-        if (objectTypeInfo.Type is not INamedTypeSymbol objectType
-            || !IsAncestorOrSelfOfType(objectType, "System.Windows.Forms.Control"))
+        // For implicit calls, we check the containing type of the method itself.
+        if (containingType is null || !IsAncestorOrSelfOfType(containingType, "System.Windows.Forms.Control"))


Should we also check if this binary references winforms? THeoretically it's possible to have user assembly that defines the same type.

Tanya-Solyanik · 2025-04-17T05:20:32Z

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs


-        // Does the property belong to a class which derives from Component?
+        // We never flag a property named Site of type of ISite
+        if (propertySymbol is null)


use pattern matching

Tanya-Solyanik · 2025-04-17T05:28:48Z

...er/AvoidPassingTaskWithoutCancellationToken/TestData/ImplicitInvokeAsyncOnControl/Program.cs

+
+using System.Windows.Forms;
+
+public static class Program


Is this file really needed?

Tanya-Solyanik · 2025-04-17T05:31:52Z

...tem.Windows.Forms.Analyzers/vb/tests/System.Windows.Forms.Analyzers.VisualBasic.Tests.vbproj

    <Compile Remove="Analyzer\AvoidPassingTaskWithoutCancellationToken\TestData\InvokeAsyncOnControl\AnalyzerTestCode.vb" />
    <Compile Remove="Analyzer\MissingPropertySerializationConfiguration\TestData\CustomControlScenarios\AnalyzerTestCode.vb" />
    <Compile Remove="Analyzer\MissingPropertySerializationConfiguration\TestData\CustomControlScenarios\CodeFixTestCode.vb" />
    <Compile Remove="Analyzer\MissingPropertySerializationConfiguration\TestData\CustomControlScenarios\FixedTestCode.vb" />
    <Compile Remove="Analyzer\MissingPropertySerializationConfiguration\TestData\CustomControlScenarios\Program.vb" />
+    <Compile Remove="Analyzer\MissingPropertySerializationConfiguration\TestData\EdgeCaseScenarios\AnalyzerTestCode.vb" />
+    <Compile Remove="Analyzer\MissingPropertySerializationConfiguration\TestData\EdgeCaseScenarios\Program.vb" />
+    <Compile Remove="D:\git\winforms\eng\CodeAnalysisSource.vb" />


Tanya-Solyanik · 2025-04-17T05:32:24Z

...tem.Windows.Forms.Analyzers/vb/tests/System.Windows.Forms.Analyzers.VisualBasic.Tests.vbproj

@@ -11,11 +11,16 @@
  </PropertyGroup>

  <ItemGroup>
+    <Compile Remove="Analyzer\AvoidPassingTaskWithoutCancellationToken\TestData\ImplicitInvokeAsyncOnControl\AnalyzerTestCode.vb" />


use DefaultItemExcludesInProjectFolder property

Tanya-Solyanik · 2025-04-17T05:32:41Z

...tem.Windows.Forms.Analyzers/vb/tests/System.Windows.Forms.Analyzers.VisualBasic.Tests.vbproj

@@ -50,11 +55,16 @@
  </ItemGroup>

  <ItemGroup>
+    <None Include="Analyzer\AvoidPassingTaskWithoutCancellationToken\TestData\ImplicitInvokeAsyncOnControl\AnalyzerTestCode.vb" />


what is this for?

Tanya-Solyanik · 2025-04-17T05:32:57Z

...tem.Windows.Forms.Analyzers/vb/tests/System.Windows.Forms.Analyzers.VisualBasic.Tests.vbproj

  </ItemGroup>

+


extra white line

Tanya-Solyanik · 2025-04-18T19:54:36Z

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs

            || propertySymbol.DeclaredAccessibility < Accessibility.Internal)
        {
            return;
        }

+        // Skip overridden properties since the base property should already have the appropriate serialization configuration


Is it possible to iterate over the base properties?

Tanya-Solyanik · 2025-04-18T19:56:57Z

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs

-        if (propertySymbol.SetMethod is null
+        // Is the property read/write and at least internal and doesn't have a private setter?
+        if (propertySymbol.SetMethod is not IMethodSymbol propertySetter
+            || propertySetter.DeclaredAccessibility == Accessibility.Private


Is checking DeclaredAccessibility sufficient? It should be the default accessibility.

Tanya-Solyanik · 2025-04-18T19:59:50Z

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs

-        if (propertySymbol.SetMethod is null
+        // Is the property read/write and at least internal and doesn't have a private setter?
+        if (propertySymbol.SetMethod is not IMethodSymbol propertySetter
+            || propertySetter.DeclaredAccessibility == Accessibility.Private
            || propertySymbol.DeclaredAccessibility < Accessibility.Internal)


Is this check still needed?

Tanya-Solyanik · 2025-04-18T21:51:34Z

src/System.Windows.Forms.Analyzers/prompting/AnalyzerTests-Copilot-Instructions.md

+
+## Table of Contents
+1. [Project Structure](#project-structure)
+2. [Language-Specific Considerations](#language-specific-considerations)


if you set number as 1. for all these lines, the actual numbers will be auto-generated.

Tanya-Solyanik · 2025-04-18T21:55:29Z

...ssingPropertySerializationConfiguration/MissingPropertySerializationConfigurationAnalyzer.cs

+        }
+
+        // Skip static properties since they are not serialized by the designer
+        if (propertySymbol.IsStatic)


Please front-load this check to exit fast.

github-actions bot added the area-Infrastructure label Mar 25, 2025

dotnet-policy-service bot assigned KlausLoeffelmann Mar 25, 2025

KlausLoeffelmann force-pushed the FixAnalyzerIssues9.0Rel branch 2 times, most recently from 19e5925 to fe3dd24 Compare March 25, 2025 00:12

KlausLoeffelmann changed the title ~~[release/9.0] Fixing Analyzer issues, servicing, Part 1: Actual Fixes~~ [release/9.0] Fixing Analyzer issues, servicing, Part 2: Actual Fixes Mar 25, 2025

KlausLoeffelmann changed the base branch from main to release/9.0 March 25, 2025 00:31

dotnet-policy-service bot added the draft draft PR label Mar 25, 2025

Fix Analyzer issue, where we did not make sure to test for the _right…

7935110

…_ IComponent.

KlausLoeffelmann added 3 commits March 31, 2025 09:16

Addressing the over-reporting issues in the MissingPropertySerializat…

d40fb65

…ionConfiguration. * Addresses 13205, 13206, 13207, 13208.

Add specific Copilot instructions for writing Analyzer and CodeFix te…

365caa7

…sts.

Update ReferenceAssemblyGenerator by LatestBuild Assembly resolver.

7b2aaa0

KlausLoeffelmann force-pushed the FixAnalyzerIssues9.0Rel branch from c6effd1 to 7b2aaa0 Compare April 3, 2025 08:21

Tanya-Solyanik added area-Analyzers/CodeFixes A Roslyn Analyzer is either needed for the context, needs to be scope extended or fixed. and removed area-Infrastructure labels Apr 3, 2025

KlausLoeffelmann added 3 commits April 3, 2025 17:16

Add InvokeAsync Analyzer fix both for VB and CSharp and Unittests.

3ba49f3

Update tests.

8bdf958

Fix VB tests and test data.

4436879

KlausLoeffelmann marked this pull request as ready for review April 4, 2025 17:30

KlausLoeffelmann requested a review from a team as a code owner April 4, 2025 17:30

dotnet-policy-service bot removed the draft draft PR label Apr 4, 2025

KlausLoeffelmann added the 🚫 * NO-MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Apr 4, 2025

Tanya-Solyanik reviewed Apr 4, 2025

View reviewed changes

KlausLoeffelmann changed the base branch from release/9.0 to servicing/RefactorAnalyzerTests9.0Rel April 8, 2025 17:55

KlausLoeffelmann added 2 commits April 8, 2025 16:10

Clean up comments and VB test data code.

cc55027

Fix Option Explicit placing in code.

3ee6199

Tanya-Solyanik reviewed Apr 17, 2025

View reviewed changes

Update prompt-folder for Analyzer test generation.

dc7d6ca

Tanya-Solyanik reviewed Apr 18, 2025

View reviewed changes

Address review feedback.

80c449e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[release/9.0] Fixing Analyzer issues, servicing, Part 2: Actual Fixes #13182

[release/9.0] Fixing Analyzer issues, servicing, Part 2: Actual Fixes #13182

KlausLoeffelmann commented Mar 25, 2025 •

edited

Loading

codecov bot commented Mar 31, 2025 •

edited

Loading

Tanya-Solyanik Apr 4, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 17, 2025

Tanya-Solyanik Apr 18, 2025

Tanya-Solyanik Apr 18, 2025

Tanya-Solyanik Apr 18, 2025

Tanya-Solyanik Apr 18, 2025

Tanya-Solyanik Apr 18, 2025


		using Microsoft.CodeAnalysis.Testing;

		// File-Cherry-Picked and modified a bit from Tanya Solyanik's Commit ec6a9f8,

[release/9.0] Fixing Analyzer issues, servicing, Part 2: Actual Fixes #13182

Are you sure you want to change the base?

[release/9.0] Fixing Analyzer issues, servicing, Part 2: Actual Fixes #13182

Conversation

KlausLoeffelmann commented Mar 25, 2025 • edited Loading

Microsoft Reviewers: Open in CodeFlow

codecov bot commented Mar 31, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlausLoeffelmann commented Mar 25, 2025 •

edited

Loading

codecov bot commented Mar 31, 2025 •

edited

Loading