Merge pull request #9710 from obrakmann/yaml-removals

Fix yaml removals

OpenRA.Game/GameRules/ActorInfo.cs

@@ -31,35 +31,30 @@ namespace OpenRA
 		readonly TypeDictionary traits = new TypeDictionary();
 		List<ITraitInfo> constructOrderCache = null;
 
-		public ActorInfo(string name, MiniYaml node, Dictionary<string, MiniYaml> allUnits)
+		public ActorInfo(ObjectCreator creator, string name, MiniYaml node, Dictionary<string, MiniYaml> allUnits)
 		{
 			try
 			{
+				Name = name;
+
 				var allParents = new HashSet<string>();
 				var abstractActorType = name.StartsWith("^");
 
 				// Guard against circular inheritance
 				allParents.Add(name);
-				var mergedNode = MergeWithParents(node, allUnits, allParents).ToDictionary();
-
-				Name = name;
-
-				foreach (var t in mergedNode)
-				{
-					if (t.Key[0] == '-')
-						throw new YamlException("Bogus trait removal: " + t.Key);
 
+				var partial = MergeWithParents(node, allUnits, allParents);
+				foreach (var t in MiniYaml.ApplyRemovals(partial.Nodes))
 					if (t.Key != "Inherits" && !t.Key.StartsWith("Inherits@"))
 						try
 						{
-							traits.Add(LoadTraitInfo(t.Key.Split('@')[0], t.Value));
+							traits.Add(LoadTraitInfo(creator, t.Key.Split('@')[0], t.Value));
 						}
 						catch (FieldLoader.MissingFieldsException e)
 						{
 							if (!abstractActorType)
 								throw new YamlException(e.Message);
 						}
-				}
 			}
 			catch (YamlException e)
 			{
@@ -98,18 +93,18 @@ namespace OpenRA
 					throw new YamlException(
 						"Bogus inheritance -- duplicate inheritance of {0}.".F(kv.Key));
 
-				node = MiniYaml.MergeStrict(node, MergeWithParents(kv.Value, allUnits, allParents));
+				node = MiniYaml.MergePartial(node, MergeWithParents(kv.Value, allUnits, allParents));
 			}
 
 			return node;
 		}
 
-		static ITraitInfo LoadTraitInfo(string traitName, MiniYaml my)
+		static ITraitInfo LoadTraitInfo(ObjectCreator creator, string traitName, MiniYaml my)
 		{
 			if (!string.IsNullOrEmpty(my.Value))
 				throw new YamlException("Junk value `{0}` on trait node {1}"
 				.F(my.Value, traitName));
-			var info = Game.CreateObject<ITraitInfo>(traitName + "Info");
+			var info = creator.CreateObject<ITraitInfo>(traitName + "Info");
 			try
 			{
 				FieldLoader.Load(info, my);

OpenRA.Game/GameRules/RulesetCache.cs

@@ -61,7 +61,7 @@ namespace OpenRA
 			using (new PerfTimer("Actors"))
 				actors = LoadYamlRules(actorCache, m.Rules,
 					map != null ? map.RuleDefinitions : NoMapRules,
-					(k, y) => new ActorInfo(k.Key.ToLowerInvariant(), k.Value, y));
+					(k, y) => new ActorInfo(Game.ModData.ObjectCreator, k.Key.ToLowerInvariant(), k.Value, y));
 
 			using (new PerfTimer("Weapons"))
 				weapons = LoadYamlRules(weaponCache, m.Weapons,
@@ -99,9 +99,9 @@ namespace OpenRA
 
 			var inputKey = string.Concat(string.Join("|", files), "|", nodes.WriteToString());
 
-			var mergedNodes = files
+			var partial = files
 				.Select(s => MiniYaml.FromFile(s))
-				.Aggregate(nodes, MiniYaml.MergeLiberal);
+				.Aggregate(nodes, MiniYaml.MergePartial);
 
 			Func<MiniYamlNode, Dictionary<string, MiniYaml>, T> wrap = (wkv, wyy) =>
 			{
@@ -117,8 +117,8 @@ namespace OpenRA
 				return t;
 			};
 
-			var yy = mergedNodes.ToDictionary(x => x.Key, x => x.Value);
+			var yy = partial.ToDictionary(x => x.Key, x => x.Value);
-			var itemSet = mergedNodes.ToDictionaryWithConflictLog(kv => kv.Key.ToLowerInvariant(), kv => wrap(kv, yy), "LoadYamlRules", null, null);
+			var itemSet = partial.ToDictionaryWithConflictLog(kv => kv.Key.ToLowerInvariant(), kv => wrap(kv, yy), "LoadYamlRules", null, null);
 
 			RaiseProgress();
 			return itemSet;

OpenRA.Game/Graphics/ChromeProvider.cs

@@ -33,7 +33,11 @@ namespace OpenRA.Graphics
 			cachedSheets = new Dictionary<string, Sheet>();
 			cachedSprites = new Dictionary<string, Dictionary<string, Sprite>>();
 
-			var chrome = chromeFiles.Select(s => MiniYaml.FromFile(s)).Aggregate(MiniYaml.MergeLiberal);
+			var partial = chromeFiles
+				.Select(s => MiniYaml.FromFile(s))
+				.Aggregate(MiniYaml.MergePartial);
+
+			var chrome = MiniYaml.ApplyRemovals(partial);
 
 			foreach (var c in chrome)
 				LoadCollection(c.Key, c.Value);

OpenRA.Game/Graphics/CursorProvider.cs

@@ -25,7 +25,11 @@ namespace OpenRA.Graphics
 		public CursorProvider(ModData modData)
 		{
 			var sequenceFiles = modData.Manifest.Cursors;
-			var sequences = new MiniYaml(null, sequenceFiles.Select(s => MiniYaml.FromFile(s)).Aggregate(MiniYaml.MergeLiberal));
+			var partial = sequenceFiles
+				.Select(s => MiniYaml.FromFile(s))
+				.Aggregate(MiniYaml.MergePartial);
+
+			var sequences = new MiniYaml(null, MiniYaml.ApplyRemovals(partial));
 			var shadowIndex = new int[] { };
 
 			var nodesDict = sequences.ToDictionary();

OpenRA.Game/Graphics/SequenceProvider.cs

@@ -127,10 +127,11 @@ namespace OpenRA.Graphics
 		{
 			var sequenceFiles = modData.Manifest.Sequences;
 
-			var nodes = sequenceFiles
+			var partial = sequenceFiles
 				.Select(s => MiniYaml.FromFile(s))
-				.Aggregate(sequenceNodes, MiniYaml.MergeLiberal);
+				.Aggregate(sequenceNodes, MiniYaml.MergePartial);
 
+			var nodes = MiniYaml.ApplyRemovals(partial);
 			var items = new Dictionary<string, UnitSequences>();
 			foreach (var n in nodes)
 			{

OpenRA.Game/Graphics/VoxelProvider.cs

@@ -23,10 +23,11 @@ namespace OpenRA.Graphics
 		{
 			units = new Dictionary<string, Dictionary<string, Voxel>>();
 
-			var sequences = voxelFiles
+			var partial = voxelFiles
 				.Select(s => MiniYaml.FromFile(s))
-				.Aggregate(voxelNodes, MiniYaml.MergeLiberal);
+				.Aggregate(voxelNodes, MiniYaml.MergePartial);
 
+			var sequences = MiniYaml.ApplyRemovals(partial);
 			foreach (var s in sequences)
 				LoadVoxelsForUnit(s.Key, s.Value);
 

OpenRA.Game/MiniYaml.cs

@@ -262,17 +262,12 @@ namespace OpenRA
 			return FromLines(text.Split(new[] { "\r\n", "\n" }, StringSplitOptions.RemoveEmptyEntries), fileName);
 		}
 
-		public static List<MiniYamlNode> MergeStrict(List<MiniYamlNode> a, List<MiniYamlNode> b)
+		public static List<MiniYamlNode> Merge(List<MiniYamlNode> a, List<MiniYamlNode> b)
 		{
-			return Merge(a, b, false);
+			return ApplyRemovals(MergePartial(a, b));
 		}
 
-		public static List<MiniYamlNode> MergeLiberal(List<MiniYamlNode> a, List<MiniYamlNode> b)
+		public static List<MiniYamlNode> MergePartial(List<MiniYamlNode> a, List<MiniYamlNode> b)
-		{
-			return Merge(a, b, true);
-		}
-
-		static List<MiniYamlNode> Merge(List<MiniYamlNode> a, List<MiniYamlNode> b, bool allowUnresolvedRemoves = false)
 		{
 			if (a.Count == 0)
 				return b;
@@ -286,58 +281,49 @@ namespace OpenRA
 			var dictB = b.ToDictionaryWithConflictLog(x => x.Key, "MiniYaml.Merge", null, x => "{0} (at {1})".F(x.Key, x.Location));
 			var allKeys = dictA.Keys.Union(dictB.Keys);
 
-			var keys = allKeys.Where(x => x.Length == 0 || x[0] != '-').ToList();
+			foreach (var key in allKeys)
-			var removeKeys = allKeys.Where(x => x.Length > 0 && x[0] == '-')
-				.Select(k => k.Substring(1)).ToHashSet();
-
-			foreach (var key in keys)
 			{
 				MiniYamlNode aa, bb;
 				dictA.TryGetValue(key, out aa);
 				dictB.TryGetValue(key, out bb);
 
-				if (removeKeys.Contains(key))
+				var loc = aa == null ? default(MiniYamlNode.SourceLocation) : aa.Location;
-					removeKeys.Remove(key);
+				var merged = (aa == null || bb == null) ? aa ?? bb : new MiniYamlNode(key, MergePartial(aa.Value, bb.Value), loc);
-				else
+				ret.Add(merged);
-				{
-					var loc = aa == null ? default(MiniYamlNode.SourceLocation) : aa.Location;
-					var merged = (aa == null || bb == null) ? aa ?? bb : new MiniYamlNode(key, Merge(aa.Value, bb.Value, allowUnresolvedRemoves), loc);
-					ret.Add(merged);
-				}
-			}
-
-			if (removeKeys.Any())
-			{
-				if (allowUnresolvedRemoves)
-				{
-					// Add the removal nodes back for the next pass to deal with
-					foreach (var k in removeKeys)
-					{
-						var key = "-" + k;
-						MiniYamlNode rem;
-						if (!dictA.TryGetValue(key, out rem))
-							rem = dictB[key];
-						ret.Add(rem);
-					}
-				}
-				else
-					throw new YamlException("Bogus yaml removals: {0}".F(removeKeys.JoinWith(", ")));
 			}
 
 			return ret;
 		}
 
-		public static MiniYaml MergeLiberal(MiniYaml a, MiniYaml b)
+		public static List<MiniYamlNode> ApplyRemovals(List<MiniYamlNode> a)
 		{
-			return Merge(a, b, true);
+			var removeKeys = a.Select(x => x.Key)
+				.Where(x => x.Length > 0 && x[0] == '-')
+				.Select(k => k.Substring(1))
+				.ToHashSet();
+
+			var ret = new List<MiniYamlNode>();
+			foreach (var x in a)
+			{
+				if (x.Key[0] == '-')
+					continue;
+
+				if (removeKeys.Contains(x.Key))
+					removeKeys.Remove(x.Key);
+				else
+				{
+					x.Value.Nodes = ApplyRemovals(x.Value.Nodes);
+					ret.Add(x);
+				}
+			}
+
+			if (removeKeys.Any())
+				throw new YamlException("Bogus yaml removals: {0}".F(removeKeys.JoinWith(", ")));
+
+			return ret;
 		}
 
-		public static MiniYaml MergeStrict(MiniYaml a, MiniYaml b)
+		public static MiniYaml MergePartial(MiniYaml a, MiniYaml b)
-		{
-			return Merge(a, b, false);
-		}
-
-		static MiniYaml Merge(MiniYaml a, MiniYaml b, bool allowUnresolvedRemoves)
 		{
 			if (a == null)
 				return b;
@@ -345,7 +331,18 @@ namespace OpenRA
 			if (b == null)
 				return a;
 
-			return new MiniYaml(a.Value ?? b.Value, Merge(a.Nodes, b.Nodes, allowUnresolvedRemoves));
+			return new MiniYaml(a.Value ?? b.Value, MergePartial(a.Nodes, b.Nodes));
+		}
+
+		public static MiniYaml Merge(MiniYaml a, MiniYaml b)
+		{
+			if (a == null)
+				return b;
+
+			if (b == null)
+				return a;
+
+			return new MiniYaml(a.Value ?? b.Value, Merge(a.Nodes, b.Nodes));
 		}
 
 		public IEnumerable<string> ToLines(string name)

OpenRA.Game/ModData.cs

@@ -12,6 +12,7 @@ using System;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Reflection;
 using OpenRA.FileSystem;
 using OpenRA.Graphics;
 using OpenRA.Widgets;
@@ -38,7 +39,12 @@ namespace OpenRA
 		{
 			Languages = new string[0];
 			Manifest = new Manifest(mod);
-			ObjectCreator = new ObjectCreator(Manifest);
+
+			// Allow mods to load types from the core Game assembly, and any additional assemblies they specify.
+			var assemblies =
+				new[] { typeof(Game).Assembly }.Concat(
+					Manifest.Assemblies.Select(path => Assembly.LoadFrom(Platform.ResolvePath(path))));
+			ObjectCreator = new ObjectCreator(assemblies);
 			Manifest.LoadCustomData(ObjectCreator);
 
 			if (useLoadScreen)
@@ -119,11 +125,10 @@ namespace OpenRA
 				return;
 			}
 
-			var yaml = Manifest.Translations.Select(MiniYaml.FromFile).Aggregate(MiniYaml.MergeLiberal);
+			var partial = Manifest.Translations.Select(MiniYaml.FromFile).Aggregate(MiniYaml.MergePartial);
-			Languages = yaml.Select(t => t.Key).ToArray();
+			Languages = partial.Select(t => t.Key).ToArray();
-
-			yaml = MiniYaml.MergeLiberal(map.TranslationDefinitions, yaml);
 
+			var yaml = MiniYaml.Merge(map.TranslationDefinitions, partial);
 			foreach (var y in yaml)
 			{
 				if (y.Key == Game.Settings.Graphics.Language)

OpenRA.Game/ObjectCreator.cs

@@ -22,24 +22,11 @@ namespace OpenRA
 		readonly Cache<Type, ConstructorInfo> ctorCache;
 		readonly Pair<Assembly, string>[] assemblies;
 
-		public ObjectCreator(Manifest manifest)
+		public ObjectCreator(IEnumerable<Assembly> sourceAssemblies)
 		{
 			typeCache = new Cache<string, Type>(FindType);
 			ctorCache = new Cache<Type, ConstructorInfo>(GetCtor);
-
+			assemblies = sourceAssemblies.SelectMany(asm => asm.GetNamespaces().Select(ns => Pair.New(asm, ns))).ToArray();
-			// All the core namespaces
-			var asms = typeof(Game).Assembly.GetNamespaces() // Game
-				.Select(c => Pair.New(typeof(Game).Assembly, c))
-				.ToList();
-
-			// Namespaces from each mod assembly
-			foreach (var a in manifest.Assemblies)
-			{
-				var asm = Assembly.LoadFile(Platform.ResolvePath(a));
-				asms.AddRange(asm.GetNamespaces().Select(ns => Pair.New(asm, ns)));
-			}
-
-			assemblies = asms.ToArray();
 		}
 
 		public static Action<string> MissingTypeAction =

OpenRA.Game/Widgets/ChromeMetrics.cs

@@ -20,9 +20,11 @@ namespace OpenRA.Widgets
 		public static void Initialize(IEnumerable<string> yaml)
 		{
 			data = new Dictionary<string, string>();
-			var metrics = yaml.Select(y => MiniYaml.FromFile(y))
+			var partial = yaml
-				.Aggregate(MiniYaml.MergeLiberal);
+				.Select(y => MiniYaml.FromFile(y))
+				.Aggregate(MiniYaml.MergePartial);
 
+			var metrics = MiniYaml.ApplyRemovals(partial);
 			foreach (var m in metrics)
 				foreach (var n in m.Value.Nodes)
 					data[n.Key] = n.Value.Value;

OpenRA.Mods.Common/Graphics/DefaultSpriteSequence.cs

@@ -36,7 +36,7 @@ namespace OpenRA.Mods.Common.Graphics
 			if (nodes.TryGetValue("Defaults", out defaults))
 			{
 				nodes.Remove("Defaults");
-				nodes = nodes.ToDictionary(kv => kv.Key, kv => MiniYaml.MergeStrict(kv.Value, defaults));
+				nodes = nodes.ToDictionary(kv => kv.Key, kv => MiniYaml.Merge(kv.Value, defaults));
 
 				// Merge 'Defaults' animation image value. An example follows.
 				//

OpenRA.Mods.Common/Lint/CheckSequences.cs

@@ -32,8 +32,10 @@ namespace OpenRA.Mods.Common.Lint
 			this.emitError = emitError;
 
 			var sequenceSource = map != null ? map.SequenceDefinitions : new List<MiniYamlNode>();
-			sequenceDefinitions = MiniYaml.MergeLiberal(sequenceSource,
+			var partial = Game.ModData.Manifest.Sequences
-				Game.ModData.Manifest.Sequences.Select(MiniYaml.FromFile).Aggregate(MiniYaml.MergeLiberal));
+				.Select(MiniYaml.FromFile)
+				.Aggregate(MiniYaml.MergePartial);
+			sequenceDefinitions = MiniYaml.Merge(sequenceSource, partial);
 
 			var rules = map == null ? Game.ModData.DefaultRules : map.Rules;
 			var factions = rules.Actors["world"].TraitInfos<FactionInfo>().Select(f => f.InternalName).ToArray();

OpenRA.Mods.Common/UtilityCommands/CheckSequenceSprites.cs

@@ -39,10 +39,11 @@ namespace OpenRA.Mods.Common.UtilityCommands
 				var sc = new SpriteCache(modData.SpriteLoaders, new SheetBuilder(SheetType.Indexed));
 				var sequenceFiles = modData.Manifest.Sequences;
 
-				var nodes = sequenceFiles
+				var partial = sequenceFiles
 					.Select(s => MiniYaml.FromFile(s))
-					.Aggregate(MiniYaml.MergeLiberal);
+					.Aggregate(MiniYaml.MergePartial);
 
+				var nodes = MiniYaml.ApplyRemovals(partial);
 				foreach (var n in nodes)
 					Game.ModData.SpriteSequenceLoader.ParseSequences(Game.ModData, ts, sc, n);
 			}

OpenRA.Mods.Common/Widgets/Logic/MissionBrowserLogic.cs

@@ -97,8 +97,11 @@ namespace OpenRA.Mods.Common.Widgets.Logic
 			// Add a group for each campaign
 			if (Game.ModData.Manifest.Missions.Any())
 			{
-				var yaml = Game.ModData.Manifest.Missions.Select(MiniYaml.FromFile).Aggregate(MiniYaml.MergeLiberal);
+				var partial = Game.ModData.Manifest.Missions
+					.Select(MiniYaml.FromFile)
+					.Aggregate(MiniYaml.MergePartial);
 
+				var yaml = MiniYaml.ApplyRemovals(partial);
 				foreach (var kv in yaml)
 				{
 					var missionMapPaths = kv.Value.Nodes.Select(n => Path.GetFullPath(n.Key));

OpenRA.Test/OpenRA.Game/ActorInfoTest.cs

@@ -28,6 +28,10 @@ namespace OpenRA.Test
 	class MockEInfo : MockTraitInfo, Requires<MockFInfo> { }
 	class MockFInfo : MockTraitInfo, Requires<MockDInfo> { }
 
+	class MockA2Info : MockTraitInfo { }
+	class MockB2Info : MockTraitInfo { }
+	class MockC2Info : MockTraitInfo { }
+
 	[TestFixture]
 	public class ActorInfoTest
 	{
@@ -88,5 +92,65 @@ namespace OpenRA.Test
 				Assert.That(count, Is.EqualTo(Math.Floor(count)), "Should be symmetrical");
 			}
 		}
+
+		[TestCase(TestName = "Trait inheritance and removal can be composed")]
+		public void TraitInheritanceAndRemovalCanBeComposed()
+		{
+			var baseYaml = @"
+^BaseA:
+	MockA2:
+^BaseB:
+	Inherits@a: ^BaseA
+	MockB2:
+";
+			var extendedYaml = @"
+Actor:
+	Inherits@b: ^BaseB
+	-MockA2:
+";
+			var mapYaml = @"
+^BaseC:
+	MockC2:
+Actor:
+	Inherits@c: ^BaseC
+";
+
+			var actorInfo = CreateActorInfoFromYaml("Actor", mapYaml, baseYaml, extendedYaml);
+			Assert.IsFalse(actorInfo.HasTraitInfo<MockA2Info>(), "Actor should not have the MockA2 trait, but does.");
+			Assert.IsTrue(actorInfo.HasTraitInfo<MockB2Info>(), "Actor should have the MockB2 trait, but does not.");
+			Assert.IsTrue(actorInfo.HasTraitInfo<MockC2Info>(), "Actor should have the MockC2 trait, but does not.");
+		}
+
+		[TestCase(TestName = "Trait can be removed after multiple inheritance")]
+		public void TraitCanBeRemovedAfterMultipleInheritance()
+		{
+			var baseYaml = @"
+^BaseA:
+	MockA2:
+Actor:
+	Inherits: ^BaseA
+	MockA2:
+";
+			var overrideYaml = @"
+Actor:
+	-MockA2
+";
+
+			var actorInfo = CreateActorInfoFromYaml("Actor", null, baseYaml, overrideYaml);
+			Assert.IsFalse(actorInfo.HasTraitInfo<MockA2Info>(), "Actor should not have the MockA2 trait, but does.");
+		}
+
+		// This needs to match the logic used in RulesetCache.LoadYamlRules
+		ActorInfo CreateActorInfoFromYaml(string name, string mapYaml, params string[] yamls)
+		{
+			var initialNodes = mapYaml == null ? new List<MiniYamlNode>() : MiniYaml.FromString(mapYaml);
+			var yaml = yamls
+				.Select(s => MiniYaml.FromString(s))
+				.Aggregate(initialNodes, MiniYaml.MergePartial);
+			var allUnits = yaml.ToDictionary(node => node.Key, node => node.Value);
+			var unit = allUnits[name];
+			var creator = new ObjectCreator(new[] { typeof(ActorInfoTest).Assembly });
+			return new ActorInfo(creator, name, unit, allUnits);
+		}
 	}
 }

OpenRA.Test/OpenRA.Game/MiniYamlTest.cs

@@ -73,9 +73,9 @@ Root2:
 
 			// Merge order should not matter
 			// Note: All the Merge* variants are different plumbing over the same
-			// internal logic.  Testing only MergeStrict is sufficient.
+			// Internal logic.  Testing only Merge is sufficient.
-			TestMixedMerge(MiniYaml.MergeStrict(a, b).First().Value);
+			TestMixedMerge(MiniYaml.Merge(a, b).First().Value);
-			TestMixedMerge(MiniYaml.MergeStrict(b, a).First().Value);
+			TestMixedMerge(MiniYaml.Merge(b, a).First().Value);
 		}
 
 		void TestMixedMerge(MiniYaml result)