Repository navigation
Compiler emits incorrect visibility modifier for internal constructors of abstract class #420
Description
Compile the following code:
[<AbstractClass>]
type X internal (i : int) =
internal new () = X 1
new (f : float32) = X 1Note that we've defined an abstract class with two internal constructors and one public one. When you disassemble the code you can see that the following code was generated by the compiler:
[AbstractClass]
[CompilationMapping(SourceConstructFlags.ObjectType)]
[Serializable]
public abstract class X
{
protected X(int i)
{
base..ctor();
Program.X x = this;
}
protected X()
{
this..ctor(1);
}
protected X(float f)
{
this..ctor(1);
}
}Note how the constructors' visibilities were incorrectly changed to protected; that's only ok for the public constructor, but definitely wrong for the internal ones. The internal constructors should still be internal. I noticed that problem while deriving from such a class in a C# project where all of a sudden I had the possibility to choose between different base constructors to call, whereas only the public one of the F# class should have been visible.
Happens with VS2013 and VS2015 RC in both debug and release builds. When you remove the [<AbstractClass>] attribute, the compiler emits the correct visibilities.
I realize that fixing this bug would be a breaking change, but then again, I consider this bug to be pretty serious.
Activity
latkin commentedon May 6, 2015
Problem is here, all ctors of abstract classes are naïvely set to
family(i.e.protected). That's wrong for this case, and very wrong forprivatectors.Similar bug here, although this is even worse because this code sets them all to
assembly(i.e.internal), which I assume was just a typo.For the
internalguys, we could set access toILMemberAccess.FamilyAndAssembly. Consumers must be in derived class AND same assembly. Note this is not expressible in C# -protected internalin C# maps toFamilyOrAssembly. This option seems behaviorally correct and in line with existing conversion toprotected, however this is a rare accessibility due to lack of support in C#, so a bit risky.privatectors would also need to be specially handled, and keptprivate.I think the better option is to simply stop altering the access modifiers entirely. Although the current conversion from
publictoprotectedis "safe", it's not necessary from IL standpoint. The C# compiler does not perform such a conversion. From the reflection perspective, the conversion is harmful: code indicatespublicbut reflection tells you it'sprotected.@dsyme, do you know of a compelling reason to keep the accessibility conversion, or can we take it out?
latkin commentedon May 6, 2015
While testing this I learned that F#
privatemembers always compile to ILinternal. Interesting.So my earlier statement
is not quite accurate. They would be translated to
internal.braden commentedon May 7, 2015
I was looking into minimizing the metadata the compiler emits and noticed that everything compiles to internal, is there a reason for this? If things could be private it seems like we could trivially omit a lot of metadata (it's significant for fsharp.compiler for example)
dsyme commentedon May 7, 2015
@braden, @latkin, As background: we did make a decision in F# 1.0 (and confirmed it in 2.0) to not try to make use of "private" in IL but rather use "internal" as the minimum accessibility annotation we emitted. This was a deliberate choice, and was simplifying at the time (or seemed like it), especially when taking signature files into account.
MarcSigrist commentedon Jul 19, 2015
I am glad this is fixed; it makes the behavior of the F# compiler more predictable in multi-language solutions. Thanks.