Warn when a synchronized loop never ends

src/Zomp.SyncMethodGenerator/AnalyzerReleases.Unshipped.md

@@ -10,3 +10,4 @@ ZSMGEN002 | Preprocessor | Error | DiagnosticMessages
 ZSMGEN003 | Preprocessor | Error | DiagnosticMessages
 ZSMGEN004 | Usage | Error | DiagnosticMessages
 ZSMGEN005 | Usage | Error | DiagnosticMessages
+ZSMGEN006 | Usage | Warning | DiagnosticMessages

src/Zomp.SyncMethodGenerator/AsyncToSyncRewriter.cs

@@ -2422,11 +2422,39 @@ private SyntaxList<StatementSyntax> Process(
             {
                 newStatements.Add(EmptyStatement().WithSemicolonToken(MissingToken(SyntaxKind.SemicolonToken)).WithLeadingTrivia(leadingTrivia));
             }
+
+            // A cancellation check in a loop's condition becomes true in the synchronized version,
+            // so unless something else leaves the loop it now runs forever, and whatever follows it
+            // can't be reached.
+            if (!dropOriginal
+                && statement is WhileStatementSyntax loop
+                && rewritten is WhileStatementSyntax { Statement: var body }
+                && ChecksIfNegatedIsCancellationRequested(loop.Condition))
+            {
+                var exits = new LoopExitWalker(MethodsWhichNeverReturn(loop.Statement));
+                exits.Visit(body);
+
+                if (!exits.Breaks && !exits.Jumps && !exits.LeavesMethod)
+                {
+                    diagnostics.Add(ReportedDiagnostic.Create(EndlessLoop, loop.WhileKeyword.GetLocation(), loop.Condition.ToString()));
+                }
+
+                removeRemaining |= !exits.Breaks && !exits.Jumps
+                    && !originalStatements.Skip(i + 1).OfType<LabeledStatementSyntax>().Any();
+            }
         }
 
         return List(newStatements);
     }
 
+    private HashSet<string> MethodsWhichNeverReturn(SyntaxNode node)
+        => [.. node.DescendantNodes()
+            .OfType<InvocationExpressionSyntax>()
+            .Select(GetSymbol)
+            .OfType<IMethodSymbol>()
+            .Where(static m => m.GetAttributes().Any(static a => a.AttributeClass is { Name: "DoesNotReturnAttribute" }))
+            .Select(static m => m.Name)];
+
     /// <summary>
     /// Checks whether the <c>return</c> keyword must be dropped: <c>return InvocationAsync();</c>
     /// in a method returning a bare <see cref="Task"/> or <see cref="ValueTask"/> has no value

src/Zomp.SyncMethodGenerator/DiagnosticMessages.cs

@@ -42,6 +42,14 @@ internal static class DiagnosticMessages
         DiagnosticSeverity.Error,
         isEnabledByDefault: true);
 
+    internal static readonly DiagnosticDescriptor EndlessLoop = new(
+        id: "ZSMGEN006",
+        title: "Loop never ends once synchronized",
+        messageFormat: $"The synchronized loop never ends. Its condition '{{0}}' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if {AsyncToSyncRewriter.SyncOnly} region.",
+        category: Usage,
+        DiagnosticSeverity.Warning,
+        isEnabledByDefault: true);
+
     private const string Preprocessor = "Preprocessor";
 
     private const string Usage = "Usage";

src/Zomp.SyncMethodGenerator/Helpers/LoopExitWalker.cs

@@ -0,0 +1,109 @@
+namespace Zomp.SyncMethodGenerator.Helpers;
+
+/// <summary>
+/// Finds the ways out of a loop body. Code which runs somewhere else, in a lambda or a local
+/// function, is skipped, and a <c>break</c> only counts when it belongs to the loop itself.
+/// </summary>
+/// <param name="methodsWhichNeverReturn">Names of the methods whose calls never return.</param>
+internal sealed class LoopExitWalker(ISet<string> methodsWhichNeverReturn) : CSharpSyntaxWalker
+{
+    private int nestedBreakTargets;
+
+    /// <summary>
+    /// Gets a value indicating whether a <c>break</c> leaves the loop.
+    /// </summary>
+    public bool Breaks { get; private set; }
+
+    /// <summary>
+    /// Gets a value indicating whether the loop contains a <c>goto</c>, which may leave it.
+    /// </summary>
+    public bool Jumps { get; private set; }
+
+    /// <summary>
+    /// Gets a value indicating whether the loop can leave the method: <c>return</c>,
+    /// <c>throw</c>, <c>yield break</c>, or a call which never returns.
+    /// </summary>
+    public bool LeavesMethod { get; private set; }
+
+    /// <inheritdoc/>
+    public override void VisitBreakStatement(BreakStatementSyntax node) => Breaks |= nestedBreakTargets == 0;
+
+    /// <inheritdoc/>
+    public override void VisitGotoStatement(GotoStatementSyntax node) => Jumps = true;
+
+    /// <inheritdoc/>
+    public override void VisitReturnStatement(ReturnStatementSyntax node) => LeavesMethod = true;
+
+    /// <inheritdoc/>
+    public override void VisitThrowStatement(ThrowStatementSyntax node) => LeavesMethod = true;
+
+    /// <inheritdoc/>
+    public override void VisitThrowExpression(ThrowExpressionSyntax node) => LeavesMethod = true;
+
+    /// <inheritdoc/>
+    public override void VisitYieldStatement(YieldStatementSyntax node) => LeavesMethod |= node.IsKind(SyntaxKind.YieldBreakStatement);
+
+    /// <inheritdoc/>
+    public override void VisitInvocationExpression(InvocationExpressionSyntax node)
+    {
+        var name = node.Expression switch
+        {
+            MemberAccessExpressionSyntax member => member.Name.Identifier.ValueText,
+            SimpleNameSyntax simple => simple.Identifier.ValueText,
+            _ => null,
+        };
+
+        // A static call is rewritten into one qualified identifier, so only its last part names the method
+        if (name is not null && methodsWhichNeverReturn.Contains(name[(name.LastIndexOf('.') + 1)..]))
+        {
+            LeavesMethod = true;
+        }
+
+        base.VisitInvocationExpression(node);
+    }
+
+    /// <inheritdoc/>
+    public override void VisitWhileStatement(WhileStatementSyntax node) => VisitBreakTarget(node, base.VisitWhileStatement);
+
+    /// <inheritdoc/>
+    public override void VisitDoStatement(DoStatementSyntax node) => VisitBreakTarget(node, base.VisitDoStatement);
+
+    /// <inheritdoc/>
+    public override void VisitForStatement(ForStatementSyntax node) => VisitBreakTarget(node, base.VisitForStatement);
+
+    /// <inheritdoc/>
+    public override void VisitForEachStatement(ForEachStatementSyntax node) => VisitBreakTarget(node, base.VisitForEachStatement);
+
+    /// <inheritdoc/>
+    public override void VisitForEachVariableStatement(ForEachVariableStatementSyntax node) => VisitBreakTarget(node, base.VisitForEachVariableStatement);
+
+    /// <inheritdoc/>
+    public override void VisitSwitchStatement(SwitchStatementSyntax node) => VisitBreakTarget(node, base.VisitSwitchStatement);
+
+    /// <inheritdoc/>
+    public override void VisitSimpleLambdaExpression(SimpleLambdaExpressionSyntax node)
+    {
+    }
+
+    /// <inheritdoc/>
+    public override void VisitParenthesizedLambdaExpression(ParenthesizedLambdaExpressionSyntax node)
+    {
+    }
+
+    /// <inheritdoc/>
+    public override void VisitAnonymousMethodExpression(AnonymousMethodExpressionSyntax node)
+    {
+    }
+
+    /// <inheritdoc/>
+    public override void VisitLocalFunctionStatement(LocalFunctionStatementSyntax node)
+    {
+    }
+
+    private void VisitBreakTarget<TNode>(TNode node, Action<TNode> visit)
+    {
+        ++nestedBreakTargets;
+        visit(node);
+        --nestedBreakTargets;
+    }
+}

tests/Generator.Tests/IsCancellationRequestedTests.cs

@@ -24,5 +24,109 @@ public Task IfNotCancelled() => $$"""
 {
     await Task.Delay(120000, ct);
 }
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task WhileNotCancelledThenThrow() => """
+while (!ct.IsCancellationRequested)
+{
+    await Task.Delay(120000, ct);
+}
+
+throw new OperationCanceledException();
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task WhileNotCancelledChecksInside() => """
+while (!ct.IsCancellationRequested)
+{
+    await Task.Delay(120000, ct);
+    if (ct.IsCancellationRequested)
+    {
+        break;
+    }
+}
+
+Console.WriteLine("Stopped");
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task WhileNotCancelledReturns() => """
+while (!ct.IsCancellationRequested)
+{
+    await Task.Delay(120000, ct);
+    if (Environment.TickCount > 0)
+    {
+        return;
+    }
+}
+
+Console.WriteLine("Stopped");
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task WhileNotCancelledBreaks() => """
+while (!ct.IsCancellationRequested)
+{
+    await Task.Delay(120000, ct);
+    if (Environment.TickCount > 0)
+    {
+        break;
+    }
+}
+
+Console.WriteLine("Stopped");
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task BreakInsideSwitchStaysInLoop() => """
+while (!ct.IsCancellationRequested)
+{
+    await Task.Delay(120000, ct);
+    switch (Environment.TickCount)
+    {
+        case 0:
+            break;
+    }
+}
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task WhileNotCancelledNeverReturns() => """
+[CreateSyncVersion]
+async Task MethodAsync(CancellationToken ct)
+{
+    while (!ct.IsCancellationRequested)
+    {
+        await Task.Delay(120000, ct);
+        if (Environment.TickCount > 0)
+        {
+            Fail();
+        }
+    }
+}
+
+[System.Diagnostics.CodeAnalysis.DoesNotReturn]
+static void Fail() => throw new InvalidOperationException();
+""".Verify();
+
+    [Fact]
+    public Task WhileTrueWrittenByHand() => """
+while (true)
+{
+    await Task.Delay(120000, ct);
+}
+""".Verify(sourceType: SourceType.MethodBody);
+
+    [Fact]
+    public Task WhileNotCancelledReplacedBySyncOnly() => """
+#if SYNC_ONLY
+System.Threading.Thread.Sleep(120000);
+#else
+while (!ct.IsCancellationRequested)
+{
+    await Task.Delay(120000, ct);
+}
+#endif
 """.Verify(sourceType: SourceType.MethodBody);
 }

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.BreakInsideSwitchStaysInLoop#g.verified.cs

@@ -0,0 +1,5 @@
+//HintName: Test.Class.MethodAsync.g.cs
+while (true)
+{
+    global::System.Threading.Thread.Sleep(120000);
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.BreakInsideSwitchStaysInLoop.verified.txt

@@ -0,0 +1,18 @@
+{
+  Diagnostics: [
+    {
+      Location: : (6,8)-(6,13),
+      Message: The synchronized loop never ends. Its condition '!ct.IsCancellationRequested' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if SYNC_ONLY region.,
+      Severity: Warning,
+      WarningLevel: 1,
+      Descriptor: {
+        Id: ZSMGEN006,
+        Title: Loop never ends once synchronized,
+        MessageFormat: The synchronized loop never ends. Its condition '{0}' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if SYNC_ONLY region.,
+        Category: Usage,
+        DefaultSeverity: Warning,
+        IsEnabledByDefault: true
+      }
+    }
+  ]
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelled.verified.txt

@@ -0,0 +1,18 @@
+{
+  Diagnostics: [
+    {
+      Location: : (6,8)-(6,13),
+      Message: The synchronized loop never ends. Its condition '((!((ct.IsCancellationRequested))))' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if SYNC_ONLY region.,
+      Severity: Warning,
+      WarningLevel: 1,
+      Descriptor: {
+        Id: ZSMGEN006,
+        Title: Loop never ends once synchronized,
+        MessageFormat: The synchronized loop never ends. Its condition '{0}' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if SYNC_ONLY region.,
+        Category: Usage,
+        DefaultSeverity: Warning,
+        IsEnabledByDefault: true
+      }
+    }
+  ]
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledBreaks#g.verified.cs

@@ -0,0 +1,11 @@
+//HintName: Test.Class.MethodAsync.g.cs
+while (true)
+{
+    global::System.Threading.Thread.Sleep(120000);
+    if (global::System.Environment.TickCount > 0)
+    {
+        break;
+    }
+}
+
+global::System.Console.WriteLine("Stopped");

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledChecksInside#g.verified.cs

@@ -0,0 +1,5 @@
+//HintName: Test.Class.MethodAsync.g.cs
+while (true)
+{
+    global::System.Threading.Thread.Sleep(120000);
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledChecksInside.verified.txt

@@ -0,0 +1,18 @@
+{
+  Diagnostics: [
+    {
+      Location: : (6,8)-(6,13),
+      Message: The synchronized loop never ends. Its condition '!ct.IsCancellationRequested' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if SYNC_ONLY region.,
+      Severity: Warning,
+      WarningLevel: 1,
+      Descriptor: {
+        Id: ZSMGEN006,
+        Title: Loop never ends once synchronized,
+        MessageFormat: The synchronized loop never ends. Its condition '{0}' becomes 'true' without a CancellationToken, and nothing else leaves the loop. Provide a synchronous implementation in an #if SYNC_ONLY region.,
+        Category: Usage,
+        DefaultSeverity: Warning,
+        IsEnabledByDefault: true
+      }
+    }
+  ]
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledNeverReturns#g.verified.cs

@@ -0,0 +1,9 @@
+//HintName: Test.Class.MethodAsync.g.cs
+while (true)
+{
+    global::System.Threading.Thread.Sleep(120000);
+    if (global::System.Environment.TickCount > 0)
+    {
+        global::Test.Class.Fail();
+    }
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledReplacedBySyncOnly#g.verified.cs

@@ -0,0 +1,2 @@
+//HintName: Test.Class.MethodAsync.g.cs
+System.Threading.Thread.Sleep(120000);

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledReturns#g.verified.cs

@@ -0,0 +1,9 @@
+//HintName: Test.Class.MethodAsync.g.cs
+while (true)
+{
+    global::System.Threading.Thread.Sleep(120000);
+    if (global::System.Environment.TickCount > 0)
+    {
+        return;
+    }
+}

tests/Generator.Tests/Snapshots/IsCancellationRequestedTests.WhileNotCancelledThenThrow#g.verified.cs

@@ -0,0 +1,5 @@
+//HintName: Test.Class.MethodAsync.g.cs
+while (true)
+{
+    global::System.Threading.Thread.Sleep(120000);
+}