Improve FormattedMessage exception handling (#5170)

* Improve FormattedMessage exception handling

* comments

RELEASE-NOTES.md

@@ -39,7 +39,7 @@ END TEMPLATE-->
 
 ### New features
 
-*None yet*
+* Added several new `FormattedMessage` methods for better exception tolerance when parsing markup. Several existing methods have been marked as obsolete, with new renamed methods taking their place.
 
 ### Bugfixes
 

Robust.Shared/Utility/FormattedMessage.MarkupParser.cs

@@ -1,4 +1,6 @@
+using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using Pidgin;
 using Robust.Shared.Maths;
 using static Pidgin.Parser;
@@ -15,29 +17,51 @@ public sealed partial class FormattedMessage
     /// <returns>true if the markup is valid</returns>
     public static bool ValidMarkup(string markup)
     {
-        return ParseResult(markup).Success;
+        return TryParse(markup, out _, out _);
     }
 
+    /// <summary>
+    /// Attempts to add markup. If an error occurs, it will do nothing and return an error message.
+    /// This method does NOT fall back to using the permissive parser (which parses invalid markup as text).
+    /// </summary>
+    public bool TryAddMarkup(string markup, [NotNullWhen(false)] out string? error)
+    {
+        if (!TryParse(markup, out var nodes, out error))
+            return false;
+
+        _nodes.AddRange(nodes);
+        return true;
+    }
+
+    [Obsolete("Use AddMarkupOrThrow or TryAddMarkup")]
+    public void AddMarkup(string markup) => AddMarkupOrThrow(markup);
+
     /// <summary>
     /// Parses the given markup and adds the resulting nodes to this formatted message
     /// </summary>
     /// <param name="markup">The markup to parse</param>
-    public void AddMarkup(string markup)
+    /// <exception cref="ParseException">Thrown when an error occurs while trying to parse the markup.</exception>
+    public void AddMarkupOrThrow(string markup)
     {
-        _nodes.AddRange(Parse(markup));
+        _nodes.AddRange(ParseOrThrow(markup));
     }
 
     /// <summary>
-    ///     Same as <see cref="AddMarkup"/> but will parse invalid markup tags as text.
+    ///     Same as <see cref="AddMarkup"/> but will attempt to parse invalid markup tags as text.
     /// </summary>
-    public void AddMarkupPermissive(string markup)
+    /// <exception cref="ParseException">Thrown when an error occurs even when using the permissive parser.</exception>
+    public void AddMarkupPermissive(string markup, out string? error)
     {
-        _nodes.AddRange(ParseSafe(markup));
+        _nodes.AddRange(ParsePermissive(markup, out error));
     }
 
+    /// <inheritdoc cref="AddMarkupPermissive(string,out string?)"/>
+    public void AddMarkupPermissive(string markup) => AddMarkupPermissive(markup, out _);
+
     /// <summary>
     /// Same as <see cref="AddMarkup"/> but adds a newline too.
     /// </summary>
+    [Obsolete]
     public void PushMarkup(string markup)
     {
         AddMarkup(markup);
@@ -52,13 +76,37 @@ public sealed partial class FormattedMessage
     /// This parser doesn't use backtracking by chaining pidgins parsers in such a way that branches that don't apply
     /// always fail on the first character
     /// </summary>
-    private static IEnumerable<MarkupNode> Parse(string input) => ParseNodes.ParseOrThrow(input);
+    private static List<MarkupNode> ParseOrThrow(string input) => ParseNodes.ParseOrThrow(input);
 
     /// <summary>
-    /// Same as <see cref="Parse"/> but uses backtracking once to ensure invalid markup just gets parsed as text
+    /// Attempt to parse the given input. Returns an error message if it fails. Does not fall back to the permissive parser
     /// </summary>
-    private static IEnumerable<MarkupNode> ParseSafe(string input) => ParseNodesSafe.ParseOrThrow(input);
-    private static Result<char, List<MarkupNode>> ParseResult(string input) => ParseNodes.Parse(input);
+    public static bool TryParse(string input, [NotNullWhen(true)] out List<MarkupNode>? nodes, [NotNullWhen(false)] out string? error)
+    {
+        var result = ParseNodes.Parse(input);
+        if (result.Success)
+        {
+            nodes = result.Value;
+            error = null;
+            return true;
+        }
+
+        error = result.Error!.RenderErrorMessage();
+        nodes = null;
+        return false;
+    }
+
+    /// <summary>
+    /// Variant of <see cref="TryParse"/> that falls back to using the permissive parser if an error occurs.
+    /// </summary>
+    /// <exception cref="ParseException">Thrown when an error occurs even when using the permissive parser.</exception>
+    public static List<MarkupNode> ParsePermissive(string input, out string? error)
+    {
+        if (TryParse(input, out var nodes, out error))
+            return nodes;
+
+        return ParseNodesSafe.ParseOrThrow(input);
+    }
 
     //TODO: Make Begin and End a cvar
     // Parser definitions for reserved characters

Robust.Shared/Utility/FormattedMessage.cs

@@ -1,8 +1,10 @@
 using System;
 using System.Collections;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Text;
 using JetBrains.Annotations;
+using Nett.Parser;
 using Robust.Shared.Maths;
 using Robust.Shared.Serialization;
 
@@ -49,30 +51,72 @@ public sealed partial class FormattedMessage
     /// <param name="toCopy">The message to copy.</param>
     public FormattedMessage(FormattedMessage toCopy)
     {
-        _nodes = Extensions.ShallowClone<MarkupNode>(toCopy._nodes);
+        _nodes = toCopy._nodes.ShallowClone();
     }
 
-    public static FormattedMessage FromMarkup(string markup)
+    private FormattedMessage(List<MarkupNode> nodes)
+    {
+        _nodes = nodes;
+    }
+
+    /// <summary>
+    /// Attempt to create a new formatted message from some markup text. Returns an error if it fails.
+    /// </summary>
+    public static bool TryFromMarkup(string markup, [NotNullWhen(true)] out FormattedMessage? msg, [NotNullWhen(false)] out string? error)
+    {
+        if (!TryParse(markup, out var nodes, out error))
+        {
+            msg = null;
+            return false;
+        }
+
+        msg = new FormattedMessage(nodes);
+        return true;
+    }
+
+    /// <summary>
+    /// Attempt to create a new formatted message from some markup text.
+    /// </summary>
+    public static bool TryFromMarkup(string markup, [NotNullWhen(true)] out FormattedMessage? msg)
+        => TryFromMarkup(markup, out msg, out _);
+
+    /// <summary>
+    /// Attempt to create a new formatted message from some markup text. Throws if the markup is invalid.
+    /// </summary>
+    /// <exception cref="ParseException">Thrown when an error occurs while trying to parse the markup.</exception>
+    public static FormattedMessage FromMarkupOrThrow(string markup)
     {
         var msg = new FormattedMessage();
-        msg.AddMarkup(markup);
+        msg.AddMarkupOrThrow(markup);
         return msg;
     }
 
-    public static FormattedMessage FromUnformatted(string markup)
+    [Obsolete("Use FromMarkupOrThrow or TryFromMarkup")]
+    public static FormattedMessage FromMarkup(string markup) => FromMarkupOrThrow(markup);
+
+    public static FormattedMessage FromUnformatted(string text)
     {
         var msg = new FormattedMessage();
-        msg.AddText(markup);
+        msg.AddText(text);
         return msg;
     }
 
-    public static FormattedMessage FromMarkupPermissive(string markup)
+    /// <summary>
+    /// Variant of <see cref="TryFromMarkup(string,out Robust.Shared.Utility.FormattedMessage?,out string?)"/> that
+    /// attempts to fall back to using the permissive parser that interprets invalid markup tags as normal text.
+    /// This may still throw if the permissive parser fails.
+    /// </summary>
+    /// <exception cref="ParseException">Thrown when an error occurs while trying to parse the markup.</exception>
+    public static FormattedMessage FromMarkupPermissive(string markup, out string? error)
     {
         var msg = new FormattedMessage();
-        msg.AddMarkupPermissive(markup);
+        msg.AddMarkupPermissive(markup, out error);
         return msg;
     }
 
+    /// <inheritdoc cref="FromMarkupPermissive(string,out string?)"/>
+    public static FormattedMessage FromMarkupPermissive(string markup) => FromMarkupPermissive(markup, out _);
+
     /// <summary>
     ///     Escape a string of text to be able to be formatted into markup.
     /// </summary>
@@ -82,13 +126,28 @@ public sealed partial class FormattedMessage
     }
 
     /// <summary>
-    ///     Remove all markup, leaving only the basic text content behind.
+    ///     Remove all markup, leaving only the basic text content behind. Throws if it fails to parse the markup tags.
     /// </summary>
-    public static string RemoveMarkup(string text)
+    /// <exception cref="ParseException">Thrown when an error occurs while trying to parse the markup.</exception>
+    public static string RemoveMarkupOrThrow(string markup)
     {
-        return FromMarkup(text).ToString();
+        return FromMarkupOrThrow(markup).ToString();
     }
 
+    /// <summary>
+    /// Attempts to remove all valid markup tags, leaving only the basic text content behind.
+    /// If this markup contains invalid tags that cannot be parsed, they will not be removed and will instead be trated
+    /// as normal text. Hence the output should probably only be parsed using try-parse the permissive parser.
+    /// </summary>
+    /// <exception cref="ParseException">Thrown when an error occurs while trying to fall back to the permissive parser.</exception>
+    public static string RemoveMarkupPermissive(string markup)
+    {
+        return FromMarkupPermissive(markup).ToString();
+    }
+
+    [Obsolete("Use RemoveMarkupOrThrow or RemoveMarkupPermissive")]
+    public static string RemoveMarkup(string markup) => RemoveMarkupOrThrow(markup);
+
     /// <summary>
     /// Adds a text node.
     /// This node doesn't need to be closed with <see cref="Pop"/>.