From 0ec189decee0d8b5a97e0c1e950cdd17b498bbc5 Mon Sep 17 00:00:00 2001 From: PJB3005 Date: Tue, 22 Jul 2025 00:11:59 +0200 Subject: [PATCH] IPrototypeManager TryIndex changes This effectively gracefully reverts 94f98073b07bd3fa3133ae6799b34d90f46f467e. IPrototypeManager.TryIndex now no longer logs an error. This is done by adding a new overload without the logError parameter, so most existing code switches to it. The overload with the logError parameter is now obsolete. As a replacement for defensive programming situations, the new Resolve() should be used instead. IPrototypeManager.TryIndex() should not be used for handling IDs that should always be valid, only for handling user input and similar. I also added a lot of docs. --- RELEASE-NOTES.md | 2 + Robust.Shared/Prototypes/IPrototypeManager.cs | 264 +++++++++++++++++- .../Prototypes/PrototypeManager.Categories.cs | 4 +- Robust.Shared/Prototypes/PrototypeManager.cs | 110 ++++++-- 4 files changed, 349 insertions(+), 31 deletions(-) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index aff9028bf..ae5ae44ea 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -36,10 +36,12 @@ END TEMPLATE--> ### Breaking changes * More members in `IntegrationInstance` now enforce that the instance is idle before accessing it. +* `IPrototypeManager.TryIndex` no longer logs errors unless using the overload with an optional parameter. Use `Resolve()` instead if error logging is desired. ### New features * `RobustClientPackaging.WriteClientResources()` and `RobustServerPackaging.WriteServerResources()` now have an overload taking in a set of things to ignore in the content resources directory. +* Added `IPrototypeManager.Resolve()`, which logs an error if the resolved prototype does not exist. This is effectively the previous (but not original) default behavior of `IPrototypeManager.TryIndex`. ### Bugfixes diff --git a/Robust.Shared/Prototypes/IPrototypeManager.cs b/Robust.Shared/Prototypes/IPrototypeManager.cs index 49be0fc80..6e4978fdf 100644 --- a/Robust.Shared/Prototypes/IPrototypeManager.cs +++ b/Robust.Shared/Prototypes/IPrototypeManager.cs @@ -143,21 +143,269 @@ public interface IPrototypeManager /// FrozenDictionary GetInstances() where T : IPrototype; - /// - bool TryIndex([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype, bool logError = true); + // For obsolete APIs. + // ReSharper disable MethodOverloadWithOptionalParameter /// - /// Attempt to retrieve the prototype corresponding to the given prototype id. - /// Unless otherwise specified, this will log an error if the id does not match any known prototype. + /// Resolve an by ID, logging an error if it does not exist. /// - bool TryIndex([ForbidLiteral] ProtoId id, [NotNullWhen(true)] out T? prototype, bool logError = true) where T : class, IPrototype; + /// + /// + /// This method does not throw if the prototype does not exist, and instead simply logs an error and returns false. + /// + /// + /// This method can be used if an invalid prototype ID indicates a programming error somewhere else, + /// acting as a convenient function to make code more defensive. + /// Ideally such errors should also have some other way of being validated, however (e.g. static field validation). + /// + /// + /// This method should not be used when handling IDs that are expected to be invalid, such as user input. + /// Use instead for those. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// True if the prototype exists, false if it does not. + /// + bool Resolve([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype); - /// - bool TryIndex([ForbidLiteral] EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype, bool logError = true); + /// + /// Retrieve an by ID, optionally logging an error if it does not exist. + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// If true (default), log an error if the prototype does not exist. + /// True if the prototype exists, false if it does not. + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] + bool TryIndex( + [ForbidLiteral] EntProtoId id, + [NotNullWhen(true)] out EntityPrototype? prototype, + bool logError = true); - /// + /// + /// Resolve an by ID. + /// + /// + /// + /// This method does not throw if the prototype does not exist, and instead simply returns false. + /// + /// + /// It is appropriate to use this method when handling IDs from external sources + /// (user input, old save records, etc.), where it is expected that data may be invalid. + /// + /// + /// It is not necessarily appropriate to use this method if an invalid ID indicates a programming error, + /// such as an invalid ID specified in a YAML prototype. In this scenario, + /// usage of this method should always be combined with proper error logging and other methods of validation + /// (e.g. static field validation). + /// can be used as a convenient helper in this scenario. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// True if the prototype exists, false if it does not. + /// + bool TryIndex([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype); + + /// + /// Resolve a prototype by ID, logging an error if it does not exist. + /// + /// + /// + /// This method does not throw if the prototype does not exist, and instead simply logs an error and returns false. + /// + /// + /// This method can be used if an invalid prototype ID indicates a programming error somewhere else, + /// acting as a convenient function to make code more defensive. + /// Ideally such errors should also have some other way of being validated, however (e.g. static field validation). + /// + /// + /// This method should not be used when handling IDs that are expected to be invalid, such as user input. + /// Use instead for those. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// True if the prototype exists, false if it does not. + /// + bool Resolve([ForbidLiteral] ProtoId id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype; + + /// + /// Retrieve a prototype by ID, optionally logging an error if it does not exist. + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// If true (default), log an error if the prototype does not exist. + /// True if the prototype exists, false if it does not. + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] + bool TryIndex([ForbidLiteral] ProtoId id, [NotNullWhen(true)] out T? prototype, bool logError = true) + where T : class, IPrototype; + + /// + /// Resolve a prototype by ID. + /// + /// + /// + /// This method does not throw if the prototype does not exist, and instead simply returns false. + /// + /// + /// It is appropriate to use this method when handling IDs from external sources + /// (user input, old save records, etc.), where it is expected that data may be invalid. + /// + /// + /// It is not necessarily appropriate to use this method if an invalid ID indicates a programming error, + /// such as an invalid ID specified in a YAML prototype. In this scenario, + /// usage of this method should always be combined with proper error logging and other methods of validation + /// (e.g. static field validation). + /// can be used as a convenient helper in this scenario. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// True if the prototype exists, false if it does not. + /// + bool TryIndex([ForbidLiteral] ProtoId id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype; + + /// + /// Resolve an by ID, gracefully handling null, + /// and logging an error if it does not exist. + /// + /// + /// + /// This method does not throw if the prototype is invalid, and instead simply logs an error and returns false. + /// No error is reported if the ID is simply null. + /// + /// + /// This method can be used if an invalid prototype ID indicates a programming error somewhere else, + /// acting as a convenient function to make code more defensive. + /// Ideally such errors should also have some other way of being validated, however (e.g. static field validation). + /// + /// + /// This method should not be used when handling IDs that are expected to be invalid, such as user input. + /// Use instead for those. + /// + /// + /// The prototype ID to look up. May be null. + /// + /// The prototype that was resolved, null if was null or did not exist. + /// + /// True if the prototype exists, false if was null, or it does not exist. + /// + bool Resolve([ForbidLiteral] EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype); + + /// + /// Retrieve an by ID, gracefully handling null, + /// and optionally logging an error if it does not exist. + /// + /// + /// + /// No error is logged if is null. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// If true (default), log an error if the prototype does not exist. + /// True if the prototype exists, false if was null, or it does not exist. + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] + bool TryIndex( + [ForbidLiteral] EntProtoId? id, + [NotNullWhen(true)] out EntityPrototype? prototype, + bool logError = true); + + /// + /// Resolve an by ID, gracefully handling null. + /// + /// + /// + /// This method does not throw if the prototype does not exist or is null, and instead simply returns false. + /// + /// + /// It is appropriate to use this method when handling IDs from external sources + /// (user input, old save records, etc.), where it is expected that data may be invalid. + /// + /// + /// It is not necessarily appropriate to use this method if an invalid ID indicates a programming error, + /// such as an invalid ID specified in a YAML prototype. In this scenario, + /// usage of this method should always be combined with proper error logging and other methods of validation + /// (e.g. static field validation). + /// can be used as a convenient helper in this scenario. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// True if the prototype exists, false if it does not. + /// + bool TryIndex([ForbidLiteral] EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype); + + /// + /// Resolve a prototype by ID, gracefully handling null, and logging an error if it does not exist. + /// + /// + /// + /// This method does not throw if the prototype is invalid, and instead simply logs an error and returns false. + /// No error is reported if the ID is simply null. + /// + /// + /// This method can be used if an invalid prototype ID indicates a programming error somewhere else, + /// acting as a convenient function to make code more defensive. + /// Ideally such errors should also have some other way of being validated, however (e.g. static field validation). + /// + /// + /// This method should not be used when handling IDs that are expected to be invalid, such as user input. + /// Use instead for those. + /// + /// + /// The prototype ID to look up. May be null. + /// + /// The prototype that was resolved, null if was null or did not exist. + /// + /// True if the prototype exists, false if was null, or it does not exist. + /// + bool Resolve([ForbidLiteral] ProtoId? id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype; + + /// + /// Retrieve a prototype by ID, gracefully handling null, + /// and optionally logging an error if it does not exist. + /// + /// + /// + /// No error is logged if is null. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// If true (default), log an error if the prototype does not exist. + /// True if the prototype exists, false if was null, or it does not exist. + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] bool TryIndex([ForbidLiteral] ProtoId? id, [NotNullWhen(true)] out T? prototype, bool logError = true) where T : class, IPrototype; + /// + /// Resolve a prototype by ID, gracefully handling null. + /// + /// + /// + /// This method does not throw if the prototype does not exist or is null, and instead simply returns false. + /// + /// + /// It is appropriate to use this method when handling IDs from external sources + /// (user input, old save records, etc.), where it is expected that data may be invalid. + /// + /// + /// It is not necessarily appropriate to use this method if an invalid ID indicates a programming error, + /// such as an invalid ID specified in a YAML prototype. In this scenario, + /// usage of this method should always be combined with proper error logging and other methods of validation + /// (e.g. static field validation). + /// can be used as a convenient helper in this scenario. + /// + /// + /// The prototype ID to look up. + /// The prototype that was resolved, null if it does not exist. + /// True if the prototype exists, false if it does not. + /// + bool TryIndex([ForbidLiteral] ProtoId? id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype; + + // ReSharper restore MethodOverloadWithOptionalParameter + bool HasMapping(string id); bool TryGetMapping(Type kind, string id, [NotNullWhen(true)] out MappingDataNode? mappings); diff --git a/Robust.Shared/Prototypes/PrototypeManager.Categories.cs b/Robust.Shared/Prototypes/PrototypeManager.Categories.cs index feba747cf..739a8eee3 100644 --- a/Robust.Shared/Prototypes/PrototypeManager.Categories.cs +++ b/Robust.Shared/Prototypes/PrototypeManager.Categories.cs @@ -108,7 +108,7 @@ public abstract partial class PrototypeManager : IPrototypeManagerInternal } } - DebugTools.Assert(!TryIndex(id, out var instance, false) + DebugTools.Assert(!TryIndex(id, out var instance) || instance.CategoriesInternal == null || instance.CategoriesInternal.All(x => set.Any(y => y.ID == x))); @@ -124,7 +124,7 @@ public abstract partial class PrototypeManager : IPrototypeManagerInternal } } - if (!TryIndex(id, out var protoInstance, false)) + if (!TryIndex(id, out var protoInstance)) { // Prototype is abstract cache.Add(id, set); diff --git a/Robust.Shared/Prototypes/PrototypeManager.cs b/Robust.Shared/Prototypes/PrototypeManager.cs index 76d92fb80..dfd9a0c9f 100644 --- a/Robust.Shared/Prototypes/PrototypeManager.cs +++ b/Robust.Shared/Prototypes/PrototypeManager.cs @@ -788,42 +788,76 @@ namespace Robust.Shared.Prototypes return index.Instances.TryGetValue(id, out prototype); } - /// + + // For obsolete APIs. + // ReSharper disable MethodOverloadWithOptionalParameter + + public bool Resolve(EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype) + { + if (TryIndex(id.Id, out prototype)) + return true; + + Sawmill.Error($"Attempted to resolve invalid {nameof(EntProtoId)}: {id.Id}\n{Environment.StackTrace}"); + return false; + } + + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] public bool TryIndex(EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype, bool logError = true) { - if (TryIndex(id.Id, out prototype)) - return true; - if (logError) - Sawmill.Error($"Attempted to resolve invalid {nameof(EntProtoId)}: {id.Id}"); - return false; + return Resolve(id, out prototype); + return TryIndex(id, out prototype); } - /// - public bool TryIndex(ProtoId id, [NotNullWhen(true)] out T? prototype, bool logError = true) where T : class, IPrototype + public bool TryIndex([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype) + { + return TryIndex(id.Id, out prototype); + } + + public bool Resolve(ProtoId id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype { if (TryIndex(id.Id, out prototype)) return true; - if (logError) - Sawmill.Error($"Attempted to resolve invalid ProtoId<{typeof(T).Name}>: {id.Id}"); + Sawmill.Error($"Attempted to resolve invalid ProtoId<{typeof(T).Name}>: {id.Id}\n{Environment.StackTrace}"); return false; } - /// + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] + public bool TryIndex(ProtoId id, [NotNullWhen(true)] out T? prototype, bool logError = true) + where T : class, IPrototype + { + if (logError) + return Resolve(id, out prototype); + return TryIndex(id, out prototype); + } + + public bool TryIndex(ProtoId id, [NotNullWhen(true)] out T? prototype) + where T : class, IPrototype + { + return TryIndex(id.Id, out prototype); + } + + public bool Resolve(EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype) + { + if (id == null) + { + prototype = null; + return false; + } + + return Resolve(id.Value, out prototype); + } + + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] public bool TryIndex(EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype, bool logError = true) { - if (id == null) - { - prototype = null; - return false; - } - - return TryIndex(id.Value, out prototype, logError); + if (logError) + return Resolve(id, out prototype); + return TryIndex(id, out prototype); } - /// - public bool TryIndex(ProtoId? id, [NotNullWhen(true)] out T? prototype, bool logError = true) where T : class, IPrototype + public bool TryIndex(EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype) { if (id == null) { @@ -831,9 +865,43 @@ namespace Robust.Shared.Prototypes return false; } - return TryIndex(id.Value, out prototype, logError); + return TryIndex(id.Value, out prototype); } + public bool Resolve(ProtoId? id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype + { + if (id == null) + { + prototype = null; + return false; + } + + return Resolve(id.Value, out prototype); + } + + [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")] + public bool TryIndex(ProtoId? id, [NotNullWhen(true)] out T? prototype, bool logError = true) + where T : class, IPrototype + { + if (logError) + return Resolve(id, out prototype); + return TryIndex(id, out prototype); + } + + public bool TryIndex(ProtoId? id, [NotNullWhen(true)] out T? prototype) + where T : class, IPrototype + { + if (id == null) + { + prototype = null; + return false; + } + + return TryIndex(id.Value, out prototype); + } + + // ReSharper restore MethodOverloadWithOptionalParameter + /// public bool HasMapping(string id) {