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

[Proposal]: Support readonly modifier for classes and records #3885

Open
1 of 4 tasks
HaloFour opened this issue Sep 12, 2020 · 25 comments
Open
1 of 4 tasks

[Proposal]: Support readonly modifier for classes and records #3885

HaloFour opened this issue Sep 12, 2020 · 25 comments

Comments

@HaloFour
Copy link
Contributor

HaloFour commented Sep 12, 2020

Readonly Modifier for Classes and Records

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
  • Specification: Not Started

Summary

Support applying the readonly modifier to classes and records. This keyword will cause the compiler to enforce that all fields declared in the type are also readonly and result in a compiler error if any field is not readonly or any auto-property has a set accessor.

Motivation

There seems to be a lot of confusion around the records feature regarding "immutability" as it is being designed for C# 9.0. This is exacerbated by the fact that the docs very clearly state that record types make it easier to declare "immutable" types and that they are "immutable" by default, neither of which is really true. There has been a bit of surprise expressed in a number of issues/discussions on this repo as well as on Gitter and Discord about the fact that auto-properties with setters are legal. In my opinion there would be a lot of value in being able to state that a record type is in fact "immutable" at least to the extent that the fields cannot be reassigned.

Further motivation is that it is likely that C# 10.0 will gain "struct records". There is already support for readonly struct in the C# language and I expect that a readonly struct can also be a record by a combination of the modifiers. As such I think it makes sense to allow the readonly modifier to be applied universally to types with the same compiler enforcement.

Detailed design

The readonly modifier can be applied to a type of class or record. The modifier can be applied in any other with other modifiers. The modifier by itself does nothing. However, when applied the modifier will enforce that the type can only have readonly fields and either readonly or init-only auto-properties:

public readonly class Foo {
    private readonly int x; // fine
    private int y; // error CS8340: Instance fields of readonly types must be readonly.

    public int X { get; } // fine
    public int Y { get; init; } // fine
    public int Z { get; set; } // error CS8341: Auto-implemented instance properties in readonly types must be readonly.
}

public readonly record Bar {
    private readonly int x; // fine
    private int y; // error CS8340: Instance fields of readonly types must be readonly.

    public int X { get; } // fine
    public int Y { get; init; } // fine
    public int Z { get; set; } // error CS8341: Auto-implemented instance properties in readonly types must be readonly.
}

public readonly struct record Baz {
    private readonly int x; // fine
    private int y; // error CS8340: Instance fields of readonly types must be readonly.

    public int X { get; } // fine
    public int Y { get; init; } // fine
    public int Z { get; set; } // error CS8341: Auto-implemented instance properties in readonly types must be readonly.
}

Drawbacks

This feature could be confused with immutable types which aim to enforce deep immutability but is much stricter in that every field must also be of an immutable type.

Alternatives

This could be supported today by an analyzer. In my opinion there is value in supporting this as a first-class citizen, especially since readonly struct already exists and I expect that readonly struct record will be added in C# 10.0

Unresolved questions

  1. Can a readonly class or record inherit from a non-readonly class or record?

Design meetings

https://github.com/dotnet/csharplang/blob/master/meetings/2020/LDM-2020-10-26.md#readonly-classes-and-records
https://github.com/dotnet/csharplang/blob/main/meetings/2022/LDM-2022-09-28.md#ungrouped

@jnm2
Copy link
Contributor

jnm2 commented Sep 12, 2020

To add to the motivation (I've wanted readonly class since before readonly struct was shipped): Having it built in makes it more widespread. Where it shines is when you see it at the top of the file and instantly know something about the whole class. The chances of this becoming widespread if it's based on an attribute + analyzer are very low. The chances are good if it's built in and available everywhere because of factors like ReSharper (or people reviewing your code) suggesting it. Both of these things are already happening today with readonly struct.

Can a readonly class or record inherit from a non-readonly class or record?

I think the answer has to be no. The real usefulness of readonly is when it tells you about the whole object state, including state declared by the base. If a readonly class is permitted to inherit from a non-readonly class that happens to have no mutable state at the time of compilation, it then becomes a breaking change for a non-readonly nonsealed class to add a mutable field. In contrast, removing a readonly modifier from a struct is already known to be a breaking change. It would make sense to extend this to classes too and require base classes to be marked readonly in order for a derived class to be marked readonly.

@jaredpar
Copy link
Member

This proposal essentially adds shallow immutability to class types. That is a readonly class cannot be directly mutated after construction but is not deeply immutable because it can legally mutate data held within it.

I've considered a feature like this a few times and I remain a bit skeptical as to whether it's worth the added complexity to the language. The guarantee here is mostly beneficial to the type author(s) because it's a declarative reminder that they wanted to avoid direct mutations in this type. The guarantee doesn't have much benefit to the type consumers though because it does not mean the type is free of mutations. The type can freely mutate via nested data hence knowing a type is readonly class vs class doesn't change the consumption of the type in a meaningful way.

Compare this to readonly struct where there are tangible benefits to the type consumer. The compiler can elide many struct copy operations when it knows it's dealing with a readonly struct or a readonly member on a struct. That means even though the type can't make any strong guarantees about mutability it's still providing a benefit to the consumer.

Couple of other areas to consider or possibly explain why you discarded them

  • Why not extend to readonly interface? Thought about this for a bit and because readonly on reference types doesn't provide much benefit to the consumer this seems to not really add any value.
  • Why not allow readonly on normal class members similar to how readonly can be applied to struct members.

@333fred
Copy link
Member

333fred commented Sep 12, 2020

I'm not as skeptical as Jared here. I'll champion this.

@HaloFour
Copy link
Contributor Author

@jaredpar

I agree with your skepticism, this proposal would not provide the degree of benefit that readonly struct would, especially since there are no guarantees of deep immutability nor are there added benefits to consuming such a type. This is, at best, guardrails for the developer. But I do think that there is value to this proposal, and I do think that there is (and will be) a strong desire to pair this feature with record.

Why not extend to readonly interface?

This has come up before. IMO, an interface declares what a type can do, not what it can't. I've always argued that interfaces named IReadOnly don't make a lot of sense. The interface may only provide read members but doesn't dictate the limit of the capabilities of the underlying class regarding mutability or anything else.

Why not allow readonly on normal class members similar to how readonly can be applied to struct members.

I wouldn't be opposed to readonly members, but I personally don't think they would be as useful as readonly types. At best they would be guardrails that a particular instance member cannot reassign the mutable fields of the class, right?

@IS4Code
Copy link

IS4Code commented Sep 15, 2020

This is a bad idea for classes, at least when it comes to using the readonly keyword.

readonly on structs is important to the user of the struct, and not the creator, because it allows the user to call methods on its values without copying when the variables are readonly/in. That is the reason readonly on structs, and by extension on their methods, exists in the first place – it empowers the user.

readonly on classes, as proposed here, doesn't really empower anyone. To the user of the class, there is absolutely no difference between a normal class and a readonly class, since a readonly class doesn't and cannot guarantee immutability. As long as you can store references to mutable classes in it or use the reference as a key to ConditionalWeakTable, you can, technically, "mutate" it even with readonly anywhere.

A keyword just to force you to add readonly to fields? We all know this is a task better suited for a code analyzer.

You could argue static on classes also forces you to have all members static, but it also matters to the user – you cannot create a variable of its type or use it as a generic argument; it is really a distinct "kind" of a type. You needn't add static to members because static enforces that, you need to because it is a whole different thing by its nature.

In a sense, all classes (and their methods) are implicitly readonly. Calling a readonly method on a struct means that the bytes of the variable where it is located will not be modified. Calling any method on a class already guarantees that – it is a reference and you cannot assign to this!


Why not extend to readonly interface?

This has come up before. IMO, an interface declares what a type can do, not what it can't. I've always argued that interfaces named IReadOnly don't make a lot of sense. The interface may only provide read members but doesn't dictate the limit of the capabilities of the underlying class regarding mutability or anything else.

This is a bit misleading. IReadOnly… is named so because the interface provides read-only access, it doesn't tell anything about the mutability of the type. You can mark a file on your drive read-only and then smash it with a hammer, that's what "read-only" means. And unless C# gets readonly object references in the future, we will probably never have true immutability encoded in the language ever.

The same for saying "interfaces don't declare what a type cannot do":

public interface IMyCollection
{
    int Count { readonly get; }
}

Am I really the only one who thinks "I can provide you my Count without modifying the value you used." isn't completely unreasonable? This is something that is actually needed.

And in other languages, interfaces absolutely can declare what a type "cannot" do:

public interface java.io.Closeable {
  void close() throws IOException;
}

public interface java.lang.AutoCloseable {
  void close() throws Exception;
}

The same for noexcept in C++.


To sum things up, readonly on classes isn't needed since they have always been readonly, in a sense. It is needed on interfaces and their members. Can't say for certain for records, but I am inclined to believe it is the same as for classes.

@IS4Code
Copy link

IS4Code commented Sep 15, 2020

By the way, I think this technically *could* be a non-issue in the future:

public readonly struct S
{
    public int Value; // notice the lack of readonly
    public override string ToString() => Value.ToString();
}

readonly on the struct is there so that the consumer knows that its instance API doesn't modify its bytes. But you always know whether that happens when you access a field! The field itself could be modifiable from external code, just not from methods (which are implicitly readonly). I am not proposing this, since it is too far-fetched, but such a change doesn't break anything. Mutability is the concern of the essence, not the form.

Just something to bear in mind.

@HaloFour
Copy link
Contributor Author

@IllidanS4

readonly on classes, as proposed here, doesn't really empower anyone.

It empowers the author of the class by enforcing their intent. By that same token readonly reference fields are just as pointless.

since a readonly class doesn't and cannot guarantee immutability.

Nor does it try, which is why I'm not suggesting that keyword here.

or use the reference as a key to ConditionalWeakTable, you can, technically, "mutate" it even with readonly anywhere.

If that's the bar that needs to be solved then a solution will simply never exist in this language/runtime.

A keyword just to force you to add readonly to fields? We all know this is a task better suited for a code analyzer.

And something that enough people want to do that I feel it is worth being a first-class citizen.

Am I really the only one who thinks "I can provide you my Count without modifying the value you used." isn't completely unreasonable?

The interface says, "I can provide you my Count," and that's all. How it provides that value isn't your concern, and if the collection uses lazy evaluation then it can (and should) mutate it's internal state.

And in other languages, interfaces absolutely can declare what a type "cannot" do:

Except that they can't. From Java you can choose to not throw an Exception, or you can throw any runtime exception. From every other JVM language you can throw whatever you want because none of them care about checked exceptions and the runtime can't provide any guarantees.

If this proposal pushes conversation and effort on #2543 then I'll be happy, but I'd rather some solution now than a better solution never.

@IS4Code
Copy link

IS4Code commented Sep 15, 2020

And in other languages, interfaces absolutely can declare what a type "cannot" do:

Except that they can't. From Java you can choose to not throw an Exception, or you can throw any runtime exception. From every other JVM language you can throw whatever you want because none of them care about checked exceptions and the runtime can't provide any guarantees.

So you can implement Closeable, but the interfaces tells you how. The interface says "You can close this, but only throw exceptions of this type." Of course you can break this in the JVM, but within the world of Java, you cannot choose to throw something like ParseException. You could say that an exception is actually part of the return type, but changes to this are as well, from the perspective of functional programming.

Am I really the only one who thinks "I can provide you my Count without modifying the value you used." isn't completely unreasonable?

The interface says, "I can provide you my Count," and that's all. How it provides that value isn't your concern, and if the collection uses lazy evaluation then it can (and should) mutate it's internal state.

I am not arguing about mutability here, only about the changes to the value. That is only limited to values of fields of structs. A struct can implement IMyCollection only with readonly Count. A class can implement it without restrictions.

By that same token readonly reference fields are just as pointless.

Well nobody had to make a proposal for "readonly private class fields" since that came automatically with readonly fields. And I suppose even readonly private class fields could make a difference for optimisation, serialization and reflection.

Even readonly structs are less useful in retrospect now that there are readonly methods, so they only help you when you don't want to declare all methods readonly. But methods of classes are implicitly readonly, so it doesn't help there anyway.

@HaloFour
Copy link
Contributor Author

@IllidanS4

This proposal does not preclude any of those other proposals. If you think it's analyzer territory you're welcome to your opinion. I do explicitly mention that above. But I stand by my position that a common request should be a first class citizen. If the conversation yields something more than this proposal then wonderful. I'd be happy if immutable is added to the language instead of readonly, but it's been more than five years and there hasn't been any movement on the subject.

@DavidArno
Copy link

@jaredpar,

This proposal essentially adds shallow immutability to class types. That is a readonly class cannot be directly mutated after construction but is not deeply immutable because it can legally mutate data held within it. ... Compare this to readonly struct where there are tangible benefits to the type consumer. The compiler can elide many struct copy operations when it knows it's dealing with a readonly struct or a readonly member on a struct. ...

Whilst you are correct that read-only structs offer performance benefits, it would be wrong to suggest that is the only benefit to them. For those of us less worried about micro-managing performance and more interested in our code communicating ideas, such performance benefits are effectively an implementation detail of the feature. What's really important is that that keyword offers a read-only contract to the user of the type. It is directly comparable to read-only fields: they only offer "shallow immutability" too. yet people mark fields as read-only as it communicates intent, even if it cannot enforce true immutability.

Read-only classes offer the same communication of intent. They do not guarantee immutability, just as read-only fields and structs do not guarantee immutability. It does though communicate an intent to the the user. And for many developers, that is a significant benefit, whether tangible or not.

@SirIntruder
Copy link

I've considered a feature like this a few times and I remain a bit skeptical as to whether it's worth the added complexity to the language.

2cents: the fact that structs can be 'readonly' and have 'readonly' members, while the classes can't, already adds a bit of complexity for the user. I did find myself performing "remove readonly from all the methods" dance a couple of times.

@iSazonov
Copy link

iSazonov commented Nov 8, 2020

Triaged into the Working Set. We'll look at this with low priority, and particularly try to see what the scenarios around hierarchical enforcement look like, as those were more generally palatable to LDT members.

I believe it should be high priority because it blocks important scenarios and improvements.
I have several stories, let me start... And I hope this helps @333fred to get a progress.


I am actively working on developing of PowerShell and one scnenario we have a persistent concern is a startup scenario.

Historically PowerShell loaded some XML resource files at startup time and converted its in internal structures. It is slow, very slow. And we implemented an improvement - we converted the resource file to C# code at design time. We get a great speedup of the startup scenario. (See PowerShell/PowerShell#10898 for details)
But we could get more and the proposal is a key in.

But let's start with more simpler example of code pattern that's used dozens of times in PowerShell code.

private static readonly char[] s_pathSeparators = new char[] { '/', '\\' };

Design intention is obviously it is immutable - elements are never changed and never added or removed.
Gaps:

  • C# can not define it as immutable.
  • Initialization of the field does an extra operation - create the array in heap and copy elements to it. It can slow down startup scenario too.

Today Roslyn does the optimization (remove extra copy) for follow:

private static ReadOnlySpan<byte> AsciiCharInfo => new byte[] { ... };

See https://vcsjones.dev/2019/02/01/csharp-readonly-span-bytes-static/ for details.
The optimization uses an immutable nature of the ReadOnlySpan.

But why don't we generalize this to char? Other primitive types? (I understand that there is cross-architecture complexity in implementation (and this is not discussed here) but it could be in R2R.)

Next step would be to generalize this to all struct types.
Notice, we still say about mutable array elements.

Next step, if we take a struct and replace struct keyword with class we can apply the optimization to the simple class while we keep struct limitations (like lost inheritance and so on).

Here we see that optimizations specific to structures only can be applied to simple classes and this can be very useful for real applications - see my example above about PowerShell XML resource files - yes, they could be converted to simple classes and yes, this could speed up PowerShell startup scenario.

Let's take a step back to the example because we are talking about immutability:

private static readonly char[] s_pathSeparators = new char[] { '/', '\\' };

We need the array being immutable and I can think about:

private static readonly char[] s_pathSeparators = new readonly char[] { '/', '\\' };
// or the same in short form:
private static immutable char[] s_pathSeparators = new char[] { '/', '\\' };

Is it possible to implement? I think yes - if today we can set readonly to all struct members and get fully (I remeber about auto-implemented properties and hope they will be fixed) immutable structs why can't we set readonly to class members? It can be difficult in common but as we have saw above for simple classes we can.
To be specific we are talking about Array class where T is a value type.
Can we distinguish between modifying operations for structures? Yes, we can.
Can we distinguish between modifying operations for value types? Yes, we can.
And now - can we generalize and distinguish between modifying operations for simple classes like Array? Yes, we can.

So we can implement s_pathSeparators as immutable.
I would call it effictive immutability - Array is not immutable but we can force its immutability.

Let's return to startup scenario. Converting XML to code obviously requires links between class instances.
Can we enhance the simple classes and allow such references while keeping immutability?
Based on the above considerations, I suppose yes because immutability can be defined recursively by transitivity - a (simple) type is immutable if all members is immutable and is defined on immutable (simple) types.

Yes, this is a special case, but my goal is to show that it is useful in real-world scenarios both to improve performance and to improve the facilities of the language itself.

The last thing I would like to note is that the implementation of this proposal makes "copy-on-write" optimization possible.
Let's again see the code:

private static readonly char[] s_pathSeparators = new char[] { '/', '\\' };

Now let's consider it as mutable.
With first optimization we would not copy the data to heap like in ReadOnlySpan case.
Then if we can distinguish between mutable and immutable operations, we could defer the creation of the copy on the heap until the first mutable operation.
For PowerShell this could be huge OOB performance optimization.
If we would refactored PowerShell classes to befefit from the "we can distinguish between mutable and immutable operations" we did get another huge performance optimization on application level because PowerShell makes extensive use of cloning of internal structures although these clones are not modified in most cases.
I can't argue that this is exactly the case for complex structures, but at least the implementation of this proposal for classes will open the way for research in this area.

@svick
Copy link
Contributor

svick commented Nov 8, 2020

@iSazonov This proposal is about a single quite simple thing: adding the readonly modified to a class. I don't see how just that on its own is going to help with any of the things you're talking about. And specifically, I don't see how it would affect performance at all.

What you're talking about sounds useful (though I have my doubts on whether it's feasible), but it's quite a lot bigger than this proposal.

@IS4Code
Copy link

IS4Code commented Nov 8, 2020

I really hope this at least gets considered together with #3055, which does actually bring performance benefits when you care about code principles.

@CyrusNajmabadi
Copy link
Member

I really hope this at least gets considered together with #3055, which does actually bring performance benefits when you care about code principles.

This proposal is solely about a shorthand way of stating your intent that all members of a class need to be readonly.

@ericsampson
Copy link

@jaredpar wrote:This proposal essentially adds shallow immutability to class types. That is a readonly class cannot be directly mutated after construction but is not deeply immutable because it can legally mutate data held within it.

Is there a parallel plan to keep working towards something like #2543 ? Being able to specify deep immutability seems like much greater value than shallow immutability discussed in this proposal, especially in light of the introduction of records.

@theunrepentantgeek
Copy link

Consumers can't tell the difference whether a class is deeply immutable or whether it's just shallow - in both cases they have something that won't change state that can be safely shared.

Shallow immutability gives me a lot more power as a type author - for example, I can defer an expensive computation for GetHashCode() until it's needed. With enforced deep immutability, I'd be forced to either compute it in the constructor (wasteful if it's never needed) or compute it every time (wasteful if needed multiple times).

In my experience, languages that have enforcement of deep immutability tend to end up with multiple parallel type systems (a mutable list and an immutable list, a mutable string and an immutable string, a mutable vector and an immutable vector, a mutable set and an immutable set, and on and on and on) resulting in a lot of needless marshalling between otherwise equivalent types.

@IS4Code
Copy link

IS4Code commented Nov 22, 2020

@theunrepentantgeek That is a potential danger, but it is far from absolute. C++ has avoided it, for example, although only by having const as a part of a type reference.

@AartBluestoke
Copy link
Contributor

@IllidanS4 c++ const-ness is just a strong suggestion. Many complex 'const' classes end up having volatile members, or const cast away immutability as needed. (Some may argue those are indicators of poor design by specific standards, but that is a different discussion, as that is how that language is used. )

@isaacabraham
Copy link

isaacabraham commented Dec 28, 2020

I suspect that people may find "top-level" immutability sufficient. I think a common pattern will be people going either "all in" on immutable records for object graphs, or barely at all.

In the former case, top level immutability will be fine because every field will either be a primitive type or another record.

I suspect that an analyser would then be sufficient to warn if you have an read-only record that contains a field that is eg a mutable class.

Perhaps I'm totally wrong, but I'd be surprised to find mixing immutable records and mutable classes a common use case.

@joshfisk
Copy link

I don't like this proposal because it uses up the readonly keyword for something that could be done with an attribute. Keywords are valuable, and this would kill the potential of a readonly class being something more significant, and useful in the future.

@jnm2
Copy link
Contributor

jnm2 commented Jan 19, 2021

@joshfisk It seems like it would not be ideal for the keyword to differ in meaning between classes and structs, so we should be able to already know the semantics and go ahead and add it now if ever. An attribute would look particularly odd and provoke people to wonder why things have to be the way they are.

@jnm2
Copy link
Contributor

jnm2 commented Jan 19, 2021

@isaacabraham I have a couple instances of trees of immutable state that have references to mutable classes (treated opaquely) by necessity, deep in the tree. I would not be able to use deep immutability for parts of the tree above this, but I would not stop wanting to make each part a single-level readonly class.

@joshfisk
Copy link

joshfisk commented Jan 20, 2021

@jnm2 Yeah, it should match what struct is doing. It makes it easier to learn, and reduces a step if you need to convert between a class, and a struct. I still feel like adding a keyword to the type should change the way the type works, perhaps instead of adding error messages, and making you add readonly to every field, it could implicitly add it to all the fields.

@jnm2
Copy link
Contributor

jnm2 commented Jan 22, 2021

I still feel like adding a keyword to the type should change the way the type works, perhaps instead of adding error messages, and making you add readonly to every field, it could implicitly add it to all the fields.

This would deviate from how it works with structs. (Also from how the static keyword works.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests