fslang-suggestions icon indicating copy to clipboard operation
fslang-suggestions copied to clipboard

Allow explicit public on members to emit public in .NET metadata even in internal types

Open dsyme opened this issue 4 years ago • 9 comments

I propose we allow an attribute CLIPublic to change .NET codegen to emit .NET accessibility public on a member at the point of implementation even if the member is part of an internal type. This would apply even if the member is completely hidden by a signature (which, in the absence of an explicit public, would cause the emit of internal)

For example

type internal C() = 
     [<CLIPublic>]
     member x.P

The same rule would apply to types in internal/private modules.

module internal M =
    [<CLIPublic>]
    type C() =    // nested type - now gets .NET public
         [<CLIPublic>]
         member x.P    // member - now gets .NET public

The alternative - where an explicit public causes a change in .NET accessibility, would be considered a breaking change, since it changes the behaviour of .NET reflection w.r.t. the type we will need to discuss.

There is no existing way of approaching this problem (reflection over "public" members of private types) in F#. You have to use reflection over private members.

Background

For background and discussion see https://github.com/dotnet/fsharp/issues/11953#issuecomment-896878201

@dsyme says: 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.

Pros and Cons

The advantages of making this adjustment to F# are ability to emit code that follows .NET norms

The disadvantages of making this adjustment to F# are (see rant in https://github.com/dotnet/fsharp/issues/11953#issuecomment-896878201)

Extra information

Estimated cost (XS, S, M, L, XL, XXL): M (because of compat concerns)

Affidavit (please submit!)

Please tick this by placing a cross in the box:

  • [x] This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
  • [x] I have searched both open and closed suggestions on this site and believe this is not a duplicate
  • [ ] This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

  • [ ] This is not a breaking change to the F# language design
  • [x] I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the :+1: emoji on this issue. These counts are used to generally order the suggestions by engagement.

dsyme avatar Aug 11 '21 14:08 dsyme

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

With apologies for jumping in out of my depth, I don't get this.

type internal A() =
    member _.X = ()

type internal B() =
    member internal _.Y = ()

type internal C() =
    member private _.Z = ()

let x1 = A().X // allowed because X is public
let x2 = B().Y // allowed because Y is internal
let x3 = C().Z // not allowed because Z is private

If X isn't a public method, what is it?

charlesroddie avatar Aug 11 '21 18:08 charlesroddie

If X isn't a public method, what is it?

I updated your code to use "internal" since the answer is then clearer, also it's clearer when using static members.

Here type internal means "private to the assembly" . So A.X has the same logical accessibility as A - that is, it is not public, but rather internal, since it can only be accessed by things internal to the assembly. Note that when I say "logical accessibility" I mean "the set of code locations where a static access to this construct is permitted".

Thus:

type internal A() =
    static member X = ()

type internal B() =
    static member internal Y = ()

type internal C() =
    static member private Z = ()

let x1 = A.X // allowed because X it's logical visibility is **internal**
let x2 = B.Y // allowed because Y is internal
let x3 = C.Z // not allowed because Z is private

I just don't really get why we'd ever want a thing that has logical accessibility "internal" to have declared accessibility "public". It's just bringing in a new notion called "declared accessiblity" that in my mind corresponds to some random bits of information on the declaration that are useful only to reflection. If there was no reflection, there would be no point in putting "public" on members of an internal type.

Note the meaning of type private in F# is "private to the enclosing module/namespace fragment/file", an accessibility notion that doesn't really exist in C# and .NET, so easier to explain using "internal".

To move us out of OO world, consider

module internal M =
    let x = 1

Is the logical accessibility of M.x public? No. It's internal, because it's in an internal thing.

dsyme avatar Aug 11 '21 18:08 dsyme

Can CLIPublic be used on members with private/internal modifiers?

E.g.


type private A() =
    [<CLIPublic>]
    member private _.X = ()

Shall this be disallowed?

vzarytovskii avatar Aug 11 '21 21:08 vzarytovskii

Shall this be disallowed?

I don't think so. You can use it on any member or nested type (including F# types in modules)

dsyme avatar Aug 11 '21 23:08 dsyme

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.

I guess for a library that uses reflection that'd involve something like

let t = someobj.GetType()
if t.IsNestedPrivate then
    t.GetProperties(Public ||| NonPublic)
else
    t.GetProperties(Public)

Though I don't think that's a safe solution, since .NET still allows private types to have public properties.. what if a private type has both public and private properties? The author's intention might be that one should only access its publics, but stay away from its privates.. which would turn this into something like:

let t = someobj.GetType()
if t.HasPublicProperties then
    t.GetProperties(Public)
elif t.IsNestedPrivate then
    t.GetProperties(NonPublic)
else []

... seems quite complicated and comes with several assumptions to do the right thing for both .NET and F# (also it's probably still wrong).

EDIT: just verified, it really is wrong because IsNestedPrivate is false for both type private Foo = { ... } as well as for type Foo = { ... } contained in a module private ... =... but whatever... proves the point this is complicated to get right for both F# and .NET.

I think a library (like Dapper) that receives "some object" shouldn't have to mind the type's accessibility, if all it needs are the type's properties. The easiest and safest thing for a library to do would be to stick to only touching an object's publics, because one way or another they have been marked as public by their author.

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

Just my opinion but I'd prefer the public over attributes because it reads better and both type Foo = private { ... } as well as type Foo = public { ... } are already supported by the parser (even if the later is silently ignored right now).

As for compat concerns I wonder if any F# code actually uses public on private types right now.. I doubt that there's a lot such code, if any at all, since it currently has no effect / is ignored.

Whichever you pick (public or attribute), I think type private Foo = public { ... } should either do what the code says (make the members public), or emit a compiler error / warning. Right now the public is silently ignored (which took us some time to figure out why the code didn't work as intended), while type Foo = private { ... } does have an effect already.

stmax82 avatar Aug 12 '21 09:08 stmax82

Shall this be disallowed?

I don't think so. You can use it on any member or nested type (including F# types in modules)

Gotcha, so attribute shall take precedence over any member modifiers?

vzarytovskii avatar Aug 12 '21 11:08 vzarytovskii

As for compat concerns I wonder if any F# code actually uses public on private types right now.. I doubt that there's a lot such code, if any at all, since it currently has no effect / is ignored.

For compat, there is "some" code, which is too much. And undoubtedly some will use reflection.

dsyme avatar Aug 12 '21 18:08 dsyme

It looks like SRTP is also broken by this issue. In the example below in all cases it is the same error: "The member or object constructor 'get_Foo' is not public. Private members may only be accessed from within the declaring type. Protected members may only be accessed from an extending type and cannot be accessed from inner lambda expressions."

module InlineFunction =
    let inline getFoo x = (^a: (member Foo: int) x)

module UseSite1 =
    type internal MyType() =
        member this.Foo = 1

        member this.Test() = InlineFunction.getFoo this // Error

    let test () =
        let value = MyType()

        InlineFunction.getFoo value // Error

module internal UseSite2 =
    type MyType() =
        member this.Foo = 1

        member this.Test() = InlineFunction.getFoo this // Error

    let test () =
        let value = MyType()

        InlineFunction.getFoo value // Error

Tested in a library project on .NET 6. The cure is the same: make the type public all the way up.

So I think this issue needs a proper resolution and not just a "fix it" attribute.

igapanovich avatar Aug 05 '22 07:08 igapanovich

@GapanovichIgor That is covered by #230

dsyme avatar Aug 15 '22 16:08 dsyme