Skip to content
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.

Sign up for GitHub

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

Report diagnostics from TryFindDisposePatternMethod when binding foreach loop #75422

Merged
merged 8 commits into from
Oct 16, 2024
Merged

Report diagnostics from TryFindDisposePatternMethod when binding foreach loop #75422

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9bab481
Report obsolete DisposeAsync method in await foreach
jcouv Oct 7, 2024
ed0cb34
Address feedback
jcouv Oct 10, 2024
bbd8ba6
Merge remote-tracking branch 'dotnet/main' into obsolete-dispose
jcouv Oct 11, 2024
adfc9f2
Add suggested test
jcouv Oct 11, 2024
c31ec4e
Add test for TryGetCollectionIterationType
jcouv Oct 14, 2024
0139aac
Remove comment
jcouv Oct 15, 2024
75a746a
formatting
jcouv Oct 15, 2024
92328fe
Merge branch 'main' into obsolete-dispose
jcouv Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 46 additions & 0 deletions docs/compilers/CSharp/Compiler Breaking Changes - DotNet 10.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


***Introduced in Visual Studio 2022 version 17.13***

For instance, an obsolete `DisposeAsync` method is now reported in `await foreach`.
```csharp
await foreach (var i in new C()) { } // 'C.AsyncEnumerator.DisposeAsync()' is obsolete

class C
{
public AsyncEnumerator GetAsyncEnumerator(System.Threading.CancellationToken token = default)
{
throw null;
}

public sealed class AsyncEnumerator : System.IAsyncDisposable
{
public int Current { get => throw null; }
public Task<bool> MoveNextAsync() => throw null;

[System.Obsolete]
public ValueTask DisposeAsync() => throw null;
}
}
```

Similarly, an `[UnmanagedCallersOnly]` `Dispose` method is now reported in `foreach` with a `ref struct` enumerator.
```csharp
public struct S
{
public static void M2(S s)
{
foreach (var i in s) { } // 'SEnumerator.Dispose()' is attributed with 'UnmanagedCallersOnly' and cannot be called directly.
}
public static SEnumerator GetEnumerator() => throw null;
}
public ref struct SEnumerator
{
public bool MoveNext() => throw null;
public int Current => throw null;
[UnmanagedCallersOnly]
public void Dispose() => throw null;
}
```
6 changes: 6 additions & 0 deletions src/Compilers/CSharp/Portable/Binder/Binder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Diagnostics;
using System.Threading;
using Microsoft.CodeAnalysis.CSharp.Symbols;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.PooledObjects;
using Roslyn.Utilities;

Expand Down Expand Up @@ -727,6 +728,11 @@ internal static ObsoleteDiagnosticKind ReportDiagnosticsIfObsoleteInternal(Diagn

if (info != null)
{
if (node.AsNode() is ForEachStatementSyntax foreachSyntax)
{
node = foreachSyntax.ForEachKeyword;
}

diagnostics.Add(info, node.GetLocation());
}

Expand Down
5 changes: 3 additions & 2 deletions src/Compilers/CSharp/Portable/Binder/ForEachLoopBinder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1193,15 +1193,16 @@ private void GetDisposalInfoForEnumerator(SyntaxNode syntax, ref ForEachEnumerat

if (enumeratorType.IsRefLikeType || isAsync)
{
// we throw away any binding diagnostics, and assume it's not disposable if we encounter errors
var receiver = new BoundDisposableValuePlaceholder(syntax, enumeratorType);
MethodSymbol patternDisposeMethod = TryFindDisposePatternMethod(receiver, syntax, isAsync, BindingDiagnosticBag.Discarded, out bool expanded);
BindingDiagnosticBag patternDiagnostics = BindingDiagnosticBag.GetInstance(diagnostics);
MethodSymbol patternDisposeMethod = TryFindDisposePatternMethod(receiver, syntax, isAsync, patternDiagnostics, out bool expanded);
if (patternDisposeMethod is object)
{
Debug.Assert(!patternDisposeMethod.IsExtensionMethod);
Debug.Assert(patternDisposeMethod.ParameterRefKinds.IsDefaultOrEmpty ||
patternDisposeMethod.ParameterRefKinds.All(static refKind => refKind is RefKind.None or RefKind.In or RefKind.RefReadOnlyParameter));

diagnostics.AddRangeAndFree(patternDiagnostics);
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

var argsBuilder = ArrayBuilder<BoundExpression>.GetInstance(patternDisposeMethod.ParameterCount);
var argsToParams = default(ImmutableArray<int>);

Expand Down
62 changes: 55 additions & 7 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAwaitForeachTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4821,8 +4821,8 @@ public static class Extensions
CompileAndVerify(comp, expectedOutput: "NextAsync(0) Current(1) Got(1,-1) NextAsync(1) Current(2) Got(2,-2) NextAsync(2) Dispose(3) Done");
}

[Fact]
public void TestWithPatternAndObsolete()
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/30257")]
public void TestWithPatternAndObsolete_WithDisposableInterface()
{
string source = @"
using System.Threading.Tasks;
Expand Down Expand Up @@ -4852,6 +4852,9 @@ public sealed class AsyncEnumerator : System.IAsyncDisposable
}";
var comp = CreateCompilationWithTasksExtensions(source + s_IAsyncEnumerable, options: TestOptions.DebugExe);
comp.VerifyDiagnostics(
// (7,15): warning CS0612: 'C.AsyncEnumerator.DisposeAsync()' is obsolete
// await foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.DisposeAsync()").WithLocation(7, 15),
// (7,15): warning CS0612: 'C.GetAsyncEnumerator(CancellationToken)' is obsolete
// await foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.GetAsyncEnumerator(System.Threading.CancellationToken)").WithLocation(7, 15),
Expand All @@ -4860,9 +4863,52 @@ public sealed class AsyncEnumerator : System.IAsyncDisposable
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)
);
// Note: Obsolete on DisposeAsync is not reported since always called through IAsyncDisposable interface
Copy link
Member

@jjonescz jjonescz Oct 9, 2024 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@AlekseyTs AlekseyTs Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

);

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

}

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

@jjonescz jjonescz Oct 9, 2024 •
edited by jcouv
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake. Added now :-)

{
string source = @"
using System.Threading.Tasks;
class C
{
static async System.Threading.Tasks.Task Main()
{
await foreach (var i in new C())
{
}
}
[System.Obsolete]
public AsyncEnumerator GetAsyncEnumerator(System.Threading.CancellationToken token = default)
{
throw null;
}
[System.Obsolete]
public sealed class AsyncEnumerator
{
[System.Obsolete]
public int Current { get => throw null; }
[System.Obsolete]
public Task<bool> MoveNextAsync() => throw null;
[System.Obsolete]
public ValueTask DisposeAsync() => throw null;
}
}";
var comp = CreateCompilationWithTasksExtensions(source + s_IAsyncEnumerable, options: TestOptions.DebugExe);
comp.VerifyDiagnostics(
// (7,15): warning CS0612: 'C.AsyncEnumerator.DisposeAsync()' is obsolete
// await foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.AsyncEnumerator.DisposeAsync()").WithLocation(7, 15),
// (7,15): warning CS0612: 'C.GetAsyncEnumerator(CancellationToken)' is obsolete
// await foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.GetAsyncEnumerator(System.Threading.CancellationToken)").WithLocation(7, 15),
// (7,15): warning CS0612: 'C.AsyncEnumerator.MoveNextAsync()' is obsolete
// await foreach (var i in new C())
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));
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

}

[Fact]
Expand Down Expand Up @@ -8495,6 +8541,9 @@ public static class Extensions
}";
var comp = CreateCompilationWithMscorlib46(source, options: TestOptions.DebugExe, parseOptions: TestOptions.Regular9);
comp.VerifyDiagnostics(
// (8,15): warning CS0612: 'C.Enumerator.DisposeAsync()' is obsolete
// await foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.Enumerator.DisposeAsync()").WithLocation(8, 15),
// (8,15): warning CS0612: 'Extensions.GetAsyncEnumerator(C)' is obsolete
// await foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("Extensions.GetAsyncEnumerator(C)").WithLocation(8, 15),
Expand All @@ -8503,8 +8552,7 @@ public static class Extensions
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.Enumerator.MoveNextAsync()").WithLocation(8, 15),
// (8,15): warning CS0612: 'C.Enumerator.Current' is obsolete
// 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));
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

);

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

CompileAndVerify(comp, expectedOutput: "123Disposed");
}

Expand Down
128 changes: 128 additions & 0 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenForEachTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5593,5 +5593,133 @@ public static void Dispose(this int i, params int[] other) { }
CompileAndVerify(comp, expectedOutput: ExecutionConditionUtil.IsMonoOrCoreClr ? "42" : null, verify: Verification.Skipped)
.VerifyDiagnostics();
}

[Fact]
public void TestWithPatternAndObsolete_WithDisposableInterface()
{
string source = """
foreach (var i in new C())
{
}
class C
{
[System.Obsolete]
public MyEnumerator GetEnumerator()
{
throw null;
}
[System.Obsolete]
public sealed class MyEnumerator : System.IDisposable
{
[System.Obsolete]
public int Current { get => throw null; }
[System.Obsolete]
public bool MoveNext() => throw null;
[System.Obsolete("error", true)]
public void Dispose() => throw null;
}
}
""";
var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (1,1): warning CS0612: 'C.GetEnumerator()' is obsolete
// foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.GetEnumerator()").WithLocation(1, 1),
// (1,1): warning CS0612: 'C.MyEnumerator.MoveNext()' is obsolete
// foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.MyEnumerator.MoveNext()").WithLocation(1, 1),
// (1,1): warning CS0612: 'C.MyEnumerator.Current' is obsolete
// foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.MyEnumerator.Current").WithLocation(1, 1));
var verifier = CompileAndVerify(comp);
verifier.VerifyIL("<top-level-statements-entry-point>", """
{
// Code size 41 (0x29)
.maxstack 1
.locals init (C.MyEnumerator V_0)
IL_0000: newobj "C..ctor()"
IL_0005: call "C.MyEnumerator C.GetEnumerator()"
IL_000a: stloc.0
.try
{
IL_000b: br.s IL_0014
IL_000d: ldloc.0
IL_000e: callvirt "int C.MyEnumerator.Current.get"
IL_0013: pop
IL_0014: ldloc.0
IL_0015: callvirt "bool C.MyEnumerator.MoveNext()"
IL_001a: brtrue.s IL_000d
IL_001c: leave.s IL_0028
}
finally
{
IL_001e: ldloc.0
IL_001f: brfalse.s IL_0027
IL_0021: ldloc.0
IL_0022: callvirt "void System.IDisposable.Dispose()"
IL_0027: endfinally
}
IL_0028: ret
}
""");
}

[Fact]
public void TestWithPatternAndObsolete_WithoutDisposableInterface()
{
string source = """
foreach (var i in new C())
{
}
class C
{
[System.Obsolete]
public MyEnumerator GetEnumerator()
{
throw null;
}
[System.Obsolete]
public sealed class MyEnumerator
{
[System.Obsolete]
public int Current { get => throw null; }
[System.Obsolete]
public bool MoveNext() => throw null;
[System.Obsolete("error", true)]
public void Dispose() => throw null;
}
}
""";
var comp = CreateCompilation(source);
comp.VerifyEmitDiagnostics(
// (1,1): warning CS0612: 'C.GetEnumerator()' is obsolete
// foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.GetEnumerator()").WithLocation(1, 1),
// (1,1): warning CS0612: 'C.MyEnumerator.MoveNext()' is obsolete
// foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.MyEnumerator.MoveNext()").WithLocation(1, 1),
// (1,1): warning CS0612: 'C.MyEnumerator.Current' is obsolete
// foreach (var i in new C())
Diagnostic(ErrorCode.WRN_DeprecatedSymbol, "foreach").WithArguments("C.MyEnumerator.Current").WithLocation(1, 1));
var verifier = CompileAndVerify(comp);
verifier.VerifyIL("<top-level-statements-entry-point>", """
{
// Code size 29 (0x1d)
.maxstack 1
.locals init (C.MyEnumerator V_0)
IL_0000: newobj "C..ctor()"
IL_0005: call "C.MyEnumerator C.GetEnumerator()"
IL_000a: stloc.0
IL_000b: br.s IL_0014
IL_000d: ldloc.0
IL_000e: callvirt "int C.MyEnumerator.Current.get"
IL_0013: pop
IL_0014: ldloc.0
IL_0015: callvirt "bool C.MyEnumerator.MoveNext()"
IL_001a: brtrue.s IL_000d
IL_001c: ret
}
""");
}
}
}
10 changes: 6 additions & 4 deletions src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenFunctionPointersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9398,7 +9398,7 @@ public static class CExt
);
}

[Fact]
[Fact, WorkItem("https://github.com/dotnet/roslyn/issues/73934")]
public void UnmanagedCallersOnlyDeclaredOnPatternDispose()
{
var comp = CreateCompilation(new[] { @"
Expand All @@ -9424,10 +9424,12 @@ public static class CExt
", UnmanagedCallersOnlyAttribute });

comp.VerifyDiagnostics(
// (14,6): error CS8896: 'UnmanagedCallersOnly' can only be applied to ordinary static non-abstract, non-virtual methods or static local functions.
// 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.
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.cs

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

// [UnmanagedCallersOnly]
Diagnostic(ErrorCode.ERR_UnmanagedCallersOnlyRequiresStatic, "UnmanagedCallersOnly").WithLocation(14, 6)
);
Diagnostic(ErrorCode.ERR_UnmanagedCallersOnlyRequiresStatic, "UnmanagedCallersOnly").WithLocation(14, 6));
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

);

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

}

[Fact]
Expand Down
Loading