From: Stephen Toub Date: Tue, 17 Aug 2021 18:17:14 +0000 (-0400) Subject: Make all interpolated string handlers pass by ref (#57536) X-Git-Tag: accepted/tizen/unified/20220110.054933~315 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=80c7acc9dbfb1ad6021ca0ee2a3ca5adb46bf467;p=platform%2Fupstream%2Fdotnet%2Fruntime.git 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 cd05cdf..d36acad 100644 --- 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,22 +50,32 @@ 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() { Debug.WriteIfInterpolatedStringHandler handler; @@ -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 317e319..337f149 100644 --- 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); /// Provides an interpolated string handler for that only performs formatting if the assert fails. [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; } } /// Extracts the built string from the handler. - 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; + } /// Writes the specified string to the handler. /// The string to write. @@ -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 } /// Extracts the built string from the handler. - 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; + } /// Writes the specified string to the handler. /// The string to write. diff --git a/src/libraries/System.Runtime/ref/System.Runtime.cs b/src/libraries/System.Runtime/ref/System.Runtime.cs index a8ec06e..5de2f8d 100644 --- 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