Add Analyzer for finding errors with ternaries in interpolated strings

why is this not part of the SDK

.global.editorconfig.ini

@@ -36,6 +36,9 @@ dotnet_diagnostic.BHI1120.severity = silent
 
 # Check result of IDictionary.TryGetValue, or discard it if default(T) is desired
 dotnet_diagnostic.BHI1200.severity = error
+# Inferred type of bramches of ternary expression in interpolation don't match
+dotnet_diagnostic.BHI1210.severity = error
+
 # Call to FirstOrDefault when elements are of a value type; FirstOrNull may have been intended
 dotnet_diagnostic.BHI3100.severity = error
 # Use .Order()/.OrderDescending() shorthand

ExternalProjects/BizHawk.Analyzer/TernaryInferredTypeMismatchAnalyzer.cs

@@ -0,0 +1,89 @@
+namespace BizHawk.Analyzers;
+
+using System.Collections.Immutable;
+
+using Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.Diagnostics;
+using Microsoft.CodeAnalysis.Operations;
+
+[DiagnosticAnalyzer(LanguageNames.CSharp)]
+public sealed class TernaryInferredTypeMismatchAnalyzer : DiagnosticAnalyzer
+{
+	private static readonly DiagnosticDescriptor DiagTernaryInferredTypeMismatch = new(
+		id: "BHI1210",
+		title: "Inferred type of bramches of ternary expression in interpolation don't match",
+//		messageFormat: "Inferred type of ternary expression is object (missing ToString call?)",
+		messageFormat: "{0}",
+		category: "Usage",
+		defaultSeverity: DiagnosticSeverity.Warning,
+		isEnabledByDefault: true);
+
+	public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics { get; } = ImmutableArray.Create(DiagTernaryInferredTypeMismatch);
+
+	public override void Initialize(AnalysisContext context)
+	{
+		context.ConfigureGeneratedCodeAnalysis(GeneratedCodeAnalysisFlags.None);
+		context.EnableConcurrentExecution();
+		context.RegisterCompilationStartAction(initContext =>
+		{
+			var objectSym = initContext.Compilation.GetTypeByMetadataName("System.Object")!;
+			var stringSym = initContext.Compilation.GetTypeByMetadataName("System.String")!;
+			initContext.RegisterOperationAction(oac =>
+				{
+					var ifelseOrTernaryOp = (IConditionalOperation) oac.Operation;
+					if (ifelseOrTernaryOp.WhenFalse is null) return;
+					var parent = ifelseOrTernaryOp.Parent!;
+					if (parent.Kind is OperationKind.Conversion) parent = parent.Parent!;
+					if (parent.Kind is not OperationKind.Interpolation) return;
+					var ternaryOp = ifelseOrTernaryOp;
+					var typeTernary = ternaryOp.Type!;
+#if false // never hit; either both branches are string and there are no conversions, or conversions are necessary
+					if (stringSym.Matches(typeTernary)) return;
+#endif
+					var lhs = ternaryOp.WhenTrue;
+					var rhs = ternaryOp.WhenFalse;
+
+					static IOperation TrimImplicitCast(IOperation op)
+						=> op is IConversionOperation { Conversion.IsImplicit: true } implCastOp ? implCastOp.Operand : op;
+					var typeLHS = TrimImplicitCast(lhs).Type!;
+					var typeRHS = TrimImplicitCast(rhs).Type!;
+					if (typeLHS.Matches(typeRHS)) return; // unnecessary conversion operators on each branch? seen with `? this : this`
+
+					const string ERR_MSG_OBJECT = "missing ToString means ternary branches are upcast to object";
+					var fatal = false;
+					IOperation flaggedOp = ternaryOp;
+					string message;
+					if (stringSym.Matches(typeLHS))
+					{
+						flaggedOp = rhs;
+						message = ERR_MSG_OBJECT;
+					}
+					else if (stringSym.Matches(typeRHS))
+					{
+						flaggedOp = lhs;
+						message = ERR_MSG_OBJECT;
+					}
+					else if (objectSym.Matches(typeTernary))
+					{
+						fatal = true;
+						message = "ternary branches are upcast to object! add ToString calls, or convert one to the other's type";
+					}
+					else
+					{
+						// if one's already an e.g. int literal, flag the e.g. char literal
+						if (typeTernary.Matches(typeLHS)) flaggedOp = rhs;
+						else if (typeTernary.Matches(typeRHS)) flaggedOp = lhs;
+						message = $"ternary branches are converted to {typeTernary} before serialisation, possibly unintended";
+					}
+					oac.ReportDiagnostic(Diagnostic.Create(
+						DiagTernaryInferredTypeMismatch,
+						flaggedOp.Syntax.GetLocation(),
+						fatal ? DiagnosticSeverity.Error : DiagnosticSeverity.Warning,
+						additionalLocations: null,
+						properties: null,
+						messageArgs: message));
+				},
+				OperationKind.Conditional);
+		});
+	}
+}

ExternalProjects/BizHawk.AnalyzersTests/BizHawk.Analyzer/TernaryInferredTypeMismatchAnalyzerTests.cs

@@ -0,0 +1,34 @@
+namespace BizHawk.Tests.Analyzers;
+
+using System.Threading.Tasks;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+using Verify = Microsoft.CodeAnalysis.CSharp.Testing.CSharpAnalyzerVerifier<
+	BizHawk.Analyzers.TernaryInferredTypeMismatchAnalyzer,
+	Microsoft.CodeAnalysis.Testing.DefaultVerifier>;
+
+[TestClass]
+public sealed class TernaryInferredTypeMismatchAnalyzerTests
+{
+	[TestMethod]
+	public Task CheckMisuseOfTernariesInInterpolations()
+		=> Verify.VerifyAnalyzerAsync("""
+			using System.Collections.Generic;
+			using System.Diagnostics;
+			public static class Cases {
+				private static string X(bool cond)
+					=> $"{(cond ? 'p'.ToString() : 9.ToString())}";
+				private static string Y(bool cond)
+					=> $"{(cond ? "p" : 9.ToString())}";
+				private static string Z(bool cond)
+					=> $"{(cond ? "p" : "q")}";
+				private static string A(bool cond)
+					=> $"{(cond ? "p" : {|BHI1210:9|})}";
+				private static string B(bool cond)
+					=> $"{(cond ? {|BHI1210:'p'|} : 9)}";
+				private static string C(bool cond, Process p, Queue<int> q)
+					=> $"{({|BHI1210:cond ? p : q|})}";
+			}
+		""");
+}

src/BizHawk.Client.EmuHawk/MainForm.cs

@@ -68,7 +68,7 @@ namespace BizHawk.Client.EmuHawk
 					var temp = new ToolStripMenuItemEx
 					{
 						Tag = i,
-						Text = $"{(quotient > 0 ? quotient : "")}&{remainder}x"
+						Text = $"{(quotient is not 0L ? quotient.ToString() : string.Empty)}&{remainder}x",
 					};
 					temp.Click += this.WindowSize_Click;
 					WindowSizeSubMenu.DropDownItems.Insert(i - 1, temp);

src/BizHawk.Emulation.Common/filetype_detectors/SatellaviewFileTypeDetector.cs

@@ -87,7 +87,7 @@ namespace BizHawk.Emulation.Common
 				=> _header = header;
 
 			public override string ToString()
-				=> $"[{ContentTypeField >> 4:X1}] {Title} r{Revision} ({(IsSelfDestructing ? RemainingPlays : "unlimited")} plays left)";
+				=> $"[{ContentTypeField >> 4:X1}] {Title} r{Revision} ({(IsSelfDestructing ? RemainingPlays.ToString() : "unlimited")} plays left)";
 
 			public bool VerifyChecksum(ReadOnlySpan<byte> rom)
 				=> true; //TODO need to parse page mapping from offset 0x20..0x23 in order to calculate this

src/BizHawk.Emulation.Cores/CPUs/FairchildF8/F3850.Disassembler.cs

@@ -28,7 +28,7 @@ namespace BizHawk.Emulation.Cores.Components.FairchildF8
 			if (format.ContainsOrdinal('d'))
 			{
 				var b = unchecked((sbyte)read(addr++));
-				format = format.Replace("d", $"{(b < 0 ? '-' : '+')}{(b < 0 ? -b : b):X2}h");
+				format = format.Replace("d", $"{(b < 0 ? '-' : '+')}{Math.Abs((short) b):X2}h");
 			}
 
 			return format;

src/BizHawk.Emulation.Cores/CPUs/Z80A/NewDisassembler.cs

@@ -374,7 +374,7 @@ namespace BizHawk.Emulation.Cores.Components.Z80A
 			if (format.ContainsOrdinal('d'))
 			{
 				var b = unchecked ((sbyte) read(addr++));
-				format = format.Replace("d", $"{(b < 0 ? '-' : '+')}{(b < 0 ? -b : b):X2}h");
+				format = format.Replace("d", $"{(b < 0 ? '-' : '+')}{Math.Abs((short) b):X2}h");
 			}
 
 			return format;