Consider exposing System.Diagnostics.StackTraceHiddenAttribute publicly

[john-h-k]

This attribute hides methods and types marked by it in the error message stacktraces. It is used extensively in the CoreLib to e.g. hide implementation details of async methods that are always the same and just add clutter to the error messages.

It appears to be a very useful attribute, particular as System.ThrowHelper is internal, this would be invaluable for user-written throw helper types to keep the best possible exception semantics while still having the advantages of a throw helper. It could be abused, but not really in a "breaking" way, just using it stupidly could make debugging a little harder, so unless there is something I am really missing I cannot see a strong case against making it public.

Proposed API

namespace System.Diagnostics
{
    [AttributeUsage(AttributeTargets.Class | AttributeTargets.Method | AttributeTargets.Constructor | AttributeTargets.Struct, Inherited = false)]
    public sealed class StackTraceHiddenAttribute : Attribute
    {
        public StackTraceHiddenAttribute() { }
    }
}

[danmoseley]

@tommcdon

[tommcdon]

cc @noahfalk @sywhang

[MuiBienCarlota]

This can also be usable with the already public DoesNotReturnAttribute.

[terrajobst]

Video

• Since we can apply it to classes and structs, it seem one should be able to apply it to properties and events as well. Should we add Property, Event, Delegate?
• We considered Interface (DIM), but that seems ill-defined for non-DIMs. If people wanted them on DIMs, they can apply it to the method

namespace System.Diagnostics
{
    [AttributeUsage(AttributeTargets.Class |
                    AttributeTargets.Method |
                    AttributeTargets.Constructor |
                    AttributeTargets.Struct, Inherited = false)]
    public sealed class StackTraceHiddenAttribute : Attribute
    {
        public StackTraceHiddenAttribute();
    }
}

[PathogenDavid]

• Should we add Property, Event, Delegate?

Adding those would require updating consumers of this attribute to recognize it in those places. For example:

runtime/src/libraries/System.Private.CoreLib/src/System/Diagnostics/StackTrace.cs

Lines 356 to 369 in 6072e4d

	if (mb.IsDefined(typeof(StackTraceHiddenAttribute), inherit: false))
	{
	// Don't show where StackTraceHidden is applied to the method.
	return false;
	}
	Type? declaringType = mb.DeclaringType;
	// Methods don't always have containing types, for example dynamic RefEmit generated methods.
	if (declaringType != null &&
	declaringType.IsDefined(typeof(StackTraceHiddenAttribute), inherit: false))
	{
	// Don't show where StackTraceHidden is applied to the containing Type of the method.
	return false;
	}

I'm not sure if it's worth it to add the extra checks here for those scenarios. I can't really imagine why you'd need to hide a get/set/add/remove method from stack traces, and if you need to you could just apply them to the individual methods.

As for delegates, what would that mean? That invocations of the delegate are hidden? Found time to watch the video, I believe the answer is yes.

[john-h-k]

I don't think there is really any value in adding to delegate, particularly given it would possibly require additional support from tooling, and for prop/event you can just add to the individual accessor

I would be happy to do the PR for this (don't think it will be a very tricky one 😆 )

[benaadams]

Should also delete the upstream one in ASP.NET when its exposed (and propagates) https://github.com/dotnet/aspnetcore/blob/723e32a47dd0101b6c169ac4b7fbf61a25e83b32/src/Servers/Kestrel/Core/src/Internal/Infrastructure/StackTraceHiddenAttribute.cs#L12

[stephentoub]

Does the one in ASP.NET do anything? The stack trace code is searching for the attribute based on type rather than name, using the type from corelib, right?

[benaadams]

Does the one in ASP.NET do anything?

Just allows methods to be annotated with it; which is then used in ASP.NET's stack trace cleaning https://github.com/dotnet/aspnetcore/blob/e657de4b778bcefbaeeea89883f0975cbcae3107/src/Shared/StackTrace/StackFrame/StackTraceHelper.cs#L254-L257

Which could also be changed to a type check rather than a string comparison (is string comparison to pick up both the runtime's internal type and aspnet's type)

[stephentoub]

I see, thanks, so same name but used for a different (related) purpose.