Proposal: Allow setting readonly fields to null in Dispose

[JohanLarsson]

public class Foo : IDisposable
{
    private readonly IDisposable disposable;

    public Foo()
    {
        this.disposable = new Disposable();
    }

    public void Dispose()
    {
        this.disposable?.Dispose();
        this.disposable = null;
    }
}

[HaloFour]

The CLR forbids assigning readonly/initonly fields anywhere outside of the constructor. The CLR has no special knowledge of IDisposable or the Dispose method. As a language C# couldn't do this without modifications to the CLR.

[jnm2]

You could do some wacky things to protected readonly fields by inheriting and implementing IDisposable.

[DavidArno]

If you want to modify a field, then don't make it readonly. Disposable items are inherently mutable, so I'd consider:

private readonly IDisposable disposable;

as a code smell and worthy of a compiler/analyzer warning that IDisposable fields should not be marked read-only.

Get rid of that readonly and your Dispose code will work just fine.

[JohanLarsson]

@DavidArno of course we can get rid of the readonly and make the code compile, problem is that it makes the code less clear, readonly is nice as it makes it obvious that there is no mutation going on.
The dispose method is special though as it is normally the last time an instance is touched from the outside. After the instance is disposed touching it should normally throw an ObjectDisposedException. Yes there are exceptions like Task<T>.

[bondsbw]

If you want to prevent use of disposable after calling Dispose(), then (with this proposal) you would need null checks for each use of the field.

But is that better than checking a flag called IsDisposed? That would communicate purpose better.

[jnm2]

@DavidArno Thoroughly disagree that readonly references equate to immutable graphs. Whether or not you specify readonly is independent of whether the object it is referencing at any given time can mutate.

[JohanLarsson]

@bondsbw agreed a flag is clearer.

Most of the time I don't set things to null in the dispose method so readonly is not an issue, was still curious about what others think so I posted this issue.

[yaakov-h]

I usually don't bother nulling out the IDisposable object either, since the documentation for IDisposable.Dispose() states:

If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times.

I don't think I've yet encountered an object that visibly violates this.

@JohanLarsson readonly makes it nice as it makes it obvious (and enforces) that there is no mutation going on, however by nulling out the reference there is mutation going on. You can't really have it both ways.

[JohanLarsson]

@yaakov-h yeah it would be mutation but it would be restricted to ctor & dispose.

[yaakov-h]

that's not readonly then, that's readonlyexceptonespecialmethod.

[DavidArno]

@HaloFour,

I'm confused by your assertion regarding the CLR and read-only fields. Does reflection use a weird loophole? That latter approach can be used to modify read-only fields:

using static System.Console;
using static System.Reflection.BindingFlags;

namespace MutableReadonly
{
    internal class Program
    {
        private readonly string _foo = "foo";

        private static void Main()
        {
            var x = new Program();
            WriteLine(x._foo); // prints foo
            typeof(Program).GetField("_foo", Instance|NonPublic).SetValue(x, "bar");
            WriteLine(x._foo); // prints bar
        }
    }
}

@jnm2,

To be clear, I'm not claiming that readonly makes an instance immutable. I'm unclear whether you thought I was saying that, though.

To my mind, the FxCop violation, CA2104: Do not declare read only mutable reference types hits the nail on the head. It's a shame in my view that this check was buried away in a rarely used tool, rather than being a compiler warning, but it is what it is.

If I see readonly, I want to be able to assume that the thing in question really is read-only, and ideally that it is immutable too. Therefore, in my view, the following code is a misuse of readonly:

private readonly SomeType _field;

...

_field.X = 1;

I'd hesitate to call in an anti-pattern, but I definitely view it as poorly written code. You are of course entitled to disagree as that is just my opinion.

[JohanLarsson]

@DavidArno here is a bit of trivia for you:

struct Foo
{
    public readonly int Value;

    public Foo(int value)
    {
        this.Value = value;
    }

    public void Update(int newValue)
    {
        this = new Foo(newValue);
    }
}

[DavidArno]

@JohanLarsson,

😱😱😱😱😱

Why is that even possible? That's not an anti-pattern; that's a war crime! 😭

[JohanLarsson]

@yaakov-h a pedant would say *twospexialmethods (ctor & dispose). Ctor is different though as the mutation is not observable from the outside.

[jnm2]

@DavidArno I'm surprised that such a central tenet of C# is considered poor practice. It's still valuable to prevent mutation of the reference even when the target mutates. It's so common in OOP where the central tenet is that every object is a black box and it should be irrelevant whether it has internal state.

[GeirGrusom]

They come from the "immutable by default" world of FP.

They come from math. It's an ordered, finite array of values. That's it. It makes no statement about mutability.

Mutability is an advantage for C# because deconstruction can turn into a single stloc instruction. It's a clear win.

[jnm2]

The core principle of tuples is to be an analog of an argument list, to the extent possible.
Parameters are mutable; therefore, the simplest expectation would be that tuple fields are mutable as well.

[CyrusNajmabadi]

"How is it expensive to pass immutable classes around?"

I didn't say it was expensive to pass them around. I said "But they're extremely expensive. Especially when code wants to actually change values. "

Each immutable instance requires an allocation. Allocations are not free, and we've learned painfully over the past decade exactly how non-free they are. In a world where tuples are immutable classes, then every element change is also an allocation.

Right now Roslyn itself can't use several C# features in certain areas because these costs are too high. I'm spending the majority of my engineering efforts on addressing this in the IDE because allocation cost, and hte impact to the user, is the single largest perf issue we have.

[CyrusNajmabadi]

"But they're terrible for passing around. now you can't safely give you value to someone else" makes no sense. The whole reason for making them mutable is to allow them to be mutated. If you don't want them to be mutated, then don't make them mutable.

That's why i said it was a con. Because now that you have mutable classes, you can't safely share them. Someone might accidently mutate the tuple and break your copy. This is not an issue with the struct-form.

Again how are they expensive to pass around?

I did not say they are expensive ot pass around. I said they are expensive just as immutable classes are. See my previous post on this. Allocs and GC are extremely detrimental to performance in many domains. As people want things to run well on everything from Mobile to the Datacenter (where inefficiency translates to wasted money), we don't want to impose unnecessary costs on people.

[CyrusNajmabadi]

No, they are poor for passing around . That's why you added ref returns to the language: because coping structs is expensive.

This is the case when structs are quite large. The expected usage pattern in practice for tuples is usually <4 elements. In that case structs are fantastic for passing around.

And if you are using a very large struct, then yes you can use ref. value-structs allow for fast perf for the overwhleming common case, as well as fast perf for the uncommon case. Importantly, they are safe to share around.

[CyrusNajmabadi]

Sure. As Mads says in Proposal: Language support for Tuples:
In their very motivation, though, tuples are ephemeral. You would use them when the parts are more important than the whole. So the common pattern would be to construct, return and immediately deconstruct them. In this situation structs are clearly preferable.

Nothing about that states that they should be immutable.

They come from the "immutable by default" world of FP.

They really don't. Tuples predate FP significantly.

You can call my claim that ""make 'em all mutable; it's efficient you know" is a strawman, but I disagree. It's an accurate representation of what you did with tuples: you put speed before "common pattern" good design.

That was not your claim. Your claim was:

"I just think it sad that the team have shifted the official guidance from "structs should be immutable unless really necessary" to "make 'em all mutable; it's efficient you know"."

We have not shifted our guidance on structs. Our guidance on structs is that generally being mutable is probably not what you want. However, in some domains it's exactly what you want. And when that is the case, you should absolutely use them as they're great for this purpose.

[HaloFour]

@CyrusNajmabadi @DavidArno

My karma ran over your dogma. 🐕 🚗

Wait, what was this proposal about again?

[CyrusNajmabadi]

No, there isn't really any argument over what "short-lived" means for tuples. Mads defined it well

I think you're mixing up concepts here. Take Roslyn as a good example. The Roslyn API embraces immutability heavily. Huge swaths of our API give you a surface area that is observably immutable and is safe to use a in massively threaded scenarios.

However, internally, mutability is embraced heavily to be a means to present this immutable facade. It's the basis of incremental, lazy computation. Something which came heavily from the FP world.

We achieve a scalable immutable API precisely by embracing mutability and using it effectively to provide something actually efficient.

[PathogenDavid]

HaloFour: The CLR forbids assigning readonly/initonly fields anywhere outside of the constructor.

DavidArno: I'm confused by your assertion regarding the CLR and read-only fields. Does reflection use a weird loophole?

@DavidArno

It's maybe worth noting that the runtime will happily JIT code for non-ctor methods which modify readonly fields. Unsurprisingly however, it does not produce a verifiable assembly. Basically it works, it just isn't kosher.

Here's a proof of concept I made to test this a while back: ForceWriteReadonly. I can upload the full solution if someone wants to build this atrocity for some reason. (This also includes an example for calling a constructor as if it were a normal method, which somehow feels even more evil.)

I imagine that producing verifiable assemblies is basically mandatory for Roslyn, so without CLR changes this proposal is a non-starter. IMO, CLR should not be aware of the IDisposable pattern, so I wouldn't think it should change either.

---

My primary motivation for wanting a feature similar to this is that we null out IDisposable fields so that accidental access of disposed fields causes the offending code to die in a fiery NullReferenceException. However, as pointed out earlier in this thread, you should be getting an ObjectDisposedException before that happens, so it really only prevents cases where you aren't checking if the object has been disposed.

As shown above, you could use an evil Fody weaver like I've shared to accomplish this feature if you really want it. However, you're better off using something like Fody.Janitor to add automatic throwing of ObjectDisposedException instead.

TL;DR: You can do this if you really want to using evil IL manipulation, but you probably don't actually need this.

[GeirGrusom]

Couldn't code that alters readonly values in principle cause a segfault if the CLR for some reason would place the value in read-only memory? The CLR does already make assumptions that readonly values never change.

Otherwise it's just as @DavidArno says: if you want to change it's value it shouldn't be readonly. What's the point of marking something readonly when it in practice isn't.

[PathogenDavid]

CLR for some reason would place the value in read-only memory

I don't picture a situation where CLR could/would do that. It'd require allocating a whole page of memory (usually 4 KB on Windows) for readonly fields. If it wanted to reuse that 4 KB page (because few objects will have 4 KB of readonly fields) then it'd also mean changing the protection of that page when a constructor is run, and changing it back when it is finished.

What's the point of marking something readonly when it in practice isn't.

I'm not really arguing in favor of this proposal, but to play devil's advocate: You might want to change readonly fields in the Dispose method because the fields should never be read ever again, so you make them an invalid value in case someone does read them by mistake. Essentially readonly becomes "readonly during the valid lifetime of the object" instead of "readonly after construction".

[GeirGrusom]

I don't picture a situation where CLR could/would do that.

It's an implementation detail. The point is more that the CLR assumes that initonly doesn't change value after creation, and can do whatever it likes to protect that (or do nothing at all). It could potentially pool objects it finds where all fields are readonly, or it may not.

I wouldn't bet anything on all implementations of the CLR allowing code that writes to readonly fields to execute in the future.

[jnm2]

@PathogenDavid

Essentially readonly becomes "readonly during the valid lifetime of the object" instead of "readonly after construction".

Except that from the point of view of the runtime, the object's valid lifetime is from construction to finalization and IDisposable is irrelevant.

[PathogenDavid]

I wouldn't bet anything on all implementations of the CLR allowing code that writes to readonly fields to execute in the future.

For sure. I presented my Fody weaver for the sake of trivia for the two of the two bigger implementations of CLR as that exist today. It's super evil and you should never do it, I did not mean to imply otherwise.

Except that from the point of view of the runtime, the object's valid lifetime is from construction to finalization and IDisposable is irrelevant.

Correct, but IDisposable is the mechanism for the developer to explicitly say when an object's valid lifetime ends as far as the developer is concerned, which is what I was referring to. This proposal (which I am not in favor of, BTW) would seek to make CLR vaguely ware of IDisposable if it were implemented (which it probably won't/shouldn't be.)

[jnm2]

I'm also not in favor of moving that direction. #1451 (comment)