Make all interpolated string handlers pass by ref (#57536)

We currently pass all of our interpolated string handlers to their destination methods by ref, with the exception of those for Debug.  I think it makes sense to centralize on the pattern of always passing by ref: it doesn’t negatively impact the user experience, these are typically larger structs and so by ref helps (though it doesn’t matter for Debug), making it a ref makes it even less attractive to try to call directly / makes it look even more special, and most importantly, it gives us the opportunity to more safely clean up at the end of the operation.  This changes the Debug.Assert/Write{Line}If overloads we added in Preview 7 to pass the handler’s by ref.

This also then updates the Debug.Write{Line}If methods to create the StringBuilder used via StringBuilderCache, just in case these are used in a scenario where true is frequently passed.

diff --git a/src/libraries/System.Diagnostics.Debug/tests/DebugTestsNoListeners.Interpolation.cs b/src/libraries/System.Diagnostics.Debug/tests/DebugTestsNoListeners.Interpolation.cs
index cd05cdfdc8bd139ef50325e08c49adfa5983fc76..d36acad1f825afac6265651a2eba7349e3c42650 100644 (file)
--- a/src/libraries/System.Diagnostics.Debug/tests/DebugTestsNoListeners.Interpolation.cs
+++ b/src/libraries/System.Diagnostics.Debug/tests/DebugTestsNoListeners.Interpolation.cs
@@ -16,21 +16,30 @@ namespace System.Diagnostics.Tests
             bool shouldAppend;
 
             message = new Debug.AssertInterpolatedStringHandler(0, 0, true, out shouldAppend);
-            VerifyLogged(() => Debug.Assert(true, message), "");
+            VerifyLogged(() => Debug.Assert(true, ref message), "");
 
             message = new Debug.AssertInterpolatedStringHandler(0, 0, true, out shouldAppend);
             detailedMessage = new Debug.AssertInterpolatedStringHandler(0, 0, true, out shouldAppend);
-            VerifyLogged(() => Debug.Assert(true, message, detailedMessage), "");
+            VerifyLogged(() => Debug.Assert(true, ref message, ref detailedMessage), "");
 
             message = new Debug.AssertInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            message.AppendLiteral("assert passed");
-            VerifyAssert(() => Debug.Assert(false, message), "assert passed");
+            message.AppendLiteral("uh oh");
+            VerifyAssert(() => Debug.Assert(false, ref message), "uh oh");
 
             message = new Debug.AssertInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            message.AppendLiteral("assert passed");
+            message.AppendLiteral("uh oh");
             detailedMessage = new Debug.AssertInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            detailedMessage.AppendLiteral("nothing is wrong");
-            VerifyAssert(() => Debug.Assert(false, message, detailedMessage), "assert passed", "nothing is wrong");
+            detailedMessage.AppendLiteral("something went wrong");
+            VerifyAssert(() => Debug.Assert(false, ref message, ref detailedMessage), "uh oh", "something went wrong");
+        }
+
+        [Fact]
+        public void Asserts_Interpolation_Syntax()
+        {
+            VerifyLogged(() => Debug.Assert(true, $"you won't see this {EmptyToString.Instance}"), "");
+            VerifyLogged(() => Debug.Assert(true, $"you won't see this {EmptyToString.Instance}", $"you won't see this {EmptyToString.Instance}"), "");
+            VerifyAssert(() => Debug.Assert(false, $"uh oh{EmptyToString.Instance}"), "uh oh");
+            VerifyAssert(() => Debug.Assert(false, $"uh oh{EmptyToString.Instance}", $"something went wrong{EmptyToString.Instance}"), "uh oh", "something went wrong");
         }
 
         [Fact]
@@ -41,21 +50,31 @@ namespace System.Diagnostics.Tests
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, true, out shouldAppend);
             handler.AppendLiteral("logged");
-            VerifyLogged(() => Debug.WriteIf(true, handler), "logged");
+            VerifyLogged(() => Debug.WriteIf(true, ref handler), "logged");
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            VerifyLogged(() => Debug.WriteIf(false, handler), "");
+            VerifyLogged(() => Debug.WriteIf(false, ref handler), "");
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, true, out shouldAppend);
             handler.AppendLiteral("logged");
-            VerifyLogged(() => Debug.WriteIf(true, handler, "category"), "category: logged");
+            VerifyLogged(() => Debug.WriteIf(true, ref handler, "category"), "category: logged");
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            VerifyLogged(() => Debug.WriteIf(false, handler, "category"), "");
+            VerifyLogged(() => Debug.WriteIf(false, ref handler, "category"), "");
 
             GoToNextLine();
         }
 
+        [Fact]
+        public void WriteIf_Interpolation_Syntax()
+        {
+            VerifyLogged(() => Debug.WriteIf(true, $"{EmptyToString.Instance}logged"), "logged");
+            VerifyLogged(() => Debug.WriteIf(false, $"{EmptyToString.Instance}logged"), "");
+            VerifyLogged(() => Debug.WriteIf(true, $"{EmptyToString.Instance}logged", "category"), "category: logged");
+            VerifyLogged(() => Debug.WriteIf(false, $"{EmptyToString.Instance}logged", "category"), "");
+            GoToNextLine();
+        }
+
         [Fact]
         public void WriteLineIf_Interpolation()
         {
@@ -64,17 +83,27 @@ namespace System.Diagnostics.Tests
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, true, out shouldAppend);
             handler.AppendLiteral("logged");
-            VerifyLogged(() => Debug.WriteLineIf(true, handler), "logged" + Environment.NewLine);
+            VerifyLogged(() => Debug.WriteLineIf(true, ref handler), "logged" + Environment.NewLine);
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            VerifyLogged(() => Debug.WriteLineIf(false, handler), "");
+            VerifyLogged(() => Debug.WriteLineIf(false, ref handler), "");
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, true, out shouldAppend);
             handler.AppendLiteral("logged");
-            VerifyLogged(() => Debug.WriteLineIf(true, handler, "category"), "category: logged" + Environment.NewLine);
+            VerifyLogged(() => Debug.WriteLineIf(true, ref handler, "category"), "category: logged" + Environment.NewLine);
 
             handler = new Debug.WriteIfInterpolatedStringHandler(0, 0, false, out shouldAppend);
-            VerifyLogged(() => Debug.WriteLineIf(false, handler, "category"), "");
+            VerifyLogged(() => Debug.WriteLineIf(false, ref handler, "category"), "");
+        }
+
+        [Fact]
+        public void WriteLineIf_Interpolation_Syntax()
+        {
+            VerifyLogged(() => Debug.WriteLineIf(true, $"{EmptyToString.Instance}logged"), "logged" + Environment.NewLine);
+            VerifyLogged(() => Debug.WriteLineIf(false, $"{EmptyToString.Instance}logged"), "");
+            VerifyLogged(() => Debug.WriteLineIf(true, $"{EmptyToString.Instance}logged", "category"), "category: logged" + Environment.NewLine);
+            VerifyLogged(() => Debug.WriteLineIf(false, $"{EmptyToString.Instance}logged", "category"), "");
+            GoToNextLine();
         }
 
         [Theory]
@@ -131,7 +160,7 @@ namespace System.Diagnostics.Tests
             actual.AppendFormatted((object)DayOfWeek.Monday, 42, null);
             expected.AppendFormatted((object)DayOfWeek.Monday, 42, null);
 
-            VerifyAssert(() => Debug.Assert(false, actual), sb.ToString());
+            VerifyAssert(() => Debug.Assert(false, ref actual), sb.ToString());
         }
 
         [Fact]
@@ -174,7 +203,13 @@ namespace System.Diagnostics.Tests
             actual.AppendFormatted((object)DayOfWeek.Monday, 42, null);
             expected.AppendFormatted((object)DayOfWeek.Monday, 42, null);
 
-            VerifyLogged(() => Debug.WriteIf(true, actual), sb.ToString());
+            VerifyLogged(() => Debug.WriteIf(true, ref actual), sb.ToString());
+        }
+
+        private sealed class EmptyToString
+        {
+            public static EmptyToString Instance { get; } = new EmptyToString();
+            public override string ToString() => "";
         }
     }
 }

diff --git a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs
index 317e319e3f90a53f011914dce9ec1b4af03ebfd0..337f1491ae1d4e721f495bd68ed323f6fd3258d2 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Diagnostics/Debug.cs
@@ -87,8 +87,8 @@ namespace System.Diagnostics
             Assert(condition, message, string.Empty);
 
         [Conditional("DEBUG")]
-        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message) =>
-            Assert(condition, message.ToString());
+        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] ref AssertInterpolatedStringHandler message) =>
+            Assert(condition, message.ToStringAndClear());
 
         [Conditional("DEBUG")]
         public static void Assert([DoesNotReturnIf(false)] bool condition, string? message, string? detailMessage)
@@ -100,8 +100,8 @@ namespace System.Diagnostics
         }
 
         [Conditional("DEBUG")]
-        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler detailMessage) =>
-            Assert(condition, message.ToString(), detailMessage.ToString());
+        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] ref AssertInterpolatedStringHandler message, [InterpolatedStringHandlerArgument("condition")] ref AssertInterpolatedStringHandler detailMessage) =>
+            Assert(condition, message.ToStringAndClear(), detailMessage.ToStringAndClear());
 
         [Conditional("DEBUG")]
         public static void Assert([DoesNotReturnIf(false)] bool condition, string? message, string detailMessageFormat, params object?[] args) =>
@@ -197,8 +197,8 @@ namespace System.Diagnostics
         }
 
         [Conditional("DEBUG")]
-        public static void WriteIf(bool condition, [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message) =>
-            WriteIf(condition, message.ToString());
+        public static void WriteIf(bool condition, [InterpolatedStringHandlerArgument("condition")] ref WriteIfInterpolatedStringHandler message) =>
+            WriteIf(condition, message.ToStringAndClear());
 
         [Conditional("DEBUG")]
         public static void WriteIf(bool condition, object? value)
@@ -219,8 +219,8 @@ namespace System.Diagnostics
         }
 
         [Conditional("DEBUG")]
-        public static void WriteIf(bool condition, [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message, string? category) =>
-            WriteIf(condition, message.ToString(), category);
+        public static void WriteIf(bool condition, [InterpolatedStringHandlerArgument("condition")] ref WriteIfInterpolatedStringHandler message, string? category) =>
+            WriteIf(condition, message.ToStringAndClear(), category);
 
         [Conditional("DEBUG")]
         public static void WriteIf(bool condition, object? value, string? category)
@@ -259,8 +259,8 @@ namespace System.Diagnostics
         }
 
         [Conditional("DEBUG")]
-        public static void WriteLineIf(bool condition, [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message) =>
-            WriteLineIf(condition, message.ToString());
+        public static void WriteLineIf(bool condition, [InterpolatedStringHandlerArgument("condition")] ref WriteIfInterpolatedStringHandler message) =>
+            WriteLineIf(condition, message.ToStringAndClear());
 
         [Conditional("DEBUG")]
         public static void WriteLineIf(bool condition, string? message, string? category)
@@ -272,8 +272,8 @@ namespace System.Diagnostics
         }
 
         [Conditional("DEBUG")]
-        public static void WriteLineIf(bool condition, [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message, string? category) =>
-            WriteLineIf(condition, message.ToString(), category);
+        public static void WriteLineIf(bool condition, [InterpolatedStringHandlerArgument("condition")] ref WriteIfInterpolatedStringHandler message, string? category) =>
+            WriteLineIf(condition, message.ToStringAndClear(), category);
 
         /// <summary>Provides an interpolated string handler for <see cref="Debug.Assert"/> that only performs formatting if the assert fails.</summary>
         [EditorBrowsable(EditorBrowsableState.Never)]
@@ -298,16 +298,21 @@ namespace System.Diagnostics
                 }
                 else
                 {
-                    _stringBuilderHandler = new StringBuilder.AppendInterpolatedStringHandler(literalLength, formattedCount, new StringBuilder(DefaultInterpolatedStringHandler.GetDefaultLength(literalLength, formattedCount)));
+                    // Only used when failing an assert.  Additional allocation here doesn't matter; just create a new StringBuilder.
+                    _stringBuilderHandler = new StringBuilder.AppendInterpolatedStringHandler(literalLength, formattedCount, new StringBuilder());
                     shouldAppend = true;
                 }
             }
 
             /// <summary>Extracts the built string from the handler.</summary>
-            internal new string ToString() =>
-                _stringBuilderHandler._stringBuilder is StringBuilder sb ?
+            internal string ToStringAndClear()
+            {
+                string s = _stringBuilderHandler._stringBuilder is StringBuilder sb ?
                     sb.ToString() :
                     string.Empty;
+                _stringBuilderHandler = default;
+                return s;
+            }
 
             /// <summary>Writes the specified string to the handler.</summary>
             /// <param name="value">The string to write.</param>
@@ -378,7 +383,9 @@ namespace System.Diagnostics
             {
                 if (condition)
                 {
-                    _stringBuilderHandler = new StringBuilder.AppendInterpolatedStringHandler(literalLength, formattedCount, new StringBuilder(DefaultInterpolatedStringHandler.GetDefaultLength(literalLength, formattedCount)));
+                    // Only used in debug, but could be used on non-failure code paths, so use a cached builder.
+                    _stringBuilderHandler = new StringBuilder.AppendInterpolatedStringHandler(literalLength, formattedCount,
+                        StringBuilderCache.Acquire(DefaultInterpolatedStringHandler.GetDefaultLength(literalLength, formattedCount)));
                     shouldAppend = true;
                 }
                 else
@@ -389,10 +396,14 @@ namespace System.Diagnostics
             }
 
             /// <summary>Extracts the built string from the handler.</summary>
-            internal new string ToString() =>
-                _stringBuilderHandler._stringBuilder is StringBuilder sb ?
-                    sb.ToString() :
+            internal string ToStringAndClear()
+            {
+                string s = _stringBuilderHandler._stringBuilder is StringBuilder sb ?
+                    StringBuilderCache.GetStringAndRelease(sb) :
                     string.Empty;
+                _stringBuilderHandler = default;
+                return s;
+            }
 
             /// <summary>Writes the specified string to the handler.</summary>
             /// <param name="value">The string to write.</param>

diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs
index a8ec06e1dfa738108488a0b145128d27a710502d..5de2f8d1c1605dd234863e2dac1254b7a6e494f2 100644 (file)
--- a/src/libraries/System.Runtime/ref/System.Runtime.cs
+++ b/src/libraries/System.Runtime/ref/System.Runtime.cs
@@ -8674,11 +8674,11 @@ namespace System.Diagnostics
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, string? message) { }
         [System.Diagnostics.Conditional("DEBUG")]
-        public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.AssertInterpolatedStringHandler message) { }
+        public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.AssertInterpolatedStringHandler message) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, string? message, string? detailMessage) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
-        public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.AssertInterpolatedStringHandler message, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.AssertInterpolatedStringHandler detailMessage) { }
+        public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.AssertInterpolatedStringHandler message, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.AssertInterpolatedStringHandler detailMessage) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void Assert([System.Diagnostics.CodeAnalysis.DoesNotReturnIfAttribute(false)] bool condition, string? message, string detailMessageFormat, params object?[] args) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
@@ -8714,11 +8714,11 @@ namespace System.Diagnostics
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void WriteIf(bool condition, string? message) { }
         [System.Diagnostics.Conditional("DEBUG")]
-        public static void WriteIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message) { }
+        public static void WriteIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void WriteIf(bool condition, string? message, string? category) { }
         [System.Diagnostics.Conditional("DEBUG")]
-        public static void WriteIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message, string? category) { }
+        public static void WriteIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message, string? category) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void WriteLine(object? value) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
@@ -8736,11 +8736,11 @@ namespace System.Diagnostics
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void WriteLineIf(bool condition, string? message) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
-        public static void WriteLineIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message) { }
+        public static void WriteLineIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
         public static void WriteLineIf(bool condition, string? message, string? category) { }
         [System.Diagnostics.ConditionalAttribute("DEBUG")]
-        public static void WriteLineIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message, string? category) { }
+        public static void WriteLineIf(bool condition, [System.Runtime.CompilerServices.InterpolatedStringHandlerArgumentAttribute("condition")] ref System.Diagnostics.Debug.WriteIfInterpolatedStringHandler message, string? category) { }
         [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)]
         [System.Runtime.CompilerServices.InterpolatedStringHandlerAttribute]
         public struct AssertInterpolatedStringHandler