-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Improve async iterator diagnostic by removing redundant error #81282
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.
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
base: main
Are you sure you want to change the base?
Conversation
Add new error code ERR_BadAwaitWithoutAsyncIterator (CS4037) that provides a better error message when an async iterator method is missing the 'async' keyword. The new error only suggests adding 'async' without suggesting to change the return type, since the return type is already correct for async iterators (IAsyncEnumerable<T> or IAsyncEnumerator<T>). Co-authored-by: CyrusNajmabadi <[email protected]>
| method.ReturnType.IsIAsyncEnumeratorType(Compilation))) | ||
| { | ||
| // For async iterators, don't suggest changing the return type | ||
| info = new CSDiagnosticInfo(ErrorCode.ERR_BadAwaitWithoutAsyncIterator); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot instead of a new error, can you use ERR_BadAwaitWithoutAsync? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3de3769. Using ERR_BadAwaitWithoutAsync which provides a simpler message without suggesting a return type change.
| [WorkItem(31113, "https://github.com/dotnet/roslyn/issues/31113")] | ||
| public void AsyncIteratorReturningEnumerable_WithoutAsync() | ||
| { | ||
| string source = @" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot use raw string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in cac74c6. Converted both tests to use raw string literals and updated the line numbers in diagnostic expectations.
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs
Outdated
Show resolved
Hide resolved
src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenAsyncIteratorTests.cs
Outdated
Show resolved
Hide resolved
Address review feedback to use the existing ERR_BadAwaitWithoutAsync error instead of creating a new error code. This error already has an appropriate message for async iterators that doesn't suggest changing the return type. Co-authored-by: CyrusNajmabadi <[email protected]>
|
|
||
| // This error message is rather poor. Tracked by https://github.com/dotnet/roslyn/issues/31113 | ||
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/31113")] | ||
| public void AsyncIteratorReturningEnumerable_WithoutAsync() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot add test with IAsyncEnumerator<...> as well. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both test cases are already present:
AsyncIteratorReturningEnumerator_WithoutAsync(line 1984) testsIAsyncEnumerator<int>AsyncIteratorReturningEnumerable_WithoutAsync(line 2008) testsIAsyncEnumerable<int>
Both were added in commit 4ea5dc9.
Convert verbatim strings to raw string literals and update line numbers in diagnostic expectations accordingly. Co-authored-by: CyrusNajmabadi <[email protected]>
| { | ||
| info = new CSDiagnosticInfo(ErrorCode.ERR_BadAwaitWithoutVoidAsyncMethod); | ||
| } | ||
| else if (method.IsIterator && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot fix formatting errors (excess spaces on line). #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in b7d326e. Removed trailing whitespace from lines 192-194.
| [WorkItem(31608, "https://github.com/dotnet/roslyn/issues/31608")] | ||
| public void AsyncIteratorReturningEnumerator_WithoutAsync() | ||
| { | ||
| string source = @" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot keep the original test string for tests you're updating. For new tests you can use raw-strings. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 3b78eef. Reverted the existing AsyncIteratorReturningEnumerator_WithoutAsync test to use verbatim strings. Only the new AsyncIteratorReturningEnumerable_WithoutAsync test uses raw string literals.
Remove trailing whitespace from the conditional expression in Binder_Await.cs. Co-authored-by: CyrusNajmabadi <[email protected]>
Keep the original verbatim string format for the existing AsyncIteratorReturningEnumerator_WithoutAsync test. Only the new AsyncIteratorReturningEnumerable_WithoutAsync test uses raw string literals. Co-authored-by: CyrusNajmabadi <[email protected]>
| // (3,61): error CS8403: Method 'C.M(int)' with an iterator block must be 'async' to return 'IAsyncEnumerable<int>' | ||
| // static System.Collections.Generic.IAsyncEnumerable<int> M(int value) | ||
| Diagnostic(ErrorCode.ERR_IteratorMustBeAsync, "M").WithArguments("C.M(int)", "System.Collections.Generic.IAsyncEnumerable<int>").WithLocation(3, 61), | ||
| // (6,9): error CS1992: The 'await' operator can only be used when contained within a method or lambda expression marked with the 'async' modifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot do we even need this error? It looks like we already report error CS8403: Method 'C.M(int)' with an iterator block must be 'async' to return 'IAsyncEnumerable<int>'. Since we already have that error, can we just not report this error on await? If so, update the code to check for that and not report, but give a comment in the code explaining why that's ok. #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a5766a3. Since ERR_IteratorMustBeAsync already tells the user to add 'async', we now skip reporting the await error to avoid redundancy. Added a comment explaining why this is okay.
For async iterators, ERR_IteratorMustBeAsync already tells the user to add 'async', so we don't need to also report an error on the await expression. This avoids redundant error messages. Co-authored-by: CyrusNajmabadi <[email protected]>
| else if (method.IsIterator && | ||
| (method.ReturnType.IsIAsyncEnumerableType(Compilation) || | ||
| method.ReturnType.IsIAsyncEnumeratorType(Compilation))) | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot mention which method reports this ERR_IteratorMustBeAsync error. Also, if possible, can we share code between that location and this location to ensure that we're checking all the same conditions to report an error there and not report an error here? #Closed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 5b4c38e. Now using the shared InMethodBinder.IsAsyncStreamInterface method to ensure we check the same conditions, and updated the comment to mention ExecutableCodeBinder.ValidateIteratorMethod as the location that reports the error.
Updated to use InMethodBinder.IsAsyncStreamInterface to share the same logic for detecting async stream interfaces, ensuring consistency between where the error is reported and where we skip reporting. Also updated the comment to mention ExecutableCodeBinder.ValidateIteratorMethod as the location that reports ERR_IteratorMustBeAsync. Co-authored-by: CyrusNajmabadi <[email protected]>
|
@dotnet/roslyn-compiler ptal. |
jcouv
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM Thanks (commit 11)
Improvement to async iterator diagnostics - Complete ✅
This PR improves the diagnostic experience when an async iterator method (returning
IAsyncEnumerable<T>orIAsyncEnumerator<T>) withyieldstatements is missing theasynckeyword.Before:
Two redundant errors were reported:
❌ Multiple errors with incorrect return type suggestion
After:
Single, clear error message:
✅ One focused error without incorrect return type suggestion
Changes
Binder_Await.csto skip reporting await errors for async iterators sinceERR_IteratorMustBeAsyncis already reported byExecutableCodeBinder.ValidateIteratorMethodInMethodBinder.IsAsyncStreamInterfacemethod to ensure consistent detection logicIAsyncEnumerable<T>andIAsyncEnumerator<T>casesTest Results
Fixes #31113
Original prompt
asynckeyword #31113💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.