Move the editor onto ktsu.UndoRedo 3, stop saves and loads from silently losing a schema, and refuse unreadable or unsizable input

Directory.Packages.props

@@ -15,7 +15,7 @@
     <PackageVersion Include="ktsu.ThemeProvider" Version="3.2.1" />
     <PackageVersion Include="ktsu.IntervalAction" Version="1.6.0" />
     <PackageVersion Include="ktsu.Keybinding" Version="2.1.7" />
-    <PackageVersion Include="ktsu.UndoRedo.Core" Version="1.1.4" />
+    <PackageVersion Include="ktsu.UndoRedo" Version="3.4.0" />
     <PackageVersion Include="ktsu.RoundTripStringJsonConverter" Version="1.0.76" />
     <PackageVersion Include="ktsu.Semantics.Quantities" Version="5.12.1" />
     <PackageVersion Include="ktsu.Semantics.Strings" Version="5.12.1" />
@@ -25,7 +25,7 @@
     <PackageVersion Include="ktsu.ImGui.Popups" Version="3.75.3" />
     <PackageVersion Include="ktsu.ImGui.Probes" Version="3.75.3" />
     <PackageVersion Include="ktsu.ImGui.Widgets" Version="3.75.3" />
-    <PackageVersion Include="ktsu.ImGui.NodeEditor" Version="3.38.5" />
+    <PackageVersion Include="ktsu.ImGui.NodeEditor" Version="3.75.3" />
     <PackageVersion Include="ktsu.NodeGraph" Version="1.0.0" />
     <PackageVersion Include="ktsu.AppDataStorage" Version="1.18.2" />
     <PackageVersion Include="Microsoft.CodeAnalysis.CSharp" Version="5.9.0" />

Schema.Cpp.Test/CppGeneratorOptionsFileTests.cs

@@ -148,6 +148,44 @@ public void RefusesAnExistingTypeWithNoName()
 		Assert.Contains("Kilograms", message, StringComparison.Ordinal);
 	}
 
+	/// <summary>
+	/// A null in place of the whole table is refused by name rather than escaping as a
+	/// NullReferenceException.
+	/// </summary>
+	[TestMethod]
+	public void RefusesANullExistingTypesTable()
+	{
+		const string json = """{ "existingTypes": null }""";
+
+		Assert.IsFalse(CppGeneratorOptionsFile.TryParse(json, out CppGeneratorOptions? _, out string? message));
+		Assert.Contains("existingTypes", message, StringComparison.Ordinal);
+	}
+
+	/// <summary>The same, for one entry in the table.</summary>
+	[TestMethod]
+	public void RefusesANullExistingType()
+	{
+		const string json = """{ "existingTypes": { "Kilograms": null } }""";
+
+		Assert.IsFalse(CppGeneratorOptionsFile.TryParse(json, out CppGeneratorOptions? _, out string? message));
+		Assert.Contains("existingTypes.Kilograms", message, StringComparison.Ordinal);
+	}
+
+	/// <summary>
+	/// A null anywhere else the options need a value is refused too, so no other key reaches the
+	/// generator as a null it was never written to expect.
+	/// </summary>
+	[TestMethod]
+	[DataRow("""{ "generatedBy": null }""", "generatedBy")]
+	[DataRow("""{ "headerExtension": null }""", "headerExtension")]
+	[DataRow("""{ "vector3": { "name": "holo::Vec3", "include": null } }""", "include")]
+	[DataRow("""{ "quantities": { "namespace": "holo", "include": "q.hpp", "storage": null } }""", "storage")]
+	public void RefusesANullWhereAValueIsNeeded(string json, string key)
+	{
+		Assert.IsFalse(CppGeneratorOptionsFile.TryParse(json, out CppGeneratorOptions? _, out string? message));
+		Assert.Contains(key, message, StringComparison.OrdinalIgnoreCase);
+	}
+
 	/// <summary>
 	/// Malformed JSON reports where it went wrong, which is the whole of what makes a message about
 	/// a hand-written file useful.

Schema.Cpp/CppGeneratorOptionsFile.cs

@@ -62,6 +62,12 @@ public static class CppGeneratorOptionsFile
 		ReadCommentHandling = JsonCommentHandling.Skip,
 		AllowTrailingCommas = true,
 		UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow,
+
+		// A JSON null in a property the options declare non-nullable would otherwise be written
+		// straight in, and fail later as a NullReferenceException with nothing pointing at the file.
+		// This covers properties and constructor parameters; dictionary values are checked in
+		// IsSpelled, because the serializer does not apply annotations to a type argument.
+		RespectNullableAnnotations = true,
 		Converters = { new JsonStringEnumConverter() },
 	};
 
@@ -130,9 +136,9 @@ private static bool IsSpelled(CppGeneratorOptions options, [NotNullWhen(false)]
 			}
 		}
 
-		foreach ((string schemaName, CppTypeSpelling spelling) in options.ExistingTypes)
+		foreach ((string schemaName, CppTypeSpelling? spelling) in options.ExistingTypes)
 		{
-			if (string.IsNullOrWhiteSpace(spelling.Name))
+			if (spelling is null || string.IsNullOrWhiteSpace(spelling.Name))
 			{
 				message = $"'existingTypes.{schemaName}' has no name.";
 				return false;

Schema.Editor.Test/SchemaFileTests.cs

@@ -72,6 +72,72 @@
 		Assert.IsTrue(File.Exists(path));
 	}
 
+	/// <summary>
+	/// A write that fails partway leaves the previous save as it was. Writing straight over the target
+	/// truncated it first, so a full disk or a dropped share destroyed the only saved copy.
+	/// </summary>
+	[TestMethod]
+	public void AFailedWriteLeavesThePreviousSaveIntact()
+	{
+		AbsoluteFilePath path = ScratchFile("kept.schema.json");
+		Schema original = new();
+		original.AddClass("User".As<ClassName>());
+		Assert.IsTrue(SchemaFile.TrySave(original, path));
+		byte[] before = File.ReadAllBytes(path);
+
+		Schema edited = new();
+		edited.AddClass("Order".As<ClassName>());
+		bool saved = SchemaFile.TrySave(edited, path, static (target, content) =>
+		{
+			File.WriteAllText(target, content[..(content.Length / 2)]);
+			throw new IOException("The disk is full.");
+		});
+
+		Assert.IsFalse(saved);
+		Assert.AreSequenceEqual(before, File.ReadAllBytes(path));
+	}
+
+	/// <summary>
+	/// The temporary file a failed save wrote into is removed, so failures do not litter the folder
+	/// the schema lives in.
+	/// </summary>
+	[TestMethod]
+	public void AFailedWriteLeavesNoTemporaryFileBehind()
+	{
+		AbsoluteFilePath path = ScratchFile("tidy.schema.json");
+
+		bool saved = SchemaFile.TrySave(new Schema(), path, static (target, content) =>
+		{
+			File.WriteAllText(target, content);
+			throw new IOException("The share went away.");
+		});
+
+		Assert.IsFalse(saved);
+		Assert.HasCount(0, Directory.GetFiles(scratchDirectory));
+	}
+
+	/// <summary>
+	/// Saving over an existing file replaces its content and leaves only that file in the folder.
+	/// </summary>
+	[TestMethod]
+	public void SavingOverAnExistingFileReplacesIt()
+	{
+		AbsoluteFilePath path = ScratchFile("replaced.schema.json");
+		Schema first = new();
+		first.AddClass("User".As<ClassName>());
+		Schema second = new();
+		second.AddClass("Order".As<ClassName>());
+
+		Assert.IsTrue(SchemaFile.TrySave(first, path));
+		Assert.IsTrue(SchemaFile.TrySave(second, path));
+		Assert.IsTrue(SchemaFile.TryLoad(path, out Schema? loaded));
+
+		Assert.IsNotNull(loaded);
+		Assert.IsNotNull(loaded.GetClass("Order".As<ClassName>()));
+		Assert.IsNull(loaded.GetClass("User".As<ClassName>()));
+		Assert.HasCount(1, Directory.GetFiles(scratchDirectory));
+	}
+
 	[TestMethod]
 	public void SavingWithNoPathFails() =>
 		Assert.IsFalse(SchemaFile.TrySave(new Schema(), new AbsoluteFilePath()));

Schema.Editor/Schema.Editor.csproj

@@ -26,7 +26,7 @@
     <PackageReference Include="ktsu.Semantics.Quantities" />
     <PackageReference Include="ktsu.ThemeProvider" />
     <PackageReference Include="ktsu.Semantics.Strings" />
-    <PackageReference Include="ktsu.UndoRedo.Core" />
+    <PackageReference Include="ktsu.UndoRedo" />
     <PackageReference Include="Microsoft.SourceLink.GitHub" />
     <PackageReference Include="Microsoft.SourceLink.AzureRepos.Git" />
     <PackageReference Include="Polyfill" PrivateAssets="all" />

Schema.Editor/SchemaFile.cs

@@ -85,7 +85,23 @@ public static SchemaLoadResult Load(AbsoluteFilePath filePath)
 	/// <param name="schema">The schema to save.</param>
 	/// <param name="filePath">The path to save the schema to.</param>
 	/// <returns>True if the schema was saved successfully, false otherwise.</returns>
-	public static bool TrySave(Schema schema, AbsoluteFilePath filePath)
+	public static bool TrySave(Schema schema, AbsoluteFilePath filePath) =>
+		TrySave(schema, filePath, File.WriteAllText);
+
+	/// <summary>
+	/// Saves a Schema to a file path, writing the content through <paramref name="writeFile"/>.
+	/// </summary>
+	/// <remarks>
+	/// The content goes to a temporary file beside the target and is swapped into place only once it
+	/// has been written whole, so a write that fails partway - a full disk, a dropped network share -
+	/// leaves the previous save intact rather than truncated. <paramref name="writeFile"/> is the seam
+	/// a test uses to make that write fail; it is only ever given the temporary path.
+	/// </remarks>
+	/// <param name="schema">The schema to save.</param>
+	/// <param name="filePath">The path to save the schema to.</param>
+	/// <param name="writeFile">Writes the given text to the given path.</param>
+	/// <returns>True if the schema was saved successfully, false otherwise.</returns>
+	internal static bool TrySave(Schema schema, AbsoluteFilePath filePath, Action<string, string> writeFile)
 	{
 		if (schema is null || string.IsNullOrEmpty(filePath))
 		{
@@ -102,7 +118,7 @@ public static bool TrySave(Schema schema, AbsoluteFilePath filePath)
 				Directory.CreateDirectory(directory);
 			}
 
-			File.WriteAllText(filePath, json);
+			WriteAtomically(filePath, json, writeFile);
 			return true;
 		}
 		catch (JsonException)
@@ -116,4 +132,39 @@ public static bool TrySave(Schema schema, AbsoluteFilePath filePath)
 
 		return false;
 	}
+
+	/// <summary>
+	/// Writes <paramref name="content"/> to a temporary file in the target's directory, then moves it
+	/// over the target, so the target only ever holds a complete previous save or a complete new one.
+	/// </summary>
+	/// <remarks>
+	/// The temporary file is in the same directory so the final step is a rename on one volume rather
+	/// than a copy. It is deleted on any failure, which is what the <c>finally</c> is for: an
+	/// exception the caller does not catch still must not leave it behind.
+	/// </remarks>
+	private static void WriteAtomically(string targetPath, string content, Action<string, string> writeFile)
+	{
+		string temporaryPath = $"{targetPath}.tmp-{Guid.NewGuid():N}";
+
+		try
+		{
+			writeFile(temporaryPath, content);
+
+			if (File.Exists(targetPath))
+			{
+				File.Replace(temporaryPath, targetPath, destinationBackupFileName: null);
+			}
+			else
+			{
+				File.Move(temporaryPath, targetPath);
+			}
+		}
+		finally
+		{
+			if (File.Exists(temporaryPath))
+			{
+				File.Delete(temporaryPath);
+			}
+		}
+	}
 }

Schema.Test/ContainmentCycleTests.cs

@@ -0,0 +1,128 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.Schema.Tests;
+
+using System.Collections.ObjectModel;
+
+using ktsu.Schema.Models;
+using ktsu.Schema.Models.Names;
+using ktsu.Schema.Models.Types;
+using ktsu.Semantics.Strings;
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+/// <summary>
+/// Covers the refusal of a class that holds itself by value, which no generator can turn into a
+/// type of finite size.
+/// </summary>
+[TestClass]
+public sealed class ContainmentCycleTests
+{
+	/// <summary>
+	/// A class holding itself directly is the commonest way to write this, and it is reported at
+	/// the member that does it.
+	/// </summary>
+	[TestMethod]
+	public void AClassHoldingItselfIsAnError()
+	{
+		Schema schema = new();
+		SchemaClass node = schema.AddClass("TreeNode".As<ClassName>())!;
+		node.AddMember("Parent".As<MemberName>())!.SetType(ObjectOf("TreeNode"));
+
+		SchemaValidationIssue error = Errors(schema).Single();
+
+		Assert.AreEqual("TreeNode.Parent", error.Path);
+		StringAssert.Contains(error.Message, "TreeNode.Parent → TreeNode");
+	}
+
+	/// <summary>
+	/// A cycle through other classes is one issue, naming every step of it, rather than one per
+	/// class on it.
+	/// </summary>
+	[TestMethod]
+	public void ACycleThroughSeveralClassesIsOneErrorNamingTheRoute()
+	{
+		Schema schema = new();
+		schema.AddClass("A".As<ClassName>())!.AddMember("b".As<MemberName>())!.SetType(ObjectOf("B"));
+		schema.AddClass("B".As<ClassName>())!.AddMember("c".As<MemberName>())!.SetType(ObjectOf("C"));
+		schema.AddClass("C".As<ClassName>())!.AddMember("a".As<MemberName>())!.SetType(ObjectOf("A"));
+
+		SchemaValidationIssue error = Errors(schema).Single();
+
+		Assert.AreEqual("C.a", error.Path);
+		StringAssert.Contains(error.Message, "A.b → B.c → C.a → A");
+	}
+
+	/// <summary>
+	/// An optional and a result hold what they wrap in place, so a cycle through either is still
+	/// a cycle.
+	/// </summary>
+	/// <param name="wrapper">The wrapper the self-reference goes through.</param>
+	[TestMethod]
+	[DataRow("Optional")]
+	[DataRow("Result")]
+	public void ACycleThroughAnInlineWrapperIsAnError(string wrapper)
+	{
+		BaseType element = ObjectOf("TreeNode");
+		BaseType type = wrapper == "Optional" ? new Optional { ElementType = element } : new Result { ElementType = element };
+
+		Schema schema = new();
+		schema.AddClass("TreeNode".As<ClassName>())!.AddMember("Parent".As<MemberName>())!.SetType(type);
+
+		StringAssert.Contains(Errors(schema).Single(issue => issue.Message.Contains("holds itself", StringComparison.Ordinal)).Message, "holds itself by value");
+	}
+
+	/// <summary>
+	/// An array and a handle reach their element by indirection, which is how a recursive
+	/// structure is meant to be written, so neither is a cycle.
+	/// </summary>
+	/// <param name="indirection">The indirection the self-reference goes through.</param>
+	[TestMethod]
+	[DataRow("Array")]
+	[DataRow("Handle")]
+	public void AReferenceThroughAnIndirectionIsNotACycle(string indirection)
+	{
+		BaseType element = ObjectOf("TreeNode");
+		BaseType type = indirection == "Array" ? new Models.Types.Array { ElementType = element } : new Handle { ElementType = element };
+
+		Schema schema = new();
+		schema.AddClass("TreeNode".As<ClassName>())!.AddMember("Children".As<MemberName>())!.SetType(type);
+
+		Assert.IsFalse(
+			schema.Validate().Any(i => i.Message.Contains("holds itself by value", StringComparison.Ordinal)),
+			string.Join("; ", schema.Validate().Select(i => i.Message)));
+	}
+
+	/// <summary>
+	/// Two classes holding the same third class is a shared part, not a cycle.
+	/// </summary>
+	[TestMethod]
+	public void ADiamondIsNotACycle()
+	{
+		Schema schema = new();
+		schema.AddClass("Point".As<ClassName>())!.AddMember("X".As<MemberName>())!.SetType(new Float());
+		SchemaClass line = schema.AddClass("Line".As<ClassName>())!;
+		line.AddMember("Start".As<MemberName>())!.SetType(ObjectOf("Point"));
+		line.AddMember("End".As<MemberName>())!.SetType(ObjectOf("Point"));
+		schema.AddClass("Shape".As<ClassName>())!.AddMember("Edge".As<MemberName>())!.SetType(ObjectOf("Line"));
+
+		Assert.IsEmpty(Errors(schema), string.Join("; ", Errors(schema).Select(e => e.Message)));
+	}
+
+	/// <summary>
+	/// A class named by a member but not declared is reported elsewhere, and is not walked into.
+	/// </summary>
+	[TestMethod]
+	public void AnUnresolvedClassIsNotWalked()
+	{
+		Schema schema = new();
+		schema.AddClass("A".As<ClassName>())!.AddMember("b".As<MemberName>())!.SetType(ObjectOf("Missing"));
+
+		Assert.IsFalse(schema.Validate().Any(i => i.Message.Contains("holds itself by value", StringComparison.Ordinal)));
+	}
+
+	private static Models.Types.Object ObjectOf(string className) => new() { ClassName = className.As<ClassName>() };
+
+	private static Collection<SchemaValidationIssue> Errors(Schema schema) =>
+		[.. schema.Validate().Where(i => i.Severity == SchemaValidationSeverity.Error)];
+}