Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API Proposal: Debug.Assert overloads with interpolated string handler #53211

Closed
stephentoub opened this issue May 25, 2021 · 13 comments
Closed
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics feature-request
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented May 25, 2021

Based on #52894 (comment)...

Background and Motivation

Debug.Assert has three overloads today:

[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition);
 
[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition, string? message);
 
[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition, string? message, string? detailMessage);

We're generally not concerned with the performance of asserts; after all, they're debug-only code. However, heavy use of asserts, or asserts where the developer wants to log significant details about the failure, can be non-trivially expensive and slow down debug execution to the point where it's impactful. For such cases, a developer can rewrite their assert:

Debug.Assert(condition, $"Details: {GetDetails()}");

to instead be:

if (!condition)
{
    Debug.Fail($"Details: {GetDetails()}");
}

but such a transformation is a) annoying to write when it's exactly what Debug.Assert is for, and b) can start to make the code less readable.

We can take advantage of the new language support for interpolated string handlers to keep the simple code and effectively have the compiler generate the guards in such a way that the interpolation won't happen unless the assert fails.

Proposed API

namespace System.Diagnostics
{
    public static class Debug
    {
        [Conditional("DEBUG")]
        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message);
 
        [Conditional("DEBUG")]
        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler detailedMessage);

        [InterpolatedStringHandlerAttribute]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public struct AssertInterpolatedStringHandler
        {
            public AssertInterpolatedStringHandler(int literalLength, int formattedCount, bool condition, out bool assert);
            public void AppendLiteral(string value);
            public void AppendFormatted<T>(T value);
            public void AppendFormatted<T>(T value, string? format);
            public void AppendFormatted<T>(T value, int alignment);
            public void AppendFormatted<T>(T value, int alignment, string? format);
            public void AppendFormatted(System.ReadOnlySpan<char> value);
            public void AppendFormatted(System.ReadOnlySpan<char> value, int alignment = 0, string? format = null);
            public void AppendFormatted(object? value, int alignment = 0, string? format = null);
            public void AppendFormatted(string? value);
            public void AppendFormatted(string? value, int alignment = 0, string? format = null);
        }
    }
}

Additional options:

  • We could expose similar overloads for Trace.Assert (they could use Debug+AssertInterpolatedStringHandler).
  • We could expose similar overloads for Debug.Write{Line}If (again using the same handler).

Usage Examples

Code like:

Debug.Assert(computed, $"{GetId()} => {GetResult()}");

would compile down to code along the lines of:

var handler = new AssertInterpolatedStringHandler(4, 2, computed, out bool assert);
if (assert)
{
    handler.AppendFormatted(GetId());
    handler.AppendLiteral(" => ");
    handler.AppendFormatted(GetResult());
}
Debug.Assert(computed, handler);

enabling the formatting and evaluation of the arguments to be done conditionally based on "assert", which would be set equal to !condition by the ctor.

Risks

  • Existing Debug.Assert calls using string interpolated arguments would start binding to the new overloads. Arguments in the format holes would start to be executed even more conditionally than they already are.
  • Using Debug.Assert(condition, $"interpolated", "non-interpolated") would silently fall back to the existing overload, since there's no proposed overload that covers all combinations of strings and handlers.

cc: @333fred

@stephentoub stephentoub added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 25, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone May 25, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 25, 2021
@ghost
Copy link

ghost commented May 25, 2021

Tagging subscribers to this area: @tommcdon, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Based on #52894 (comment)...

Background and Motivation

Debug.Assert has three overloads today:

[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition);
 
[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition, string? message);
 
[Conditional("DEBUG")]
public static void Assert([DoesNotReturnIf(false)] bool condition, string? message, string? detailMessage);

We're generally not concerned with the performance of asserts; after all, they're debug-only code. However, heavy use of asserts, or asserts where the developer wants to log significant details about the failure, can be non-trivially expensive and slow down debug execution to the point where it's impactful. For such cases, a developer can rewrite their assert:

Debug.Assert(condition, $"Details: {GetDetails()}");

to instead be:

if (!condition)
{
    Debug.Fail($"Details: {GetDetails()}");
}

but such a transformation is a) annoying to write when it's exactly what Debug.Assert is for, and b) can start to make the code less readable.

We can take advantage of the new language support for interpolated string handlers to keep the simple code and effectively have the compiler generate the guards in such a way that the interpolation won't happen unless the assert fails.

Proposed API

namespace System.Diagnostics
{
    public static class Debug
    {
        [Conditional("DEBUG")]
        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message);
 
        [Conditional("DEBUG")]
        public static void Assert([DoesNotReturnIf(false)] bool condition, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler detailedMessage);

        [InterpolatedStringHandlerAttribute]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public struct AssertInterpolatedStringHandler
        {
            public AssertInterpolatedStringHandler(int literalLength, int formattedCount, bool condition, out bool assert);
            public void AppendLiteral(string value);
            public void AppendFormatted<T>(T value);
            public void AppendFormatted<T>(T value, string? format);
            public void AppendFormatted<T>(T value, int alignment);
            public void AppendFormatted<T>(T value, int alignment, string? format);
            public void AppendFormatted(System.ReadOnlySpan<char> value);
            public void AppendFormatted(System.ReadOnlySpan<char> value, int alignment = 0, string? format = null);
            public void AppendFormatted(object? value, int alignment = 0, string? format = null);
            public void AppendFormatted(string? value);
            public void AppendFormatted(string? value, int alignment = 0, string? format = null);
        }
    }
}

Additional options:

  • We could expose similar overloads for Trace.Assert (they could use Debug+AssertInterpolatedStringHandler).
  • We could expose similar overloads for Debug.Write{Line}If (again using the same handler).

Usage Examples

Code like:

Debug.Assert(computed, $"{GetId()} => {GetResult()}");

would compile down to code along the lines of:

var handler = new AssertInterpolatedStringHandler(4, 2, computed, out bool assert);
if (assert)
{
    handler.AppendFormatted(GetId());
    handler.AppendLiteral(" => ");
    handler.AppendFormatted(GetResult());
}
Debug.Assert(computed, handler);

enabling the formatting and evaluation of the arguments to be done conditionally based on "assert", which would be set equal to !condition by the ctor.

Risks

  • Existing Debug.Assert calls using string interpolated arguments would start binding to the new overloads. Arguments in the format holes would start to be executed even more conditionally than they already are.
  • Using Debug.Assert(condition, $"interpolated", "non-interpolated") would silently fall back to the existing overload, since there's no proposed overload that covers all combinations of strings and handlers.
Author: stephentoub
Assignees: -
Labels:

api-suggestion, area-System.Diagnostics, untriaged

Milestone: 6.0.0

@danmoseley
Copy link
Member

danmoseley commented May 25, 2021

It's appealing that this change would magically speed up existing code on recompile. But as you say (and we mentioned in the original issue) it will be visible if generating the format string causes side effects. That would be weird and likely unintentional, but nevertheless might be surprising and would require code changes to avoid. would anyone write Debug.Assert(value > 10, $"{WriteValueToLog(value)}"); ?

@marklio thoughts?

@stephentoub
Copy link
Member Author

would anyone write Debug.Assert(value > 10, $"{WriteValueToLog(value)}"); ?

Debug.Assert is already [Conditional("DEBUG")]. From my perspective, if you're mutating state in your message, you deserve to be broken :)

@danmoseley
Copy link
Member

As you know, that's not necessarily justification for a breaking change, even for a SxS product 😄

I've certainly reported/fixed bugs where asserts where unintentionally mutating state, it would be ironic to create bugs where they stopped mutating state. (Maybe that's going to fix bugs..)

@GrabYourPitchforks
Copy link
Member

I like the application to both Debug.Assert and Debug.WriteLine. Don't the aspnet folks have their own debug/trace capability as well? Wonder if they'd find this builder useful as a reusable component.

@tommcdon tommcdon added enhancement Product code improvement that does NOT require public API changes/additions and removed enhancement Product code improvement that does NOT require public API changes/additions untriaged New issue has not been triaged by the area owner labels May 25, 2021
@stephentoub stephentoub added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 14, 2021
@terrajobst
Copy link
Member

@danmoseley

As you know, that's not necessarily justification for a breaking change, even for a SxS product 😄

I've certainly reported/fixed bugs where asserts where unintentionally mutating state, it would be ironic to create bugs where they stopped mutating state. (Maybe that's going to fix bugs..)

That's fair but it seems somewhat fringe for customers to ship code with the DEBUG symbol defined. And for local debugging, we don't apply the runtime breaking change policy but the design time/source breaking change policy which I'd argue would allow for these kind of fringe, but possible breaks.

@bartonjs
Copy link
Member

bartonjs commented Jun 17, 2021

Video

  • We acknowledge that there's a mild risk of breaking programs (e.g. Trace.Assert(condition, $"Condition failed on iteration {x++}");), but think it's generally good.
  • Approved as-is, plus all the "we could"s
namespace System.Diagnostics
{
    public static class Debug
    {
        [Conditional("DEBUG")]
        public static void Assert(
            [DoesNotReturnIf(false)] bool condition,
            [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message);
 
        [Conditional("DEBUG")]
        public static void Assert(
            [DoesNotReturnIf(false)] bool condition,
            [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message,
            [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler detailedMessage);

        [InterpolatedStringHandlerAttribute]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public struct AssertInterpolatedStringHandler
        {
            public AssertInterpolatedStringHandler(int literalLength, int formattedCount, bool condition, out bool assert);
            public void AppendLiteral(string value);
            public void AppendFormatted<T>(T value);
            public void AppendFormatted<T>(T value, string? format);
            public void AppendFormatted<T>(T value, int alignment);
            public void AppendFormatted<T>(T value, int alignment, string? format);
            public void AppendFormatted(System.ReadOnlySpan<char> value);
            public void AppendFormatted(System.ReadOnlySpan<char> value, int alignment = 0, string? format = null);
            public void AppendFormatted(object? value, int alignment = 0, string? format = null);
            public void AppendFormatted(string? value);
            public void AppendFormatted(string? value, int alignment = 0, string? format = null);
        }

        [Conditional("DEBUG")]
        public static void WriteIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message);

        [Conditional("DEBUG")]
        public static void WriteIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message,
            string category);

        [Conditional("DEBUG")]
        public static void WriteLineIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message);

        [Conditional("DEBUG")]
        public static void WriteLineIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message,
            string category);

        [InterpolatedStringHandlerAttribute]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public struct WriteIfInterpolatedStringHandler
        {
            public WriteIfInterpolatedStringHandler(int literalLength, int formattedCount, bool condition, out bool assert);
            public void AppendLiteral(string value);
            public void AppendFormatted<T>(T value);
            public void AppendFormatted<T>(T value, string? format);
            public void AppendFormatted<T>(T value, int alignment);
            public void AppendFormatted<T>(T value, int alignment, string? format);
            public void AppendFormatted(System.ReadOnlySpan<char> value);
            public void AppendFormatted(System.ReadOnlySpan<char> value, int alignment = 0, string? format = null);
            public void AppendFormatted(object? value, int alignment = 0, string? format = null);
            public void AppendFormatted(string? value);
            public void AppendFormatted(string? value, int alignment = 0, string? format = null);
        }
    }

    public static class Trace
    {
        [Conditional("TRACE")]
        public static void Assert(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message);
 
        [Conditional("TRACE")]
        public static void Assert(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler message, [InterpolatedStringHandlerArgument("condition")] AssertInterpolatedStringHandler detailedMessage);

        [InterpolatedStringHandlerAttribute]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public struct AssertInterpolatedStringHandler
        {
            public AssertInterpolatedStringHandler(int literalLength, int formattedCount, bool condition, out bool assert);
            public void AppendLiteral(string value);
            public void AppendFormatted<T>(T value);
            public void AppendFormatted<T>(T value, string? format);
            public void AppendFormatted<T>(T value, int alignment);
            public void AppendFormatted<T>(T value, int alignment, string? format);
            public void AppendFormatted(System.ReadOnlySpan<char> value);
            public void AppendFormatted(System.ReadOnlySpan<char> value, int alignment = 0, string? format = null);
            public void AppendFormatted(object? value, int alignment = 0, string? format = null);
            public void AppendFormatted(string? value);
            public void AppendFormatted(string? value, int alignment = 0, string? format = null);
        }

        [Conditional("TRACE")]
        public static void WriteIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message);

        [Conditional("TRACE")]
        public static void WriteIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message,
            string category);

        [Conditional("TRACE")]
        public static void WriteLineIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message);

        [Conditional("TRACE")]
        public static void WriteLineIf(
            bool condition,
            [InterpolatedStringHandlerArgument("condition")] WriteIfInterpolatedStringHandler message,
            string category);

        [InterpolatedStringHandlerAttribute]
        [EditorBrowsable(EditorBrowsableState.Never)]
        public struct WriteIfInterpolatedStringHandler
        {
            public WriteIfInterpolatedStringHandler(int literalLength, int formattedCount, bool condition, out bool assert);
            public void AppendLiteral(string value);
            public void AppendFormatted<T>(T value);
            public void AppendFormatted<T>(T value, string? format);
            public void AppendFormatted<T>(T value, int alignment);
            public void AppendFormatted<T>(T value, int alignment, string? format);
            public void AppendFormatted(System.ReadOnlySpan<char> value);
            public void AppendFormatted(System.ReadOnlySpan<char> value, int alignment = 0, string? format = null);
            public void AppendFormatted(object? value, int alignment = 0, string? format = null);
            public void AppendFormatted(string? value);
            public void AppendFormatted(string? value, int alignment = 0, string? format = null);
        }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jun 17, 2021
@marklio
Copy link

marklio commented Jun 17, 2021

Unfortunately, bringing data to this conversation is really hard. Spotting side effects in string creation is tricky statically. However, if we think about how someone would experience this break, I think risk becomes clearer. A library or app containing such code simply rolling onto a framework with this new API would not experience a break or any change in behavior. Their callsites remain bound to the string-based overloads. They would have to recompile the sensitive code against the new APIs, which puts us in source compat territory. Code sensitive to the break (where DEBUG is turned on in all used configurations so they've never experienced the conditionalness of these APIs, and string creation via interpolation has side effects) would then be broken in all configurations (readily observable/debuggable). It's kind of the optimal situation for a breaking change that delivers clear value, and the break is experienced directly by the developer compiling code (as opposed to a 3rd party). Scoped to a narrow scenario that is fundamentally opposed to recommendations and delivers easy to understand value to most customers. I tend to agree with @stephentoub and @terrajobst that this is goodness for the platform.

@danmoseley
Copy link
Member

@stephentoub I see this is marked blocking. Suggesting someones planning to do it? Wondering whether we should mark up for grabs.

@stephentoub stephentoub self-assigned this Jun 18, 2021
@stephentoub
Copy link
Member Author

I see this is marked blocking. Suggesting someones planning to do it? Wondering whether we should mark up for grabs.

I added the Debug.Assert/Write{Line}If support to #51653.

I didn't yet add the Trace APIs as I'm questioning whether they're really worthwhile... I'm not aware of a lot of new code being written to use Trace.Assert/Write{Line}If, and combine that with code also using interpolation in the messages...

@stephentoub stephentoub removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jun 21, 2021
@stephentoub stephentoub modified the milestones: 6.0.0, Future Jul 13, 2021
@stephentoub
Copy link
Member Author

The Debug overloads have been added for .NET 6. I've changed the milestone to Future for the Trace overloads; we can add them if we get meaningful feedback they'd actually be helpful.

@stephentoub stephentoub removed their assignment Jul 18, 2021
@stephentoub
Copy link
Member Author

We've not received any feedback yet about the lack of this on Trace. I'm going to close it for now.

@ghost ghost locked as resolved and limited conversation to collaborators Apr 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Diagnostics feature-request
Projects
None yet
Development

No branches or pull requests

8 participants