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

[Proposal]: file local types #5529

Open
4 tasks done
stephentoub opened this issue Dec 9, 2021 · 69 comments
Open
4 tasks done

[Proposal]: file local types #5529

stephentoub opened this issue Dec 9, 2021 · 69 comments
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Dec 9, 2021

"file private" visibility

Design meetings

@HaloFour
Copy link
Contributor

HaloFour commented Dec 9, 2021

Is a new accessibility modifier necessary or could the compiler be "relaxed" so that private can be applied to top-level types?

@333fred
Copy link
Member

333fred commented Dec 9, 2021

Is a new accessibility modifier necessary or could the compiler be "relaxed" so that private can be applied to top-level types?

What if you're generating part of a partial type?

@HaloFour
Copy link
Contributor

HaloFour commented Dec 9, 2021

What if you're generating part of a partial type?

Ah, I see, so this could also apply to a nested class emitted within the generated portion of a partial class, where private would not hide the nested class. That makes sense.

To play devil's advocate, would it not be sufficient to have the compiler recognize an attribute to enforce this behavior?

@jaredpar
Copy link
Member

jaredpar commented Dec 9, 2021

The idea of having file private types / methods is a mechanism I use a lot when coding in F#. There you can use a signature / implementation file pairing. Only types / methods that are present in the signature file are available to the rest of the code in the assembly. The rest are only visible to the code within the implementation file.

I've found this to be very advantageous when coding in F# and I make use of it often. It's a way of defining helper types that just don't have any visibility within the rest of the assembly. It's a clean separation and really allows you to avoid spaghetti code in larger apps and instead force communication across defined boundaries.

I mention F# here because it's the language I've used this feature the most in. But it's present in other languages like Go via exported / non-exported types in packages.

I've often longed for a similar feature in C#. When @stephentoub was chatting with me about the problems they are hitting isolating their generator code it occurred to me that this would also be a mechanism for generators to effectively isolate themselves. It gives them a greater degree of freedom in what they generate without the fear that consumers will depend on implementation details that generators never intended to expose.

@vladd
Copy link

vladd commented Dec 11, 2021

Split into files has traditionally (except file-scoped namespaces) been neglectable in C#. Wouldn't it be better to make the types not file-private, but partial class part private? Other usecases seem to be covered by private inner classes.

@svick
Copy link
Contributor

svick commented Dec 11, 2021

@vladd I don't think that would work well. Consider code like the following (using the RegexGenerator that's in the current .Net 7 alpha build):

partial class Server
{
    [RegexGenerator(@"(?:[0-9]{1,3}\.){3}[0-9]{1,3}", RegexOptions.IgnoreCase)]
    public partial Regex CreateIpAddressRegex(); 
}

partial class User
{
    [RegexGenerator("(.+)@(.+)")]
    public partial Regex CreateEmailRegex();
}

If the generated code for these two methods wanted to share some helper code, there is no way to use partial class part private types or private inner types. But file private types make it very easy.

@TahirAhmadov
Copy link

Is it possible to hide the generated code as nested private types inside a special class? I was going to say it's an easy workaround but thinking about it more, it actually looks like a solution, if not the solution. This "container" class can be made partial, hence allowing the generation of a significantly large amount of "private" code without creating enormous files. In abstract terms (not abstract keyword), it makes sense, too - the inner workings of your generated functionality are hidden as private members of a "public" class (whether it's public or merely visible to the rest of the project).

@TahirAhmadov
Copy link

@vladd I don't think that would work well. Consider code like the following (using the RegexGenerator that's in the current .Net 7 alpha build):

partial class Server
{
    [RegexGenerator(@"(?:[0-9]{1,3}\.){3}[0-9]{1,3}", RegexOptions.IgnoreCase)]
    public partial Regex CreateIpAddressRegex(); 
}

partial class User
{
    [RegexGenerator("(.+)@(.+)")]
    public partial Regex CreateEmailRegex();
}

If the generated code for these two methods wanted to share some helper code, there is no way to use partial class part private types or private inner types. But file private types make it very easy.

Good point. How about an attribute like [PrivatesVisibleTo(typeof(User))]?

@jnm2
Copy link
Contributor

jnm2 commented Dec 11, 2021

@TahirAhmadov That name, though

@CyrusNajmabadi
Copy link
Member

I would be amenable to this idea. My personal take is that the symbols not be available outside the file ever (no attributes to expose it), and that they likely be implemented with a name mangling strategy to avoid cross file collisions.

@jaredpar
Copy link
Member

The cross file symbol collision is definitely a consideration to take into account. It is a bit of a sore spot in other languages. IIRC F# gives a bit of a cryptic error when you have collisions instead of resolving them.

I think we'd likely want to keep the simple names of the symbols the same. For example

namespace Example { 
    file private class Widget { ... }
}

Think the identifier should still be Widget here. in metadata To do otherwise would end up subverting a lot of expectations.

Instead think we should consider inserting a generated name between the namespace and the type name. In this case having Widget emitted as Example.<>_generated.Widget. That would allow us to avoid collisions while minimally subverting expectations around how types map to metadata names.

@CyrusNajmabadi
Copy link
Member

CyrusNajmabadi commented Dec 13, 2021

@jaredpar . I think that makes a lot of sense. I agree with you that there is a strong desire to likely have the metadata name match (though it's not sacrosanct for me). Your approach seems like a good best-of-both worlds option.

For things like file fields, if <>_generated was just a mutable struct, then this becomes effectively free at runtime (presuming the runtime doesn't fall off a cliff in any cases). Are there things that would not work if these fields moved to such a struct? Things like ref would still work and whatnot. I presume FieldOffset might make no sense though...

Just trying to think through corner cases here. Thoughts?

@jaredpar
Copy link
Member

That is a good point about fields. In my head I'd been focusing on the cases where the only type itself was marked as file private I hadn't really thought about the cases where you got down to the individual members (self fail)

I think at that level of granularity it gets really hard to maintain both:

  1. Don't let different file private conflict with each other
  2. Don't break the mental model of developers on how code is emitted to metadata

Imagine for example reflection. The moment we start name mangling individual members then it becomes really hard to think about how to use features like reflection to use them.

Thinking along the lines of corner cases, I'm still trying to decide if partial makes sense when one of the declarations is file private. If the goal is that one file private is completely hidden from another then I don't think you can allow partial. Because then you run into cases like this:

// file1 .cs 
file private partial class Widget { 
  public overrides string ToString() => "";
}

// file2.cs
partial class Widget { 
   // Error: multiple overrides of ToString
  public overrides string ToString() => "something";

Part of my brain is screaming "only allow file private on type declarations" because that allows us to give the guarantees we want and provides a clean solution for tools like generators that want an isolated space for their work.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Dec 14, 2021

Can't we use a modreq for this? Something like class FilePrivate { }? Of course when you import an assembly containing such modreqs, the 'handling' for them is to simply assume you can't access anything with the modreq..

@jaredpar
Copy link
Member

@RikkiGibson you can't put a modreq on a type, only on method signatures. That is why we have to tricks like [Obsolete] when emitting a ref struct.

A modreq could be a solution for non-virtual method conflicts though. The basic pattern we could employ is generating types into the assembly in the pattern of Microsoft.CodeAnalysis.<>FilePrivateN where N equals the number of files which contain at least one file private modifier. Then every method in a file gets a modreq(MS.CA.<>FilePrivateN which makes it unique.

That does pose a problem though: such methods could not be used as implicit implementations of interface members. The presence of the modreq would make the signatures not match. We could counter by emitting an explicit implementation thunk under the hood though but we're back to that could conflict with explicit implementations in other files.

We'd still need solutions for virtual methods, interface hookups, fields, etc ...

The more we talk about this, and it's fantastic feedback, the more I'm wondering if forcing this at the type level is the best solution. It solves the core problems here, allows us to maintain the idea of it being truly private (don't worry abuot name conflicts) and no other language I'm aware of allowed parts of types to be file private (it's all or nothing).

@BhaaLseN
Copy link

I'm not really sure if this is actually needed (at least in the proposed way)? My first thought for making helper types unlikely to be used by non-generator code would simply be a nested namespace.

namespace Whatever.YourRegular.CodeUses
{
  partial class TypeToExtend
  {
    partial void DoTheThing() => DontUseThis.Helper.DoTheSharedThing();
    // or, if you dislike putting the namespace in front:
    //using DontUseThis;
    //partial void DoTheThing() => Helper.DoTheSharedThing();
  }
  namespace DontUseThis
  {
    internal static class Helper
    {
      public static void DoTheSharedThing() => throw null;
    }
  }
}

This doesn't prevent non-generator code from using it, but it is more obvious if anyone does attempt to use it, and it doesn't show up by default (since it isn't in the same namespace and you'd have to be using it first). Plus, it works today without any changes.

I do wonder if I could make use of the proposed solution anywhere (outside of source generators). I can't really think of any other situation where this is both a useful addition and something that can't be done otherwise.
How would those types fare in the face of reflection? I briefly started typing about one thing I do that discovers types that implement a certain interface (and shouldn't really be used by anyone else), but dropped it from my reply because it seems a fairly specialized and niche thing.

I could see a top-level class using the private modifier to trigger this though (which today is not allowed and raises CS1527).
But if we go from there, it somewhat also opens the flood-gates for "invisible" base classes. Consolidate common functions in a non-public (or internal) class, derive from those, and callers see it as if the class had no base class (basically inlining it; but I don't know if IL could do this in any way without duplicating the code as well). And thats something for another day (and a different proposal, if at all).

@stephentoub
Copy link
Member Author

This doesn't prevent non-generator code from using it

And that's the problem. Then we change the generator, and code breaks.

@vladd
Copy link

vladd commented Dec 17, 2021

Can a namespace be file private? C++ uses anonymous namespaces for this purpose:

namespace
{
    // the code here is effectively file private
}

@HaloFour
Copy link
Contributor

@vladd

Namespaces don't really exist, they're just part of the class name so they can't have their own metadata like accessibility levels. The same namespace can be used freely between different assemblies as well. The individual types within the namespace would need to carry that metadata.

@vladd
Copy link

vladd commented Dec 17, 2021

@HaloFour
Well, this seems to be not a problem in C++, it looks like the anonymous namespace has a unique unspeakable name from the linker‘s point of view.

@HaloFour
Copy link
Contributor

HaloFour commented Dec 17, 2021

@vladd

Different languages, different problems :)

Implicitly treating a file private class as within it's own unspeakable namespace might be a way for the compiler to lower the class as something that can't be referenced externally without affecting the simple type name. I don't know if that works for nested classes, though. The relationship is defined by an entry in the NestedClass metadata table so perhaps the name of the nested class could be completely unrelated to the enclosing class, but you'd end up with something not expressable in C# or even IL which both require lexical nesting.

@vladd
Copy link

vladd commented Dec 17, 2021

@HaloFour
Well, I basically asked if the C++ approach is a way to go: no explicit file private modifier but using an anonymous namespace for it. This way we would avoid thinking in files but will be thinking in namespaces instead.

@HaloFour
Copy link
Contributor

@vladd

I don't think that'd work for nested classes unless C# allowed you to define namespaces within a class.

@PathogenDavid
Copy link

@vladd

I feel like anonymous namespaces are borderline trivia in C++. It's not really obvious what they do unless you already know whereas something like file private class is more self-evident.


As an aside, personally I've found it to be valuable for generated source to be digestible by humans for debugging/learning purposes. I worry that this feature as proposed could end up forcing generator authors to put their entire output into one file (potentially making it painful for humans to interact with.)

It has its own downsides, but I wonder if friend types (#2073) would be a better solution to this issue. (As-is, this whole proposal is basically a less flexible special-case of them.)

@lsoft
Copy link

lsoft commented Dec 24, 2021

Friends, let me share (may be) unpopular opinion: please do not complicate C# for minor reasons. For SG-produced code the EditorBrowsable approach is enough, we need only make a final design and implement it. Also, @vladd 's proposal about unspeakable namespaces is better because of its lesser impact on the language (as I see it).

if we want can fulfill the need without modifying the language - probalby it better to do that... we have a "preprocessor" directives (#nullable enable), project settings (<Nullable>Enable</Nullable) and visual editor level (EditorBrowsable). why we need to modify the language? let's put #file private on on the top of SG-produced file (we will need a #file private off also) ! so the language will be modified about 1 new preprocessor directive...

There is a lot of choices, please do not make C# more complex for the minor reason like hiding SG code.

Thanks!
(sorry for poor English, I hope my point is clear)

@teo-tsirpanis
Copy link

Regarding metadata representation, we can represent file private members with the ECMA.335 compilercontrolled visibility.

Having read the spec, my understanding is that type members with this visibility must be referenced with a TypeDefinition metadata token, making them effectively internal, but very internal; not even InternalsVisibleTo or its evil twin we don't talk about can expose them to another assembly module, and that many compiler-controlled members in a type can have the same name, solving concerns about naming collisions.

Some outstanding questions are:

  1. What about file private types? Only members may be marked as compiler-controlled. The types could be marked with a special attribute (NotReferencableAttribute or something like this), and/or have their names made unspeakable.
  2. How would the debugger handle many type members with the same name? I ran a demo and both Visual Studio and Rider show two identically named fields like this:
    image
    I couldn't exhibit how to test the debugger's reaction when I hover over a field name that has a duplicate.
  3. How would the reflection handle compiler-controlled types with the same name? I ran a demo that calls GetField("_x", BindingFlags.NonPublic | BindingFlags.Instance), it threw an AmbiguousMatchException.

I don't think (2) and (3) are very important issues; how likely is to have such collisions? But the runtime wouldn't bother and nor would the compiler who would totally ignore compiler-controlled members in referenced assemblies.

@acaly
Copy link
Contributor

acaly commented Jan 9, 2022

There has been many other proposals, especially related to member visibility, that has been rejected due to

Language proposals which prevent specific syntax from occurring can be achieved with a Roslyn analyzer. Proposals that only make existing syntax optionally illegal will be rejected by the language design committee to prevent increased language complexity.

Can anyone here clearly explain why this proposal is not rejected for the same reason? We have been forced to use dirty attributes everywhere to disallow specific usage, and many of there are related with source generators. I don't think hiding something is anything different.

@AraHaan
Copy link
Member

AraHaan commented Apr 19, 2022

I would rather have private types only visible to the same assembly, but InternalsVisibleTo would make it seem like the type never existed in terms of other projects referencing the assembly that defines it.

@ViIvanov
Copy link
Contributor

As an idea to define "file-private" types I would suggest unnamed namespaces:

namespace Foo.Bar {
  internal partial class UserType  {
    internal partial void SomePartialMethod()  {
      My.Own.Stuff.Zoo.Blah(); // OK only in current file
      new Baz(); // OK only in current file
    }    
  }
}

namespace { // Unnamed namespace declaration, all internal types are "file-private"
  namespace My.Own.Stuff { // Named namespace inside unnamed, if needed
    class Zoo { // No access modifier, it is required. Or only optional "private" / "internal" allowed
      public static void Blah() { }
    }
  }

  class Baz { } // "Top-level" "file-private" enum
}

@AraHaan
Copy link
Member

AraHaan commented Apr 22, 2022

I got better idea for file private, and that is to allow private keyword on normal ClassDeclarationSyntax's that are namespace level and problem could be solved.

@CyrusNajmabadi
Copy link
Member

@AraHaan that wouldn't solve a core case where you want the type hidden from code in the same assembly. A private class would be visible to siblings (just as a private nested type is visible to it's siblings).

@jnm2
Copy link
Contributor

jnm2 commented Apr 23, 2022

@ViIvanov I like that, but it doesn't provide a solution for if you want only a nested type to be file-private.

@ViIvanov
Copy link
Contributor

ViIvanov commented Apr 26, 2022

@jnm2, Thank you. Sorry, I've missed this case.

@Pankraty
Copy link

Will this make it possible to create private extension methods? I was in a situation a couple of times when something might be nicely expressed as an extension method, but that would be too specific for exposing as a method visible for the entire assembly. Top-level private class would solve that problem.

@RikkiGibson
Copy link
Contributor

I would expect to be able to define extension methods in a 'file' class and only be able to use them in the same file. That's a great scenario to test, thanks.

@BlinD-HuNTeR
Copy link

I don't wanna be the party pooper, but, what's wrong with nested types? If you are writing a source generator, and you don't want the generated types to be visible to the main assembly, just make all the generated sources be part of a big partial class, and the generated types as nested private types in that class.

In other words, that big class is just acting as a namespace for all purposes. Even source compatibility is preserved, you're just transforming a namespace into a class that spans multiple files. The fully-qualified names of all the nested types will remain the same.

This should definitely be mentioned in the "Alternatives" section of the proposal, after all, it's a "Do Nothing" alternative.

@CyrusNajmabadi
Copy link
Member

@BlinD-HuNTeR

"Do nothing" is always an implied alternative. When we decide we want to do something, it's because we feel that the value has already proven itself over "do nothing" :)

@CyrusNajmabadi
Copy link
Member

In other words, that big class is just acting as a namespace for all purposes.

This came up as an option. We felt that this wasn't ideal and that defining something just for this purpose is unpleasant. We debated it but ended up here.

@stephentoub
Copy link
Member Author

stephentoub commented Apr 29, 2022

I don't wanna be the party pooper, but, what's wrong with nested types? If you are writing a source generator, and you don't want the generated types to be visible to the main assembly, just make all the generated sources be part of a big partial class, and the generated types as nested private types in that class.

There's no way to hide it completely. If the source generator is filling in a partial method on the user's type, that method needs access to the generated code; whatever it has access to, so will any other user code in the type containing that partial method. The only thing it can completely hide is anything it can generate inside the method itself, and without local types, there's no way to hide such types (and even if it had local types, there'd be no way to share that generated code across multiple partial method implementations without exposing it to the containing type's user code).

@eerhardt
Copy link
Member

eerhardt commented Jan 9, 2023

This was implemented in C# 11 right? https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/file.

Can this issue be closed?

@stephentoub
Copy link
Member Author

@333fred, we keep these open until they're spec'd, yes? Or is that tracked elsewhere?

@333fred
Copy link
Member

333fred commented Jan 9, 2023

That's correct, but thank you for reminding me to label all the 11 issues appropriately.

@333fred 333fred modified the milestones: Needs More Work, 11.0 Jan 9, 2023
@333fred 333fred added the Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification label Jan 9, 2023
@Xyncgas

This comment was marked as off-topic.

@AraHaan
Copy link
Member

AraHaan commented Jan 10, 2023

@Xyncgas if that is the case, then you can safely skip using this feature without issues.

@jrmoreno1
Copy link

@333fred: Could the check marks be added for prototype and implemented? I understand not closing until there is a documented spec, but I look for the check marks to know what stage it’s in

@333fred 333fred changed the title [Proposal]: "file private" visibility [Proposal]: file local types Mar 20, 2023
@333fred
Copy link
Member

333fred commented Mar 20, 2023

Done.

@rob-ack
Copy link

rob-ack commented Apr 6, 2023

Consider 'file' not only being useful for type definitions. In the scenario of "code generation" or/and "partial types" the file keyword is "useful on any member".
It would then allow to hide members which are needed for the generated or partial code to the other partial implementation details.

In my opinion file should be possible on any member as well (fields, properties, events, methods ...).

@acaly
Copy link
Contributor

acaly commented Apr 6, 2023

I found that issue that the IDE suggests me to convert some [DllImport] to [LibraryImport] on imported functions in a file scoped class. This is not possible because the generated implementation is in another source file, which has no access to the original definition.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Apr 6, 2023

In my opinion file should be possible on any member as well (fields, properties, events, methods ...).

Absolutely agree. We just need to come to a consensus for how file relates to accessibility outside of top-level types.

I would personally like us to be able to "just see file as another access modifier", but I don't think the LDM necessarily agrees with me there. What I mean is: file remains an exclusive "accessibility level" incompatible with other modifiers like internal/protected/private on a given member. There is a temptation to start defining things like file internal (another type in same file can use it) and file private (only the partial type declaration in the current file can use it), which starts to feel like a significant complexity burden.

When it comes to accessibility checks, file and private when the containing type is partial is kind of a Venn diagram. So when you want to start using file types in private signatures or vice-versa, the accessibility consistency errors could become painful.

@stephentoub stephentoub removed their assignment Oct 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Implemented Needs ECMA Spec This feature has been implemented in C#, but still needs to be merged into the ECMA specification Proposal champion Proposal
Projects
None yet
Development

No branches or pull requests