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

Champion "readonly for locals and parameters" #188

Open
5 tasks
gafter opened this issue Feb 26, 2017 · 1,083 comments
Open
5 tasks

Champion "readonly for locals and parameters" #188

gafter opened this issue Feb 26, 2017 · 1,083 comments

Comments

@gafter
Copy link
Member

gafter commented Feb 26, 2017

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

See also dotnet/roslyn#115

Design review

@gulshan
Copy link

gulshan commented Mar 23, 2017

Any plan for readonly types(classes and structs)?

@Thaina
Copy link

Thaina commented Mar 27, 2017

should support

readonly i = 0; // shorthand for readonly var
const j = 0; // shorthand for const var

@jnm2
Copy link
Contributor

jnm2 commented Mar 27, 2017

Wasn't val going to be short for readonly var?

@Thaina
Copy link

Thaina commented Mar 28, 2017

@jnm2 I just don't like the idea of adding new keyword. Especially it is already have keyword in the language that has the same meaning

readonly might be a bit longer but we already preserved it from the start. We should reuse it. And to be shorter, just let we use readonly without var

At least I have seen some suggestion to use let that would still better than val because we already have it as keyword, even for linq scope

Especially because val was not keyword. I really have my code var val = 0; and I bet there are many people have val as variable or field name in their code like me. I think val is a bad choice

@jnm2
Copy link
Contributor

jnm2 commented Mar 28, 2017

@Thaina Yes, I'm inclined to agree.

@HaloFour
Copy link
Contributor

@Thaina

Especially because val was not keyword. I really have my code var val = 0; and I bet there are many people have val as variable or field name in their code like me. I think val is a bad choice

And you could continue to. Like var, val would be a contextual keyword, in that it only behaves like the keyword when it doesn't make sense for it to behave like anything else. So var val = 0; would remain perfectly legal.

Although I do prefer let to val, mostly because I think it looks sufficiently different.

@jnm2
Copy link
Contributor

jnm2 commented Mar 28, 2017

Oh yes! let was the one I liked. Thanks!

@Thaina
Copy link

Thaina commented Mar 28, 2017

@HaloFour It not breaking change I understand but it still ambiguous

BTW I still don't like let. I prefer readonly. But at least let is better than val

@benaadams
Copy link
Member

let is a bit early basic; also was read write. Not sure how let implies readonly?

@HaloFour
Copy link
Contributor

@benaadams

let is a bit early basic; also was read write. Not sure how let implies readonly?

let is also F# where it is the readonly (by default) binding of an identifier. let is also C# LINQ where it is the declaration of a readonly range variable.

Personally the latter reason is enough for me. It's already a contextual keyword, and in that existing context it creates a readonly identifier.

@benaadams
Copy link
Member

let is also C# LINQ where it is the declaration of a readonly range variable.

Fair enough 😄

@Richiban
Copy link

@gulshan

Any plan for readonly types(classes and structs)?

Do you mean immutable types? If so, that's a completely separate proposal (I'm pretty sure it's been made before).

@soroshsabz
Copy link

ITNOA

@Richiban where I can found it?

@Richiban
Copy link

@soroshsabz

ITNOA

That's a new one!

dotnet/roslyn#7626 and https://github.com/dotnet/roslyn/issues/159 are probably what you're looking for.

@HaloFour
Copy link
Contributor

HaloFour commented Mar 30, 2017

Something I like about final locals in Java is that it's possible to declare one as not assigned and then have branching logic to assign it. The Java compiler uses flow analysis to ensure that for each branch that the local is assigned exactly once.

final String name;
if (entity instanceof Person) {
    Person person = (Person)person;
    name = person.getFirstName() + " " + person.getLastName();
}
else if (entity instanceof Company) {
    Company company = (Company)company;
    name = company.getFirmName();
}

This can be useful in those scenarios where you want the local to be readonly, the expression to calculate it can throw and you want the scope of the local to exist beyond a try block.

@soroshsabz
Copy link

soroshsabz commented Mar 30, 2017

@Richiban thanks, but I hope to see comprehensive proposal about immutable object in csharplang project.

@soroshsabz
Copy link

@HaloFour Is conditional operator ( ?: ) not sufficient for this purpose?

@HaloFour
Copy link
Contributor

@soroshsabz

Sometimes not. I amended my comment to mention try/catch scenarios where C# offers no single expression. You could extract the logic to a separate function but that's more verbose ceremony. Even if it could be expressed as a single conditional expression sometimes it's more readable expanded out into multiple statements.

Either way, Java supports this, and I make use of it frequently enough that I think it would be useful here.

@soroshsabz
Copy link

soroshsabz commented Mar 30, 2017

I think simple rule like "All local readonly variables must be initializing immediately after declaration." cause to improve simplicity and readability for programmers. In Java case some programmers maybe surprise to final variable has not initialize and must be track code to find the where is this variable initialize?

@DavidArno
Copy link

@HaloFour,

That's yet another use-case for match:

let name = entity match (
    case Person person : $"{person.FirstName} {person.LastName}",
    case Company company : company.FirmName
);

@HaloFour
Copy link
Contributor

@soroshsabz

We may differ on opinion there. If the expression has to be overly complex in order to satisfy an overly strict language feature that only decreases overall maintainability and readability. I'd rather the flow be logical and the compiler enforce readonly-ness where appropriate.

And as a Java programmer who works directly with hundreds of other Java programmers I can say that this has never been a source of confusion. It's a pattern used fairly frequently across the Java ecosystem. If anything I think I would find it much more annoying that I couldn't declare and assign a readonly variable like this.

@DavidArno

It's just one exceptionally simple case. match won't handle the exception handling scenario. And again, forcing the developer to try to pack it all into a one-liner, or to extract that local logic elsewhere, does not improve the readability or maintainability of the program.

@tannergooding
Copy link
Member

Unlikely that it would help the optimizer. It is just as easy for the optimizer to see that a local that is allowed to be reassigned never actually is reassigned.

Something being readonly at the C# level, doesn't necessarily mean it is or should be readonly at the IL level.

In fact, there are many cases where performance oriented code explicitly wants to reuse locals to reduce the overhead on the stack or where it allows a reduction in copies or avoids needing complex analysis required to prove that a local is actually readonly (in the context of a complex flow graph) so that the JIT could itself determine that folding two locals into one slot was possible.

@sirinath
Copy link

sirinath commented Dec 2, 2023

Roc Lang is a functional language which used mutability inference to reuse variable which are immutable at the language level.

@5cover
Copy link

5cover commented Dec 25, 2023

I believe one or once could be used, they are relatively short and relevant, indeed a readonly local is only gonna be assigned once.
Of course, it would be a contextual keyword to preserve compatibility with existing code.

It's important that we choose a keyword that is short to avoid cluttering the code, and that carries sufficient meaning to be understandable even when seeing it for the first time.

It would look like this:

once int Foo = 10;

It might look prettier to put the modifier after the type, even though this breaks the consensus of modifiers going before the type.

int once Bar = 10;

@DavidArno
Copy link

@5cover,

The choice of word isn't the important point here. The issue that is blocking this feature is that the moment readonly locals and parameters are added to the language, it automatically becomes best practice to mark everything as readonly, once, foo or whatever word is chosen. Regardless of the word chosen, C# instantly becomes "noisier" as the dominant opinion amongst (at least the vocal elements of) the language team is that marking them readonly serves no useful purpose.

Whether that opinion is a valid one is moot: C# isn't a democratic language. Once the language team have made a decision, it's monumentally difficult for the community to change their collective mind on the matter.

@FrankBakkerNl
Copy link

I would say there are some small benefits to readonly locals and parameters, but imho those do not outweigh the extra noise of a modifier on 98% of all locals.

I would only like to have this if you can opt-in per project to have readonly be the default.

@Richiban
Copy link

It's for exactly this reason that I really wish we could separate this into two feature requests: one for "readonly locals" and another for "readonly parameters".

Although there's obviously some overlap between the two, I expect the resultant change to the C# code people write to be very different in each case.

@CyrusNajmabadi
Copy link
Member

@Richiban we would think of this as two separate language changes and feature requests.

There's no point in creating multiple issues though as all the discussion has happened here.

@CyrusNajmabadi
Copy link
Member

C# isn't a democratic language. Once the language team have made a decision, it's monumentally difficult for the community to change their collective mind on the

Changing minds doesn't happen through democratic means. Changing minds happens through sound arguments and addressing the concerns brought up by the team. :-)

as the dominant opinion amongst (at least the vocal elements of

This topic has been discussed by the ldm numerous times over the years (as we both want to reassess based on new community arguments, as well as to see if things have changed since the last time we looked at things). The statements made here reflect the positions and decisions of the ldm as a whole.

Note, an example of how things changed were your points about readonly and primary construction parameters. Which is why we're amenable to making changes there @DavidArno, and that's something we're likely to start up soon this year.

@SRNissen
Copy link

SRNissen commented Dec 30, 2023

[...] the moment readonly locals and parameters are added to the language, it automatically becomes best practice to mark everything as readonly, once, foo or whatever [...]

As one of those people - close but not quite. Rather: "Best practice, in any language, is to make it harder to write bugs."

I don't want readonly parameters to force you to use them, I want readonly parameters so the compiler will tell me if I mistakenly make changes to a parameter the user passed me in expectation that I would only read from it.

@KennethHoff
Copy link

KennethHoff commented Dec 30, 2023

There's practically no scenario where I reassign local - let alone a parameter. I would use readonly on everything, which tells me that the readonly keyword is a poor language decision.

I think mirroring the nullable approach is preferable, adding an attribute([Mutable]), and an analyzer that ensures you follow it. This would have to be applied to locals though - which currently doesn't support attributes I believe - so it'd have to follow the implementation of the compiler-only attribute proposal.

I do think attributes is substantially uglier than keywords however - looks like a hack - but I don't see them adding a mut keyword anytime soon.

@HaloFour
Copy link
Contributor

Ultimately I think an analyzer is the only path forward here. That gives you the ability to toggle the behavior of the entire project without having to change any code, and knobs to customize the behavior to your preferences. It's not really possible to compete with how succinct declarations are today (except in specific cases, e.g. let/val vs. var) and I seriously doubt that the team would be willing to consider a language feature with project-level settings like NRTs.

@CyrusNajmabadi
Copy link
Member

and I seriously doubt that the team would be willing to consider a language feature with project-level settings like NRTs.

I would be willing to consider it. Though I agree it would have an uphill battle here.

@CyrusNajmabadi
Copy link
Member

Best practice, in any language, is to make it harder to write bugs."

I've been coding for nearly 30 years in at least a couple dozen languages. I genuinely don't remember a single case of a bug that would have been prevented by such a feature. That's how rare it seems to be to me. That sort of rarity space is much better served by an analyzer versus a lang feature imo.

So, imo, best practice in a language is not to make it harder to write any bugs. Rather, a language should be judicious in the sorts of bugs it is trying to stamp out.

@HaloFour
Copy link
Contributor

@CyrusNajmabadi

I would be willing to consider it. Though I agree it would have an uphill battle here.

I've been coding for nearly 30 years in at least a couple dozen languages. I genuinely don't remember a single case of a bug that would have been prevented by such a feature. That's how rare it seems to be to me

I think those two statements sum that up nicely. Unlike NRTs (which are analyzers on steroids), this isn't attempting to solve a massive known problem. It's more attempting to enforce a degree of correctness. It's a worthy goal, but does it rise to the level of a dialect? Even I have to admit that it's not. I'd rather nip it in the bud and get to work exploring viable alternatives than drag this conversation out another 7 years.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Dec 30, 2023

IMO, the real benefit here is simply as an instant signal to the code reader: hey, this won't get reassigned, so you can cross off one of a zillion aspects of the code that you do need to pay attention to as you work on understanding it.

I agree that an analyzer or analyzer-like approach is basically the only way here. And since the language has not "defaulted to readonly" to date, a lite form of dialect-ization a la NRT will be needed. (At least, I lean against simply allowing the readonly keyword in all these places and making that something "people just want to add on 99% of the time.") Whether that happens "officially in the language and compiler", or outside through a configurable analyzer, possibly with indirect language support such as attributes on local variables, is up for debate. And, as always, whether the feature will happen at all is not known. "No" is not always a permanent state, but it can also be quite a long lasting one, as we've seen.

@FrankBakkerNl
Copy link

FrankBakkerNl commented Jan 2, 2024

IMO, the real benefit here is simply as an instant signal to the code reader: hey, this won't get reassigned, so you can cross off one of a zillion aspects of the code that you do need to pay attention to as you work on understanding it.

Agreed, it does make reasoning about code easier knowing a value is never reassigned. That would be the only real benefit I see of this feature. Rider / Resharper by default underlines all variables that are reassigned in the IDE. This gives exactly the mental help I personally need while reading code to pay more attention to these variables. It does not need to be suppressed with a pragma or attribute, does not clutter code more than necessary. It even helps me read code in large legacy code bases with long complicated methods without needing to update to a new compiler version or add new analysers.

I do not know if VS (code) also have such a feature, but maybe if it gets added there will be a lot less demand for this as a language feature.

@CyrusNajmabadi
Copy link
Member

I do not know if VS (code) also have such a feature,

We have this feature:

image

@GitClickOk
Copy link

@FrankBakkerNl Sadly it does not help on code reviews... unless you use the IDE for this matter.

@binki
Copy link

binki commented Jan 2, 2024

The built-in code highlighting doesn’t provide a way to indicate intent. Just because a variable doesn’t change currently doesn’t mean it shouldn’t be reassigned in a future code change.

Being able to mark variables as non-reassignable provides protection and ease of mind against future changes to the code-base. When updating a variable to allow reassignment, it is obvious that more prudence is necessary to avoid potential regressions. After all, the original developer went out of their way to mark the variable non-reassignable.

For full stack developers working with multiple programming languages, lacking this feature increases dissonance. This is just one more thing which doesn’t have a one-to-one mapping from other modern popular multi-paradigm languages.

@tats-u
Copy link

tats-u commented Jan 3, 2024

We sometimes have to read code written a long time ago or by others in other than IDEs or VS Code. GitHub does not track variables until we click them. Other similar services do not at all.

@qsdfplkj
Copy link

Why not use the in keyword for the parameter as with methods? Doesn't it have the right semantics already?

https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/method-parameters?WT.mc_id=dotnet-00000-bramin#in-parameter-modifier

@HaloFour
Copy link
Contributor

The in keyword is already used for parameters, and it means something different. Specifically, in means readonly ref, but there is no way to describe a non-ref parameter that should also be readonly.

@Strandedpirate
Copy link

2024 here, I need readonly capabilities in my primary constructors. I don't think our company will ever adopt primary constructors unless we specify this modifier.

public class Derp(readonly ILikeToDerp derpage)
{
    public void Test() {
        // shouldn't compile
        derpage = new YouLikeToDerp();
    }
}

@vittorioromeo
Copy link

vittorioromeo commented Jan 27, 2024

The choice of word isn't the important point here. The issue that is blocking this feature is that the moment readonly locals and parameters are added to the language, it automatically becomes best practice to mark everything as readonly, once, foo or whatever word is chosen. Regardless of the word chosen, C# instantly becomes "noisier" as the dominant opinion amongst (at least the vocal elements of) the language team is that marking them readonly serves no useful purpose.

Sorry for the bluntness, but it is insane that between

  1. Optionally noisier but safer and more readable

  2. Enforced conciseness but no safety/readability benefits

(2) is preferred.

As a C++ developer that uses C# from time to time, not being able to use const for local variables or parameters is a major pain point that really harms readability and can sometimes lead to accidental mutation.

Let me explicitly mark what the moving parts or non-moving parts of my algorithms and functions are.

@qsdfplkj
Copy link

Why not use the same as record class. Some init properties.

@DavidArno
Copy link

2024 here, I need readonly capabilities in my primary constructors. I don't think our company will ever adopt primary constructors unless we specify this modifier.

@Strandedpirate,
The approach I took to make primary constructor parameters acceptable to the company I work for was to roll out the PrimaryConstructor analyzer. It simply mandates that all PC parameters must always be readonly. No exceptions. It works perfectly for us as we've yet to find a single use-case for needing mutable PC parameters.

@DavidArno
Copy link

@vittorioromeo,

There are many in the community that agree with you. However, as the language team at Microsoft like to remind us on occasion, C# is not a democracy. The team do not agree with us and so they do not implement it.

I will disagree with you though that it is insane. They do have logical reasons for this position as mutable locals and parameters aren't a significant source of bugs therefore marking them readonly adds to the "noise" and makes the code less readable. Clearly though, mutable locals and parameters do significantly add to the cognitive load when reading code and so also make the code less readable. Which is the worst offender of reducing code readability is - as this thread shows - a huge source of debate.

For me, the whole debacle of releasing non-recordPC parameters without readonly support has thrown an interesting curveball into the argument. People have responded by producing analyzers that report modifying PC parameters as a warning (easily promoted to an error). As there doesn't seem to be any meaningful use cases for mutating those parameters, such analysers solve the issue completely. And that therefore could well just be the end of the matter.

But not everyone wants to use an analyzer and so the team may remain under pressure to allow those parameters to be explicitly marked as readonly. If they give in to that pressure, what then happens? The readonly keyword isn't needed as those analyzers offer a way of implicitly making them readonly. I think the private keyword offers a clue as to which way people will go. That keyword arguably serves no useful purpose (ignoring the private protected combo), but a great many people (myself included) specify it even though it's not needed (except with that private protected combo).

I suspect the same will happen when readonly PC parameters are supported: even though it's not technically necessary to mark them readonly, people will, and new analyzers will appear that warn on not specifying them. The reason is simple: specifying that private or readonly makes the intent of the author clear. It shows they applied discipline when writing the code. Ultimately it suggests the code will be of good quality as the author has taken care when writing it.

For me, this is the killer argument for readonly locals and parameters. It's not about "sanity" and it's not about "noise". It's simply that explicit intent trumps saving a few characters and the use of private provides strong evidence to back that assertion up.

Despite this, I'm sure the team will stick to their "absolument pas" response on this topic. And as C# is not a democracy, nothing will change regarding readonly locals and (non PC) paramters...

@Richiban
Copy link

The thing I don't understand in the whole discussion about readonly-ness (when it comes to both PC parameters and normal parameters / locals) is that the team's position seems to be "Being able to mark locals and parameters as readonly is a low-value feature".

I'm not going to argue with that assessment (although I disagree) but instead I'm going to ask a question in reply:

"Why does C# have readonly fields?" Obviously the team decided that the juice was worth the squeeze on that one (with the caveat that it's a very old feature and the LDT might be made up of different people now than it was then).

As far as I can tell the arguments both for and against readonly fields are the same as they are for readonly locals, but one was added and one was not? I simply don't understand the reasoning.

It could well be that I'm simply missing something here, so I'm happy to be enlightened.

@FrankBakkerNl
Copy link

An important difference between locals and fields is their scope. A field can be (re)assigned by any line of the type, while a local by definition only by the member in which it is defined. For most cases this makes it easier to track what happens to a local than to a field. Therefore I do think readonly for locals has smaller benefit than for fields. Which helps flip the scale in this tradeoff.

Ofc now PC parameters kind of blur this distinction.

@HaloFour
Copy link
Contributor

@Richiban

Also the timing. Readonly fields have been there from the start, it's not a question of justifying a language modification after the fact. Given the inspiration/influence on the CLR from Java, it's more notable that .NET/C# included readonly fields but did not include readonly parameters and locals, as Java did support both at the time.

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