Open 333fred opened 1 month ago
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.
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.
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.
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.
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
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.
@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.
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.
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.
- 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.
- 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
.
Also for immutability by default will not fly as I have seen many people suggest else where.
Please keep discussion of readonly locals out of this issue. You can continue discussing that topic in https://github.com/dotnet/csharplang/discussions/8479.
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
To be honest, at this point I think we shouldn't do it even for PC parameters. In cases where it's typically needed, like a class with multiple dependencies on other interfaces, it would create noise just like it would have for locals. And non-PC parameters, it falls under the rubric of general readonly for locals and parameters, which has been ruled out.
At this point, the best solution is (are) analyzer(s) to allow individual projects opt in to "readonly"-ness by default.
At this point, the best solution is (are) analyzer(s) to allow individual projects opt in to "readonly"-ness by default.
Having an analyzer that triggers an error/warning for something that is already triggered by a dedicated keyword in the language (even if said keyword cannot be used in that particular case) would feel like a hacky solution to me.
I can understand an analyzer for something that is not otherwise captured in the language... but readonly
already exists and already provides the same capability that such analyzer would provide.
Having an analyzer that triggers an error/warning for something that is already triggered by a dedicated keyword in the language (even if said keyword cannot be used in that particular case)
That goes back to the general conversation in #8479, and it was decided once and for all - readonly
is not being introduced to locals, despite being available for fields. The same reasoning applies here - just because readonly
is available for fields, it doesn't mean it has to be available for parameters, even PC parameters, as well.
For the IMO very few cases where PC parameters need to be mutable, they can be decorated with [Mut]
attribute and the analyzer uses that.
On a tangent, I think we should use the word "write" (writable) for the opposite of "readonly", to avoid confusion with deep mutability. Perhaps said analyzer should use use [Writable]
or [CanWrite]
? Minor naming pedantry, but still. It would help to disambiguate writability from mutability, which is valuable given how many times people confused the two.
Summary
We allow parameters to be marked with
readonly
. This disallows them from being assigned to or being passed byref
orout
.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:readonly
on parameters and locals would be an attractive nuisance more than a helpful addition.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:readonly
as a local modifier.readonly var
, or may say thatreadonly var
is not allowed, and the separate shorthand is required for areadonly
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 otherwisereadonly
is required.Specification
https://github.com/dotnet/csharplang/blob/main/proposals/readonly-parameters.md
LDM Discussions