Unify whether throw helpers are [StackTraceHidden] or not

[Neme12]

Edit Please vote by leaving 👍 if want them hidden 👎 if want to include them in stack traces

Currently, most throw helpers (like ArgumentNullException.ThrowIfNull(p)) don't use the [StackTraceHidden], which makes them appear in stack traces. Some throw helpers are internally chained, which causes up to four additional frames. However, most are a single frame see all examples.

We have two options:

1. Hide all throw helpers. Vote 👍
   • Pro: Simpler stack traces
   • Pro: First line in stack trace points to user code
   • Con: Stack trace doesn't show which argument validation failed, you need to read message and/or look at the code
2. Hide none of the throw helpers. Vote 👎
   • Pro: Stack trace more accurately reflects reality
   • Pro: Stack trace alone shows which argument validation failed
   • Con: Stack trace is more noisy

There doesn't seem to be an argument that clearly shows one of the two options as superior. It's unlikely that we'll make the debugger configurable in this regard, so we'll have to pick one option, which is why user voting seems like a good way to decide.

Example code:

B(null!);

void B(string arg)
{
   ArgumentException.ThrowIfNullOrEmpty(arg);
}

Current experience:

System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at System.ArgumentException.ThrowNullOrEmptyException(String argument, String paramName)
   at System.ArgumentException.ThrowIfNullOrEmpty(String argument, String paramName)
   at Program.<<Main>$>g__B|0_2(String arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 26
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 10

Proposed experience:

System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at Program.<<Main>$>g__B|0_2(String arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 26
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 10

API proposal (hidden because we first want users to vote on the experience)

```C# namespace System; public partial class ArgumentException : SystemException { [StackTraceHidden] public static void ThrowIfNullOrEmpty([NotNull] string? argument, [CallerArgumentExpression("argument")] string? paramName = null); [StackTraceHidden] public static void ThrowIfNullOrWhiteSpace([NotNull] string? argument, [CallerArgumentExpression("argument")] string? paramName = null); } public partial class ArgumentOutOfRangeException : ArgumentException { [StackTraceHidden] public static void ThrowIfEqual(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) where T, IEquatable?; [StackTraceHidden] public static void ThrowIfGreaterThan(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) where T, IComparable!; [StackTraceHidden] public static void ThrowIfGreaterThanOrEqual(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) where T, IComparable!; [StackTraceHidden] public static void ThrowIfLessThan(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) where T, IComparable!; [StackTraceHidden] public static void ThrowIfLessThanOrEqual(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) where T, IComparable!; [StackTraceHidden] public static void ThrowIfNegative(T value, [CallerArgumentExpression("value")] string? paramName = null) where T, INumberBase!; [StackTraceHidden] public static void ThrowIfNegativeOrZero(T value, [CallerArgumentExpression("value")] string? paramName = null) where T, INumberBase!; [StackTraceHidden] public static void ThrowIfNotEqual(T value, T other, [CallerArgumentExpression("value")] string? paramName = null) where T, IEquatable?; [StackTraceHidden] public static void ThrowIfZero(T value, [CallerArgumentExpression("value")] string? paramName = null) where T, INumberBase!; } // Already marked public partial class ObjectDisposedException : InvalidOperationException { [StackTraceHidden] public static void ThrowIf([DoesNotReturnIf(true)] bool condition, object! instance); [StackTraceHidden] public static void ThrowIf([DoesNotReturnIf(true)] bool condition, Type! type); } ```

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime See info in area-owners.md if you want to be subscribed.

Issue Details

---

Currently, `ObjectDisposedException.ThrowIf` helpers have the `[StackTraceHidden]` attribute, which makes sense, but throw helpers on `ArgumentNullException` and `ArgumentException` do not. I think they should either all have it or none should have it so that it's consistent.

Author:	Neme12
Assignees:	-
Labels:	`area-System.Runtime`, `untriaged`, `needs-area-label`
Milestone:	-

[tannergooding]

CC. @bartonjs, @stephentoub, I vaguely remember us or API review having discussed this previously. Do you recall what the decision had been?

This basically just impacts the overall readability of stack traces by putting the method users actually want to track as having thrown at the top, rather than the helper.

[bartonjs]

I don't remember the discussion or outcome offhand, but I don't (personally) see value as to having ThrowIfNull on top (unless there's something degenerate about method inlining and a(n AoT?) lack of resource strings such that then make it ambiguous/confusing where the throw originates/what it is about).

[julealgon]

@bartonjs

(unless there's something degenerate about method inlining and a(n AoT?) lack of resource strings such that then make it ambiguous/confusing where the throw originates/what it is about).

If that's even possible, wouldn't it be better to have some sort of hardcoded fallback for the missing resource, in the implementation itself?

[jkotas]

there's something degenerate about method inlining and a(n AoT?) lack of resource strings

There are no degenerate situations like that (modulo bugs).

[tannergooding]

I've marked this as help-wanted given the above. We already do the same of utilizing StackTraceHidden for many of our own internal throw helpers.

@bartonjs, let me know if you'd rather take this formally to API-review or for me to send out a mail to API review just to double check there is no other pushback first.

[bartonjs]

It looks like we talked about it for ObjectDisposedException at https://youtu.be/-t3ELCIJpZo?t=887 or so (I don't have sound right now, so I went by where the captions seemed to be talking about it); and the best I can tell is that the PR had it on one overload, we decided it should be on both because (more or less) "[we] don't want to see the helper at the top of the call stack". So, I don't think there'd really be any pushback for adding it on the other throw helpers.

But sending an email just so we can be sure no one wants to argue seems good.

[terrajobst]

Personally, I think it would be desirable to have all throw helpers be [StackTraceHidden].

The only argument against it is if the throw helper throws because of argument exception but even in those cases it doesn't seem horrible if the user ends up in the caller by default (I say by default because IIRC turning off Just My Code will turn off [StackTraceHidden] behavior in the debugger).

[julealgon]

The only argument against it is if the throw helper throws because of argument exception but even in those cases it doesn't seem horrible if the user ends up in the caller by default (I say by default because IIRC turning off Just My Code will turn off [StackTraceHidden] behavior in the debugger).

This is an interesting consideration... an exception thrown inside of the guards due to incorrect arguments would be a bug in caller code and it would indeed be nice to show the guard methods in the trace in those cases.

Would it be possible to make [StackTraceHidden] somehow conditional, so that it would only apply on the expected set of exceptions?

[jkotas]

Would it be possible to make [StackTraceHidden] somehow conditional

It is not possible nor feasible.

Do you have a real repro of this issue? It feels like a theoretical concern.

The argument exception for bad arguments passed to the throw helper will be likely thrown from the slow part of the throw helper. You will see that in the stacktrace. It won't the be the public method name, but you will get an idea. It is no different from the inlining effects in other cases.

[julealgon]

@jkotas

Would it be possible to make [StackTraceHidden] somehow conditional

It is not possible nor feasible.

Ok.

Do you have a real repro of this issue? It feels like a theoretical concern.

It is a corner case, but not theoretical. Here is an example of one guard method that could have this issue:

https://github.com/dotnet/runtime/blob/9c9155dccb1f0f5b840710af1ace8515d20e9cd3/src/libraries/System.Private.CoreLib/src/System/ArgumentOutOfRangeException.cs#L176-L185

If you provide a null instance to this:

public void MyMethod(MyCustomComparableClass input)
{
    ArgumentOutOfRangeException.ThrowIfGreaterThan(input, MyCustomComparableClass.MaxValue);
}

Here, input could be null, which would result in a NullReferenceException on the value.CompareTo call inside of the guard method. If the whole method is hidden from the stack, won't this end up being confusing?

And yes, this is only possible today on a few of the existing guard methods since others either all work on value types, or handle nulls explicitly.

The argument exception for bad arguments passed to the throw helper will be likely thrown from the slow part of the throw helper. You will see that in the stacktrace. It won't the be the public method name, but you will get an idea. It is no different from the inlining effects in other cases.

I'm a bit confused by this. If it will still show in the stacktrace, what does [StackTraceHidden] do exactly?

[tannergooding]

If the whole method is hidden from the stack, won't this end up being confusing?

NullReferenceException is representative of a developer error and this would likely have been caught at build time by NRT had it been enabled.

If you didn't have NRT enabled it would flag the exception on whatever line is calling ThrowIfGreaterThan, which should be indicative that the user has passed in something that was null and should handle it explicitly. It's not exactly a leap of logic to follow and understand the premise of the failure and greatly improves the experience otherwise.

[julealgon]

@tannergooding

If the whole method is hidden from the stack, won't this end up being confusing?

NullReferenceException is representative of a developer error and this would likely have been caught at build time by NRT had it been enabled.

Sure, I'm aware of that.

If you didn't have NRT enabled it would flag the exception on whatever line is calling ThrowIfGreaterThan, which should be indicative that the user has passed in something that was null and should handle it explicitly. It's not exactly a leap of logic to follow and understand the premise of the failure and greatly improves the experience otherwise.

Oh, don't get me wrong: I'm definitely not advocating for removing the attribute. I was just wondering about these valid cases where one could end up with a stacktrace for invalid usage and we would want for the helper method to show up in the trace.

If it is not feasible like @jkotas said to make it conditional (on the exception type in this case) then that's fair enough in my book.

[jkotas]

Here, input could be null, which would result in a NullReferenceException on the value.CompareTo call inside of the guard method. If the whole method is hidden from the stack, won't this end up being confusing?

It can be confusing because some of these throw helpers took shortcut and do not follow the standard pattern for argument checking. I guess it was done for performance reasons. The standard behavior would be to see ArgumentNullException for value. I did not know about these shortcuts when I wrote my earlier comment.

[tannergooding]

I guess it was done for performance reasons

They're meant to do the minimal validation and only check the thing in question, as otherwise we would introduce new exceptions to paths that didn't expect them and duplicate checks users are likely to have already done themselves when following the standard patterns.

That is, if we had ThrowIfGreaterThan behave as if it first did ThrowIfNull(value) then users who had logic such as if (x < 0) { throw new ArgumentOutOfRangeException(); } couldn't transition over to ArgumentOutOfRangeException.ThrowIfGreaterThan(x, 0).

Similarly it would have duplicated a check in the case the user had already done ArgumentNullException.ThrowIfNull(x); and potentially obsfucated that there was an intentional ArgumentNullException being thrown for such inputs.

[terrajobst]

Sounds like we collectively believe that hiding all throw helpers from the stack trace is desirable then?

[stephentoub]

Sounds like we collectively believe that hiding all throw helpers from the stack trace is desirable then?

Personally I find a bit of value in having the throw helper in the stack, in particular when line information isn't available it can help to highlight where the exception is coming from (if it's not obvious from the exception details). I'm also not particularly concerned with whether there's one extra line in the stack trace.

But, I don't have a particularly strong opinion either way. If others feel it's better for us to mark all of our throw helpers with the attribute, fine.

[terrajobst]

I think the uber item here is consistency. I assume you'd also be OK if none of the throw helpers would be marked as [StackTraceHidden]?

[stephentoub]

Yup

[terrajobst]

Let's take it to API review and see what others think. Seems either outcome (all or none) would be viable.

[terrajobst]

Video

• The primary value of making it [StackTraceHidden] is that the debugger breaks at the call side for unhandled exception, rather than in the body of the throw helper
  • However, this is also the behavior of "Just My Code", which most people have turned on
  • Let's be consistent by not marking any throw helpers
  • We should probably remove it from internal throw helpers too

namespace System;

public partial class ObjectDisposedException : InvalidOperationException
{
    // Remove: [StackTraceHidden]
    public static void ThrowIf([DoesNotReturnIf(true)] bool condition, object! instance);

    // Remove: [StackTraceHidden]
    public static void ThrowIf([DoesNotReturnIf(true)] bool condition, Type! type);
}

[jkotas]

The primary value of making it [StackTraceHidden]

I would say that the primary value of the [StackTraceHidden] attribute is that it removes clutter from the exception logs.

For example, you will get this with ArgumentException that is not annotated with [StackTraceHidden] currently:

MyMethod(null);

void MyMethod(string myArgument)
{
    ArgumentException.ThrowIfNullOrEmpty(myArgument);
}

is going to produce:

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'myArgument')
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at System.ArgumentException.ThrowNullOrEmptyException(String argument, String paramName)
   at System.ArgumentException.ThrowIfNullOrEmpty(String argument, String paramName)
   at Program.<<Main>$>g__MyMethod|0_0(String myArgument) in c:\repro\ConsoleApp1\ConsoleApp1\Program.cs:line 5
   at Program.<Main>$(String[] args) in c:\repro\ConsoleApp1\ConsoleApp1\Program.cs:line 1

Does anybody want to see 4 frames of exception throwing clutter before the actual method with the problem? I think one frame would be ok as a clue, but 4 frames is too much.

[julealgon]

Agreed with @jkotas . I'm pretty sure one of the ideas behind the original throw helpers was to streamline the way we throw in the most transparent way possible. The [StackTraceHidden] addition would make stacktraces feel similar to ones where the throw helpers were not used, which would be a big win IMHO.

The added noise is really bad.

Particularly, this statement from @stephentoub is not really correct based on the above example:

I'm also not particularly concerned with whether there's one extra line in the stack trace.

The impact is much more substantial for some of those throw helpers than "one extra line".

[terrajobst]

@jkotas

I would say that the primary value of the [StackTraceHidden] attribute is that it removes clutter from the exception logs.

Personally, I'd agree with that too, but that was the point where different people feel different. @stephentoub thought that in the particular case where you only see the exception log, the frame makes it easier to see what the error condition is, without having to look at the line of code (or reading the exception message).

The point we all agreed on was that during debugging it improves usability by activating the user's stack frame.

The fact that one throw helper can result in four lines feels noisy though. I don't think we considered that.

@stephentoub does the example given by @jkotas change your assessment?

[terrajobst]

@julealgon

The impact is much more substantial for some of those throw helpers than "one extra line".

Could you provide some more details on why you feel that is? Is it only because of @jkotas example where one throw helper results in multiple lines or is there something else in the UX?

Unhandled exception. System.ArgumentNullException: Value cannot be null. (Parameter 'myArgument') at System.ArgumentNullException.Throw(String paramName) at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName) at System.ArgumentException.ThrowNullOrEmptyException(String argument, String paramName) at System.ArgumentException.ThrowIfNullOrEmpty(String argument, String paramName)

[stephentoub]

For this:

using System.Runtime.CompilerServices;

void Print(Exception e)
{
    Console.WriteLine(e.StackTrace);
    Console.WriteLine("---------------------");
}

try { A(null); } catch (Exception e) { Print(e); }
try { B(null); } catch (Exception e) { Print(e); }
try { C(null); } catch (Exception e) { Print(e); }
try { D(1, 1); } catch (Exception e) { Print(e); }
try { E(1, 2); } catch (Exception e) { Print(e); }
try { F(1, 2); } catch (Exception e) { Print(e); }
try { G(1, 2); } catch (Exception e) { Print(e); }
try { H(3, 2); } catch (Exception e) { Print(e); }
try { I(3, 2); } catch (Exception e) { Print(e); }
try { J(0); } catch (Exception e) { Print(e); }
try { K(-1); } catch (Exception e) { Print(e); }
try { L(-1); } catch (Exception e) { Print(e); }

[MethodImpl(MethodImplOptions.NoInlining)]
static void A(object? arg) => ArgumentNullException.ThrowIfNull(arg);

[MethodImpl(MethodImplOptions.NoInlining)]
static void B(string? arg) => ArgumentException.ThrowIfNullOrEmpty(arg);

[MethodImpl(MethodImplOptions.NoInlining)]
static void C(string? arg) => ArgumentException.ThrowIfNullOrWhiteSpace(arg);

[MethodImpl(MethodImplOptions.NoInlining)]
static void D(int a, int b) => ArgumentOutOfRangeException.ThrowIfEqual(a, b);

[MethodImpl(MethodImplOptions.NoInlining)]
static void E(int a, int b) => ArgumentOutOfRangeException.ThrowIfNotEqual(a, b);

[MethodImpl(MethodImplOptions.NoInlining)]
static void F(int a, int b) => ArgumentOutOfRangeException.ThrowIfLessThan(a, b);

[MethodImpl(MethodImplOptions.NoInlining)]
static void G(int a, int b) => ArgumentOutOfRangeException.ThrowIfLessThanOrEqual(a, b);

[MethodImpl(MethodImplOptions.NoInlining)]
static void H(int a, int b) => ArgumentOutOfRangeException.ThrowIfGreaterThan(a, b);

[MethodImpl(MethodImplOptions.NoInlining)]
static void I(int a, int b) => ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual(a, b);

[MethodImpl(MethodImplOptions.NoInlining)]
static void J(int a) => ArgumentOutOfRangeException.ThrowIfZero(a);

[MethodImpl(MethodImplOptions.NoInlining)]
static void K(int a) => ArgumentOutOfRangeException.ThrowIfNegative(a);

[MethodImpl(MethodImplOptions.NoInlining)]
static void L(int a) => ArgumentOutOfRangeException.ThrowIfNegativeOrZero(a);

in release with tiered compilation disabled (since optimizations won't kick in otherwise), I get this:

   at System.ArgumentNullException.Throw(String paramName)
   at Program.<<Main>$>g__A|0_1(Object arg) in F:\source\ConsoleApp1\Program.cs:line 23
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 9
---------------------
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at System.ArgumentException.ThrowNullOrEmptyException(String argument, String paramName)
   at Program.<<Main>$>g__B|0_2(String arg) in F:\source\ConsoleApp1\Program.cs:line 26
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 10
---------------------
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at System.ArgumentException.ThrowNullOrWhiteSpaceException(String argument, String paramName)
   at Program.<<Main>$>g__C|0_3(String arg) in F:\source\ConsoleApp1\Program.cs:line 29
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 11
---------------------
   at System.ArgumentOutOfRangeException.ThrowEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__D|0_4(Int32 a, Int32 b) in F:\source\ConsoleApp1\Program.cs:line 32
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 12
---------------------
   at System.ArgumentOutOfRangeException.ThrowNotEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__E|0_5(Int32 a, Int32 b) in F:\source\ConsoleApp1\Program.cs:line 35
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 13
---------------------
   at System.ArgumentOutOfRangeException.ThrowLess[T](T value, T other, String paramName)
   at Program.<<Main>$>g__F|0_6(Int32 a, Int32 b) in F:\source\ConsoleApp1\Program.cs:line 38
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 14
---------------------
   at System.ArgumentOutOfRangeException.ThrowLessEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__G|0_7(Int32 a, Int32 b) in F:\source\ConsoleApp1\Program.cs:line 41
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 15
---------------------
   at System.ArgumentOutOfRangeException.ThrowGreater[T](T value, T other, String paramName)
   at Program.<<Main>$>g__H|0_8(Int32 a, Int32 b) in F:\source\ConsoleApp1\Program.cs:line 44
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 16
---------------------
   at System.ArgumentOutOfRangeException.ThrowGreaterEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__I|0_9(Int32 a, Int32 b) in F:\source\ConsoleApp1\Program.cs:line 47
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 17
---------------------
   at System.ArgumentOutOfRangeException.ThrowZero[T](T value, String paramName)
   at Program.<<Main>$>g__J|0_10(Int32 a) in F:\source\ConsoleApp1\Program.cs:line 50
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 18
---------------------
   at System.ArgumentOutOfRangeException.ThrowNegative[T](T value, String paramName)
   at Program.<<Main>$>g__K|0_11(Int32 a) in F:\source\ConsoleApp1\Program.cs:line 53
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 19
---------------------
   at System.ArgumentOutOfRangeException.ThrowNegativeOrZero[T](T value, String paramName)
   at Program.<<Main>$>g__L|0_12(Int32 a) in F:\source\ConsoleApp1\Program.cs:line 56
   at Program.<Main>$(String[] args) in F:\source\ConsoleApp1\Program.cs:line 20
---------------------

So it's one line for all of them except for ThrowIfNullOrEmpty and ThrowIfNullOrWhiteSpace where it's three. If we're really concerned about those two and we want to change how they're structured to have them only be one, I'm not going to die on that hill, but I don't really understand the concern.

[jkotas]

The point we all agreed on was that during debugging it improves usability by activating the user's stack frame.

The documentation for [StackTraceHidden] attribute says "Types and Methods attributed with StackTraceHidden will be omitted from the stack trace text shown in StackTrace.ToString() and Exception.StackTrace.".

The debugger is a nice touch on top of it, but it does not matter most of the time with these system helpers thanks to "just my code" as you have pointed out.

release with tiered compilation disabled

I expect that people are more likely to see exception stacktraces with Tier 0 codegen when the code is executed for the first time. We should be looking at the experience with both Tier 0 and Tier 1 codegen.

[stephentoub]

We should be looking at the experience with both Tier 0 and Tier 1 codegen.

In tier 0, all of those numbers increase by 1, which, to me, is still not a big deal.

[tannergooding]

My own preference is still to use [StackTraceHidden]. To me, it improves the overall UX, reduces noise, and helps make things more expected.

Things like throw helpers are largely an unimportant impl detail and I'd much rather see the top of the stack being the thing that conceptually faulted and not have to look 1+ levels past a bunch of helpers to find it.

[julealgon]

Is there any way to make this configurable, instead of completely static and dependent on the attribute?

Perhaps the attribute should indicate whether a method participates in the option to be hidden, but the ability to hide/show would be driven by some sort of external toggle.

This way, the team here could define what they think is a "good default" for this but still give the option to consumers to customize as they see fit.

---

The impact is much more substantial for some of those throw helpers than "one extra line".

Could you provide some more details on why you feel that is? Is it only because of @jkotas example where one throw helper results in multiple lines or is there something else in the UX?

I was just referring to the added lines in the trace @terrajobst . At this point I don't have any other concerns with this.

[terrajobst]

@julealgon

Is there any way to make this configurable, instead of completely static and dependent on the attribute?

I don't believe so and I think adding more knobs to debugging aren't necessarily making the UX better, due to added complexity. I think the right fix is for us to agree on a policy.

I was just referring to the added lines in the trace @terrajobst. At this point I don't have any other concerns with this.

👍

It looks like this is more of an exercise of UX preference over technical correctness. I'll run a poll to get a handle on what our user base would prefer.

[benaadams]

Doesn't add value; is plumbing

I need to know what the exception is and where it came from; not how the message was constructed.

For this:

using System.Runtime.CompilerServices;

void Print(Exception e)
{
    Console.WriteLine(e.StackTrace);
    Console.WriteLine("---------------------");
}

This doesn't create the output people look at; they see the .ToString() so really the output is

System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at Program.<<Main>$>g__A|0_1(Object arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 23
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 9
---------------------
System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at System.ArgumentException.ThrowNullOrEmptyException(String argument, String paramName)
   at System.ArgumentException.ThrowIfNullOrEmpty(String argument, String paramName)
   at Program.<<Main>$>g__B|0_2(String arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 26
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 10
---------------------
System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at System.ArgumentNullException.Throw(String paramName)
   at System.ArgumentNullException.ThrowIfNull(Object argument, String paramName)
   at System.ArgumentException.ThrowNullOrWhiteSpaceException(String argument, String paramName)
   at System.ArgumentException.ThrowIfNullOrWhiteSpace(String argument, String paramName)
   at Program.<<Main>$>g__C|0_3(String arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 29
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 11
---------------------
System.ArgumentOutOfRangeException: a ('1') must not be equal to '1'. (Parameter 'a')
Actual value was 1.
   at System.ArgumentOutOfRangeException.ThrowEqual[T](T value, T other, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__D|0_4(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 32
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 12
---------------------
System.ArgumentOutOfRangeException: a ('1') must be equal to '2'. (Parameter 'a')
Actual value was 1.
   at System.ArgumentOutOfRangeException.ThrowNotEqual[T](T value, T other, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfNotEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__E|0_5(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 35
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 13
---------------------
System.ArgumentOutOfRangeException: a ('1') must be greater than or equal to '2'. (Parameter 'a')
Actual value was 1.
   at System.ArgumentOutOfRangeException.ThrowLess[T](T value, T other, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfLessThan[T](T value, T other, String paramName)
   at Program.<<Main>$>g__F|0_6(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 38
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 14
---------------------
System.ArgumentOutOfRangeException: a ('1') must be greater than '2'. (Parameter 'a')
Actual value was 1.
   at System.ArgumentOutOfRangeException.ThrowLessEqual[T](T value, T other, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfLessThanOrEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__G|0_7(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 41
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 15
---------------------
System.ArgumentOutOfRangeException: a ('3') must be less than or equal to '2'. (Parameter 'a')
Actual value was 3.
   at System.ArgumentOutOfRangeException.ThrowGreater[T](T value, T other, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfGreaterThan[T](T value, T other, String paramName)
   at Program.<<Main>$>g__H|0_8(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 44
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 16
---------------------
System.ArgumentOutOfRangeException: a ('3') must be less than '2'. (Parameter 'a')
Actual value was 3.
   at System.ArgumentOutOfRangeException.ThrowGreaterEqual[T](T value, T other, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfGreaterThanOrEqual[T](T value, T other, String paramName)
   at Program.<<Main>$>g__I|0_9(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 47
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 17
---------------------
System.ArgumentOutOfRangeException: a ('0') must be a non-zero value. (Parameter 'a')
Actual value was 0.
   at System.ArgumentOutOfRangeException.ThrowZero[T](T value, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfZero[T](T value, String paramName)
   at Program.<<Main>$>g__J|0_10(Int32 a) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 50
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 18
---------------------
System.ArgumentOutOfRangeException: a ('-1') must be a non-negative value. (Parameter 'a')
Actual value was -1.
   at System.ArgumentOutOfRangeException.ThrowNegative[T](T value, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfNegative[T](T value, String paramName)
   at Program.<<Main>$>g__K|0_11(Int32 a) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 53
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 19
---------------------
System.ArgumentOutOfRangeException: a ('-1') must be a non-negative and non-zero value. (Parameter 'a')
Actual value was -1.
   at System.ArgumentOutOfRangeException.ThrowNegativeOrZero[T](T value, String paramName)
   at System.ArgumentOutOfRangeException.ThrowIfNegativeOrZero[T](T value, String paramName)
   at Program.<<Main>$>g__L|0_12(Int32 a) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 56
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 20
---------------------

Does that add value over this?

System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at Program.<<Main>$>g__A|0_1(Object arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 23
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 9
---------------------
System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at Program.<<Main>$>g__B|0_2(String arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 26
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 10
---------------------
System.ArgumentNullException: Value cannot be null. (Parameter 'arg')
   at Program.<<Main>$>g__C|0_3(String arg) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 29
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 11
---------------------
System.ArgumentOutOfRangeException: a ('1') must not be equal to '1'. (Parameter 'a')
Actual value was 1.
   at Program.<<Main>$>g__D|0_4(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 32
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 12
---------------------
System.ArgumentOutOfRangeException: a ('1') must be equal to '2'. (Parameter 'a')
Actual value was 1.
   at Program.<<Main>$>g__E|0_5(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 35
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 13
---------------------
System.ArgumentOutOfRangeException: a ('1') must be greater than or equal to '2'. (Parameter 'a')
Actual value was 1.
   at Program.<<Main>$>g__F|0_6(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 38
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 14
---------------------
System.ArgumentOutOfRangeException: a ('1') must be greater than '2'. (Parameter 'a')
Actual value was 1.
   at Program.<<Main>$>g__G|0_7(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 41
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 15
---------------------
System.ArgumentOutOfRangeException: a ('3') must be less than or equal to '2'. (Parameter 'a')
Actual value was 3.
   at Program.<<Main>$>g__H|0_8(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 44
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 16
---------------------
System.ArgumentOutOfRangeException: a ('3') must be less than '2'. (Parameter 'a')
Actual value was 3.
   at Program.<<Main>$>g__I|0_9(Int32 a, Int32 b) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 47
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 17
---------------------
System.ArgumentOutOfRangeException: a ('0') must be a non-zero value. (Parameter 'a')
Actual value was 0.
   at Program.<<Main>$>g__J|0_10(Int32 a) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 50
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 18
---------------------
System.ArgumentOutOfRangeException: a ('-1') must be a non-negative value. (Parameter 'a')
Actual value was -1.
   at Program.<<Main>$>g__K|0_11(Int32 a) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 53
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 19
---------------------
System.ArgumentOutOfRangeException: a ('-1') must be a non-negative and non-zero value. (Parameter 'a')
Actual value was -1.
   at Program.<<Main>$>g__L|0_12(Int32 a) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 56
   at Program.<Main>$(String[] args) in C:\Users\thund\source\repos\ConsoleApp3\ConsoleApp3\Program.cs:line 20
---------------------

Its adding additional frames over not using a throw helper; is there a benefit to the programmer from that?

[stephentoub]

is there a benefit to the programmer from that?

If there's no PDB and thus no line numbers, yes, it further helps to more easily figure out where the error is coming from.

As I said in the API review yesterday, I don't actually feel strongly about it one way or the other, other than being consistent. But: a) in general I err on the side of diagnostic tools reflecting reality, and hiding stack frames deviates from that, b) I do think removing frames in a case like this hurts the ability to determine as quickly as possible where an error came from (at least one frame related to it), and c) Across all libraries we have a much better chance of consistency if the default is "don't do anything special" rather than the default being "to be consistent you must add this attribute to all methods involved in validation of inputs".

[benaadams]

is there a benefit to the programmer from that?

If there's no PDB and thus no line numbers, yes, it further helps to more easily figure out where the error is coming from.

If you are starting from a point of

if (arg is null)
    throw new ArgumentOutOfRangeException()

and moving to

ArgumentNullException.ThrowIfNull(arg)

Would you therefore say the additional stack traces is a selling point of the conversion?

[tannergooding]

Notably I've found that the combination of exception kind and message is typically more than sufficient to know exactly what line threw the exception.

If its ArgumentNullException and arg, well that should only be one specific line and should be unambiguous for a given method M

[stephentoub]

Would you therefore say the additional stack traces is a selling point of the conversion?

Yes, one of. From my perspective, the benefits in order:

1. ANE.ThrowIfNull provides a simple way to express what's often a more verbose 2 or 3 line construct, depending on style rules.
2. ANE.ThrowIfNull then provides some perf benefits, albeit ones it'd be nice if in the fullness of time the JIT could automatically handle with outlining.
3. ANE.ThrowIfNull provides a non-inlineable marker in the stack trace.

[terrajobst]

Looks like a majority of our users would prefer to exclude the frames:

[colejohnson66]

I wish there was a way to keep the stack trace, but get the undocumented(?) benefit of debugger breaking on the callsite, not throw helper. Because, for me, when I'm writing a library with throw helpers and testing it, "just my code" doesn't help. I'd rather the break be at the callsite, not the inner throw method. It also doesn't help if I throw (heh) my throw helpers into a dedicated sibling library because it's still considered "my code".

Sure, I could litter my throw helpers with

#if DEBUG
        [StackTraceHidden]
#endif
        public static void ThrowIfXyz(/* ... */) { /* ... */ }

but that's not ideal, IMO.

I'm fine with the team removing [StackTraceHidden], but I'd like them to consider some way to keep both benefits.

[jkotas]

keep the stack trace, but get the benefit of debugger breaking on the callsite

Have you tried [DebuggerHidden]?

[colejohnson66]

Yes that worked. Idk why it never crossed my mind to look at other attributes in System.Diagnostics

[bartonjs]

My preferences are either "nothing is hidden" or "everything past ArgumentException.ThrowIfNull is hidden", but not "ArgumentException.ThrowIfNull and beyond is hidden".

[benaadams]

My preferences are either "nothing is hidden" or "everything past ArgumentException.ThrowIfNull is hidden", but not "ArgumentException.ThrowIfNull and beyond is hidden".

"everything past ArgumentException.ThrowIfNull is hidden"

and

ArgumentException.ThrowIfNull is inlined so its also hidden 😅

[colejohnson66]

It is inlined, but because the logic for it was still executed, it appears in the stack trace. Unlike C/C++ compilers tend to do, dotnet preserves stack traces levels for inlined methods.

[benaadams]

It is inlined, but because the logic for it was still executed, it appears in the stack trace. Unlike C/C++ compilers tend to do, dotnet preserves stack traces levels for inlined methods.

Fairly sure it doesn't 🤔

[terrajobst]

Marking this as api-needs-work because it seems folks are unhappy with the conclusion from the last review:

• Some throw helpers result in 4 frames, not one frame
• Customer evidence from Twitter suggest around 2/3 of our social media base would prefer hiding them
• It seems on FXDC there is a minor preference for not hiding, but no major objection to hiding them
• It seems we can purely decide based on customer preference, which is why I edited the issue description with the two options, their pros & cons and instructions to vote.

I'll be spreading this issues in as many places as I can think of, please do the same.

[vadimkantorov]

Regarding 4 frames, one hack could be to rewrite/inline those throw helpers to ensure that every throw helper results in 1 frame... Then hiding problem would be less severe...

[jzabroski]

Does anybody want to see 4 frames of exception throwing clutter before the actual method with the problem? I think one frame would be ok as a clue, but 4 frames is too much.

Absolutely do not want clutter.

In addition to just being noisy:

Ingest for DataDog is $0.10 per GB of ingest. While ArgumentNullException should be rare, if it occurs in the wild on a large cluster with a busy number of requests/second, it will add to the bill unnecessarily.

[tannergooding]

My opinion is still the same as expressed in API review.

I'm very much in favor of these all being annotated with [StackTraceHidden]:

• it gives the most consistent experience across unoptimized and optimized code
• hides what is largely just an implementation detail of a convenience helper API
• matches what I'd already been historically doing in my own throw helpers class
• matches what the BCL has historically done for its internal throw helper APIs
• makes stack traces smaller (which means smaller logs)
• surfaces the information I care about to the top of the stack trace
• can still be trivially mapped to the failing line, regardless of whether PDBs with line numbers are available or not
  • our throw helpers include parameter names by default and its atypical to have two lines throwing for the same parameter, so the message/exception info will typically let you know exactly what line threw regardless
• etc

[colejohnson66]

Maybe a compromise? Leave the actual throw helpers alone (optionally with [DebuggerHidden]), and annotate just the actual throwing methods with [StackTraceHidden]? Then there'd only be one frame of throw helpers in the stack trace.

 namespace System
 {
     public class ArgumentNullException : ArgumentException
     {
+        [DebuggerHidden]
         [Intrinsic]
         public static void ThrowIfNull([NotNull] object? argument, [CallerArgumentExpression(nameof(argument))] string? paramName = null)
         {
             if (argument is null)
             {
                 Throw(paramName);
             }
         }

         [DoesNotReturn]
+        [StackTraceHidden]
         internal static void Throw(string? paramName) =>
             throw new ArgumentNullException(paramName);
     }
 }