Add new ObjectDisposedException constructor overload

[marek-safar]

Background and Motivation

There is a very common pattern used to handle disposed object state and it involves throwing ObjectDisposedException. It requires extra conversion before constructing the exception. There are about 400 cases of this pattern in runtime which could be unified and simplified and more in .NET libraries.

	throw new ObjectDisposedException(GetType().FullName);
	// or
	throw new ObjectDisposedException(GetType().Name);
	// or
	throw new ObjectDisposedException(GetType().ToString());
	// or
	throw new ObjectDisposedException(nameof(NameOfThisType));

Proposed API

namespace System
{
     public class ObjectDisposedException {
+		public ObjectDisposedException(object instance);
     }

Usage Examples

All the examples from above could be replaced with simple construction which would extract the name from the instance.

	throw new ObjectDisposedException(this);

[dotnet-issue-labeler]

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.

[stephentoub]

throw new ObjectDisposedException(GetType().FullName);
// or
throw new ObjectDisposedException(GetType().Name);
// or
throw new ObjectDisposedException(GetType().ToString());
// or
throw new ObjectDisposedException(nameof(NameOfThisType));

Those all do different things. Which are you proposing the new overload maps to? And are you proposing that all of the other cases in use today change their output as a result?

[Wraith2]

For new ObjectDisposedException(GetType().Name); and new ObjectDisposedException(nameof(NameOfThisType)); would it be better to have the type as a generic parameter so the jit can see the real type and provide a static simple implementation that doesn't rely on runtime GetType() or reflection?

public sealed class ObjectDisposedException
{
+    public static ObjectDisposedException CreateFor<T>() => return new ObjectDisposedException(nameof(T));
}

[marek-safar]

Which are you proposing the new overload maps to? And are you proposing that all of the other cases in use today change their output as a result?

I think the different output today is not intentional as I doubt that many developers know the difference between FullName and ToString(). The new implementation could call GetType().FullName to always provide the most accurate information.

Would it be better to have the type as a generic parameter so the jit can see the real type

It could be another overload but that one will be that useful. There are much fewer cases which do

	throw new ObjectDisposedException(typeof(T).{FullName|Name|ToString())

and the nameof cases would be better replaced with this argument.

[GrabYourPitchforks]

a static simple implementation that doesn't rely on runtime GetType() or reflection?

If needed, we can provide a specialized implementation of this ctor that bypasses most of the reflection stack, bypasses caching, and tries to do the most naïve, least complicated thing possible.

[jkotas]

I would not worry about cost of computing the type name for the purpose of this API. It is the least of the problem for throwing ObjectDisposedException.

[GrabYourPitchforks]

I would not worry about cost of computing the type name for the purpose of this API.

I'm not worried about the performance of the ctor call. I'm more worried about the GetType().FullName pattern more generally initializing a bunch of reflection caches and growing the application's working set - all just to throw an exception.

[jkotas]

The reflection caches are collectible. If it is just one time occurence, the GC will take care of them.

[terrajobst]

• Let's also add an overload that takes Type directly

namespace System
{
    public partial class ObjectDisposedException
    {
        public ObjectDisposedException(object instance);
        public ObjectDisposedException(Type type);
    }
}

[stephentoub]

In the spirit of #48573, I wonder if this should actually be a Throw(object) / Throw(Type) set of static methods?

[terrajobst]

Interesting; I'm not opposed to that.

[bartonjs]

Reset to api-ready-for-review since the PR seems to have gone ahead with Steve's suggestion. So we should discuss Steve's suggestion.

namespace System
{
    public partial class ObjectDisposedException
    {
        [System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute]
        [System.Diagnostics.StackTraceHidden]
        public static void Throw(System.Object instance) => throw null;
        [System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute]
        public static void Throw(System.Type type) => throw null;
    }
}

Part of the motivation being that new ObjectDisposedException(null) is currently valid (uses the default message), and as the new ctors thus introduce a null-literal compile-time ambiguity/break. The other part being that it doesn't seem likely that anyone really wants to do anything with an ODE other than throw it.

Also suggestion, public static void Throw<T>() => Throw(typeof(T));. (FWIW, I don't think it's useful, since there's no inference or return type from it)

[GrabYourPitchforks]

Two comments on Jeremy's latest proposal.

First, should the instance and type arguments be nullable? If somebody passes in null, I think the behavior should be equivalent to the generalized throw new ObjectDisposedException();.

Second, agree that Throw<T> isn't too useful. Using Throw(this) will make life a bit easier for the runtime.

[bartonjs]

First, should the instance and type arguments be nullable?

What's the scenario? I see them being used as either

public sealed partial class Foo
{
    public void SomethingFun()
    {
        if (_disposed)
            ObjectDisposedException.Throw(this);
 
        ...
    }
}

or

public abstract partial class Bar
{
    public void DoAnAwesomeThing()
    {
        if (_disposed)
            ObjectDisposedException.Throw(typeof(Bar));
    }
}

I don't see a case where null should arise. Literal-this or literal typeof(...) seem like the only target inputs.

[GrabYourPitchforks]

Scenario would be something like:

if (this._disposed) { ObjectDisposedException.Throw(this._wrappedResource); }

Where _wrappedResource could possibly be null after this is disposed. I agree it's a bit contrived, and maybe not worth thinking too hard about, but wanted to raise it as a scenario.

[bartonjs]

Video

We discussed the method pattern here, and debated for a bit on whether the parameters should be nullable and we concluded no. Since we're early in the release we have time to learn that was wrong. (And going "more nullable" is not breaking)

namespace System
{
    public partial class ObjectDisposedException
    {
        [System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute]
        [System.Diagnostics.StackTraceHidden]
        public static void Throw(System.Object instance) => throw null;
        [System.Diagnostics.StackTraceHidden]
        [System.Diagnostics.CodeAnalysis.DoesNotReturnAttribute]
        public static void Throw(System.Type type) => throw null;
    }
}

One notable change from the PR state: We added StackTraceHidden to both methods.

[stephentoub]

Of the hundreds of call sites changed in #58684, I think all but maybe 4 were of the form:

if (condition)
{
    ObjectDisposedException.Throw(...);
}

and two of the remaining ones could have been written that way. In addition to or instead of Throw, should we have a ThrowIf?

public static void ThrowIf(bool condition, object instance)
{
    if (condition)
    {
        Throw(instance);
    }
}

such that those hundreds of call sites change from:

if (condition)
{
    ObjectDisposedException.Throw(...);
}

to instead be:

ObjectDisposedException.ThrowIf(condition, ...);

?

[bartonjs]

Either adding ThrowIf or changing Throw to ThrowIf makes sense to me, looking at the diff.

If we want to go with less-is-more then I'd say we should just change it, callers with no condition can call ObjectDisposedException.ThrowIf(true, this), and when we feel that there are enough of them we could add Throw back.

Or we could just do it as in-addition, and make ThrowIf be

[StackTraceHidden]
public static void ThrowIf([DoesNotReturnIf(true)] bool condition, Type type)
{
    if (condition)
    {
        Throw(type);
    }
}

Do you want to bounce it back for review again, @stephentoub ?

[Wraith2]

Converting to ObjectDisposedException.ThrowIf(condition, ...); given the evidence of usage seems reasonable, gets my vote.

[terrajobst]

Video

• Makese sense. It seems we should establish as a general pattern for throw helpers.

namespace System
{
    public partial class ObjectDisposedException
    {
        public static void ThrowIf([DoesNotReturnIf(true)] bool condition, object instance);
        public static void ThrowIf([DoesNotReturnIf(true)] bool condition, Type type);
    }
}