dotnet
Fields and methods on internal types are impossible to make public
This is causing #2184 - I don't think the Roslyn EE is doing anything wrong here.
module internal InternalModule =
type DebugProxy() =
member __.PublicMethod() = 15
member __.CustomGetter with public get() = 15
member val Items = 15
typeof<InternalModule.DebugProxy>.GetMembers(System.Reflection.BindingFlags.Public ||| System.Reflection.BindingFlags.Instance) |> printfn "%A"
// [|System.String ToString(); Boolean Equals(System.Object); Int32 GetHashCode(); System.Type GetType(); Void .ctor()|]
Note how the only public instance methods are ones inherited from System.Object. PublicMethod, CustomGetter, Items nor get_Items are available on the public interface. This is in contrast to C#, where public members on internal types stay public.
Compare the same GetMembers call on a type in a public module:
module PublicModule =
type DebugProxy() =
member __.PublicMethod() = 15
member __.CustomGetter with public get() = 15
member val Items = 15
typeof<PublicModule.DebugProxy>.GetMembers(System.Reflection.BindingFlags.Public ||| System.Reflection.BindingFlags.Instance) |> printfn "%A"
// [|Int32 PublicMethod(); Int32 get_CustomGetter(); Int32 get_Items(); System.String ToString(); Boolean Equals(System.Object); Int32 GetHashCode(); System.Type GetType(); Void .ctor(); Int32 CustomGetter; Int32 Items|]
Expected behavior
I would expect that public members are always public, irrespective of whether they're contained in non-public modules/types.
Actual behavior
Members in non-public modules/types always have their public methods set to non-public.
Known workarounds
None :(
/cc @dsyme
@saul Why did these attributes work in the past? Is this behavior different from before?
Kevin
@KevinRansom, I'm quoting the code from Roslyn EE:
// Native EE uses the accessibility of the property rather than getter // so "public object P { private get; set; }" is treated as public. // Instead, we drop properties if the getter is inaccessible.
Unfortunately in F# the getters are non public as the type is internal, so the proxy doesn't work on Roslyn EE. However I think what Roslyn is doing is reasonable, I don't think the F# compiler should make members non public just because the type is. We should follow C# in this regard.
I don't think the F# compiler should make members non public just because the type is. We should follow C# in this regard.
Right now, this is "by design in F#", though maybe we have to do something here
In my lonely logician's view of the world, it makes absolutely no sense whatsoever for a container to be internal, and its contents to be public. That's just a crazy logical situation, particularly when the default or things is "public". For example, given this:
module M = ...
let f x = ...
I really expect f to be public. I also think this
module internal M = ...
let public f x = ...
is a terrible code smell that honestly deserves a warning - f is not public in any meaningful sense.
Of course, as is often the case, my personal logical view of the world collides with the reality that C# allows you to make this distinction and reflection programmers keep writing GetMember queries which mean that the above distinctions have meaning to .NET tools.
Given this I'm ok with someone adding a PR where explicitly labeling a member as public will make its accessibility be public. It's probably just a matter of plumbing a "this really is explicitly public" flag down through the compiler, from type checker to IlxGen.fs
@saul ... could Roslyn make F# proxies work? After all these used to work, and so this is a breaking change.
I think before we introduce a language change we should explore whether Roslyn can just fix the issue.
Make sense?
Kevin
@Pilchie Who do we need to follow up with?
@KevinRansom sure. They could revert to the old behaviour of the native EE.
Of course, as is often the case, my personal logical view of the world collides with the reality that C# allows you to make this distinction and reflection programmers keep writing GetMember queries which mean that the above distinctions have meaning to .NET tools.
@dsyme I think that's exactly the point. It's completely reasonable to access a type's members with GetMember, and you would expect a public property to be public. I find it bizarre that I can explicitly mark a property as public, and it's just ignored. This took way too long for me to figure out.
You mention that this is by design - is it documented anywhere (such as in the spec itself)? This behaviour just doesn't sit right with me. It doesn't feel intuitive even after it's been explained, and as you said it runs contrary to the rest of the .Net languages. At the very least I think we should be updating GetMember etc documentation to reflect the fact that seemingly public properties are not public.
My main issue is that we're diverging from C# behaviour here, and so if you have .Net code that uses reflection, you're always going to have to keep a thought in the back of your mind like, "I need to be careful about Public/NonPublic as I may be using types generated by the F# compiler".
I've raised an issue on the Roslyn repo to get the old behaviour: https://github.com/dotnet/roslyn/issues/18581
You mention that this is by design - is it documented anywhere (such as in the spec itself)? This behaviour just doesn't sit right with me, and as you said it runs contrary to the rest of the .Net languages. At the very least I think we should be updating GetMember etc documentation to reflect the fact that seemingly public properties are not public.
@saul Yes, all in all it's totally reasonable to expect an explicit "public" on a member to actually be give a member public accessibility in the IL (or at very least give a compiler warning). I would approve a PR to give this effect.
If you'd like a hand to try to make a PR to make this happen please share a WIP branch and I'll take a look :)
I tried to take a look at this over the weekend but the compiler is way too complex for me to grok in the couple of hours that I had. If you can point me to a function that I need to change I can probably work my way out from there :) I'm very keen to get this fixed as the debugging story in F# on VS 2017 is really poor at the minute due to the lack of info from maps/lists/sets.
I just found this in the compiler;
// Workaround for .NET and Visual Studio restriction w.r.t debugger type proxys
// Mark internal constructors in internal classes as public.
let access =
if access = ILMemberAccess.Assembly && vspec.IsConstructor && IsHiddenTycon eenv.sigToImplRemapInfo vspec.MemberApparentParent.Deref then
ILMemberAccess.Public
else
access
Apparently we already apply an internal hack to make them work, perhaps slightly more of a hack is now required..
@saul checkout this: https://github.com/Microsoft/visualfsharp/pull/2830
I tried it out on my machine, Even with a public setter the debug proxy didn't work. Any ideas?
@saul Do you think it is now safe to close this as by-design, given the roslyn fix is in the works?
thanks
It's not unreasonable for me to want to have a type that isn't on the public API, but has public members that can be consumed by libraries that I'm using.
@saul Putting aside the question of what gets done for enums by default, are the any cases where this is actually needed (except when accessing things via reflection)?
@saul Reopening this since it came up again in the context of Enum codegen #4821
To me, it make no logical sense in a programming language design to allow the decalaration of "public" things inside "internal" or "private" types. If a member is inside an internal type type then it is logically speaking (at most) internal.
However, there are definitely cases where .NET reflection sees the disctinction, and where tools rely on it. As a result, if people really feel strongly that it should be possible to write
type internal X() =
static member public M() = ...
then I suppose we can support it, if the declarations are explicit.
Personally I feel such a declaration always deserves a warning saying "no, no no, your M is not accessible to the public, it is effectively internal, why are you using public?". We could add that warning with a note to suppress the warning if that is intended.
Note that this is still a separate issue to the question of codegen for enum types, since we don't support explicit accessibility attributes on the different cases of an enum type.
@dsyme, our internal programming guidelines when I was in Windows working on WCF, was to declare the members of internal types with their 'natural' visibility, I.e. the visibility you would want to give the member if the type had been declared public, I believe the rational was that we might want to, in the future, make the type public at which point allocating the visibility based on a decent public design would likely be tricky based on existing usage.
This caused us to have both public and internal members on an internal type. In general it felt perfectly natural, and seems to have been the guidance followed in most places where I have seen C# code used since.
@dsyme it’s always reflection. For example, if I want to JSON serialise a private type, I can’t do that with Newtonsoft.Json because all of the properties are private. This affects me relatively often unfortunately.
Found this issue when trying to figure out why did a yaml deserializer claim that my type had no public properties or fields. This is a big problem for any reflection based library.
I stumble on this issue while trying to use Verify for writing my tests.
This is unfortunate because when designing a library it's common to mark types as private or internal to not pollute the final API.
It means that if someone want to use Verify to test their library they need to be extra careful to work only on string or make sure to not have any internal or private type in the chain.
