-
Notifications
You must be signed in to change notification settings - Fork 1k
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]: readonly
parameters
#8478
Comments
I always viewed this proposal as a stepping stone into a potential wider application of That said, being able to apply 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 |
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. |
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 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. |
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 |
@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 To me, the explicit Would I have preferred all arguments to be 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. |
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.
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 |
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. |
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. |
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. |
This comment has been minimized.
This comment has been minimized.
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 #8479. |
since
|
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. |
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 |
That goes back to the general conversation in #8479, and it was decided once and for all - 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 |
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 shorthandis (
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 ashorthand that combines the meanings, but otherwise
readonly
is required.Specification
https://github.com/dotnet/csharplang/blob/main/proposals/readonly-parameters.md
LDM Discussions
The text was updated successfully, but these errors were encountered: