dotnet / csharplang

The official repo for the design of the C# programming language
10.95k stars 999 forks source link

Champion "readonly for locals and parameters" #188

Open gafter opened 7 years ago

gafter commented 7 years ago

See also https://github.com/dotnet/roslyn/issues/115

Design review

svick commented 6 years ago

@HaloFour

the C# team could always decide to reverse the decision that the iteration variable is immutable

Considering that the only advantage of this would be improved consistency, but at the cost of increasing the chance of bugs, that doesn't seem like a good approach to me. And it would be even more confusing if ref foreach (https://github.com/dotnet/csharplang/issues/461) is also added:

foreach (ref var item in array)
{
    item  = 0; // sets the item in array to zero
}

foreach (var item in array)
{
    item  = 0; // valid, but doesn't do anything
}
Thaina commented 6 years ago

@svick It did. It change item to zero in that loop. So they would do calculation with that item afterward but not affect the actual item in array

consider this

foreach (var item in array)
{
    // calculate something
    item  = item * item; // same as Select((x) => Math.Pow(x,2)
    // calculate some other things
}

I think it wrong from the start that foreach was making immutable implicitly. We should have it explicit for consistency. And I think it was normal behaviour that var from any returning function will be copied unless it return ref

svick commented 6 years ago

@Thaina The comment was in the context of the code I wrote, where there is no other code following the assignment.

I think this falls into the "pit of success" philosophy: Writing to the iteration variable of a foreach would often be a bug. And in cases where it's actually useful, you can easily work around it (by declaring a separate variable). Which is why it's better that the variable is read only.

Thaina commented 6 years ago

@svick I think opposite. I think we just misbuttoned our shirt from the start that we don't allow return ref in foreach

The actual behaviour for iterate through any collection since the C is index a pointer reference (array). So we actually always able to modified item in the iteration loop. But while C# just not allow return ref from the start we then lack this feature from the foreach

I bring this up to say that, it is the wrong decision to make foreach immutable to avoid the "pit of success" you talk about while the actual behaviour is foreach itself should be able to mutate the item in collection

Now it's time to fix this altogether. It should change the perspective that foreach must not modified things. And the default keyword for foreach we should use is let not var after this change

I mean we should fix all example we use var in foreach to let or ref as a default. Just discourage using var in foreach after this

Then it would be more obvious that, whenever you declare var it will not be an actual object in the collection, just a local variable unless it has ref

IMO, the default keyword for foreach should be ref

orthoxerox commented 6 years ago

@Thaina It can't be the default, because IEnumerable<out T> is the default enumerator. Changing it to return refs will make it invariant and will break every other program written after variance was added to the language. We need a separate IRefEnumerable<T> to be able to mutate iteration variables.

Pzixel commented 6 years ago

I think we just have to

  1. Make a loop variable declared via let, or even without any prefix, just foreach(value in values)
  2. Add a warning if var is used in foreach. Suggest a quick fix to change it to let. I totally agree with Eric:

If you're going to make a backward-compatibility-breaking change, no time is better than now; things will be worse in the future.

Thaina commented 6 years ago

@orthoxerox Well that's right. I just state my opinion that we just start wrong and now we need to compensate for legacy. I mean I actually want all collection to be IRefEnumerable<T> if possible

But yeah, seem not possible

HaloFour commented 6 years ago

The team has stated many, many times that var does not imply mutability, only type inference. This new let/val keyword is only intended to be shorthand for readonly var. As such it's unnecessary to revisit where var appears in the language and results in a readonly variable. There's no reason to alter the syntax (or behavior) of foreach.

aluanhaddad commented 6 years ago

@HaloFour quite so. I mentioned foreach in my remarks simply because it would not appear to be logically wrong to expect to be able to use the new syntax in that position. It would not express anything other than the existing semantics and in fact could be regarded as redundant.

Thaina commented 6 years ago

@HaloFour For me the existence of readonly is imply mutability. Hence the lack of it should always imply that variable is mutable. foreach is the unnecessary exception

ceztko commented 6 years ago

What does it mean the release milestone change? Is this feature delayed or it got merged with #38 ?

iam3yal commented 6 years ago

@ceztko Got delayed.

aluanhaddad commented 6 years ago

Indeed. That is a shame considering this should be relatively straightforward to implement. Also, readonly locals will reduce the performance cost of CFA when non-nullable reference types are added.

As much as I 🚲:house: about the syntax, I want the feature, whatever the keyword.

oledid commented 6 years ago

This issue was recently discussed on reddit, in /r/csharp. Link

jnm2 commented 6 years ago

Wow, that guy was just tripping over himself, don't you think? That was not a good approach to convince people of something. =)

HaloFour commented 6 years ago

@jnm2

You ain't kidding. I suddenly really don't want the team to consider or implement this proposal....

... just kidding, it's still a good idea, regardless of any lengthy self-obsessed rant by someone who apparently has a major problem interfacing with other people.

DavidArno commented 6 years ago

Hmm, reading that felt like looking in the mirror. I thought it a well-reasoned piece. 😁

jnm2 commented 6 years ago

@DavidArno Just so you know, my comment had nothing to do with his position and everything to do with his interpersonal skills (or lack thereof). You know, the kind where you already agreed with him before you read it, but by the time you're halfway through you feel really dirty agreeing with someone who's talking like that?

I hope you don't actually see yourself that way. You're not there. 😃

DavidArno commented 6 years ago

As you know, I recently took a sabbatical from online activity to have a good hard think about the way I was conducting myself online. I may not be there now. Six months ago, I think I wasn’t that different to him... 😳

aluanhaddad commented 6 years ago

@DavidArno sometimes it's easy to come off like an ass if you feel strongly enough about something. Keeping a measured, civil demeanor is for the best but keep fighting the good fight.

CyrusNajmabadi commented 6 years ago

🐞 🍯

DavidArno commented 6 years ago

@joe4evr,

I read @CyrusNajmabadi's emojis as admitting he set honey traps for me to fall into. An admission of guilt on his part, I feel. 😆

CyrusNajmabadi commented 6 years ago

It was the best i could come up with for "you catch more flies with honey" :)

jnm2 commented 6 years ago

Ahhh, now I get it =D I'm still quite amused at what DavidArno read into it. Postmodern art for the win...

hickford commented 6 years ago

For completeness, a history of support:

iam3yal commented 6 years ago

@hickford I'll tell you the reason even though you didn't ask for it.

So this is how it goes, during LDMs the language design team play chess where the winner gets to decide what to do with the resources and our champion here lost too many games and this is the reason it was delayed to an unknown point in time. 😆

p.s. Some people and Mr. humor don't get along very well... oh well, maybe it's just me and my weird humor. :)

wanton7 commented 6 years ago

let should have ability to override earlier let declaration or read only parameter like in F#.

let x = 5;
let x = 6; // x becomes 6
DavidArno commented 6 years ago

@wanton7,

F#'s let behaves differently to what's proposed here, which is to provide read-only local variables. So I see no reason at all to replicate behaviour of an unrelated F# feature, just because they share the same keyword.

wanton7 commented 6 years ago

@DavidArno Maybe adding F# in there was a mistake. But idea still applies.

let text = Method("foo");
let text = Method2(text);

VS

let text = Method("foo");
let text2 = Method2(text);

With mutable values you can do this

var text = Method("foo");
text = Method2(text);
DavidArno commented 6 years ago

"With mutable values you can do this..."

Exactly. C# already supports mutable variables. If you want to mutate the variable, don't use let. What possible use would allowing the modifying of read-only variables bring to the table?

drewnoakes commented 6 years ago

@wanton7 I can imagine this making closures even more confusing. How does that work in F#?

I'm not sure I see the value, but I see potential for confusion to the user and complexity for the compiler.

iam3yal commented 6 years ago

@wanton7 People want to have less mutable variables and data structures and you want to have even more of it as if we don't have enough things to worry about...

benaadams commented 6 years ago

If you want to mutate the variable, don't use let. What possible use would allowing the modifying of read-only variables bring to the table?

I assume its just reusing the name; rather than mutating the variable. Presumably it would have to be done scoped; e.g. a let in a if would be reverted outside the if.

Likely confusing; the js devs are going with it (let allows name reused and is scoped)

wanton7 commented 6 years ago

@drewnoakes in F# everything defined after new let binding will use that value closures as well.

wanton7 commented 6 years ago

@benaadams yes it's just reusing it's name nothing else. So this works too

let foo = "text"
let foo = 5
drewnoakes commented 6 years ago

@wanton7 what would happen in this case?

let foo = 1;
PrintFoo();

let foo = 2;
PrintFoo();

void PrintFoo() => Console.WriteLine(foo);
wanton7 commented 6 years ago

@drewnoakes compile error? You would have to move PrintFoo() after first foo then it would print 1 in both cases.

iam3yal commented 6 years ago

@benaadams

I assume its just reusing the name; rather than mutating the variable

Synonyms.

Presumably it would have to be done scoped; e.g. a let in a if would be reverted outside the if.

It wouldn't be confusing at all. /s 😆

iam3yal commented 6 years ago

@wanton7 You should really make a new issue about it, we shouldn't discuss two different ideas in the same proposal.

wanton7 commented 6 years ago

@eyalsk probably, but i think it's dangerous and confusing for mutable variables and should only allowed for read-only variables.

HaloFour commented 6 years ago

@wanton7

That would pretty much contradict how all of scoping works in C#. C# never lets you hide/shadow an existing identifier (at least without another mechanism of getting back to it, e.g. via this). Your idea (not a proposal without an issue) is a complete 180 of that.

wanton7 commented 6 years ago

Maybe it's just bad idea for C#. I just feel that naming is one of the hardest part of coding, so re-using names would be awesome.

DavidArno commented 6 years ago

Maybe it's just bad idea for C#. I just feel that naming is one of the hardest part of coding, so re-using names would be awesome.

That's a matter of opinion. I'd actually say it would be the total opposite of awesome. It would be "totally bogus". But that's all pure opinion on both sides.

I think the keyword you are looking for is dynamic, not let:

dynamic foo = "text";
foo = 5; // compiler is happy

If fact, you could just define:

public static class Globals
{
    public static dynamic foo;
}

and re-use that one name everywhere. I'm guessing that would be the pinnacle of awesomeness for you?

wanton7 commented 6 years ago

@DavidArno didn't i just write it's bad for mutable variables while ago?

wanton7 commented 6 years ago

@DavidArno and i don't know why i made you so angry that you need to belittle me?

iam3yal commented 6 years ago

At this point I really think we should discuss this in another issue, it's a divergence of the original proposal and we don't need to mock one another for our opinions, let's keep this strictly professional guys.

Richiban commented 6 years ago

Has this feature been abandoned? I seem to remember reading on some other random issue that the team had done a bunch of work on a let feature but then dropped it without too much explanation.

iam3yal commented 6 years ago

@Richiban Well, it's still championed so I doubt it's abandoned, probably not prioritized by the looks of it.

HaloFour commented 6 years ago

@Richiban

IIRC the let proposal was a bit more important back before pattern variables were "leaky" and mutable as a way to introduce those variables in an outer scope. The readonly-ness of the variable would have been an side effect of pattern matching. Without the need to provide that functionality I think put the notion of readonly params/locals and any shorthand for them on the back burner.

DavidArno commented 6 years ago

@wanton7,

@DavidArno and i don't know why i made you so angry that you need to belittle me?

My apologies. You didn't make me angry and I was trying to criticise the idea that, since naming is hard, a solution is to re-use names. Belittling you wasn't the intention, but I accept I got carried away and ended up doing so. Sorry about that.