From b9832c1b2d2e55b06170e67c64c05e2f0cf016fc Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Mon, 1 Feb 2021 19:37:19 +0900 Subject: [PATCH 1/8] Add ModUtils class for validating mod usages --- .../osu.Game.Tests.Android.csproj | 4 + osu.Game.Tests.iOS/osu.Game.Tests.iOS.csproj | 1 + osu.Game.Tests/Mods/ModUtilsTest.cs | 64 ++++++++++ osu.Game.Tests/osu.Game.Tests.csproj | 1 + osu.Game/Utils/ModUtils.cs | 109 ++++++++++++++++++ 5 files changed, 179 insertions(+) create mode 100644 osu.Game.Tests/Mods/ModUtilsTest.cs create mode 100644 osu.Game/Utils/ModUtils.cs diff --git a/osu.Game.Tests.Android/osu.Game.Tests.Android.csproj b/osu.Game.Tests.Android/osu.Game.Tests.Android.csproj index c44ed69c4d..19e36a63f1 100644 --- a/osu.Game.Tests.Android/osu.Game.Tests.Android.csproj +++ b/osu.Game.Tests.Android/osu.Game.Tests.Android.csproj @@ -69,5 +69,9 @@ osu.Game + + + + \ No newline at end of file diff --git a/osu.Game.Tests.iOS/osu.Game.Tests.iOS.csproj b/osu.Game.Tests.iOS/osu.Game.Tests.iOS.csproj index ca68369ebb..67b2298f4c 100644 --- a/osu.Game.Tests.iOS/osu.Game.Tests.iOS.csproj +++ b/osu.Game.Tests.iOS/osu.Game.Tests.iOS.csproj @@ -45,6 +45,7 @@ + \ No newline at end of file diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs new file mode 100644 index 0000000000..fdb441343a --- /dev/null +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -0,0 +1,64 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using Moq; +using NUnit.Framework; +using osu.Game.Rulesets.Mods; +using osu.Game.Utils; + +namespace osu.Game.Tests.Mods +{ + [TestFixture] + public class ModUtilsTest + { + [Test] + public void TestModIsCompatibleByItself() + { + var mod = new Mock(); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod.Object })); + } + + [Test] + public void TestIncompatibleThroughTopLevel() + { + var mod1 = new Mock(); + var mod2 = new Mock(); + + mod1.Setup(m => m.IncompatibleMods).Returns(new[] { mod2.Object.GetType() }); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + } + + [Test] + public void TestIncompatibleThroughMultiMod() + { + var mod1 = new Mock(); + + // The nested mod. + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType() }); + + var multiMod = new MultiMod(new MultiMod(mod2.Object)); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new[] { multiMod, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); + } + + [Test] + public void TestAllowedThroughMostDerivedType() + { + var mod = new Mock(); + Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { mod.Object.GetType() })); + } + + [Test] + public void TestNotAllowedThroughBaseType() + { + var mod = new Mock(); + Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); + } + } +} diff --git a/osu.Game.Tests/osu.Game.Tests.csproj b/osu.Game.Tests/osu.Game.Tests.csproj index c0c0578391..d29ed94b5f 100644 --- a/osu.Game.Tests/osu.Game.Tests.csproj +++ b/osu.Game.Tests/osu.Game.Tests.csproj @@ -7,6 +7,7 @@ + WinExe diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs new file mode 100644 index 0000000000..808dba2900 --- /dev/null +++ b/osu.Game/Utils/ModUtils.cs @@ -0,0 +1,109 @@ +// Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. +// See the LICENCE file in the repository root for full licence text. + +using System; +using System.Collections.Generic; +using System.Linq; +using osu.Framework.Extensions.TypeExtensions; +using osu.Game.Rulesets.Mods; + +#nullable enable + +namespace osu.Game.Utils +{ + /// + /// A set of utilities to handle combinations. + /// + public static class ModUtils + { + /// + /// Checks that all s are compatible with each-other, and that all appear within a set of allowed types. + /// + /// + /// The allowed types must contain exact types for the respective s to be allowed. + /// + /// The s to check. + /// The set of allowed types. + /// Whether all s are compatible with each-other and appear in the set of allowed types. + public static bool CheckCompatibleSetAndAllowed(IEnumerable combination, IEnumerable allowedTypes) + { + // Prevent multiple-enumeration. + var combinationList = combination as ICollection ?? combination.ToArray(); + return CheckCompatibleSet(combinationList) && CheckAllowed(combinationList, allowedTypes); + } + + /// + /// Checks that all s in a combination are compatible with each-other. + /// + /// The combination to check. + /// Whether all s in the combination are compatible with each-other. + public static bool CheckCompatibleSet(IEnumerable combination) + { + var incompatibleTypes = new HashSet(); + var incomingTypes = new HashSet(); + + foreach (var mod in combination.SelectMany(FlattenMod)) + { + // Add the new mod incompatibilities, checking whether any match the existing mod types. + foreach (var t in mod.IncompatibleMods) + { + if (incomingTypes.Contains(t)) + return false; + + incompatibleTypes.Add(t); + } + + // Add the new mod types, checking whether any match the incompatible types. + foreach (var t in mod.GetType().EnumerateBaseTypes()) + { + if (incomingTypes.Contains(t)) + return false; + + incomingTypes.Add(t); + } + } + + return true; + } + + /// + /// Checks that all s in a combination appear within a set of allowed types. + /// + /// + /// The set of allowed types must contain exact types for the respective s to be allowed. + /// + /// The combination to check. + /// The set of allowed types. + /// Whether all s in the combination are allowed. + public static bool CheckAllowed(IEnumerable combination, IEnumerable allowedTypes) + { + var allowedSet = new HashSet(allowedTypes); + + return combination.SelectMany(FlattenMod) + .All(m => allowedSet.Contains(m.GetType())); + } + + /// + /// Flattens a set of s, returning a new set with all s removed. + /// + /// The set of s to flatten. + /// The new set, containing all s in recursively with all s removed. + public static IEnumerable FlattenMods(IEnumerable mods) => mods.SelectMany(FlattenMod); + + /// + /// Flattens a , returning a set of s in-place of any s. + /// + /// The to flatten. + /// A set of singular "flattened" s + public static IEnumerable FlattenMod(Mod mod) + { + if (mod is MultiMod multi) + { + foreach (var m in multi.Mods.SelectMany(FlattenMod)) + yield return m; + } + else + yield return mod; + } + } +} From 7c29386717cc7f8c96668dc9fb4d970c7b43a17a Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:01:33 +0900 Subject: [PATCH 2/8] Add failing tests --- osu.Game.Tests/Mods/ModUtilsTest.cs | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index fdb441343a..b602c082bf 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -47,6 +47,29 @@ namespace osu.Game.Tests.Mods Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); } + [Test] + public void TestCompatibleMods() + { + var mod1 = new Mock(); + var mod2 = new Mock(); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.True); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.True); + } + + [Test] + public void TestIncompatibleThroughBaseType() + { + var mod1 = new Mock(); + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType().BaseType }); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + } + [Test] public void TestAllowedThroughMostDerivedType() { From 8232d9d2fe667a201aded46f4df1dba44b93ae7e Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:01:38 +0900 Subject: [PATCH 3/8] Fix incorrect implementation --- osu.Game/Utils/ModUtils.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 808dba2900..34bc0faca4 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -56,7 +56,7 @@ namespace osu.Game.Utils // Add the new mod types, checking whether any match the incompatible types. foreach (var t in mod.GetType().EnumerateBaseTypes()) { - if (incomingTypes.Contains(t)) + if (incompatibleTypes.Contains(t)) return false; incomingTypes.Add(t); From d0655c21c6db7e73b006a837b276d2efd0492e27 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:18:57 +0900 Subject: [PATCH 4/8] Simplify implementation of CheckCompatibleSet --- osu.Game/Utils/ModUtils.cs | 40 ++++++++++++++++++-------------------- 1 file changed, 19 insertions(+), 21 deletions(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 34bc0faca4..a9271db1b5 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -4,7 +4,6 @@ using System; using System.Collections.Generic; using System.Linq; -using osu.Framework.Extensions.TypeExtensions; using osu.Game.Rulesets.Mods; #nullable enable @@ -29,7 +28,7 @@ namespace osu.Game.Utils { // Prevent multiple-enumeration. var combinationList = combination as ICollection ?? combination.ToArray(); - return CheckCompatibleSet(combinationList) && CheckAllowed(combinationList, allowedTypes); + return CheckCompatibleSet(combinationList, out _) && CheckAllowed(combinationList, allowedTypes); } /// @@ -38,32 +37,31 @@ namespace osu.Game.Utils /// The combination to check. /// Whether all s in the combination are compatible with each-other. public static bool CheckCompatibleSet(IEnumerable combination) + => CheckCompatibleSet(combination, out _); + + /// + /// Checks that all s in a combination are compatible with each-other. + /// + /// The combination to check. + /// Any invalid mods in the set. + /// Whether all s in the combination are compatible with each-other. + public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) { - var incompatibleTypes = new HashSet(); - var incomingTypes = new HashSet(); + invalidMods = null; - foreach (var mod in combination.SelectMany(FlattenMod)) + foreach (var mod in combination) { - // Add the new mod incompatibilities, checking whether any match the existing mod types. - foreach (var t in mod.IncompatibleMods) + foreach (var type in mod.IncompatibleMods) { - if (incomingTypes.Contains(t)) - return false; - - incompatibleTypes.Add(t); - } - - // Add the new mod types, checking whether any match the incompatible types. - foreach (var t in mod.GetType().EnumerateBaseTypes()) - { - if (incompatibleTypes.Contains(t)) - return false; - - incomingTypes.Add(t); + foreach (var invalid in combination.Where(m => type.IsInstanceOfType(m))) + { + invalidMods ??= new List(); + invalidMods.Add(invalid); + } } } - return true; + return invalidMods == null; } /// From 1df412a03cde9c1ef9363fc1759f8942ea73ec94 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:31:08 +0900 Subject: [PATCH 5/8] Fix incorrect handling of multi-mod incompatibilities --- osu.Game.Tests/Mods/ModUtilsTest.cs | 25 ++++++++++++++++++++++++- osu.Game/Utils/ModUtils.cs | 1 + 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index b602c082bf..7d3dea7ed5 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -32,7 +32,7 @@ namespace osu.Game.Tests.Mods } [Test] - public void TestIncompatibleThroughMultiMod() + public void TestMultiModIncompatibleWithTopLevel() { var mod1 = new Mock(); @@ -47,6 +47,21 @@ namespace osu.Game.Tests.Mods Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); } + [Test] + public void TestTopLevelIncompatibleWithMultiMod() + { + // The nested mod. + var mod1 = new Mock(); + var multiMod = new MultiMod(new MultiMod(mod1.Object)); + + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { typeof(CustomMod1) }); + + // Test both orderings. + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { multiMod, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, multiMod }), Is.False); + } + [Test] public void TestCompatibleMods() { @@ -83,5 +98,13 @@ namespace osu.Game.Tests.Mods var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); } + + public abstract class CustomMod1 : Mod + { + } + + public abstract class CustomMod2 : Mod + { + } } } diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index a9271db1b5..41f7b1b45c 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -47,6 +47,7 @@ namespace osu.Game.Utils /// Whether all s in the combination are compatible with each-other. public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) { + combination = FlattenMods(combination); invalidMods = null; foreach (var mod in combination) From 9955e0289869be6a14cb67e762a75e9b49a599b1 Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:33:12 +0900 Subject: [PATCH 6/8] Make more tests use the custom mod classes For safety purposes... In implementing the previous tests, I found that using mod.Object.GetType() can lead to bad assertions since the same ModProxy class is used for all mocked classes. --- osu.Game.Tests/Mods/ModUtilsTest.cs | 40 ++++++++++++++--------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/osu.Game.Tests/Mods/ModUtilsTest.cs b/osu.Game.Tests/Mods/ModUtilsTest.cs index 7d3dea7ed5..e4ded602aa 100644 --- a/osu.Game.Tests/Mods/ModUtilsTest.cs +++ b/osu.Game.Tests/Mods/ModUtilsTest.cs @@ -14,37 +14,37 @@ namespace osu.Game.Tests.Mods [Test] public void TestModIsCompatibleByItself() { - var mod = new Mock(); + var mod = new Mock(); Assert.That(ModUtils.CheckCompatibleSet(new[] { mod.Object })); } [Test] public void TestIncompatibleThroughTopLevel() { - var mod1 = new Mock(); - var mod2 = new Mock(); + var mod1 = new Mock(); + var mod2 = new Mock(); mod1.Setup(m => m.IncompatibleMods).Returns(new[] { mod2.Object.GetType() }); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, mod1.Object }), Is.False); } [Test] public void TestMultiModIncompatibleWithTopLevel() { - var mod1 = new Mock(); + var mod1 = new Mock(); // The nested mod. - var mod2 = new Mock(); + var mod2 = new Mock(); mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType() }); var multiMod = new MultiMod(new MultiMod(mod2.Object)); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { multiMod, mod1.Object }), Is.False); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, multiMod }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { multiMod, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, multiMod }), Is.False); } [Test] @@ -65,37 +65,37 @@ namespace osu.Game.Tests.Mods [Test] public void TestCompatibleMods() { - var mod1 = new Mock(); - var mod2 = new Mock(); + var mod1 = new Mock(); + var mod2 = new Mock(); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.True); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.True); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, mod2.Object }), Is.True); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, mod1.Object }), Is.True); } [Test] public void TestIncompatibleThroughBaseType() { - var mod1 = new Mock(); - var mod2 = new Mock(); - mod2.Setup(m => m.IncompatibleMods).Returns(new[] { mod1.Object.GetType().BaseType }); + var mod1 = new Mock(); + var mod2 = new Mock(); + mod2.Setup(m => m.IncompatibleMods).Returns(new[] { typeof(Mod) }); // Test both orderings. - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod1.Object, mod2.Object }), Is.False); - Assert.That(ModUtils.CheckCompatibleSet(new[] { mod2.Object, mod1.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod1.Object, mod2.Object }), Is.False); + Assert.That(ModUtils.CheckCompatibleSet(new Mod[] { mod2.Object, mod1.Object }), Is.False); } [Test] public void TestAllowedThroughMostDerivedType() { - var mod = new Mock(); + var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { mod.Object.GetType() })); } [Test] public void TestNotAllowedThroughBaseType() { - var mod = new Mock(); + var mod = new Mock(); Assert.That(ModUtils.CheckAllowed(new[] { mod.Object }, new[] { typeof(Mod) }), Is.False); } From 12f52316cd2f198c0d649f41b46ef2975cfbb16d Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 18:37:11 +0900 Subject: [PATCH 7/8] Prevent multiple enumeration --- osu.Game/Utils/ModUtils.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 41f7b1b45c..9336add465 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -47,7 +47,7 @@ namespace osu.Game.Utils /// Whether all s in the combination are compatible with each-other. public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) { - combination = FlattenMods(combination); + combination = FlattenMods(combination).ToArray(); invalidMods = null; foreach (var mod in combination) From 180af3c7f8311d0a4477ecb79e7f7403da9dc85a Mon Sep 17 00:00:00 2001 From: smoogipoo Date: Tue, 2 Feb 2021 19:02:09 +0900 Subject: [PATCH 8/8] Add codeanalysis attribute --- osu.Game/Utils/ModUtils.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Utils/ModUtils.cs b/osu.Game/Utils/ModUtils.cs index 9336add465..8ac5bde65a 100644 --- a/osu.Game/Utils/ModUtils.cs +++ b/osu.Game/Utils/ModUtils.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Linq; using osu.Game.Rulesets.Mods; @@ -45,7 +46,7 @@ namespace osu.Game.Utils /// The combination to check. /// Any invalid mods in the set. /// Whether all s in the combination are compatible with each-other. - public static bool CheckCompatibleSet(IEnumerable combination, out List? invalidMods) + public static bool CheckCompatibleSet(IEnumerable combination, [NotNullWhen(false)] out List? invalidMods) { combination = FlattenMods(combination).ToArray(); invalidMods = null;