csharplang icon indicating copy to clipboard operation
csharplang copied to clipboard

[Proposal]: Add `private` and `namespace` accessibility modifiers for top-level types.

Open CyrusNajmabadi opened this issue 2 years ago • 79 comments

private and namespace accessibility modifiers for top-level types

  • [x] Proposed
  • [ ] Prototype: Not Started
  • [ ] Implementation: Not Started
  • [ ] Specification: Not Started

Summary

[summary]: Enable the private modifier to be used for top level types in C#. The semantics of this would differ from internal in that while internal means "this assembly and any assemblies with IVT to this", private would mean "this assembly only". This is already the meaning for nested types, but is not available for top level types.

Similarly, enable the namespace modifier to be used for types in c#. The semantics of this are that these types are only accessible within that namespace only (not including sub-namespaces).

Motivation

[motivation]: Currently, based on the v1 rules of the language, the narrowest accessibility a top-level-type (i.e. one directly in a namespace) can get is internal. This was fine in v1 as that meant "within this assembly only". Later, this was found to be problematic for components (including Roslyn, the BCL, 3rd parties) that wish to keep things non-public, but which want to share some types across assemblies. To that end, we added the InternalsVisibleTo (IVT) attribute to allow these types to be seeing by a particular clique of assemblies.

This came with a downside though. Using IVT opens up your entire space of internal types to all these assemblies, even if it is only wanted/desirable for a narrow subset of those types. It also makes it more difficult to know what portion of your symbols actually are actually intended for use in these downstream assemblies, and what just got pulled in because there was no way to be excluded.

For tooling, this also makes things more expensive. For example, in the IDE, tools like 'find refs' have to figure out the set of domains they need to look for something in. Once IVT is involved in a project, then effectively all top level symbols have to be looked for in those larger cliques. Being able to explicitly make types exist only in a single assembly can greatly improve performance.

Similar issues exist for things like ref-assemblies. Right now they must include internal types (if IVT) is on, and will be updated whenever any of those change (even if there is no intent or usage of those types downstream). Having these types truly be 'private' (and thus not included in these artifacts), limits the surface area, and makes more changes less impactful downstream.

Detailed design

Allow private as a modifier for top-level-types. These types would only be visible within that assembly, including when the IVT attribute was present.

Similar to how accessibility is checked today, a private-type could not be exposed by some type/member taht itself was exposed beyond the assembly. For example, it could not be the base-type of an internal type, or a parameter/return type of some internal visible member.

Drawbacks

  1. This definitely makes the language more complex.
  2. This would likely push for codebases to now switch over to 'private' by default.
  3. For back compat reasons, we'd likely have to make it so that a type without accessibility still defaulted to internal.

Design meetings

  • https://github.com/dotnet/csharplang/blob/main/meetings/2023/LDM-2023-10-09.md#add-private-and-namespace-accessibility-modifiers-for-top-level-types

CyrusNajmabadi avatar Dec 12 '22 22:12 CyrusNajmabadi

Does the runtime already support this or is there an implied runtime change dependency?

HaloFour avatar Dec 12 '22 22:12 HaloFour

private class C1
{
}

internal class C2
{
    internal void M(C1 c1) { }
}

I'd like to be able to call C2.M(C1) throughout my own assembly, but accessibility consistency checks will error here because C2.M() is supposed to be callable through an IVT, and C1 is not accessible through an IVT. What do I do?

RikkiGibson avatar Dec 12 '22 22:12 RikkiGibson

What do I do?

Make M private, or make C1 internal. Right now this is the IVT problem. you're exposing MS to everything in IVT. Unclear if we have have a middle ground for members that says "internal, but only to this assembly".

CyrusNajmabadi avatar Dec 12 '22 22:12 CyrusNajmabadi

@RikkiGibson To me, that example says that since a new kind of accessibility is being added, it needs a new keyword, that can then be used everywhere. And private internal sounds right to me (it's more "private" than internal accessibility), except it's too long for something that's meant to be widely used. So maybe private on a top-level type could be a shorthand for private internal?

svick avatar Dec 12 '22 22:12 svick

I feel it seems a bit difficult to teach beginners the difference between private and intenal, because there are not so many opportunities for beginners to use IVT attributes.

ufcpp avatar Dec 13 '22 01:12 ufcpp

I imagine you wouldn't teach beginners this.

CyrusNajmabadi avatar Dec 13 '22 05:12 CyrusNajmabadi

Isn't the private keyword too casual to use for something that should not be taught?

ufcpp avatar Dec 13 '22 05:12 ufcpp

Why would this not be taught? It may not be a beginner topic, but that hardly relevant here.

CyrusNajmabadi avatar Dec 13 '22 06:12 CyrusNajmabadi

Even now, people try to put private on top-level types mistaking it for internal. I think that if we introduce the private modifier, people will use private unintentionally and without worrying about the difference between it and internal. My concern is that private - a common word - is associated with IVT - a less familiar feature.

ufcpp avatar Dec 13 '22 07:12 ufcpp

would namespace visibility be visible across assemblies? as in, if I opened up some namespace i dont control from an assembly i'm just depending on, would I be able to see their namespaced types?

I see some positives here if that is the case, although it might lead to people doing things they really shouldn't and creating fragile code when another assembly pushes a new version.

RoyAwesome avatar Dec 13 '22 07:12 RoyAwesome

Even now, people try to put private on top-level types mistaking it for internal. I think that if we introduce the private modifier, people will use private unintentionally and without worrying about the difference between it and internal.

Why would that be a problem?

If they're not using ivt it won't matter. If they are using ivt, then private will still be a good choice for limited visibility. And if they actually need to expose the type to one of their other assemblies, they can make it internal.

CyrusNajmabadi avatar Dec 13 '22 09:12 CyrusNajmabadi

would namespace visibility be visible across assemblies?

No. This would be within an assembly.

CyrusNajmabadi avatar Dec 13 '22 09:12 CyrusNajmabadi

These days, C# tends to make the most recommended style the easiest to write. If you want to encourage people to use private on top-level types and thus ignore IVTs, I have no objection to this keyword. Am I correct in that understanding?

ufcpp avatar Dec 13 '22 09:12 ufcpp

I have no dog in the race about encouragement. I think people should use whatever pattern makes sense for their codebase. If people want to make their types private, because that fits their goals better, then i'm all for that. If they want to keep them internal, i'm also fine with that.

What i care most about is that those who do want to use IVT, but not expose as much, have an option to help themselves out here :)

CyrusNajmabadi avatar Dec 13 '22 18:12 CyrusNajmabadi

Can I add a vote for making them sealed too?

Making what sealed?

CyrusNajmabadi avatar Dec 14 '22 23:12 CyrusNajmabadi

The use case for this top-level private seems extremely narrow. IVT already has to be part of the declaring assembly; you are explicitly opting into sharing your internals with another assembly. Meaning that this feature would not be applicable for any package referenced from NuGet, potentially only useful for library authors with multiple assemblies. So we're already talking about a use case that only applies to assemblies that are owned by the same organization and have implemented IVT (which I'd wager is a vanishingly small percentage of C# code out there- most devs don't even know IVT exists). Is this use case really compelling enough that it outweighs the additional language complexity?

For namespace accessibility, what has changed since you said this two years ago?

CyrusNajmabadi on Nov 19, 2020: I don't see it providing much benefit tbh. Indeed, it would almost immediately run into problems. For example, while i might see some scenarios where i would want a type filtered down to a namespace Foo, i would also want it available in Foo.Bar. Also, namespaces are a cross-assembly construct. So it would be just very weird. You'd be filtering down types to a specific namespace so it would only usable there. But anyone could still get access to it just by defining their own namespace with the same name.

As above though, when you want to restrict legal code, an analyzer is a simple and easy thing to do that already works right now. So def the preferred way to go.

MgSam avatar Dec 15 '22 14:12 MgSam

I have to agree that I think these are both pretty niche, probably on a similar order to private protected. I'm ambivalent to their addition if the LDM feels it's worth the effort. I can't imagine IVTs are that commonly used, especially outside of testing scenarios.

As for driving accessibility from namespaces, that just feels ... off ... to me. Like someone used to Java accessibility wants to recreate it in C#. I think there are numerous questions about how such accessibility would be enforced. Does that accessibility extend between assemblies, like Java 1-8, or is it limited to only the current assembly, like Java 9+? What about "child" namespaces? I think you could ask 5 different developers and get 10 different answers, which to me lends itself more to enforcement via analyzer.

HaloFour avatar Dec 15 '22 14:12 HaloFour

For namespace accessibility, what has changed since you said https://github.com/dotnet/csharplang/discussions/4153#discussioncomment-134083 two years ago?

Namespace accessibility was added at the request of another LDM member who was interested in it and felt it would be good to have the discussion at the same time. My views on it have not changed.

CyrusNajmabadi avatar Dec 15 '22 15:12 CyrusNajmabadi

Is this use case really compelling enough that it outweighs the additional language complexity?

That's what will have to be determined. Very possible it won't be.

CyrusNajmabadi avatar Dec 15 '22 15:12 CyrusNajmabadi

The emitted class?

I don't know what you're asking for. Can you give an example of what you want to write and what you want it's to compile down to @buvinghausen ?

CyrusNajmabadi avatar Dec 15 '22 16:12 CyrusNajmabadi

Can I add a vote for making them sealed too?

Why would a private type be implicitly sealed? I don't see why you'd want to treat them any differently than you typically treat internal types today.

HaloFour avatar Dec 15 '22 17:12 HaloFour

Because I have code analysis turned on to make all non inherited non public classes sealed

A private class doesn't mean that it can't or won't be inherited. It only means that the class is not accessible outside of that assembly, even with InternalsVisibleToAttribute applied. The class could still be take part in a hierarchy within the same assembly.

HaloFour avatar Dec 15 '22 19:12 HaloFour

Just emitting the class as sealed

I don't understand waht you're asking for. You're showing an example of code that is already legal and has meaning :) Can you show me what you want to be able to write, and what semantics it woudl have? Thanks!

CyrusNajmabadi avatar Dec 15 '22 19:12 CyrusNajmabadi

Because the class is code generated

I don't know what this means. There's nothing about generated code being discussed here. This is about accessibility placed by the user on types.

please review the test repository I put together.

I have no idea what you're asking to be reviewed in that repo... :)

Can you please walk us through what you're asking for? I genuinely don't really understand, and i'm not seeing what it has to do with this proposal. Thanks!

CyrusNajmabadi avatar Dec 15 '22 19:12 CyrusNajmabadi

+1 for this, I need this feature for my roslyn source generator. I don't want the generated internal classes cause conflict if user uses [InternalsVisibleTo], when multiple assemblies use same source generator. Though I do think this could be an attribute instead of language keyword? Like [InvisibleToFriends] .. or something.

cathei avatar Dec 18 '22 03:12 cathei

I don't want the generated internal classes cause conflict if user uses [InternalsVisibleTo], when multiple assemblies use same source generator.

Weren't file-scoped classes supposed to help in this kind of scenario (at least, in part. Not sure about the IVT part)?

VelvetToroyashi avatar Dec 18 '22 04:12 VelvetToroyashi

Yes. private is effectively very similar to file, just without the restriction that things may be in just one file. file works well for the SG case as it's totally reasonable for a tool to generate everything into one file. However, it's not an ergonomic solution for code that is intended to be user-editable, where placing into separate files is desirable.

CyrusNajmabadi avatar Dec 18 '22 04:12 CyrusNajmabadi

@VelvetThePanda Thanks for the input, I wasn't aware of file keyword in C# 11. However it cannot be used for my case as my SG will generate extension class that will be accessed from user code. Also for incremental SG, it would be good to have this feature.

cathei avatar Dec 18 '22 04:12 cathei

Since the "assembly: InternalsVisibleTo[....]" is an optional attribute, if you ignore it, the internal functions will never be published to the other classes in other assemblies....It just equals to "private class" functions. So here the "private class" in the namespace is just making an enhancement that none of the members inside the class can be published, it can be ONLY used in the self assembly....Am I right?

If so, do we really need to create this feature to make a big change? Any other very special cases? I think maybe it's a bit duplicated with the risk of running the three risks below, as what you've mentioned above?

All in all, I think the easy solution just depends on whether you want to add the attribute or not to decide whether the class is a "private one" or not to ease the problem....

  1. This definitely makes the language more complex.
  2. This would likely push for codebases to now switch over to 'private' by default.
  3. For back compat reasons, we'd likely have to make it so that a type without accessibility still defaulted to internal.

SEWeiTung avatar Dec 19 '22 09:12 SEWeiTung

I'm not sure what you're saying @MaledongGit .

All in all, I think the easy solution just depends on whether you want to add the attribute or not to decide whether the class is a "private one" or not to ease the problem....

What attribute can you add to decide if a class is a private one or not?

CyrusNajmabadi avatar Dec 19 '22 14:12 CyrusNajmabadi