Readonly PC parameters - a case of closing the stable doors after the horse has bolted?

[DavidArno]

There has been considerable demand from the community for C# to support readonly PC parameters for a while now and I've been a vocal supporter of that position. But since C# 12 was released last month and I started using the feature, I've began to question the wisdom of doing this.

To be clear, I haven't suddenly abandoned my "quality before convenience" principles and embraced mutable PC parameters. Instead I've gone the opposite route: I've made PC parameters immutable by default, which is the behaviour I wanted all along. I've started using the PrimaryConstructorAnalyzer package, which reports any attempt to mutate a PC parameter as a warning. And since I'm a "quality before convenience" sort of person, I naturally have "treat warnings as errors" enabled, so mutating a PC parameter has become a compiler warning.

For me, this is even better than having to put readonly on every single PC parameter. And for those vanishingly rare occasions where I'd want the field "capture" to be mutable, I simply explicitly assign the parameter to a non-readonly field. Perfect example of "pit of success" in action in C#.

But that got me thinking: what happens if the language team were to add readonly parameters to a future version of C#? I'd have been using the pit of success via an analyzer technique for at least a year by then (for work, it'll be two years as we only update with LTS versions of .NET, so I'd be waiting for C# 14 there). Would I really want to bloat my code with readonly at that stage when I'd have already been using a more succinct solution prior to that? Difficult to say for certain as I tend to change my mind on things over time, but currently I'd say no.

Currently, that analyzer solution is good, but it creates a non-standard language dialect. Those using the analyzer get very different compilation results to those not doing so; it's effectively a compiler behaviour flag. Whilst the language team are heavily adverse to compiler flags, they do support the .NET analyzers' CAxxxx quality rules. So rather than seeing readonly supported for those parameters, I'd like to see the team introduce a new CAxxxx rule, "Do not mutate constructor parameters". This would be a warning, so the "convenience before quality" camp get to ignore it and those of us that favour quality get official support for disabling PC parameter mutations. In addition, it allows the whole sorry saga of "can we just do readonly for PC parameters?" to be quietly dropped. This solution therefore seems to be the best of all worlds.

[HaloFour]

Would I really want to bloat my code with readonly at that stage when I'd have already been using a more succinct solution prior to that? Difficult to say for certain as I tend to change my mind on things over time, but currently I'd say no.

It sounds like that's been the crux of the argument against adding readonly modifiers to parameters/locals in the language in general, and why the language team has been reluctant to support adding them.

I'm of two minds here. I do think analyzers can get the job done, complete with a bunch of different knobs to control policy (as I'd want these guardrails in cases other than primary constructors). But I'd love to see this thing ship with the compiler by default, even if all of the rules are turned off by default. Then projects can dial-up the error level of each rule as they see fit.

Notably the scenarios I'd like to see an analyzer support are:

1. Reassignment of a local
2. Reassignment of a method parameter
3. Reassignment of a local/parameter captured by a lambda or local function
4. Reassignment of a constructor parameter
5. Reassignment of a primary constructor parameter after initialization
6. Reassignment of a primary constructor parameter after construction
7. Reassignment of a primary constructor parameter during construction

An analyzer approach does have advantages aside succinctness in that you could also use syntax like comments to explicitly mark locals/parameters that you want to be mutable, e.g. /*mut*/ int foo, kind of like a poor man's source attribute.

But part of me thinks that this should be a first-class citizen, only because I feel that these kinds of guardrails are important enough. But then it'd have to be opt-in rather than opt-out, and given it's a feature I would probably use in the vast majority of cases that would be a lot of readonly to sprinkle around my projects.

[CyrusNajmabadi]

Currently, that analyzer solution is good, but it creates a non-standard language dialect

There's nothing non-standard, or a dialect about using an analyzer here for this purpose.

This is the intended purpose of an analyzer. And, iirc, it's what was even recommended by us to you during several prior discussions on this topic.

[CyrusNajmabadi]

But then it'd have to be opt-in rather than opt-out

Fundamentally, there are always disagreements because different people land in this particular spectrum on different sides. :-)

[HaloFour]

I totally agree, that's the issue with making it a language feature. Short of a project-wide setting to make all declarations implicitly readonly, you have to opt-into making each declaration readonly through some kind of syntax, which would necessarily be more verbose (except maybe in the var case). IMO, projects that would want to adopt this would prefer that the majority of declarations be readonly, and then to opt-in to the cases that are mutable, but that doesn't seem reasonable/feasible through a language feature. It is totally feasible with an analyzer.

[CyrusNajmabadi]

but that doesn't seem reasonable/feasible through a language feature. It is totally feasible with an analyzer.

Yup. I have thought about nullable-like switches and # pp directives, so people can state which camp they're in. But that then would really be giving the language dialects here :-(

So analyzers are a good middle ground. One set of permissive language rules, with additional guardrails for things that some don't like about that.

[CyrusNajmabadi]

I'd like to see the team introduce a new CAxxxx rule, "Do not mutate constructor parameters".

Please open a discussion over at dotnet/roslyn-analyzers about this. Thanks.

[CyrusNajmabadi]

Closed as we don't need another thread on this topic. And because one of the requests seems to be for a different repo. David, can you add your thoughts to one of the current threads on primary constructor parameters, and open a discussion with Roslyn-analyzers on this? Thanks!