dotnet / csharplang

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

Champion "Private protected" (C# 7.2, VB 15.5) #37

Open MadsTorgersen opened 7 years ago

MadsTorgersen commented 7 years ago
paulomorgado commented 7 years ago

I'm still in favor of protectedinternal.

gafter commented 7 years ago

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

alrz commented 7 years ago

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

The syntax was not good enough? 😁

paulomorgado commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

marek-safar commented 7 years ago

@Joe4ever This is pure language feature, no CLR involvement

HaloFour commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@paulomorgado internally protected.

paulomorgado commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

@tannergooding,

What other keyword in C# uses underscore?

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

alrz commented 7 years ago

arglist, refvalue, makeref, reftype for one.

language construct in C# uses more than one keyword?

yield return or all other modifiers?

tannergooding commented 7 years ago

using static as well

bbarry commented 7 years ago

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

alrz commented 7 years ago

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

paulomorgado commented 7 years ago

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 usingdirective 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 commented 7 years ago

-_-

Athari commented 7 years ago

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. 😕

alrz commented 7 years ago

Head meets desk.

Athari commented 7 years ago

@alrz What?

I know I'm late to the party. Like, 3 years late or so. But I still find private protected FUBAR.

jnm2 commented 7 years ago

@Athari https://github.com/dotnet/csharplang/issues/37#issuecomment-280013888 Also, people are incredibly tired of the argument. Perpetuating the argument has prevented and will continue to prevent this useful feature from appearing in any form.

Athari commented 7 years ago

@jnm2 When did private protected win, by the way? It lost badly in the poll on CodePlex. Just because of C++/CLI?

HaloFour commented 7 years ago

@Athari

When the team decided so. At this point it's private protected or not at all.

gafter commented 7 years ago

Unfortunately all of the options other (other than private protected) are quite expensive or irregular, for example introducing new keywords (a breaking change) or adding operator characters such as & to the modifiers, or making the modifiers order-sensitive.

DavidArno commented 7 years ago

When the team decided so. At this point it's private protected or not at all.

Does that mean "not at all" is still an option? 😇

GeirGrusom commented 7 years ago

Does that mean "not at all" is still an option? 😇

It's ridiculous that a useful feature should be dropped because people get caught up in unimportant superficial issues like this. It's private protected. Now come to terms with that and simply move on.

DavidArno commented 7 years ago

@GeirGrusom,

You misunderstand. For me, this is a worthless feature. I don't care what it's called; I have zero use for it. Thus why I was asking whether "not at all" was still an option. I'd prefer the team focused on things that are useful to me... 😀

alrz commented 7 years ago

This will be useful with other concepts like exhaustiveness,. Records and ADTs could take advantage of it, maybe.

DavidArno commented 7 years ago

@alrz,

Hmm, that's a good point: the choice of keywords used here could adversely affect the trait interface proposal. For example, an internal member in a public interface could mean "only internal types can access this member" or "only internal types can implement this interface" (the latter being incredibly useful for ADTs). private internal could be used to indicate one of those options and internal, the other. Likewise, if the team go down the route of a public method in a class can implement/override a protected member in an interface, then private protected could be used to mandate that the implementation must be protected.

So I think the team should delay this feature until trait interfaces are properly worked out to avoid causing themselves problems later.

gafter commented 7 years ago

@DavidArno We are not pursuing any of the gyrations you suggest regarding protection in the traits proposal.

tannergooding commented 7 years ago

@gafter, even if you aren't pursuing it today, some future version of the language (even 5-10 years down the line) might want to pursue it (see IntPtr operators). So, I think the ask here is to ensure that being unable to (easily) support such a feature in the future is definitely the decision that wants to be made.

gafter commented 7 years ago

@tannergooding Overrides in interfaces already have nearly exact analogs to the hypothetical situations with interfaces, and never has anything like that been proposed. I don't think it is reasonable to "preserve" this particular sequence of keywords for such a hypothetical future language feature. We do not permit an override method in a class to have different access than the method it overrides, and I do not believe (if we even allow implicit overrides in interfaces) that we would want to change that in interfaces.

DavidArno commented 7 years ago

@gafter

We are not pursuing any of the gyrations you suggest regarding protection in the traits proposal.

So the discussions between yourself and Cyrus around this comment in the default methods thread came to nothing then? Perhaps I was very clear in what I said above, but I was referring to ideas discussed in that other thread and so I'm surprised you now claim all that has been cast aside.

CyrusNajmabadi commented 7 years ago

@DavidArno Sorry, i'm a bit lost. What are you trying to say?

DavidArno commented 7 years ago

@CyrusNajmabadi,

You are not the only one that's lost.

Here, @gafter states:

We can model this with the "required" methods being protected abstract, and the "provided" methods being public. ... This is not esoteric, but central to one of the use cases for the feature

Three comments above this one, he states:

We do not permit an override method in a class to have different access than the method it overrides, and I do not believe (if we even allow implicit overrides in interfaces) that we would want to change that in interfaces

Any idea which one if correct (or in what way they are not equivalent), as I'm really quite lost.

gafter commented 7 years ago

@DavidArno I think you misread that thread. "provided" in that thread means methods provided to clients (users) of the interface deeper in the hierarcy. "required" means methods that an interface's clients (users) must implement deeper in the hierarchy. Think of them like arguments (required) and returns (provided) of a trait. A given method is either one or the other.

DavidArno commented 7 years ago

As I see it the logic behind this proposal is that folk want to use inheritance as an "implementation detail", ie exposing certain methods to sub classes, but only when those sub classes are defined in the same assembly. Now "internally protected" methods are one way of achieving that, thus this proposal. However, there is an alternative way, that would have added benefits: publicly sealed classes.

For some type, let's assume the syntax is:

public abstract sealed SomeClass { ...

That class is then unsealed within its assembly, so can be extended and its protected methods accessed. Outside that assembly though, the class is sealed. This idea would address (most of, at least) the use-cases for private protected. It would also provide an immediate means of creating finite type hierarchies and would lay the groundwork for supporting discriminated union types in a future version.

This would require a CLR change, but as that team will be making changes to support trait interfaces, this change could be made at the same time.

gafter commented 7 years ago

@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.

ghost commented 7 years ago

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?

ghost commented 7 years ago

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

protected & internal
protected | internal
axel-habermaier commented 7 years ago

@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.

ghost commented 7 years ago

@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.