IPrototypeManager TryIndex changes

This effectively gracefully reverts 94f98073b0.

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

@@ -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
 

Robust.Shared/Prototypes/IPrototypeManager.cs

@@ -143,21 +143,269 @@ public interface IPrototypeManager
     /// </summary>
     FrozenDictionary<string, T> GetInstances<T>() where T : IPrototype;
 
-    /// <inheritdoc cref="TryIndex{T}(ProtoId{T}, out T, bool)"/>
-    bool TryIndex([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype,  bool logError = true);
+    // For obsolete APIs.
+    // ReSharper disable MethodOverloadWithOptionalParameter
 
     /// <summary>
-    /// 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 <see cref="EntityPrototype"/> by ID, logging an error if it does not exist.
     /// </summary>
-    bool TryIndex<T>([ForbidLiteral] ProtoId<T> id, [NotNullWhen(true)] out T? prototype,  bool logError = true) where T : class, IPrototype;
+    /// <remarks>
+    /// <para>
+    /// This method does not throw if the prototype does not exist, and instead simply logs an error and returns false.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// </para>
+    /// <para>
+    /// This method should not be used when handling IDs that are expected to be invalid, such as user input.
+    /// Use <see cref="TryIndex(EntProtoId,out EntityPrototype?)"/> instead for those.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    /// <seealso cref="TryIndex(EntProtoId,out EntityPrototype?)"/>
+    bool Resolve([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype);
 
-    /// <inheritdoc cref="TryIndex{T}(ProtoId{T}, out T, bool)"/>
-    bool TryIndex([ForbidLiteral] EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype,  bool logError = true);
+    /// <summary>
+    /// Retrieve an <see cref="EntityPrototype"/> by ID, optionally logging an error if it does not exist.
+    /// </summary>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <param name="logError">If true (default), log an error if the prototype does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    [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);
 
-    /// <inheritdoc cref="TryIndex{T}(ProtoId{T}, out T, bool)"/>
+    /// <summary>
+    /// Resolve an <see cref="EntityPrototype"/> by ID.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// This method does not throw if the prototype does not exist, and instead simply returns false.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// <see cref="Resolve(EntProtoId,out EntityPrototype?)"/> can be used as a convenient helper in this scenario.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    /// <seealso cref="Resolve(EntProtoId,out EntityPrototype?)"/>
+    bool TryIndex([ForbidLiteral] EntProtoId id, [NotNullWhen(true)] out EntityPrototype? prototype);
+
+    /// <summary>
+    /// Resolve a prototype by ID, logging an error if it does not exist.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// This method does not throw if the prototype does not exist, and instead simply logs an error and returns false.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// </para>
+    /// <para>
+    /// This method should not be used when handling IDs that are expected to be invalid, such as user input.
+    /// Use <see cref="TryIndex{T}(ProtoId{T},out T?)"/> instead for those.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    /// <seealso cref="TryIndex{T}(ProtoId{T},out T?)"/>
+    bool Resolve<T>([ForbidLiteral] ProtoId<T> id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype;
+
+    /// <summary>
+    /// Retrieve a prototype by ID, optionally logging an error if it does not exist.
+    /// </summary>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <param name="logError">If true (default), log an error if the prototype does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")]
+    bool TryIndex<T>([ForbidLiteral] ProtoId<T> id, [NotNullWhen(true)] out T? prototype, bool logError = true)
+        where T : class, IPrototype;
+
+    /// <summary>
+    /// Resolve a prototype by ID.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// This method does not throw if the prototype does not exist, and instead simply returns false.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// <see cref="Resolve{T}(ProtoId{T},out T?)"/> can be used as a convenient helper in this scenario.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    /// <seealso cref="Resolve{T}(ProtoId{T},out T?)"/>
+    bool TryIndex<T>([ForbidLiteral] ProtoId<T> id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype;
+
+    /// <summary>
+    /// Resolve an <see cref="EntityPrototype"/> by ID, gracefully handling null,
+    /// and logging an error if it does not exist.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// </para>
+    /// <para>
+    /// This method should not be used when handling IDs that are expected to be invalid, such as user input.
+    /// Use <see cref="TryIndex(EntProtoId?,out EntityPrototype?)"/> instead for those.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up. May be null.</param>
+    /// <param name="prototype">
+    /// The prototype that was resolved, null if <paramref name="id"/> was null or did not exist.
+    /// </param>
+    /// <returns>True if the prototype exists, false if <paramref name="id"/> was null, or it does not exist.</returns>
+    /// <seealso cref="TryIndex(EntProtoId?,out EntityPrototype?)"/>
+    bool Resolve([ForbidLiteral] EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype);
+
+    /// <summary>
+    /// Retrieve an <see cref="EntityPrototype"/> by ID, gracefully handling null,
+    /// and optionally logging an error if it does not exist.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// No error is logged if <paramref name="id"/> is null.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <param name="logError">If true (default), log an error if the prototype does not exist.</param>
+    /// <returns>True if the prototype exists, false if <paramref name="id"/> was null, or it does not exist.</returns>
+    [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);
+
+    /// <summary>
+    /// Resolve an <see cref="EntityPrototype"/> by ID, gracefully handling null.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// This method does not throw if the prototype does not exist or is null, and instead simply returns false.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// <see cref="Resolve(EntProtoId?,out EntityPrototype?)"/> can be used as a convenient helper in this scenario.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    /// <seealso cref="Resolve(EntProtoId?,out EntityPrototype?)"/>
+    bool TryIndex([ForbidLiteral] EntProtoId? id, [NotNullWhen(true)] out EntityPrototype? prototype);
+
+    /// <summary>
+    /// Resolve a prototype by ID, gracefully handling null, and logging an error if it does not exist.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// </para>
+    /// <para>
+    /// This method should not be used when handling IDs that are expected to be invalid, such as user input.
+    /// Use <see cref="TryIndex{T}(ProtoId{T}?,out T?)"/> instead for those.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up. May be null.</param>
+    /// <param name="prototype">
+    /// The prototype that was resolved, null if <paramref name="id"/> was null or did not exist.
+    /// </param>
+    /// <returns>True if the prototype exists, false if <paramref name="id"/> was null, or it does not exist.</returns>
+    /// <seealso cref="TryIndex{T}(ProtoId{T}?,out T?)"/>
+    bool Resolve<T>([ForbidLiteral] ProtoId<T>? id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype;
+
+    /// <summary>
+    /// Retrieve a prototype by ID, gracefully handling null,
+    /// and optionally logging an error if it does not exist.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// No error is logged if <paramref name="id"/> is null.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <param name="logError">If true (default), log an error if the prototype does not exist.</param>
+    /// <returns>True if the prototype exists, false if <paramref name="id"/> was null, or it does not exist.</returns>
+    [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")]
     bool TryIndex<T>([ForbidLiteral] ProtoId<T>? id, [NotNullWhen(true)] out T? prototype, bool logError = true) where T : class, IPrototype;
 
+    /// <summary>
+    /// Resolve a prototype by ID, gracefully handling null.
+    /// </summary>
+    /// <remarks>
+    /// <para>
+    /// This method does not throw if the prototype does not exist or is null, and instead simply returns false.
+    /// </para>
+    /// <para>
+    /// 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.
+    /// </para>
+    /// <para>
+    /// 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).
+    /// <see cref="Resolve{T}(ProtoId{T}?,out T?)"/> can be used as a convenient helper in this scenario.
+    /// </para>
+    /// </remarks>
+    /// <param name="id">The prototype ID to look up.</param>
+    /// <param name="prototype">The prototype that was resolved, null if it does not exist.</param>
+    /// <returns>True if the prototype exists, false if it does not.</returns>
+    /// <seealso cref="Resolve{T}(ProtoId{T}?,out T?)"/>
+    bool TryIndex<T>([ForbidLiteral] ProtoId<T>? id, [NotNullWhen(true)] out T? prototype) where T : class, IPrototype;
+
+    // ReSharper restore MethodOverloadWithOptionalParameter
+
     bool HasMapping<T>(string id);
     bool TryGetMapping(Type kind, string id, [NotNullWhen(true)] out MappingDataNode? mappings);
 

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);

Robust.Shared/Prototypes/PrototypeManager.cs

@@ -788,42 +788,76 @@ namespace Robust.Shared.Prototypes
             return index.Instances.TryGetValue(id, out prototype);
         }
 
-        /// <inheritdoc />
+
+        // 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);
         }
 
-        /// <inheritdoc />
-        public bool TryIndex<T>(ProtoId<T> 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<T>(ProtoId<T> 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;
         }
 
-        /// <inheritdoc />
+        [Obsolete("Use Resolve() if you want to get a prototype without throwing but while still logging an error.")]
+        public bool TryIndex<T>(ProtoId<T> 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<T>(ProtoId<T> 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);
         }
 
-        /// <inheritdoc />
-        public bool TryIndex<T>(ProtoId<T>? 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<T>(ProtoId<T>? 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<T>(ProtoId<T>? 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<T>(ProtoId<T>? 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
+
         /// <inheritdoc />
         public bool HasMapping<T>(string id)
         {