[Proposal]: `readonly` parameters

Question

[Proposal]: `readonly` parameters

333fred opened this issue 3 months ago · 17 comments

Summary

We allow parameters to be marked with readonly. This disallows them from being assigned to or being passed by ref or out.

Motivation

C# users have long requested the ability to mark both locals and parameters as readonly. The design team has somewhat resisted this for two reasons:

The view that readonly on parameters and locals would be an attractive nuisance more than a helpful addition.
Indecision on what a succinct syntax for locals would be to minimize the nuisance part of the first objection.

However, the addition of primary constructor parameters changes that calculus, at least for parameters. A significant piece of feedback from the initial preview is that users would like to be able to ensure that primary constructor parameters are not modified. The scope of such parameters is much larger, so the danger of accidental modification is much higher. We therefore propose allowing readonly as a parameter modifier.

This proposal makes a few assumptions about readonly locals as part of it:

A future design would allow readonly as a local modifier.
That future design might allow a shorthand for readonly var, or may say that readonly var is not allowed, and the separate shorthand is required for a readonly type-inferred local. What that shorthand
is (val, let, const, or some other keyword) is beyond the scope of this proposal.

These assumptions allow us to presume that to fully spell out readonlyness for a parameter or local requires a modifier, and that modifier is readonly. In places where types can be inferred, we offer a
shorthand that combines the meanings, but otherwise readonly is required.

Specification

https://github.com/dotnet/csharplang/blob/main/proposals/readonly-parameters.md

LDM Discussions

Answer 1 · 2024-10-03T20:38:53.000Z

I always viewed this proposal as a stepping stone into a potential wider application of readonly being applied to parameters and locals. However, my thinking on that subject has changed and now I question whether readonly parameters in general would be a useful feature.

That said, being able to apply readonly to primary constructor parameters specifically I think has real potential value, and perhaps could be considered as a wider proposal to allow certain modifiers on those parameters to "promote" them to type members.

There is a lot of overlap between the two, although one could argue that the scope during which the primary constructor parameter is reassignable is different depending on whether you treat it as a parameter vs. as a field.

Answer 2 · 2024-10-03T21:06:43.000Z

Would it make sense to limit this to only primary constructor parameters?

I'm slightly worried this could become out of hand for "normal" methods... for example, we probably want to have all constructor parameters always enforce this, but if we enforce it for every method it will become super noisy and verbose.

Makes me wish all parameters were readonly by default from the beginning of the language, since that seems much better as a default than the other way around.

Answer 3 · 2024-10-03T21:17:42.000Z

Would it make sense to limit this to only primary constructor parameters?

Yes, that is one of the points that the LDM has been actively debating. I don't recall that we've for certain said one way or the other, but my impression of our leaning is towards just PC parameters.

Answer 4 · 2024-10-04T09:53:27.000Z

That said, being able to apply readonly to primary constructor parameters specifically I think has real potential value, and perhaps could be considered as a wider proposal to allow certain modifiers on those parameters to "promote" them to type members.

My position on this has changed hugely over the last year. A year ago, I'd be clamouring for the ability to mark PC parameters as readonly. But as an experiment, we introduced a mandatory analyzer to our code base that treats mutating a PC parameter as an error. This provided the perfect solution as we have never found a compelling use-case that would justify allowing them to mutate. Mutating PC parameters turned out to be a YAGNI feature with obvious undesirable consequences that in reality were very easily to overcome. I do not see any point in swapping that clean solution for suddenly allowing them to mutate by default and having to add readonly to stop it.

Therefore, as far as I'm concerned, the solution here is to add a warning-wave warning on parameter mutation to a future release that embeds that analyzer into the language. Job done: this issue can then be closed.

Answer 5 · 2024-10-04T10:39:17.000Z

My position on this has changed hugely over the last year. A year ago, I'd be clamouring for the ability to mark PC parameters as readonly

As someone who has here then; can confirm - he was! 😂

I agree wholeheartedly though. I was less hesitant to try mutable PCs than David were, but even so I still wanted readonly back then. I'm much less convinced nowadays that these 9 additional characters would improve the DX

Answer 6 · 2024-10-04T12:47:08.000Z

That said, being able to apply readonly to primary constructor parameters specifically I think has real potential value, and perhaps could be considered as a wider proposal to allow certain modifiers on those parameters to "promote" them to type members.

My position on this has changed hugely over the last year. A year ago, I'd be clamouring for the ability to mark PC parameters as readonly. But as an experiment, we introduced a mandatory analyzer to our code base that treats mutating a PC parameter as an error. This provided the perfect solution as we have never found a compelling use-case that would justify allowing them to mutate. Mutating PC parameters turned out to be a YAGNI feature with obvious undesirable consequences that in reality were very easily to overcome. I do not see any point in swapping that clean solution for suddenly allowing them to mutate by default and having to add readonly to stop it.

Therefore, as far as I'm concerned, the solution here is to add a warning-wave warning on parameter mutation to a future release that embeds that analyzer into the language. Job done: this issue can then be closed.

@DavidArno I see where you are coming from here, but at the same time I think doing it your way creates a less consistent language, which is something I'm not super fond of.

If you make it so that the "default" for PC arguments is for them to be readonly by default via an analyzer, you are effectively warping normal conventions which say that arguments are mutable by default everywhere else arguments are used.

To me, the explicit readonly requirement results in a more consistent behavior, even if it is, yes, more verbose, and even if we only allow it on PC arguments (yes, this also creates an inconsistency in and of itself, but a less harsh one that completely flipping the behavior of something that has existed since the language was created.

Would I have preferred all arguments to be readonly by default? Absolutely. But that boat has sailed now... unless the team was willing to take a bit risk here and introduce a super breaking change over multiple language versions with deprecation warnings and all that stuff. In a sense, this is a similar conundrum than the (now deemed a bad decision) decision of having classes be nullable by default and the later clash with the introduction of nullable reference types: NRT will never be able to be enforced in a stronger manner without completely breaking a ton of code, so it can only live as a "lesser" analyzer/static analysis thing.

Similarly, I'd have preferred 'sealed' to be the default, and 'virtual' to be the opt-in modifier for classes, but it was similarly decided way too early on that the opposite was to be the case, and now we are stuck with a "bad default". Could we introduce an analyzer to detect inheritance and force people to use something like an attribute to indicate "virtual" and abandon the 'sealed' modifier?

All that to say that each time the team makes a decision that creates a special case, that thing stays in the ecosystem indefinitely and this compounds over time to create an overall inconsistent and confusing language. Swapping the behavior of mutability for one particular case (PC arguments) even if through a built-in analyzer, will add to that inconsistency/confusion.

Answer 7 · 2024-10-04T14:12:15.000Z

@julealgon,

I get what you are saying. But if you look through many discussions here where people say "can we have x in the language", a very common response from members of the language team is "use an analyzer/generator to achieve that". And if you suggest that analyzers and generators ought to be for domain-specific features, not language dialects, you tend to get downvoted, suggesting there is a view that they are not just for that.

The team positively encourage that less consistent language approach, therefore. But you make a good point: they are unlikely to add this to a warning wave as they do not want to create those inconsistencies themselves. That's for others to do. And this is intentional: they create generic solutions and then encourage others to customise and regulate those features according to their specific needs.

Could we introduce an analyzer to detect inheritance and force people to use something like an attribute to indicate "virtual" and abandon the 'sealed' modifier?

You really don't want to do that. Sealed classes are more performant. An analyzer that errors on a non-sealed class or record unless annotated with a [SupportsInheritance] attribute ought to definitely be on any company's mandatory analyzer list. Luckily, PC parameters do not suffer a performance hit so can be handled with an analyzer.

Answer 8 · 2024-10-04T15:04:00.000Z

The team positively encourage that less consistent language approach,

The approach is consistent. The language means the same thing everywhere, but different trans can choose which parts of the language to allow. Disallowing a part of the language doesn't make the language less consistent.

Answer 9 · 2024-10-04T15:11:16.000Z

Would I have preferred all arguments to be readonly by default? Absolutely. But that boat has sailed now... unless the team was willing to take a bit risk here and introduce a super breaking change over multiple language versions with deprecation warnings and all that stuff. In a sense, this is a similar conundrum than the (now deemed a bad decision) decision of having classes be nullable by default and the later clash with the introduction of nullable reference types: NRT will never be able to be enforced in a stronger manner without completely breaking a ton of code, so it can only live as a "lesser" analyzer/static analysis thing.

Right. And we have looked at this and decided: no, we are not willing to take that risk.

With nrt even the data there just barely was sufficient. But at least there was sufficient evidence about the bugs will null and the costs of it. That could then motivate a multi year effort across the entire ecosystem.

There legitimately is nothing like that for read-only. We do not have anything close to the number of bugs or clarity issue with mutable locals and parameters.

Instead, what we have are a set of people that just have a preference. Their mental model is to never/rarely want to mutate a local, and they want the language to cater to that by default, trapping the occasional time they accidentally violate that personal preference.

Asking for the same effort to be put in here to appease that preference is not going to happen. As David has pointed out, our view is that this is what analyzers are for. Have the language support the broad needs of all users, but use analyzers to restrict certain coding patterns you don't like in your own codebase.

Answer 10 · 2024-10-05T04:00:30.000Z

The view that readonly on parameters and locals would be an attractive nuisance more than a helpful addition.

Indecision on what a succinct syntax for locals would be to minimize the nuisance part of the first objection.

How is this possible? This is without any empirical evidence. The fact that this works very well in Java, this line of argument disproved by as it is contradictory to the real world uses.

Answer 11 · 2024-10-05T04:02:28.000Z

That future design might allow a shorthand for readonly var, or may say that readonly var is not allowed, and the separate shorthand is required for a readonly type-inferred local. What that shorthand
is (val, let, const, or some other keyword) is beyond the scope of this proposal.

readonly locals can easily be val like in Scala or let.

Answer 12 · 2024-10-05T05:57:20.000Z

Also for immutability by default will not fly as I have seen many people suggest else where.

Answer 13 · 2024-10-05T06:03:47.000Z

Please keep discussion of readonly locals out of this issue. You can continue discussing that topic in #8479.

Answer 14 · 2024-10-05T09:37:23.000Z

since readon;y is already strong keyword from the start of C# there are no reason to not allow readonly to support as default

let might be allowed and succeed readonly in the future, but that should not be any problem and should not change the usage of readonly even it was redundant