Warn for yield return in lock
#72755
Conversation
dotnet-issue-labeler
bot
added
Area-Compilers
untriaged
jjonescz
force-pushed
the
RefInAsync-02-YieldInLock
branch
from
c74941c to
b201c76
Compare
| @@ -268,6 +268,10 @@ private BoundStatement BindYieldReturnStatement(YieldStatementSyntax node, Bindi | |||
| { | |||
| Error(diagnostics, ErrorCode.ERR_YieldNotAllowedInScript, node.YieldKeyword); | |||
| } | |||
| else if (this.Flags.Includes(BinderFlags.InLockBody)) | |||
There was a problem hiding this comment.
It is very easy to make a mistake and start adding checks for other errors (not warnings) in the else of this if statement. Consider adding a comment right above it saying that all error conditions should be checked outside its else if one is added in the future. #Closed
| if (needsFilterDiagnostics) | ||
| { | ||
| bodyDiagnostics.CopyFilteredToAndFree(diagnostics, | ||
| static code => code is not ErrorCode.WRN_BadYieldInLock); |
There was a problem hiding this comment.
While it feels tempting to always report a warning and then filter it out, I would prefer us to not report the warning in the first place. A diagnostic filtering is an exception rather than a usual way of doing things. In this case, I think, we can avoid it. See, for example, how a set of LockedOrDisposedVariables is exposed and used. I think the fact whether we are in context of a "modern" lock could be accessed/exposed in a similar way, and we could simply not report the warning in the first place. #Closed
There was a problem hiding this comment.
Another thing to consider is to simplify and not attempt to suppress the warning at all. I do not think it is going to lead to any confusion and the problem with the lock object itself is a separate matter.
|
|
||
| var expectedOutput = ExecutionConditionUtil.IsDesktop | ||
| ? null | ||
| : "0123Object synchronization method was called from an unsynchronized block of code."; |
There was a problem hiding this comment.
"0123Object synchronization method was called from an unsynchronized block of code."
I do not think we should be testing this specific runtime exception behavior, this isn't an exception from a specific API or an exception explicitly thrown by compiler generated code. Consider not validating the output at all. #Closed
| Debug.Assert(this.Locals.IsDefaultOrEmpty); | ||
|
|
||
| if (needsFilterDiagnostics) |
There was a problem hiding this comment.
LockTests.Yield and Yield_Async
|
Done with review pass (commit 5) |
|
@cston for a second review, thanks |
| } | ||
| catch (SynchronizationLockException e) | ||
| { | ||
| Console.WriteLine(e.Message); |
There was a problem hiding this comment.
Originally the intent was to execute the test code; but that was removed in #72755 (comment). I will remove the caller, thanks.
Resolves #72443.
Test plan: #72662
Speclet: https://github.com/dotnet/csharplang/blob/main/proposals/ref-unsafe-in-iterators-async.md