Report diagnostics from `TryFindDisposePatternMethod` when binding `foreach` loop by jcouv · Pull Request #75422 · dotnet/roslyn

jcouv · 2024-10-07T22:34:42Z

jjonescz · 2024-10-09T14:06:05Z

                //         await foreach (var i in new C())
-                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.Current").WithLocation(7, 15)
-                );
-            // Note: Obsolete on DisposeAsync is not reported since always called through IAsyncDisposable interface


So the comment was incorrect - we do not go through IAsyncDisposable interface? #Resolved

Yes, this was changed in PR but this comment in test was missed

jjonescz · 2024-10-09T14:10:56Z

+        }
+
+        [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/30257")]
+        public void TestWithPatternAndObsolete_WithoutDisposableInterface()


Consider testing with explicit interface implementation of the DisposeAsync method. #Resolved

Did you add this test? I couldn't find it.

My mistake. Added now :-)

AlekseyTs · 2024-10-09T16:32:32Z

@@ -0,0 +1,46 @@
+# This document lists known breaking changes in Roslyn after .NET 9 all the way to .NET 10.
+
+## Diagnostics now reported for improper use of pattern-based disposal method in `foreach`


for improper use

I think we should be specific here. If we are talking about Obsolete diagnostics, we should say that. In general, I wouldn't call a usage of an obsolete API as an improper use. #Closed

AlekseyTs · 2024-10-09T16:47:44Z

                    Debug.Assert(patternDisposeMethod.ParameterRefKinds.IsDefaultOrEmpty ||
                        patternDisposeMethod.ParameterRefKinds.All(static refKind => refKind is RefKind.None or RefKind.In or RefKind.RefReadOnlyParameter));

+                    diagnostics.AddRangeAndFree(patternDiagnostics);


diagnostics.AddRangeAndFree(patternDiagnostics);

I am curious what effect this is going to have on collection expressions and params collections. Is this diagnostics going to be meaningful for them? Consider adding tests. #Closed

AlekseyTs · 2024-10-09T16:51:12Z

+                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.MoveNextAsync()").WithLocation(7, 15),
+                // (7,15): warning CS0612: 'C.AsyncEnumerator.Current' is obsolete
+                //         await foreach (var i in new C())
+                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.Current").WithLocation(7, 15));


Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.Current").WithLocation(7, 15));

Consider reverting changes on this line and reducing unnecessary diff. #Closed

AlekseyTs · 2024-10-09T16:52:21Z

                //         await foreach (var i in new C())
-                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.Enumerator.Current").WithLocation(8, 15)
-                );
+                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.Enumerator.Current").WithLocation(8, 15));


);

Consider reverting changes on this line and reducing unnecessary diff. #Closed

AlekseyTs · 2024-10-09T16:53:23Z

                //     [UnmanagedCallersOnly]
-                Diagnostic(ErrorCode.ERR_UnmanagedCallersOnlyRequiresStatic, "UnmanagedCallersOnly").WithLocation(14, 6)
-            );
+                Diagnostic(ErrorCode.ERR_UnmanagedCallersOnlyRequiresStatic, "UnmanagedCallersOnly").WithLocation(14, 6));


);

Consider reverting changes on this line and reducing unnecessary diff. #Closed

AlekseyTs · 2024-10-09T16:54:04Z

+                // 0.cs(7,9): error CS8901: 'SEnumerator.Dispose()' is attributed with 'UnmanagedCallersOnly' and cannot be called directly. Obtain a function pointer to this method.
+                //         foreach (var i in s) {}
+                Diagnostic(ErrorCode.ERR_UnmanagedCallersOnlyMethodsCannotBeCalledDirectly, "foreach (var i in s) {}").WithArguments("SEnumerator.Dispose()").WithLocation(7, 9),
+                // 0.cs(14,6): error CS8896: 'UnmanagedCallersOnly' can only be applied to ordinary static non-abstract, non-virtual methods or static local functions.


0.cs

Consider reverting changes on this line and reducing unnecessary diff. #Closed

AlekseyTs · 2024-10-11T20:12:23Z

+{
+    public static void M2(S s)
+    {
+        int[] a = [42, ..s];


..s

This is an interesting scenario, but it is not quite the scenario I had in mind. I was referring to a check for a creatable collection, which involves determination of an element type (foreach element type). #Closed

I didn't follow. How could that involve binding the Dispose method?

I didn't follow. How could that involve binding the Dispose method?

It looks like TryFindDisposePatternMethod might be reachable from TryGetCollectionIterationType.

Added a test. The behavior seems fine (because TryGetCollectionIterationType discards diagnostics)

AlekseyTs · 2024-10-11T20:14:20Z

+                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.MoveNextAsync()").WithLocation(7, 15),
+                // (7,15): warning CS0612: 'C.AsyncEnumerator.Current' is obsolete
+                //         await foreach (var i in new C())
+                Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.Current").WithLocation(7, 15));


);

Consider reverting changes on this line and reducing unnecessary diff. #Closed

AlekseyTs · 2024-10-11T20:17:34Z

Done with review pass (commit 4)

AlekseyTs

LGTM (commit 8)

jcouv added the Area-Compilers label Oct 7, 2024

jcouv self-assigned this Oct 7, 2024

ghost added the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 7, 2024

jcouv force-pushed the obsolete-dispose branch 2 times, most recently from 971b422 to e2dac8f Compare October 8, 2024 17:00

jcouv changed the title ~~Report obsolete DisposeAsync method in await foreach~~ Report diagnostics from TryFindDisposePatternMethod when binding foreach loop Oct 8, 2024

jcouv force-pushed the obsolete-dispose branch from e2dac8f to 44c38f4 Compare October 8, 2024 17:06

Report obsolete DisposeAsync method in await foreach

9bab481

jcouv force-pushed the obsolete-dispose branch from 44c38f4 to 9bab481 Compare October 8, 2024 17:08

jcouv marked this pull request as ready for review October 8, 2024 17:08

jcouv requested a review from a team as a code owner October 8, 2024 17:08

jjonescz reviewed Oct 9, 2024

View reviewed changes

AlekseyTs reviewed Oct 9, 2024

View reviewed changes

jcouv added 3 commits October 10, 2024 09:16

Address feedback

ed0cb34

Merge remote-tracking branch 'dotnet/main' into obsolete-dispose

bbd8ba6

Add suggested test

adfc9f2

AlekseyTs reviewed Oct 11, 2024

View reviewed changes

jjonescz approved these changes Oct 14, 2024

View reviewed changes

jcouv added 3 commits October 15, 2024 14:12

Add test for TryGetCollectionIterationType

c31ec4e

Remove comment

0139aac

formatting

75a746a

jcouv requested a review from AlekseyTs October 15, 2024 21:17

Merge branch 'main' into obsolete-dispose

92328fe

AlekseyTs approved these changes Oct 16, 2024

View reviewed changes

jcouv merged commit f89ffba into dotnet:main Oct 16, 2024

jcouv deleted the obsolete-dispose branch October 16, 2024 16:55

dotnet-policy-service Bot added this to the Next milestone Oct 16, 2024

This was referenced Oct 22, 2024

[Automated] PRs inserted in VS build main-35421.103 #75574

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35424.209 #75628

Closed

akhera99 modified the milestones: Next, 17.13 P1 Oct 28, 2024

dotnet-bot mentioned this pull request Oct 29, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35428.160 #75657

Closed

		@@ -0,0 +1,46 @@
		# This document lists known breaking changes in Roslyn after .NET 9 all the way to .NET 10.

		## Diagnostics now reported for improper use of pattern-based disposal method in `foreach`

Conversation

jcouv commented Oct 7, 2024

Uh oh!

jjonescz Oct 9, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 9, 2024

Choose a reason for hiding this comment

Uh oh!

jjonescz Oct 9, 2024 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjonescz Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 11, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 14, 2024

Choose a reason for hiding this comment

Uh oh!

jcouv Oct 15, 2024

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Oct 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Oct 11, 2024

Uh oh!

AlekseyTs left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jjonescz Oct 9, 2024 •

edited by jcouv

Loading

jjonescz Oct 9, 2024 •

edited by jcouv

Loading

AlekseyTs Oct 9, 2024 •

edited

Loading

AlekseyTs Oct 9, 2024 •

edited

Loading

AlekseyTs Oct 9, 2024 •

edited

Loading

AlekseyTs Oct 9, 2024 •

edited

Loading

AlekseyTs Oct 9, 2024 •

edited

Loading

AlekseyTs Oct 9, 2024 •

edited

Loading

AlekseyTs Oct 11, 2024 •

edited

Loading

AlekseyTs Oct 11, 2024 •

edited

Loading