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]: readonly parameters #8478

Open
1 of 5 tasks
333fred opened this issue Oct 3, 2024 · 17 comments
Open
1 of 5 tasks

[Proposal]: readonly parameters #8478

333fred opened this issue Oct 3, 2024 · 17 comments
Assignees

Comments

@333fred
Copy link
Member

333fred commented Oct 3, 2024

  • Proposal added
  • Discussed in LDM
  • Decision in LDM
  • Finalized (done, rejected, inactive)
  • Spec'ed

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

@HaloFour
Copy link
Contributor

HaloFour commented Oct 3, 2024

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.

@julealgon
Copy link

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.

@333fred
Copy link
Member Author

333fred commented Oct 3, 2024

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.

@DavidArno
Copy link

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.

@KennethHoff
Copy link

KennethHoff commented Oct 4, 2024

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

@julealgon
Copy link

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.

@DavidArno
Copy link

@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.

@CyrusNajmabadi
Copy link
Member

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.

@CyrusNajmabadi
Copy link
Member

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.

@sirinath
Copy link

sirinath commented Oct 5, 2024

  • 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.

@sirinath

This comment has been minimized.

@sirinath
Copy link

sirinath commented Oct 5, 2024

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

@333fred
Copy link
Member Author

333fred commented Oct 5, 2024

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

@Thaina
Copy link

Thaina commented Oct 5, 2024

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

@TahirAhmadov
Copy link

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.

@julealgon
Copy link

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.

@TahirAhmadov
Copy link

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.

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

9 participants