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 "Private protected" (C# 7.2, VB 15.5) #37

Open
MadsTorgersen opened this issue Feb 9, 2017 · 64 comments

Comments

@MadsTorgersen
Copy link
Contributor

commented Feb 9, 2017

@paulomorgado

This comment has been minimized.

Copy link

commented Feb 14, 2017

I'm still in favor of protectedinternal.

@gafter

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

@paulomorgado Do you know why this feature didn't make it into C# 6?

@alrz

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

Do you know why this feature didn't make it into C# 6?

The syntax was not good enough? 😁

@paulomorgado

This comment has been minimized.

Copy link

commented Feb 14, 2017

You know I do, @gafter! 😄

To me, protected internal says that it's protected or any derived class internal. Meaning it is accessible to any derived class or any type in the same assembly.

protectedinternal says to me protected and internal. Meaning it is accessible to any derived class in the same assembly.

protectedinternal wouldn't be the first multi word keyword.

Any other proposal either doesn't look like anything seen in C# in the last 15+ years or builds on top of not using protectedinternal introducing different ways to do the same thing.

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

Oh not this again. protectedinternal suffers from the problem of being excessively long and not different enough (not everyone uses monospace fonts in their IDEs). My opinion is that the team should just go with private protected or whatever combination of keywords they want and ignore the 500 other options that aren't any better. I'm fine with private protected. C++/CLI already uses it for this very accessibility modifier.

@alrz

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

In fact, I think protectedinternal is even more confusing as it's only different in a single whitespace. Also, the fact that private protected doesn't need syntax changes, makes it more preferable IMO.

@marek-safar

This comment has been minimized.

Copy link

commented Feb 14, 2017

If there is planned work in this area I'd like to see also introduction of private internal to control if internal types/members are affected by InternalsVisibleTo.

The idea is to use private internal if you don't want your internal types/members be visible outside of an assembly when InternalsVisibleTo is used.

@Joe4evr

This comment has been minimized.

Copy link

commented Feb 14, 2017

Does the CLR even support such a scenario, or does such a hypothetical private internal maybe not need CLR support?

@marek-safar

This comment has been minimized.

Copy link

commented Feb 14, 2017

@Joe4ever This is pure language feature, no CLR involvement

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2017

@marek-safar

IIRC, using private internal as you suggested would require CLR changes as how InternalsVisibleToAttribute works is governed by the CLR.

And it should be a separate proposal.

@gafter

This comment has been minimized.

Copy link
Member

commented Feb 14, 2017

Just to be clear everybody, the precise syntax had been debated ad nauseum a few years ago and this is the winner. If we do the feature the syntax will be private protected. If the community really hates that then we simply won't do the feature.

@paulomorgado

This comment has been minimized.

Copy link

commented Feb 15, 2017

@gafter,

Just to be clear everybody, the precise syntax had been debated ad nauseum a few years ago and this is the winner. If we do the feature the syntax will be private protected. If the community really hates that then we simply won't do the feature.

That has defeat written al over it.

Currently, access modifiers worked as a union. When more than one is allowed, each one retains the same semantics. If something is protected it doesn't change its "protectiveness" by adding or removing the internal modifier. That doesn't happen with the proposed solution.

I "hate" it because it hinders the learning of the language and the understanding of the code.

I know my proposal has a drawback that, regardless of the font used, an acidental removal or insertion of a white space will change the meaning of the program.

And I don't care that C++/CLI made the same mistake.

At this point I would prefer some new keyword like shielded or sheltered.

@jnm2

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

private protected makes enough sense. It's fine that it isn't purely etymologically consistent. No one thinks through the logic anyhow; it's just a convention we've memorized. Also, subjectively, private protected is the least awkward sounding name I've heard so far.

@axel-habermaier

This comment has been minimized.

Copy link

commented Feb 15, 2017

@paulomorgado: This has been debated over and over again. There is no good solution. It's either private protected or we'll never get the feature. I'm not happy with private protected either, but I also don't have a better solution. So let's just live with that and let's get that feature into the language!

@alrz

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

@paulomorgado internally protected.

@paulomorgado

This comment has been minimized.

Copy link

commented Feb 15, 2017

@axel-habermaier, looks like it hasn't been debated long enough.

Any outcome that is not a single keyword is not a cure. It's just a Band-Aid that we'll have to live with forever.

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

@paulomorgado

Yeah, it has been debated enough. The team should either execute or drop.

This is a feature with such a limited use case that it doesn't deserve the kind of debate that it's already received. It doesn't solve any problems. It's at most a tool to help developers not use their own source incorrectly.

And protectedinternal is awful. Not everyone uses monospace fonts and that whitespace will be totally lost, assuming someone doesn't fat-finger and mess it up to begin with.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

@paulomorgado, having a language construct that is created from multiple keywords is not that unusual. In some cases, like C++ they separate the words with underscore (thread_local for example). The use case here is potentially small, and the way it is constructed makes sense and will be familiar to anyone who has declared similar types before (such as in C++/CLI).

@paulomorgado

This comment has been minimized.

Copy link

commented Feb 15, 2017

@tannergooding,

What other keyword in C# uses underscore?

What other language construct in C# uses more than one keyword?

@alrz

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

__arglist, __refvalue, __makeref, __reftype for one.

language construct in C# uses more than one keyword?

yield return or all other modifiers?

@tannergooding

This comment has been minimized.

Copy link
Member

commented Feb 15, 2017

using static as well

@bbarry

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

I'd say async/await is a single language construct that uses more than one keyword to express.

@alrz

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2017

@bbarry Actually wait for was also considered before await just that it did not require async.

@paulomorgado

This comment has been minimized.

Copy link

commented Feb 16, 2017

I knew that, if gave enough rope, plenty would jump to wrap it around their necks. 😄

__arglist, __refvalue, __makeref, `__reftype

These keywords are not on the list of the C# keywords on the documentation or the C# Language Specification. If the recommendation is for this feature to be tuck away hidden from the non illuminati, then, by all means, __protected_internal.

yield return

I would argue that yield is one thing and return and break another. But, yes, they are the Yield Statements, not the Yield Statement.

other modifiers

Which ones? Are you counting protected internal and internal protected as two. Or are you saying that abstract, virtual, abstract, override or async are access modifiers?

using static

I would argue that static is part of the argument of the using directive which is a one keyword only construct. Just that until now there was no need for using namespace ... and using alias .....

async/await

Hardly. I can use async without any await, although I can only use one or more awaits with one async.

In any case, async is a method modifier and await is, like yield, some sort of statement modifier.

@jnm2

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2017

-_-

@gafter gafter added this to the 7.1 candidate milestone Feb 21, 2017

@Athari

This comment has been minimized.

Copy link

commented Mar 18, 2017

I wonder why it's private protected and not private internal. What makes protected part of protected-and-internal more important than internal part?

I'd rather have something messy like protected&internal and protected|internal (with protected internal left for compatibility) than a combination of keywords that leaves out 50% of information it needs to convey. 😕

@gafter

This comment has been minimized.

Copy link
Member

commented May 10, 2017

@DavidArno Please make a separate proposal for that. This issue is about providing a syntax for a protection level that is already supported in the CLR and in other languages.

@MadsTorgersen MadsTorgersen modified the milestones: 7.1 candidate, 7.2 candidate May 16, 2017

@jcouv jcouv referenced this issue May 23, 2017

Closed

Test plan for "private protected" (C# 7.2, VB 15.5) #19722

47 of 72 tasks complete
@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

I think protected internal modifier has powerless syntax. We cannot cover all possible variants...
With this new modifier you want to add one additional trick... So syntax is still powerless and this new modifier is still something not clear and universal. I personally can even say it is ridiculous because considering "protected internal" behavior "private protected" modifier should be a union of private and protected which obviously does not make any sense.

So can we introduce order here?
Let's say protected internal and internal protected are different things?

protected internal means major protection is protected (on 1st place), so internal (on 2nd place) cannot change availability in another assembly. It means it will work like it is now.

internal protected means major protection is internal (on 1st place), so protected (on 2nd place) cannot change availability in another assembly. It means that it will be available only internally being protected.

Does it make any sense?

@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

Alternatively to make it relatively universal we have to allow minimal boolean operators on modifiers.

protected & internal
protected | internal
@axel-habermaier

This comment has been minimized.

Copy link

commented Jun 11, 2017

@hardhub: At this point I think it is pretty clear that the feature either comes as private protected, or not at all. protected & internal or protected | internal have already been suggested in addition to countless other things. There simply is no good alternative. See also the old Codeplex thread.

Additionally, protected internal and internal protected already mean the same thing today, so changing the meaning of one of them would be a serious breaking change which is also not going to happen.

@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

@axel-habermaier

Codeplex

It was time when it was looking very bad to have it that way...
And I do not strongly suggest it because I also do not expect anything except English alphabet there.
But maybe for now it is smallest of all evils? If you think it is still bad.. I can agree but what is good alternative? I am sure it is not 'private protected'!

would be a serious breaking change

Yes breaking change, but not serious.
Consider that ReSharper always suggests it to be as protected internal:
https://www.jetbrains.com/help/resharper/2017.1/ArrangeModifiersOrder.html
Many things in ReSharper is bad but many things including this one are really good!
So we can hope that developers use best practices... and used "protected internal".

But let's consider worst case.. If you not followed such rule you will need only to replace simple text string with Ctrl+Shift+H in VS or any other text editor.
If you did not make that I guess you will not be able to compile code due to access level.. so you will finally replace it.

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2017

Yes breaking change, but not serious.

It's a breaking change, end of story. That makes it a non-starter.

So we can hope that developers use best practices... and used "protected internal".

There is no "best practice" there. The order of the keywords is entirely up to the developer.

As mentioned above, it's private protected (and protected private) or nothing. The arguing over other keywords is done.

@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

@HaloFour

It's a breaking change, end of story. That makes it a non-starter.

Why not? Holy Grail for MS?
Sometimes to do something better we have to think again.

There is no "best practice" there. The order of the keywords is entirely up to the developer.

There is best practice... It is absolutely normal when developer uses the same modifiers order in all cases.
And it is exactly what "best practice" definition means.. If we allowed to do A and B but we decided that it is better to always do A then A is best practice.. And I believe that mostly all developers use protected internal.

it's private protected (and protected private) or nothing

Then nothing, because it is very uninformative and unnatural... protected and internal are not exclusive terms... but private and protected are... and if member can be protected and internal at the same time it is OK, but it is not true for private and protected (and even more ridiculous way: protected private)... So the cognitive dissonance is welcome!

So... in other words... I personally think that small breaking change is better than "private protected" and I explained it enough above.

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2017

@hardhub

Why not? Holy Grail for MS?

Microsoft has existing customers with existing codebases. Unlike some companies, Microsoft prefers to not break all of their stuff on a semi-annual basis. Maintaining backward compatibility is critically important.

Sometimes to do something better we have to think again.

This will be version 8.x, not version 1.x. It's not going to be thought over. Existing syntax is off of the table. Breaking changes are off of the table.

There is best practice... It is absolutely normal when developer uses the same modifiers order in all cases.

Irrelevant. The keywords are perfectly legal in any order. Microsoft has no established "best practices" guidelines on the ordering of those keywords. JetBrains doesn't get to decide any "best practices" either for everyone either. A common convention is not "best practice".

Then nothing, because it is very uninformative and unnatural...

That's your opinion. C++ already went with private and protected and the world didn't end.

I personally think that small breaking change is better than "private protected" and I explained it enough above.

A breaking change is a deal breaker. The team has also already decided that alternative syntax is off of the table.

@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

@HaloFour

Microsoft has existing customers with existing codebases. Unlike some companies, Microsoft prefers to not break all of their stuff on a semi-annual basis. Maintaining backward compatibility is critically important.

Yeah really? Where is Flash-killer? Maybe ASP.Net Core is the same as ASP.Net? Or maybe we did not changed VS from and to msbuild (twice) during last 2 years?
RedHat also supports enterprises but sometimes after many years of LTS they can improve something with not critical but breaking changes.
I just want to say we have what we have: MS does not want any breaking changes in C#... But please do not insist that it is single way to go.

This will be version 8.x, not version 1.x. It's not going to be thought over. Existing syntax is off of the table. Breaking changes are off of the table.

Not sure that I understand.. but if I understand it correctly.. it will mean slowing down of C# evolution after some time because we cannot change what is already made even it is very appreciated.

Irrelevant. The keywords are perfectly legal in any order. Microsoft has no established "best practices".

Hmm... Relevant! (I am also able to say something without any arguments).
Where is I said it is illegal? Maybe when I said that A and B are allowed?
Or maybe it is just the problem with MS? MS can but JetBrains cannot? Fair play ))
Wake up man, it is already in wide open source community.

JetBrains doesn't get to decide any "best practices" either for everyone either. A common convention is not "best practice".

It is already so... Just read all their rules... Many of them is really very very apreciated.
And for many developers ReSharper becomes tool "must have". And it was mentioned just FYI. So many developers always use "protected internal". Also many books about C# teach developers using "protected internal" in examples, not "internal protected" (even if both allowed as I wrote previously).
If you want to understand how much projects will be impacted you can estimate it using search over GitHub open projects codebase.

C++ already went with private and protected and the world didn't end.

Who of us is crazy? Where did you found "private protected" access modifier in C++?
You mixed up the terms.

A breaking change is a deal breaker. The team has also already decided that alternative syntax is off of the table.

I think I wrote enough about breaking changes.
I think if it will be very required the community will fork and create C# Next v1.0...
Where evolution will be more important than any deals...

P.S. If you do not have really new information and arguments then we do not need to continue this discussion to avoid wasting of time.

@HaloFour

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2017

MS does not want any breaking changes in C#.

The team has stated this multiple times. Proposals that suggest redefining existing syntax in a way that changes the behavior of existing code is nearly immediately shot down. A proposal has to demonstrate massive benefit to consider even slight syntactic breaks, and even then only if that potential break would only affect esoteric code.

Hmm... Relevant! (I am also able to say something without any arguments).
Where is I said it is illegal? Maybe when I said that A and B are allowed?

There is no argument. All of the modifiers can be applied in any order. protected static internal async is just as valid as async internal static protected. Preferring one order over another doesn't justify changing the behavior due to reordering.

Who of us is crazy? Where did you found "private protected" access modifier in C++?
You mixed up the terms.

Applies to C++/CLI:

https://msdn.microsoft.com/en-us/library/ke3a209d.aspx#BKMK_Member_visibility

If you do not have really new information and arguments then we do not need to continue this discussion to avoid wasting of time.

I agree. The team already weighed in on whether or not alternate would be considered. Breaking change or not, they're not interested.

@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

Preferring one order over another doesn't justify changing the behavior due to reordering.

Why do you try to tell it again again and again? I know that.. but it does not mean that de-facto developers always sort modifiers in some specific way. But as I said to estimate real situation we can create crawler and check open sourced projects for any impact. Since we have confirmed that breaking changes is not the way we can forget about that.

Applies to C++/CLI:

When somebody tells about "C++" I imagine the following standards: C++98, C++03, C++11, C++14, C++17 but not Managed C++ (C++/CLI). So for now it is clear what you mean... But this managed C++ is designed by MS in parallel to C# and cannot be considered as some independent design. In other words if they chose wrong name and conception then it means they can do the same mistake for C#:

From MSDN:
private protected: Member is protected inside the assembly but private outside the assembly.
They called a camel a "bear kangaroo" or "kangaroo bear"! Nice ))

@tannergooding

This comment has been minimized.

Copy link
Member

commented Jun 11, 2017

Breaking changes, of any kind, all start in the same bucket: "no". It takes a very significant issue (or benefit) to change this.

C# has been around for 17 years, across 7 language versions, and is used by millions of developers around the world. A very large portion of this is closed source enterprise code that we will never be able to look at. This means we have no way of knowing if they follow "common practice" or if a breaking change will or will not impact them.

There are a couple different types of breaking-changes (this is a non-exhaustive list):

  • Those that impact binaries that have already been built/shipped. This means that if the change was taken, it could impact code that has already been compiled and has been shipped for public use. The producer of the binary does nothing and things break, this is potentially the worst type of change.
  • Those that impact existing source code. These changes will change what a users code is doing if they take it and recompile it with the new compiler (things like making the ordering of visibility modifiers relevant fall into this category). This is likely the second worst type of change, users compile their code and it now behaves differently. It can potentially impact any downstream consumers of the newly built library and the library itself.

The primary thing to take into consideration when talking about a breaking change is how much code it could potentially impact. For something that has been around since v1.0 of the compiler, it is 17 years worth of code that could be impacted. For something like accessibility modifiers, which are used everywhere, it also has a high risk of impacting some code somewhere.

If we assume that C# has at least 1 million users (recent Stack Overflow surveys indicate that it is much more than this), and we look at a change will only impact 1% of users, we are still looking at potentially breaking at least 10,000 users, which is a lot.

Every feature request (breaking change or not) has to be weighed on impact. If it is only going to benefit or be used by a couple hundred developers, it might not be worth it. If it is going to be used by a relatively small portion of users, then will the resulting code in turn be used by others? For example, there are a lot of unsafe or interop features which will be consumed by a relatively small portion of users, but it will allow them to more readily write libraries that will, in turn, be used by hundreds or thousands of other developers (things like blittable types fall into this category).

@hardhub

This comment has been minimized.

Copy link

commented Jun 11, 2017

@tannergooding

I agree except 2 things:

Those that impact binaries that have already been built/shipped.

If we are talking about changes to modifiers then it will not be necessary. CLR supports all possible modifiers. Binaries will not be impacted at all.. all binaries should work. Except maybe reflection in very specific case. But in that specific case it will not work for ANY new modifier as well (including one discussing in this issue).

Those that impact existing source code.

Yes this will impact... But why not move this issue to new major release and create migration guide?
It is very strange that they abandoned msbuild then returned it back... We had around 50 projects migrated to project.json... and now back... They abandoned Silverlight in all major browsers when official support claimed till 2021... But they did that... they did not update SL plugin considering new plugin model in browsers. To be supported by all major browsers we had to rewrite all in frontend...
But to fix access modifier we just need 5-10 mins to use Replace All. It is not comparable impact!

Alternatively let's start new generation of C#... It was absolutely possible to create new .Net Core. Why not C#!? We even can introduce build levels depending on language version and still have enough compatibility...
But all that is not our topic I think... So just to summarize regarding current issue I will repeat myself: private protected is worst name ever used/suggested for that access modifier.
I think we should not repeat Managed C++ implementation and since it is dead I even do not see any reason to do that.

@tannergooding

This comment has been minimized.

Copy link
Member

commented Jun 11, 2017

Alternatively let's start new generation of C#

This sounds like something that should go into a new issue 😉

It was absolutely possible to create new .Net Core

It's true that .NET Core is "new" and "different", but that is mostly just in some Desktop APIs not being available. Everything else is essentially compatible and changes made to .NET Core should get ported back to desktop eventually. It isn't like we went and fixed all the API issues that exist in the Desktop framework when .NET Core was created...

private protected is worst name ever used/suggested for that access modifier.

You aren't the only one that feels this way, but I believe the actual language team has already decided on this (coming from re-skimming the comments made so far). If you really feel that strongly, the source for the compiler is open and you are more than welcome to fork the repository and "fix" this in your cop (and maintain the change through future updates to the primary repo so you get other new language features as well).

@CyrusNajmabadi

This comment has been minimized.

Copy link

commented Jun 11, 2017

Roslyn already has code that uses "internal protected". Changing of the meaning of that could literally make the existing Roslyn codebase uncompilable by later versions of the compiler.

@gafter gafter changed the title Champion "Private protected" Champion "Private protected" (C# 7.2) Sep 23, 2017

@Adassko

This comment has been minimized.

Copy link

commented Sep 28, 2017

Why not just combine all three and name it "protected internal private"?
It makes more sense for me as it's more "internal" than "private"

@jnm2

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

Just no.

@Stevie-O

This comment has been minimized.

Copy link

commented Oct 18, 2017

I may be a bit late to all this, but I'm not sure I quite see this feature being particularly beneficial.

As I understand it, the goal is to add syntax to C# for the CLR's built-in famandassem accessibility mode, where the class member is only accessible to (1) members inside the declaring class, and (2) members inside a descendant class, iff that descendant class is also defined in the same assembly as the declaring class.

We can't use internal protected or protected internal because today, both of those things mean famorassem -- i.e. accessible to (1) members inside the same assembly as the declaring class, and (2) members inside a descendant class.

As far as I can tell, the only benefit this offers over 'internal' is as follows: Someone writing code in your assembly can't call your method (or access your field/property, or hook your event) unless they also inherited from your class.

Is there really that large a demand for this? If I were writing a method that required the constraints of famandassem, I would just mark it internal and clearly document for my fellow developers how it's intended to be used. If I'm really paranoid, I can just start the method with Debug.Assert(this is MyType); and let it be caught in testing. But before all that, I would seriously consider whether or not it was truly appropriate to restrict access that way.

I could see this feature being useful if:
(1) There was a major use case that could not be solved without this functionality (despite not having had it for over 12 years)
(2) We could come to an agreement on syntax we liked

But we don't seem to have either of these things. And if I were the one making the call, I would hold off on until we had at one of these two things.

@gafter

This comment has been minimized.

Copy link
Member

commented Oct 18, 2017

@Stevie-O Too late, sorry. This is already in C# 7.2.

We had a large discussion about the syntax in the C# 6.0 timeframe, and the community was much more upset by the fact that we pulled the feature because of the discussion than about any particular proposed syntax. The syntax issue was solved in C++, where private protected has been available for years.

Yes, the feature is useful. Just like private is useful, even though you could get away without it just by asking your developers to apply some self-control and spend more time reading documentation. We like to save programmer time by automating such things.

@jcouv jcouv changed the title Champion "Private protected" (C# 7.2) Champion "Private protected" (C# 7.2, VB 15.5) Nov 7, 2017

@gafter gafter added this to C# 7.2 in Language Version Planning Mar 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.