Should not be public because its superclass is internal

A problem I've heard of before, but it's unusual to find bugs in warnings.

This little code snippet will trigger a warning underlining B. To begin with, it's superclass isn't internal, but we will assume the warning refers to Foo. Obviously, there shouldn't be a problem here as long as all occurrences of Foo in A are at least internal (or stricter).

I understand this is simply a warning, but I think it is understandable too that they can be annoying when appearing in hundreds from pods and external frameworks. Especially if without a significant reason.

Oh, by the way, this is in Swift 4.1 and Xcode 9.4 beta.

Filed here SR-7349

internal class Foo {}

public class A<T> {}

public class B: A<Foo> {} // Class should not be declared public because its superclass is internal

This isn‘t a bug but indented behavior. It just warns you that the superclass which uses an internal type as a generic type parameter become internal because of the former fact, so will the subtype become internal. You might mention in your JIRA issue that you wish for better diagnostics for such a use case.

1 Like

Adrian, can you please elaborate? I didn't quite get what you meant. I am not saying the warning itself is a bug, rather the fact that it appeared in my case.

The warning clearly states that B should not be public because A<Foo> is internal. However, the is no reason for B not to be public. Better diagnostics or not showing the warning when there isn't a significant reason for it is my call.

A is by default public, but you can also have an internal type which has public members and the compiler won't warn you (IMHO it should).

internal struct S {
  public var foo = 42
}

A<Foo> is downgraded to internal because Foo cannot and should not be made visible outside your module. If you then subclass an internal class and make the subclass public you'll get your error.

internal class X {}
// error: class cannot be declared public because its superclass is internal
public class Y : X {}

This is just a general error message which is not specific in case of generic types like yours. That's why I was saying that you should ask for enhanced diagnostics for such a case.


IIRC subclassing an internal or even a private class was part of the discussion before open/public was introduced, but I'm not 100% sure here so don't take my word seriously here. ;)

Yes, I am confident this is a valid reason to show not only a warning, but an error. Since you won't be able to access foo anywhere outside your module anyway.

A<Foo> may be internal, but if it has a member that doesn't depend on Foo, they will and should be visible in public subclasses of A<Foo> (If they are public as well of course). It is sensible to hide hierarchies, i.e. when you have subclasses built upon a class and its functionality, which you want to keep internal together with the base class. Otherwise, you would have to copy-paste... and protocols won't help with that either.

@John_McCall agrees on this in an old related topic Public classes with private superclass - #7 by Tino

But, yes, you can rephrase this as "enhanced diagnostics". I just don't want the warning to show unless it really bears important information of potential danger, i.e. possible unsafe code.

1 Like

B might not even expose Foo, and I'd say it s perfectly valid to make it public in this case.

In that simple case it doesn't but consider this:

internal class Foo {}

public class A<T> { 
   public var t: T
   public init(_ t: T) { self.t = t }
}

public class B: A<Foo> {}

Then what?

This is specifically the case when that warning is needed. Or even an error. I would say an error if public is explicit, otherwise a warning.

In this case, the problem isn't that A uses an internal class - it's just the constructor that has an issue with public.
It might start getting hairy if t had a default value ;-)

It's nothing about the constructor here. The issue is that the subclass would inherit var t: T which is var t: Foo but Foo is internal. You may think this particular problem is trivial but it really isn't, see my example.

As an alternative solution here I'd wish for closed protocols here. (cc. @anandabits) Then you could just expose Foo to the public without the fear that someone could abuse it, because no one would be able to conform to it.

Oh, right - that's marked public as well.
But only humans could overlook this - a decent compiler is finicky enough to catch such situations easily ;-)

This is correct behavior. Something being public means both that it's okay to share all details of its declaration (if not necessarily its implementation) with clients, and that at the implementation level, all symbols are available. I believe public class B: A<Foo> just barely satisfies the latter today (and wouldn't if Allow Member Lookup to Find Generic Parameters goes through), and doesn't…really…qualify for the first. You'd end up with weird cases like this:

import Lib
let b = B()
let a2: A<Foo> = b // error, can't reference 'Foo'
let a: A = b // okay, generic parameter inferred

As for the compiler allowing it only if the original class doesn't expose any public members that use the type, what happens if A is in a different library, and that library adds a new public member in its next update? That definitely shouldn't be a breaking change.

I'll tweak the JIRA report to say the message should specifically talk about a generic parameter, rather than just "its superclass".

4 Likes

Just tried it myself...The compiler doesn't even let me initialize an instance (init is inaccessible due to internal protection level). I haven't stumbled upon this because I use protocols on the outside and initialize instances only within the lib. I can neither expose the internal class, nor make all of them internal – then the protocols will have to be internal as well. I wish we could apply access levels to requirements in some safe manner.

Thanks for letting us know. At least some benefit from my mistakes. Just one question:

I can't catch the difference between those. By sharing details, you mean dependencies between libraries involving the public construct?

I wasn't being very precise, but the two things I was getting at were basically the "source" and "binary" levels of a particular declaration. If you're trying to call a function, for example, the compiler needs to know about its declaration to make sure you're using it correctly (e.g. did you pass a String to a function that expects a Float argument), and the linker needs to be able to find the function in the built framework (or dylib or static archive). Types are a little more complicated, but that's the idea.