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

Do not use in as a synonym for ref readonly #1133

Closed
mikedn opened this issue Nov 22, 2017 · 54 comments
Closed

Do not use in as a synonym for ref readonly #1133

mikedn opened this issue Nov 22, 2017 · 54 comments

Comments

@mikedn
Copy link

mikedn commented Nov 22, 2017

ref readonly is a rather exotic language feature - it's only useful for certain optimizations and nothing else. The use of an innocuous and terse keyword such as in is simply not warranted and it looks like it is a significant source of confusion:

#1126
https://github.com/dotnet/corefx/issues/25355
dotnet/roslyn#23327
dotnet/runtime#77625

@jnm2
Copy link
Contributor

jnm2 commented Nov 22, 2017

Besides me liking the symmetry, which I understand doesn't mean anything to many of you, why did we have to have this conversation after the ship has sailed? 😟

@HaloFour
Copy link
Contributor

@jnm2

Besides me liking the symmetry, which I understand doesn't mean anything to many of you,

What about the in keyword makes it symmetric? The lack of a modifier already implies "in", by virtue of being by value. The in keyword doesn't clarify how the argument is passed, just that it doesn't change. It doesn't imply any of the additional caveats that come with the use of the feature.

why did we have to have this conversation after the ship has sailed?

C# 7.2 hasn't shipped yet. It may be late in the process but I'd like to think that there's still the opportunity to reconsider.

@jnm2
Copy link
Contributor

jnm2 commented Nov 22, 2017

@HaloFour

What about the in keyword makes it symmetric?

As you know, it completes the list of by-ref passing semantics. We had read-write and write but not read.

@VisualMelon What if it was required at the call site? I'd strongly prefer writing in rather than readonly ref at every call site.

@mikedn
Copy link
Author

mikedn commented Nov 22, 2017

the symmetry, which I understand doesn't mean anything to many of you

Symmetry can be useful to tip the balance one way or another when other decision factors are present. But using symmetry as the sole decision factor is a dangerous game to play in language design.

Should in int Foo() be valid as well? Or perhaps it should be out int Foo()? Or should void Foo(in int) be identical to void Foo(int) since parameters are "in" by default and we use out to change that default?

why did we have to have this conversation after the ship has sailed

Perhaps because the language team is trying to shove in features in minor language versions? Not that adding features in minor language version would be a bad thing. But you'd think they'd be more careful with that. The natural approach here would have been to allow only ref readonly and then, if the feature is found to be extremely valuable, consider adding in as a synonym.

@HaloFour
Copy link
Contributor

@jnm2

We did have "read", the lack of a modifier. The fact that it's by-value is an implementation detail and not one that should be relevant the vast majority of the time. Nothing about in clarifies that the parameter is treated in any way differently to the current "read"-only parameters. There is nothing that implies that the parameter is a ref, like there is with out by virtue of the fact that the value must be modified.

Most developers don't understand (and don't care to understand) the nuances of passing parameters by value or by reference. In the majority of interviews I give for senior level developers they confuse the difference between reference/value types and reference/value parameters. Throwing something innocuous like in into the mix will only serve to confuse developers. Not to throw @DavidArno under the bus but even he, a seasoned C# developer who I think we all agree has a decent understanding of programming languages, confused exactly how this thing works and the ramifications it would have on his code. What do we think novices will do when they hear, "this new in thing makes your code faster!"?

@jnm2
Copy link
Contributor

jnm2 commented Nov 22, 2017

C# 7.2 hasn't shipped yet. It may be late in the process but I'd like to think that there's still the opportunity to reconsider.

Every time people asked, this is how the team told us they intended us to know when things were RTM, so that's what I took this to indicate:
https://blogs.msdn.microsoft.com/dotnet/2017/11/15/welcome-to-c-7-2-and-span/

And there's https://docs.microsoft.com/en-us/dotnet/csharp/whats-new/csharp-7-2 and other docs.

But I'm just as happy to assume no one has taken dependencies on it because 15.5 is prerelease! 😃

@jnm2
Copy link
Contributor

jnm2 commented Nov 22, 2017

@HaloFour You make a strong argument but I remain unconvinced that it's the apocalypse everyone is saying. One or two or three people jumped to conclusions; that's not unique for a new language feature. I'll just wait and see who wins this one, I guess. =D

@VisualMelon
Copy link

VisualMelon commented Nov 22, 2017

@jnm2 if I was worried about an apocalypse I wouldn't be using a computer at all: I'm worried about C# becoming harder to learn and harder to use, and this proposal is a cheap and easy way to avoid what a lot of people agree could create a lot of misery. (I'm drafting another issue about requiring in at the call-site, because that allows existing APIs to change silent and break in difficult to debug ways). As Halo has already suggested, nobody understands in (question: how many devs understand the use of readonly struct fields? answer: more than will understand in, because that is only a subset of the confusion it entails). I don't even understand in, because I don't know enough about how the assumptions the compiler makes about an application being single-threaded. I don't even understand ref fully, but that's OK because ref has a sensible set of use cases that people stick to, and avoid it when it isn't appropriate, and (most importantly) they acknowledge it at the call-site: NONE of this is (or can reasonably be expected to be) true for in.

Incidentally, as someone who learned FORTRAN a while back and loved explicitly stating intent of all parameters, I imagine there will be plenty of people that actively use in because they think it conveys their intent: sadly they will be wrong and the caller may suffer. "Show your intent" is a much simpler concept to grasp than anything that starts with "when using members that can be accessed by more than one thread..."

@alrz
Copy link
Contributor

alrz commented Nov 22, 2017

The original design used ref readonly but they changed it to in when they introduced in arguments. dotnet/roslyn#22387

@alrz
Copy link
Contributor

alrz commented Nov 22, 2017

What do we think novices will do when they hear, "this new in thing makes your code faster!"?

the most probable outcome.

@MkazemAkhgary
Copy link

Its never late. You guys discuss to deprecate _ which was valid since c# 1 came out. And now you start to argue for this little mistake at very particular and special case.

Im certain good changes will not hurt any body. Those folks who already used this are fortunately not dead yet so they can fix their code soon.

@CyrusNajmabadi
Copy link
Member

Personally, i don't think 'in' is that bad. However, i do find it icky that i can write:

int Foo(in SomeType t) { }

but i can't write:

in SomeType Foo() { }

Instead, i need to type:

readonly ref SomeType Foo() { }

Seems weird.

But again, this doesn't seem to be a big deal to me.

@CyrusNajmabadi
Copy link
Member

What do we think novices will do when they hear, "this new in thing makes your code faster!"?

This argument is made so much of the time. in practice it does not seem to be an issue. Why? Because it seems like people are actually good at clearly explaining the caveats. like "This can make your code faster if you are passing around very large structs. This is a possible improvement if profiling shows there's an issue".

I think the "this will be abused/misued" boogeyman is thrown around far too often. We've literally seen people thing the sky will be falling, and dogs and cats will be living together in sin due to people abusing some of our features (even including things in C# 7 :)). And yet... life seems to go on just fine. Some people make mistakes. Things get corrected. And all is well :)

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 22, 2017

Note: i'm not really interesting in arguing this point. Needless to say, i'm sure this will be discussed and assessed. But please, let's refrain from overreacting to these sorts of things :)

--

Note, to me, the better argument would be: While 'in' serves to slim down the ugly verbosity of "readonly ref", it seems unfortunate that such a clean and simple syntax would be used for something so esoteric. It seems likely in the future that one might want to use 'in' to mean something else for parameters that would be far more common. Given how specialized readonly-ref is, better to not have it squat on a nice terse syntax that might be more generally valuable for the whole of the c# ecosystem.

in other words, don't play the "this will be abused/is-confusing boogeyman". Play the "in" could be much more useful for more widespread features, and now you've gone and used up that nice syntax for something very niche.

@DavidArno
Copy link

@ CyrusNajmabadi,

I completely agree. For example, in potentially offers a neat way of differentiating between in and out params in a Deconstruct for positional/active patterns. It would be a great shame to shut the door on such things just to save typing readonly ref.

@4creators
Copy link

IMHO replacement of ref readonly with in without any proper period for language feature discussion is one of the bad examples how to handle language development in open source community. If language devs feel that the process should be entirely arbitrary - we get rabbits from hat like deprecate _ which are marked as champions without any prior discussion, and community is informed that during one of LDMs there was decision to change ref readonly to in and change is immediately implemented in Roslyn perhaps it is better to not invite community to jointly develop a language. The proper way to handle process would be to just inform community about decisions and clearly state that we have no voice whatsoever in the process except when devs choose to hear us.

I would prefer to have a clear position on that from C# Language team as after almost 6 months of very closely watching how the process works I think there is a lot to be changed.

@iam3yal
Copy link
Contributor

iam3yal commented Nov 23, 2017

My position stands somewhere between the following points:

Nothing about in clarifies that the parameter is treated in any way differently to the current "read"-only parameters. There is nothing that implies that the parameter is a ref, like there is with out by virtue of the fact that the value must be modified.

Posted by @HaloFour

While 'in' serves to slim down the ugly verbosity of "readonly ref", it seems unfortunate that such a clean and simple syntax would be used for something so esoteric.

Posted by @CyrusNajmabadi

In my opinion, anything not related to the above points shouldn't be and isn't relevant to the discussion, especially the paranoia about it being abused.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Nov 23, 2017

Since in is supposed to help cut down the ugly verbosity of readonly ref, i have an alternative proposal. Simply shorter "readonly ref" to rof. Now you can have ref parameters, and rof parameters.

Problem solved.

@iam3yal
Copy link
Contributor

iam3yal commented Nov 23, 2017

@CyrusNajmabadi rofl works too. 🤣

@MgSam
Copy link

MgSam commented Nov 23, 2017

The in feature is confusing, full stop. It has an edge case usage, yet given its keyword it that suggests it should be commonly used. It's not just a matter of people getting used to it- it's just bad design. It is not a pit of quality. Why would we possibly need a new shortcut keyword for a feature that only a tiny fraction of C# devs should ever need to use?

It would be great if the LDC wouldn't make decisions about features and ship said decisions with giving zero notice to the community about it until after it's shipped... I don't know why I have to keep making this same complaint what seems like every few months.

@MadsTorgersen, it's really really time to delegate the posting of LDM notes to someone else who is responsible for posting them the day after the meeting takes place. It is close to useless for us to see them months after the meetings and decisions have taken place.

And not only that, it doubtless would be helpful for the members of the LDC to have actual documentation while they're going through the design process. I know in my experience at meetings, if someone doesn't take and share minutes, every member of a meeting forgets everything that was said in just a few days.

@iam3yal
Copy link
Contributor

iam3yal commented Nov 23, 2017

@CyrusNajmabadi

Not sure whether you were serious about rof but something like inref can also be considered so it isn't as verbose as readonly ref and not as terse and beautiful (😄) as in and finally it may hint something about its meaning.

@axel-habermaier
Copy link

@eyalsk: Or simply make in synonymous to readonly in method signatures, so that you can write in ref X x instead of readonly ref X x for pass-by-reference parameters as well as - in the future - in X x instead of readonly X x for pass-by-value parameters.

@MkazemAkhgary
Copy link

I don't see anything wrong about readonly ref. just forbid in all at once, problem solved.

readonly ref is self explanatory and doesn't encourage excessive usage because of its verbosity. which is all good. why keep twisting it?

@alrz
Copy link
Contributor

alrz commented Nov 23, 2017

@MgSam wapped this up. now it's just too late. Any suggestion here should be additive to be even considerable - which itself is a downside because they will look at it as a new feature in the language and it has to be something with significant value, not just aliasing keywords. also, the syntax has been chosen, so adding "better" ones won't change a thing about it.

@alrz
Copy link
Contributor

alrz commented Nov 23, 2017

I don't see anything wrong about readonly ref. just forbid in all at once

according to the issue I linked above, one of the reasons was to introduce in arguments. now, one may argue that ref readonly could be too noisy for an argument.. but

  1. it would have been optional anyways, and
  2. we could take in arguments any day (for passing to ref readonly parameters) - which is also symmetrical with out arguments .. (if you care). I think that would have been a lot better than "ref readonly return" vs. "in parameter" situation.
 // inconsistency between return ref kind and parameter ref kind
ref readonly int M(in int param)
M(in arg);

// vs

// inconsistency between parameter ref kind and argument ref kind
ref readonly int M(ref readonly int param)
M(in arg);

@HaloFour
Copy link
Contributor

I completely agree that readonly ref it's very noisy for an argument. If it was associated with a commonly used feature both to author and consume I could completely understand the desire to provide the shorthand. But it's associated with an "esoteric" feature, as @CyrusNajmabadi described it, and it's only necessary to decorate the parameters of a method since it can be omitted at the call site. That's a small percentage (library authors) of a small percentage (esoteric). I don't think that 6 extra characters remotely represents a burden in those cases.

I'm with @CyrusNajmabadi and @DavidArno in that in could be better used to describe other situations, such as input parameters to active patterns.

A quick note about my concerns with (mis|ab)use of readonly ref in general. I am not referring to pathological abuse, which can be applied to nearly every language feature. I am very specifically referring to use by developers who don't fully grasp the ramifications of the feature. It's presented as a way to improve performance, which is incredibly alluring. But the circumstances under which it actually provides that improvement are narrow and tricky. I understand that the compiler will warn when you try to use an instance member which will hopefully help. Although I know too few C# developers who actually read those warnings as long as the code compiles.

@iam3yal
Copy link
Contributor

iam3yal commented Nov 23, 2017

@axel-habermaier yup, this can work too but I still think that as noisy as it might be readonly ref is fine.

@erik-kallen
Copy link

I saw someone the other day that claimed they would use in for all parameters to prevent them from being modified in the method body because C# doesn't have Java's final or C++'s const keyword to do this. I can see other people doing this as well, and I don't think that is a good reason to change the signature of a method to contain ref types.

@DavidArno
Copy link

I saw someone the other day that claimed they would use in for all parameters to prevent them from being modified in the method body because C# doesn't have Java's final or C++'s const keyword to do this.

Really? Who would propose doing such a thing? 😱

I was that person, BTW - or there's more than one of us thinking of doing that 😉. But it's a legitimate concern. By publicly declaring that intent before actually doing it, I got lots of smart people explaining to me why it was a bad idea. So that idea is dumped by me. Others might not have access to sound advice before they head of down such a path.

@alrz
Copy link
Contributor

alrz commented Nov 23, 2017

I'd unconsciously planned to do it as well. Seriously, it really does "look" like a great idea, I'm in.

@CyrusNajmabadi
Copy link
Member

I don't actually see what the problem is TBH. It's like someone saying "i want to immutable classes for everything" and someone else else being horrified because "but then mutations will involve allocating something new! and allocations are expensive!". These are all tradeoffs. If the patterns and practices, in balance, work out better for your domain go do it :)

@bondsbw
Copy link

bondsbw commented Nov 24, 2017

Just do readonly ref for now. Let it marinade and if you still feel there is no better use of in, do that in a future version.

@iam3yal
Copy link
Contributor

iam3yal commented Nov 24, 2017

@bondsbw Yup, exactly my thoughts but by the look of things it's not going to happen and it's here to stay. 😄

@jnm2
Copy link
Contributor

jnm2 commented Dec 4, 2017

C# 7.2 is now RTW with VS 15.5, so this issue would be a breaking change.

@mikedn mikedn closed this as completed Dec 4, 2017
@erik-kallen
Copy link

Seriously? Are you just closing the most upvoted issue I have seen forever?

@jnm2
Copy link
Contributor

jnm2 commented Dec 4, 2017

@erik-kallen Here's a list of the most-upvoted issues: https://github.com/dotnet/csharplang/issues?q=sort%3Areactions-%2B1-desc. I don't even see this issue on the first page.

@MgSam
Copy link

MgSam commented Dec 4, 2017

@jnm2 To be fair, the highly upvoted issues usually come when an issue is linked to elsewhere on the internet, or a high profile member of a team posts an issue and gets their entire team to come and vote for it. Not to say those votes don't count, just that it greatly skews the relative totals and thus relative interest.

@jnm2
Copy link
Contributor

jnm2 commented Dec 4, 2017

@MgSam Perhaps this is the fastest-upvoted issue, then. :D

@mikedn
Copy link
Author

mikedn commented Dec 4, 2017

Well, there's not much point in keeping this open. C# 7.2 is out, with its good and not so good features. It is what it is.

Thanks for comments! And votes 😄

@erik-kallen
Copy link

@jnm2 All top-voted issues there are from like Feb through May, except for this issue and dotnet/roslyn#1161, which is about issues with the development process and this misfeature (IMO) is an example of how the current process doesn't work.

So I would definitely say that the community has a very strong dislike of this feature. Given how disliked the feature is, that the feature has not been out for long, and that the feature is probably not very well used, perhaps it could be used as a testbed for how to retire a poor feature? Like make it a warning in C# 7.3 and then remove it in C#8 or something. Perhaps with a code action to automate the fix (replace in with ref readonly). Just closing it because you pushed it through too quickly is not a very open way of handling it.

@erik-kallen
Copy link

erik-kallen commented Dec 4, 2017

And, @MgSam do you really think people brigade the C# language discussion forum? It is of course possible but the attitude "we're getting a lot of downvotes, it must be brigading" is not a very community-friendly one.

Edit: @MgSam said that, not @jnm2 as this comment originally stated

@CyrusNajmabadi
Copy link
Member

So I would definitely say that the community has a very strong dislike of this feature. Given how disliked the feature is, that the feature has not been out for long, and that the feature is probably not very well used, perhaps it could be used as a testbed for how to retire a poor feature?

Breaking changes have an enormous large bar to meet. Breaking changes with far more beneficial impact have been routinely rejected for not meeting that high bar.

Breaking people's code just because "the community has a very strong dislike of this feature" has practically no chance of happening.

@jnm2
Copy link
Contributor

jnm2 commented Dec 4, 2017

@erik-kallen

And, @jnm2 do you really think people brigade the C# language discussion forum? It is of course possible but the attitude "we're getting a lot of downvotes, it must be brigading" is not a very community-friendly one.

Where did I say that? You've lost me.

@erik-kallen
Copy link

erik-kallen commented Dec 4, 2017

@jnm2 Sorry, I misread the comment. Please accept my apology

The person who said people are brigading was @MgSam

@erik-kallen
Copy link

@CyrusNajmabadi

Breaking people's code just because "the community has a very strong dislike of this feature" has practically no chance of happening.

Perhaps that is something that will need to change. With the immensely higher speed of language evolvement nowadays, there will inevitably come features that were not enough thought through before they were released. If we never take breaking changes, we might end up with a messy situation 5 years down the road.

@DavidArno
Copy link

@CyrusNajmabadi,

Breaking people's code just because "the community has a very strong dislike of this feature" has practically no chance of happening.

This is of course the whole point to the existence of dotnet/roslyn#1161. Since the team will not change a feature after the event, just because the community doesn't like it, then maybe, just maybe, it would be useful to ask the community for feedback before releasing.

@CyrusNajmabadi
Copy link
Member

Perhaps that is something that will need to change.

Only if the team feels that tehre is an actual issue. We'd have to see the actual impact on the ecosystem first before preemptively going and breaking people. If there's no actual issue, or hte issue is minor in practice, then the team isn't likely to break people just because some people are asking for it.

With the immensely higher speed of language evolvement nowadays,

I don't see language speed being higher. Seems to be about the same pace as normal. :)

there will inevitably come features that were not enough thought through before they were released. If we never take breaking changes, we might end up with a messy situation 5 years down the road.

Then things can be addressed then. I don't see any reason to break people because of speculative problems that may actually not come to pass.

--

Or, in other words: i've heard dozens of times while working on the C# language about decisions that people were certain were going to be bad. So bad that we shouldn't have shipped with those decisions. So bad that we should take breaking changes to address them. In my time, how many turned out to actually need fixing? 1 (maybe 2). So i'm fine with a 'wait and see' attitude on all of this. If there's an actual problem, we have mechanisms to address it. If there's no problem, then best to not fret and do something hasty preemptively. :)

@bondsbw
Copy link

bondsbw commented Dec 5, 2017

@CyrusNajmabadi

Breaking changes with far more beneficial impact have been routinely rejected for not meeting that high bar.

But that has to be weighed against how much code is using the feature. Right now, the usage of in is practically zero and quick action on the part of Microsoft and the LDM can ensure it remains that way.

@jnm2
Copy link
Contributor

jnm2 commented Dec 5, 2017

@bondsbw I almost put an in signature into production last week. The next interop code I write will definitely have in as well as out.

@yaakov-h
Copy link
Member

yaakov-h commented Dec 5, 2017

@bondsbw as soon at it hits RTM, it's too late. Usage can grow with RTM bits even if Microsoft released a 15.0.1 within hours.

@tannergooding
Copy link
Member

To be clear, it generally becomes too late a bit before RTW even, RTW just makes it a breaking change, officially.

The product and any features shipping are locked down and finalized well before the actual ship date and as the ship date approaches, the bar for getting a change in also gets higher.

@MkazemAkhgary
Copy link

Have fun with it #1133 (comment)

@HaloFour
Copy link
Contributor

HaloFour commented Dec 5, 2017

@tannergooding

The product and any features shipping are locked down and finalized well before the actual ship date and as the ship date approaches, the bar for getting a change in also gets higher.

And before the design meeting notes on the subject are posted.

@DavidArno
Copy link

@tannergooding,

To be clear, it generally becomes too late a bit before RTW even, RTW just makes it a breaking change, officially.

Possibly just repeating @HaloFour's point here, but that is why the LDM notes need to be published in a timely fashion, before it is too late to canvas opinion on those design decisions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests