Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

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

Closed
1 of 4 tasks
CyrusNajmabadi opened this issue Dec 12, 2022 · 79 comments
Closed
1 of 4 tasks
Assignees
Milestone

Comments

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Dec 12, 2022

private and namespace accessibility modifiers for top-level types

  • 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

@HaloFour
Copy link
Contributor

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

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Dec 12, 2022

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?

@CyrusNajmabadi
Copy link
Member Author

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

@svick
Copy link
Contributor

svick commented Dec 12, 2022

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

@CyrusNajmabadi CyrusNajmabadi changed the title [Proposal]: Add private accessibility for type-level types. [Proposal]: Add private and namespace accessibility modifiers for top-level types. Dec 12, 2022
@ufcpp
Copy link

ufcpp commented Dec 13, 2022

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.

@CyrusNajmabadi
Copy link
Member Author

I imagine you wouldn't teach beginners this.

@ufcpp
Copy link

ufcpp commented Dec 13, 2022

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

@CyrusNajmabadi
Copy link
Member Author

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

@ufcpp
Copy link

ufcpp commented Dec 13, 2022

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.

@RoyAwesome
Copy link

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.

@CyrusNajmabadi
Copy link
Member Author

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
Copy link
Member Author

would namespace visibility be visible across assemblies?

No. This would be within an assembly.

@ufcpp
Copy link

ufcpp commented Dec 13, 2022

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?

@CyrusNajmabadi
Copy link
Member Author

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
Copy link
Member Author

Can I add a vote for making them sealed too?

Making what sealed?

@MgSam
Copy link

MgSam commented Dec 15, 2022

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.

@HaloFour
Copy link
Contributor

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.

@CyrusNajmabadi
Copy link
Member Author

CyrusNajmabadi commented Dec 15, 2022

For namespace accessibility, what has changed since you said #4153 (reply in thread) 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
Copy link
Member Author

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
Copy link
Member Author

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 ?

@HaloFour
Copy link
Contributor

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
Copy link
Contributor

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.

@CyrusNajmabadi
Copy link
Member Author

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
Copy link
Member Author

CyrusNajmabadi commented Dec 15, 2022

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!

@cathei
Copy link

cathei commented Dec 18, 2022

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

@VelvetToroyashi
Copy link

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)?

@CyrusNajmabadi
Copy link
Member Author

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.

@HaloFour
Copy link
Contributor

Are IVTs that common outside of testing scenarios to warrant having an accessibility modifier that is 99.9999% redundant? If that's the case, fix IVTs.

@KennethHoff
Copy link

KennethHoff commented Dec 18, 2023

Screen Shot 2023-12-18 at 17 07 00

I think it'd be very confusing if internal and assembly was identical in meaning, except that assembly was "internal, but actually this time".

private internal would necessarily mean "Access is limited to the containing class or the current assembly", which is not what we want either.

Both of these have problems. The former feels redundant, while the latter is inconsistent with other parts of the language.

What about internal internal. Sounds absurd, but at least doesn't have the problem of prior precedence with double-access modifiers meaning "A or B".

That said, given these two options, assembly feels like the lesser of two evils.

@rickbrew
Copy link

Are IVTs that common outside of testing scenarios to warrant having an accessibility modifier that is 99.9999% redundant? If that's the case, fix IVTs.

Yes. In Paint.NET I have IVTs spanning most of my app DLLs, which ensures plugins cannot access app internals, and that I can expose whatever is important for plugins to use as public. This also can't be changed on a whim because it's important to maintain binary compatibility.

Having a special access modifier for internal-but-not-IVT is also becoming increasingly important for source generator scenarios. Rather than including their own DLL with attributes for you to reference, they drop an internal attribute into your assembly (for example: https://github.com/k94ll13nn3/AutoConstructor). You then have to add some suppressions for some compiler warnings to ensure you can compile without warnings and/or errors due to duplicate type names across assemblies that are IVTs of each other.

@rickbrew
Copy link

Having internal-but-not-IVT will be useful for another source generator concern that's come up in ComputeSharp https://github.com/Sergio0694/ComputeSharp, and IIUC in some of the newer COM interop generator stuff. The code is being generated such that it can only access the target type ("shader struct") if it's public or internal, it can't be private. However, this then means that the supposed-to-be-private struct is visible across IVT assemblies, which is bad for obvious reasons, and it can gum up Intellisense and whatnot. Having another access modifier to prevent visibility to IVT assemblies would improve this.

@HaloFour
Copy link
Contributor

Yes. In Paint.NET I have IVTs spanning most of my app DLLs, which ensures plugins cannot access app internals, and that I can expose whatever is important for plugins to use as public. This also can't be changed on a whim because it's important to maintain binary compatibility.

That doesn't sound like a remotely common scenario.

Even if more than a small handful of exceptionally complicated projects can benefit from new accessibility levels, I would still suggest that it would be preferable to improve IVTs. C# already has more accessibility levels than most languages.

@CyrusNajmabadi
Copy link
Member Author

Think of "private" then as the improvement to 'IVT' :) It would generally be the norm to just make all types within an assembly private. And if you wanted to expose them outwards you'd use:

  1. public (for that well known use case).
  2. internal (with IVT to state which other assemblies to expose this to).

It would basically be flipping IVT from the very broadly exposing feature it is today, to one that could narrowly expose things.

For most users this would be a non-issue. They don't use IVT, and as such wouldn't ever have to do anything or be aware of this. All our defaults would continue working well for them. But for users that do use IVT, this would be the knob that goes with it to make separation of concerns nicer.

FWIW, this is feedback i've heard quite a bit from library authors that ship their libraries for external consumption. They want to share small bits of code among their lib dlls (and so use IVT), but now have exposed everything, even though they'd like to have additional protection from that.

@CyrusNajmabadi
Copy link
Member Author

And it's def the case that for the sample size of: MS owned libs, this is def something wanted. For 2nd and 3rd parties the desire is there. Though not as universal as hte 1st party case :)

@HaloFour
Copy link
Contributor

@CyrusNajmabadi

For most users this would be a non-issue. They don't use IVT, and as such wouldn't ever have to do anything or be aware of this. All our defaults would continue working well for them. But for users that do use IVT, this would be the knob that goes with it to make separation of concerns nicer.

That means all devs are now exposed to these accessibility levels, despite almost nobody having a need to use them. Worse, it'll probably encourage trying to further finely slice the onion only leading to more and more requests for accessibility levels.

IVTs are an advanced scenario, if they are not suitable for the extreme minority of developers that rely on them outside of test scenarios, I still think "fixing" IVTs would be a much better route.

@RenderMichael
Copy link

Would it make sense to repurpose the file modifier to be internal-but-not-IVT here? Although it’s technically internal after the name mangling, it’s not a supported scenario to use the type outside the file, so changing it shouldn’t be a problem.

It was already mentioned that they’re not an ergonomic feature, but it’s also been repeated brought up that this is a niche scenario. Maybe someone can make a private wrapper if necessary.

@HaloFour
Copy link
Contributor

In terms of "fixing IVTs", I might suggest that .NET take a page from the Java Module System and have a resource file embedded in the assembly which can explicitly specify which internal types are visible to which assemblies. Then the entire assembly could be exposed to test suites, a smaller subset exposed to sibling assemblies in the same framework and a smaller subset still could be exposed to known plugins, etc.

@FaustVX
Copy link

FaustVX commented Dec 18, 2023

Why not "just" allow IVT to be applied to types and members also.
If IVT is applied to the assembly, nothing change.
But, if it's on a type, the whole type is now IVT.
And, you can't have IVT on a type/member if it's also on the assembly.
With this, you can have a type IVT to Assembly1, but another one to Assembly2.

@CyrusNajmabadi
Copy link
Member Author

I would use private for that purpose halo.

@HaloFour
Copy link
Contributor

@CyrusNajmabadi

I would use private for that purpose halo.

And PrivatesVisibleToAssembly to share to the smaller subsets of assemblies? Although I think that attribute would get you in trouble with HR. ;)

My point is that it feels like adding more accessibility levels here feels like a very big hammer to address something experienced by an extreme minority of the developer audience.

@CyrusNajmabadi
Copy link
Member Author

@FaustVX that's what this proposal is ...

Finding a way to allow ivt, without exposing everything.

I feel like people are way too caught up on Syntax.

@CyrusNajmabadi
Copy link
Member Author

To reiterate, this proposal is about a way to more narrowly expose types to other assemblies. The private keyword is strawman Syntax for that.

Just like my other proposal is about scoping typesinside an assembly (with strawman around that).

The fundamental goal is better control around the scoping of the type, since all we have is internal today. That's both too broad within, and too broad without.

@HaloFour
Copy link
Contributor

To reiterate, this proposal is about a way to more narrowly expose types to other assemblies. The private keyword is strawman Syntax for that.

That's fair. I think it's worth exploring if there are non-language ways to accomplish that, especially given that IVT is already a non-language mechanism for expanding accessibility, and if that mechanism offered more flexibility that could satisfy the use cases without requiring language changes.

Internally, maybe it'd be more interesting to talk about adding modules to C# that are independent of namespaces, and types/members can have module accessibility within the module. Of course all of that could be defined via custom attributes and analyzers also.

@CyrusNajmabadi
Copy link
Member Author

Internally, maybe it'd be more interesting to talk about adding modules to C# that are independent of namespaces,

I'm def interested. The reason i went with namespace was because i had a gut feeling (haven't looked closely though) that this is often what namespaces are actually used for. So using namespace as a reasonable approximation would actually be a nice place to start, without having to design an entirely new system.

@HaloFour
Copy link
Contributor

@rickbrew

Having a special access modifier for internal-but-not-IVT is also becoming increasingly important for source generator scenarios. Rather than including their own DLL with attributes for you to reference, they drop an internal attribute into your assembly

You can use ConditionalAttribute to ensure that the attribute is never emitted into the final assembly.

@LoupAndSnoop
Copy link

LoupAndSnoop commented Jan 10, 2024

What would be nice is:
[FriendGroup(typeof(ClassB))]
public class ClassA {
friend int x;
public int y {get; friend set;}
}

public class ClassB {
void MyFunction(ref ClassA a) { a.y = a.x + 5; }
}

The Friend attribute would be the same as private/protected + allow any classes within the friend group to access. Namespace is just a convenient way to declare a group of classes that all have special access to each other. This would be a lot simpler to implement, I would think.

@HaloFour
Copy link
Contributor

@LoupAndSnoop

You could accomplish this today with internal and an analyzer that can recognize the attribute and enforce these extra accessibility rules.

@LoupAndSnoop
Copy link

@HaloFour Does this include hiding methods from other classes in the same assembly (that are not friends)?
If so, could you link or name some sort of resource I could use to make that happen?

@HaloFour
Copy link
Contributor

@LoupAndSnoop

Here's an analyzer that claims to have this capability: https://github.com/piotrstenke/Durian

@zvrba
Copy link

zvrba commented Jan 21, 2024

Java modules and packages, with related accessibility rules, are THE features where Java trumps C#, IMHO. C# already has a lot of accessibility modifiers and none solve problems that people (included me) actually have, and that is "friendship" which use-cases have been elaborated elsewhere. In a green-field project it's possible to split functionality across assemblies so that internal behaves as a kind-of-a-"friend" (but still too broad), but splitting is also cumbersome as you end up with many micro-assemblies.

For my part I often wanted

  1. "Namespace" visibility: the type / member is accessible to anyone in the same declaring namespace. If there exists a type Ns1.Type1 and a type Ns1.Ns2.Type2 wants to use it, Type1 must be internal or public.
  2. As for namespaces being "open" to extension from other assemblies, which @CyrusNajmabadi pointed out as a problem. Well, I don't see it as a problem; C++ allows it for example. [1] One could also introduce "sealed namespace" declaration as in sealed namespace Sealed1 { } that would mandate that new members can be added only from the current assembly.

[1] Use-case example: the namespace defines a tree framework, and predefines a Node class that exposes all members with ns-visibility to classes that implement trees. Than, to add a new tree type from another assembly: "reopen" the namespace, and add the new tree class. C++ uses the same technique (std namespace) to provide hash-functions for custom types, for example.

So namespace and sealed namespace would bear analogy with classes. If it's possible to meaningfully extend it, don't seal it. If not, seal it. I find myself more and more frequently using public nested classes (despite "official guidelines" that discourage them) to address problems such as builders of immutable objects.

And the more I think about accessibility rules in C#, the more I wonder about why namespaces exist at all. The reason for me (still) using namespaces are

  1. Less "red-tape" for top-level types.
  2. Concerns (perhaps unfounded -- just lack of knowledge) about compiler/run-time limits on class internals (number of members, size of emitted IL, etc.)

The mentioned scenario about trees could be emulated as

namespace Whatever;
public abstract class TreeFramework {
    protected TreeFramework() { }

    protected class NodeType { /* public members! */ }

    public class TreeImpl1 { ... }
}

and then in another assembly:

namespace Whatever2;
public abstract class MyCustomTree : TreeFramework {
    public class TreeImpl2 { /* ... */ }
}

The only thing that feels "iffy" about it is that it significantly deviates from C# code one sees "out in the wild", including BCL, i.e., it uses class as namespace with enhanced access control. With classes, one might run into issues with lack of MI, but one could just as well use interface to emulate namespaces now that interfaces support protected on members.

And then we have static class which exists only because we can't have free-standing functions as in C++; otherwise it's functionally the same as namespace with more fine-grained access control. I think the language designers should consider defining a "container" abstraction with own metadata in IL that's unify name grouping concepts of namespaces, classes, structs and interfaces with uniform handling of accessibility. So

  • A class, struct, interface, namespace and assembly would all be a "container".
  • All would support same means of access control, including sealing ("reopening" the namespace would be akin to inheriting from a class/interface.)
  • The difference would be in what kind of members are allowed inside it and whether "inheritance" is allowed
  • Design decision (or user-specifiable): whether NS A.B.C "inherits" from A.B wrt visibility rules.
  • A "precedent" that allows use of a class as namespace already exists: using static

As of today, interface seems to be the most flexible kind of container in C#.

@ByronMayne
Copy link

I am just here to say I wish this already existed. To me it seems really straight forward to just having them be private. You don't need it, then don't use it. Remain default to internal if not defined.

I manage very large projects that use source generators and those source generators add marker attributes. This attributes conflict with each other when you have IVT. The project still compile but the warnings never go away, unless you suppress them.

@rickbrew
Copy link

I am just here to say I wish this already existed.

I had a use for it just last week.

@DomenPigeon
Copy link

Wishing for this for a long time :)

@Mart-Bogdan
Copy link

I belive custom Roslyn analizer can be created for this and can be pluged in using nuget/mabuild.

So you have internal visibilty + some attribute to place on mambers to be hidden. And it would throw compile time error if used improperly.

You could restrict not to namespace but to friend classes in C++ way.

@dotnet dotnet locked and limited conversation to collaborators Nov 19, 2024
@CyrusNajmabadi CyrusNajmabadi converted this issue into discussion #8654 Nov 19, 2024

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Projects
None yet
Development

No branches or pull requests