From 0f56e166b16100c23dc81ae082f6155362b7c596 Mon Sep 17 00:00:00 2001
From: Vitek Karas <10670590+vitek-karas@users.noreply.github.com>
Date: Mon, 17 Jul 2023 02:06:05 -0700
Subject: [PATCH] Don't crash the trim analyzer if it finds unrecognized nodes
 in the input (#88836)

New versions of the compiler will introduce new nodes and values. The analyzer can never be 100% in sycn with the compiler, so it needs to be able to gracefully handle nodes it doesn't know anything about.

Change the several throws to just Debug.Fail. For end-users if we hit unrecognized node, we will effectively ignore that part of the code. So not 100% precise, but the analyzer will never be 100% regardles.

This is in response to #88684, but we can't add tests for it yet because the necessary compiler changes are in Preview 6, the repo is still on Preview 5.
---
 .../DataFlow/CapturedReferenceValue.cs        |  8 ++++-
 .../DataFlow/LocalDataFlowVisitor.cs          |  8 ++++-
 .../TrimAnalysis/ParameterProxy.cs            | 31 +++++++++++--------
 3 files changed, 32 insertions(+), 15 deletions(-)

diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/CapturedReferenceValue.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/CapturedReferenceValue.cs
index 94ee978b1c289..d7033d9d79961 100644
--- a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/CapturedReferenceValue.cs
+++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/CapturedReferenceValue.cs
@@ -2,6 +2,7 @@
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
 using System;
+using System.Diagnostics;
 using ILLink.Shared.DataFlow;
 using Microsoft.CodeAnalysis;
 
@@ -29,7 +30,12 @@ public CapturedReferenceValue (IOperation operation)
 				// These will just be ignored when referenced later.
 				break;
 			default:
-				throw new NotImplementedException (operation.Kind.ToString ());
+				// Assert on anything else as it means we need to implement support for it
+				// but do not throw here as it means new Roslyn version could cause the analyzer to crash
+				// which is not fixable by the user. The analyzer is not going to be 100% correct no matter what we do
+				// so effectively ignoring constructs it doesn't understand is OK.
+				Debug.Fail ($"{operation.GetType ()}: {operation.Syntax.GetLocation ().GetLineSpan ()}");
+				break;
 			}
 			Reference = operation;
 		}
diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
index b46d00e2a3a85..6143e03ab935d 100644
--- a/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
+++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/DataFlow/LocalDataFlowVisitor.cs
@@ -282,7 +282,13 @@ public override TValue VisitSimpleAssignment (ISimpleAssignmentOperation operati
 				// (don't have specific I*Operation types), such as pointer dereferences.
 				if (targetOperation.Kind is OperationKind.None)
 					break;
-				throw new NotImplementedException ($"{targetOperation.GetType ()}: {targetOperation.Syntax.GetLocation ().GetLineSpan ()}");
+
+				// Assert on anything else as it means we need to implement support for it
+				// but do not throw here as it means new Roslyn version could cause the analyzer to crash
+				// which is not fixable by the user. The analyzer is not going to be 100% correct no matter what we do
+				// so effectively ignoring constructs it doesn't understand is OK.
+				Debug.Fail ($"{targetOperation.GetType ()}: {targetOperation.Syntax.GetLocation ().GetLineSpan ()}");
+				break;
 			}
 			return Visit (operation.Value, state);
 		}
diff --git a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ParameterProxy.cs b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ParameterProxy.cs
index 8888b45404ff2..f57de9138dbf8 100644
--- a/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ParameterProxy.cs
+++ b/src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/ParameterProxy.cs
@@ -1,7 +1,7 @@
 // Copyright (c) .NET Foundation and contributors. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
-using System;
+using System.Diagnostics;
 using ILLink.RoslynAnalyzer;
 using Microsoft.CodeAnalysis;
 
@@ -15,18 +15,23 @@ public ParameterProxy (IParameterSymbol parameter)
 			Index = (ParameterIndex) parameter.Ordinal + (Method.HasImplicitThis () ? 1 : 0);
 		}
 
-		public partial ReferenceKind GetReferenceKind () =>
-			IsImplicitThis
-			? Method.Method.ContainingType.IsValueType
-				? ReferenceKind.Ref
-				: ReferenceKind.None
-			: Method.Method.Parameters[MetadataIndex].RefKind switch {
-				RefKind.Ref => ReferenceKind.Ref,
-				RefKind.In => ReferenceKind.In,
-				RefKind.Out => ReferenceKind.Out,
-				RefKind.None => ReferenceKind.None,
-				_ => throw new NotImplementedException ($"Unexpected RefKind found on parameter {GetDisplayName ()}")
-			};
+		public partial ReferenceKind GetReferenceKind ()
+		{
+			if (IsImplicitThis)
+				return Method.Method.ContainingType.IsValueType
+					? ReferenceKind.Ref
+					: ReferenceKind.None;
+
+			switch (Method.Method.Parameters[MetadataIndex].RefKind) {
+			case RefKind.Ref: return ReferenceKind.Ref;
+			case RefKind.In: return ReferenceKind.In;
+			case RefKind.Out: return ReferenceKind.Out;
+			case RefKind.None: return ReferenceKind.None;
+			default:
+				Debug.Fail ($"Unexpected RefKind {Method.Method.Parameters[MetadataIndex].RefKind} found on parameter {GetDisplayName ()}");
+				return ReferenceKind.None;
+			}
+		}
 
 		/// <summary>
 		/// Returns the IParameterSymbol representing the parameter. Returns null for the implicit this paramter.