dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.89k stars 782 forks source link

Private types with public members #11953

Closed stmax82 closed 3 years ago

stmax82 commented 3 years ago

There seems to be a discrepancy between C# and F# when access modifiers are put on members of private types.

The following C# code outputs:

Publics:
  X
Privates:
using System;
using System.Reflection;

class Program {
    // private class with public property
    private class Foo {
        public string X { get => "foo"; }
    }

    static void Main(string[] args) {
        var t = typeof(Foo);

        Console.WriteLine("Publics:");
        var publics = t.GetProperties(BindingFlags.Public | BindingFlags.Instance);
        foreach (var x in publics)
            Console.WriteLine($"  {x.Name}");

        Console.WriteLine("Privates:");
        var privates = t.GetProperties(BindingFlags.NonPublic | BindingFlags.Instance);
        foreach (var x in privates)
            Console.WriteLine($"  {x.Name}");
    }
}

Semantically the same F# code outputs:

Publics:
Privates:
  X
open System
open System.Reflection

// private class with public property
type private Foo() =
    member public this.X with get() = "foo"

[<EntryPoint>]
let main argv =
    let t = typeof<Foo>

    Console.WriteLine("Publics:")
    let publics = t.GetProperties(BindingFlags.Public ||| BindingFlags.Instance)
    for x in publics do
        Console.WriteLine($"  {x.Name}")

    Console.WriteLine("Privates:")
    let privates = t.GetProperties(BindingFlags.NonPublic ||| BindingFlags.Instance);
    for x in privates do
        Console.WriteLine($"  {x.Name}")

    0

Since property Foo.X is marked as public in both cases, I'd expect it to be public in both cases, but it turns out as private in F#.

The same problem happens with private records that should have public labels:

type private Foo = public { X: string }

Here typeof<Foo>.GetProperties(...) also shows that property X is private, even though the record's labels are marked as public.

This causes problems in all places where private types (no matter if classes or records) are processed using reflection.

For example with Dapper: it is not possible to use private F# types as SQL parameter objects because Dapper uses Type.GetProperties(), which by default only returns public properties:

type private DbRow = public {
    A: string
    B: int
}

cnn.Execute("INSERT INTO Foo (A, B) VALUES (@A, @B)", { A = "foo"; B = 42 })

... throws some exception because it doesn't find values for the SQL parameters A and B.

dsyme commented 3 years ago

This has been known as an issue for quite a while, and TBH it's always been frustrating.

Let me rant a little.... For me, the very idea of .NET allowing so-called "public" methods in "private" types is a ridiculous notion and should never have been allowed in the design of .NET (Hint: They Are Not Public, They Can't Be Accessed). This is because F# prioritises lexical accessibility more than having the programmer think about some adhoc labels in .NET metadata (Consider: why not have "green" or "purple" or "hot" or "cold" or "fishy" as extra bits you could but on members in .NET metadata? It means just as much in this case).

As a result since F# 0.4 we took the approach that the actual visibility we would emit would be the "real" one, that is the logical one, the minimum of the visibilities. This seemed particularly important when adding a signature. For example, given

type C() = 
    member x.P = 1

then without a signature C and P are public. What happens when we add a signature, with C missing?

Signature file test.fsi:

module Test

Implementation file test.fs:

module Test

type C() = 
    member x.P = 1

The way things work in F# is that if C is missing from the signature, then both C and P become implicitly assembly-private - (and indeed, logically speaking, "file private", though there's no way to express that in IL so we emit "assembly private").

This is fine, and people seem cool with it - the point is, F# is designed that you don't need to worry about these things - you can just rely on lexical scoping and a degree of access control. It's why F# is simpler to use than C# (except when the corner cases like this bite you via reflection. For example, this means things like Dapper "Just Stop Working" when you add what feels like an innocuous empty signature and your code otherwise compiles.

And what are the options to get it working wth a signature? Would you need this, an explicit public?

Signature:
 empty

Implementation:

type C() = 
    member public x.P = 1

That works at a technical level but it's just a bit weird that you need these explicit public when adding a signature (because the very idea of having something be "public" when it isn't remotely lexically accessibly to anyone except via reflection is just logically incoherent)

Anyway, I am happy to consider a change where an explicit public has actual impact on things that members that would otherwise get assembly visibility by default. We would have to take this via a language suggestion and RFC.

In the meantime I'd encourage Dapper to consider some kind of change to stop the requirement to have things-labelled-public-in-IL-that-arent-actually-public-because-they're-in-a-private-type.

dsyme commented 3 years ago

I created a language suggestion here https://github.com/fsharp/fslang-suggestions/issues/1057

Because of compat concerns it's likely that we'd need a CLIPublic attribute

dsyme commented 3 years ago

I'll close this because it's currently by-design and is tracked by the suggestion above, which I've marked approved-in-principle.

If you'd like to work on the RFC or implementation please let me know

Thanks!

robitar commented 3 years ago

@dsyme re private types having a public interface in C#, is this not meaningful for a nested type? The type itself is a private member, but it has some control over the members it exposes to its enclosing type:

class Thing {
    private class ImplementationDetail {
        public void Frob() {
            //...
        }

        private int state;
    }

    public Thing() {
        var inner = new ImplementationDetail();
        inner.Frob();

        Console.WriteLine(inner.state); //CS0122: inaccessible
    }
}

Probably not something you'd want to make a lot of use of, but handy for keeping things like inner state machines/cursors neat.

dsyme commented 3 years ago

@robitar Yes, you are right that in that case the public means "public up to Thing, which is internal", and does affect the logical accessibility. There is no way to explicitly declare that otherwise, though you could just use "assembly" (same as Thing)

Roughly equiv F# code:

module internal Thing =
    // Means "private to the enclosing module"
    type private ImplementationDetail() =

        // Declared accessibility is "internal" but the "private" on "ImplementationDetail" constrains this further
        member internal _.Frob() = ()

        // Logical accessibility is ImplementationDetail
        member private _.Glob() = ()

    let create() = 
        let inner = new ImplementationDetail()
        inner.Frob()
        inner.Glob() // not accessible
robitar commented 3 years ago

Yes interesting, it's very close. Just to be sure I follow the point about Frob in your example there (I was trying to grok it above as well), that the combination of private on the type and internal on the member constrains its accessibility further - as in a sort of intersection of assembly visibility, and that of the containing type, which is effectively equivalent to the containing type?

I'm guessing from the original request here that it's not possible or desirable to make Frob public, because it would be confusing to see public nested in a scope more restrictive than itself.

I do recall having to learn the approach C# takes, back in the day; it wasn't immediately intuitive, but once it clicks, it makes some sense to treat the accessibility modifiers in terms of 'local space only'. I also quite like your example above (if I'm understanding it) which allows you to do a kind of logical constraints-based approach.

I can see both sides all right, it's very interesting to see the design philosophies manifesting in these edge cases.

dsyme commented 3 years ago

as in a sort of intersection of assembly visibility, and that of the containing type, which is effectively equivalent to the containing type?

Yes, that's right, that's the way to compute the logical accessiblity.

Given that F# includes several logical accessibilities that don't have .NET equivalents (e.g. "private" on a type means "private to the enclosing namespace fragment or module") it always seemed best to just emit either "public" or "internal" as the .NET accessibility. We emitted "public" if the logical accessibility was public, and "internal" if not.

Anyway all in all I think it's right to use CLIPublic for this particular corner-case.

robitar commented 3 years ago

Yes that makes sense.

Thanks for taking the time to clarify!