Analyzer to make Task`1.Result scary. (space-wizards#3069)

chromiumboy · Jul 23, 2022 · ec70abe · ec70abe
1 parent 0ec6995
commit ec70abe
Show file tree

Hide file tree

Showing 8 changed files with 59 additions and 2 deletions.
diff --git a/Robust.Analyzers/Diagnostics.cs b/Robust.Analyzers/Diagnostics.cs
@@ -8,6 +8,7 @@ public static class Diagnostics
     public const string IdSerializable = "RA0001";
     public const string IdAccess = "RA0002";
     public const string IdExplicitVirtual = "RA0003";
+    public const string IdTaskResult = "RA0004";
 
     public static SuppressionDescriptor MeansImplicitAssignment =>
         new SuppressionDescriptor("RADC1000", "CS0649", "Marked as implicitly assigned.");

diff --git a/Robust.Analyzers/TaskResultAnalyzer.cs b/Robust.Analyzers/TaskResultAnalyzer.cs
@@ -0,0 +1,44 @@
+using System.Collections.Immutable;
+using System.Diagnostics.CodeAnalysis;
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+
+namespace Robust.Analyzers;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class TaskResultAnalyzer : DiagnosticAnalyzer
+{
+    [SuppressMessage("ReSharper", "RS2008")]
+    private static readonly DiagnosticDescriptor ResultRule = new DiagnosticDescriptor(
+        Diagnostics.IdTaskResult,
+        "Risk of deadlock from accessing Task<T>.Result",
+        "Accessing Task<T>.Result is dangerous and can cause deadlocks in some contexts. If you understand how this works and are certain that you aren't causing a deadlock here, mute this error with #pragma.",
+        "Usage",
+        DiagnosticSeverity.Error,
+        true);
+
+    public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics =>
+        ImmutableArray.Create(ResultRule);
+
+    public override void Initialize(AnalysisContext context)
+    {
+        context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+        context.EnableConcurrentExecution();
+        context.RegisterOperationAction(Check, OperationKind.PropertyReference);
+    }
+
+    private static void Check(OperationAnalysisContext context)
+    {
+        var taskType = context.Compilation.GetTypeByMetadataName("System.Threading.Tasks.Task`1");
+
+        var operation = (IPropertyReferenceOperation) context.Operation;
+        var member = operation.Member;
+
+        if (member.Name == "Result" && taskType.Equals(member.ContainingType.ConstructedFrom, SymbolEqualityComparer.Default))
+        {
+            var diag = Diagnostic.Create(ResultRule, operation.Syntax.GetLocation());
+            context.ReportDiagnostic(diag);
+        }
+    }
+}
diff --git a/Robust.Client/Graphics/Clyde/Windowing/Glfw.WindowThread.cs b/Robust.Client/Graphics/Clyde/Windowing/Glfw.WindowThread.cs
@@ -132,7 +132,9 @@ public void TerminateWindowLoop()
                 _cmdWriter.Complete();
 
                 // Drain command queue ignoring it until the window thread confirms completion.
+#pragma warning disable RA0004
                 while (_eventReader.WaitToReadAsync().AsTask().Result)
+#pragma warning restore RA0004
                 {
                     _eventReader.TryRead(out _);
                 }

diff --git a/Robust.Client/Graphics/Clyde/Windowing/Glfw.Windows.cs b/Robust.Client/Graphics/Clyde/Windowing/Glfw.Windows.cs
@@ -260,8 +260,9 @@ private void WinThreadWinSetFullscreen(CmdWinSetFullscreen cmd)
 
                 // Block the main thread (to avoid stuff like texture uploads being problematic).
                 WaitWindowCreate(task);
-
+#pragma warning disable RA0004
                 var (reg, errorResult) = task.Result;
+#pragma warning restore RA0004
 
                 if (reg != null)
                 {

diff --git a/Robust.Client/Graphics/Clyde/Windowing/Sdl2.WindowThread.cs b/Robust.Client/Graphics/Clyde/Windowing/Sdl2.WindowThread.cs
@@ -113,7 +113,9 @@ public void TerminateWindowLoop()
             _cmdWriter.Complete();
 
             // Drain command queue ignoring it until the window thread confirms completion.
+#pragma warning disable RA0004
             while (_eventReader.WaitToReadAsync().AsTask().Result)
+#pragma warning restore RA0004
             {
                 _eventReader.TryRead(out _);
             }

diff --git a/Robust.Client/Graphics/Clyde/Windowing/Sdl2.Windows.cs b/Robust.Client/Graphics/Clyde/Windowing/Sdl2.Windows.cs
@@ -48,7 +48,10 @@ private sealed partial class Sdl2WindowingImpl
             // Block the main thread (to avoid stuff like texture uploads being problematic).
             WaitWindowCreate(task);
 
+#pragma warning disable RA0004
+            // Block above ensured task is done, this is safe.
             var (reg, error) = task.Result;
+#pragma warning restore RA0004
             if (reg != null)
             {
                 reg.Owner = reg.Handle;

diff --git a/Robust.Shared/ContentPack/AssemblyTypeChecker.cs b/Robust.Shared/ContentPack/AssemblyTypeChecker.cs
@@ -170,7 +170,9 @@ public bool CheckAssembly(Stream assembly)
                 return true;
             }
 
+#pragma warning disable RA0004
             var loadedConfig = _config.Result;
+#pragma warning restore RA0004
 
             // We still do explicit type reference scanning, even though the actual whitelists work with raw members.
             // This is so that we can simplify handling of generic type specifications during member checking:
@@ -233,7 +235,9 @@ private bool DoVerifyIL(
                 }
             });
 
+#pragma warning disable RA0004
             var loadedCfg = _config.Result;
+#pragma warning restore RA0004
 
             var verifyErrors = false;
             foreach (var res in bag)

diff --git a/Robust.Shared/Network/NetManager.ClientConnect.cs b/Robust.Shared/Network/NetManager.ClientConnect.cs
@@ -373,7 +373,7 @@ async Task ConnectSecondDelayed(CancellationToken cancellationToken)
                             Logger.DebugS("net", "First peer failed.");
                             firstPeer.Peer.Shutdown("You failed.");
                             _toCleanNetPeers.Add(firstPeer.Peer);
-                            firstReason = firstPeerChanged.Result;
+                            firstReason = await firstPeerChanged;
                             await secondPeerChanged;
                             winningPeer = secondPeer;
                             winningConnection = secondConnection;