NestedClassesVisibility(False negative): Nested class doesn't inherit visibility from parent

[sridhar-vadlamani-ck]

From the comment,

NonCompliant

internal class NestedClassesVisibility {
   public class NestedPublicClass // should not be public
}

Compliant

 internal class NestedClassesVisibility {
     internal class NestedPublicClass
 }

In actuality, both should be Non Compliant.

This is because both are equivalent as the nested class inherits its visibility from its parent class.

So effectively, the public class in the initial example is also internal.

The way it's written, just marking an already "internal" class "internal" makes the violation go away.

[schalkms]

Sorry for handling this issue so late. This fell completely under our radar.

This is because both are equivalent as the nested class inherits its visibility from its parent class.
So effectively, the public class in the initial example is also internal.

Could you please provide a source for this statement?

Please also keep in mind that there is a reason for having this rule in the style ruleset.

[arturbosch]

If I have following code in detekt-api:

internal class A {

    class B
}

and try to use it in detekt-cli like

fun main() {
    A.B()
}

the compiler won't let you, so the visibility is inherited.

So this rule ensures that something like

class A {

    internal class B
}

is used.

[sridhar-vadlamani-ck]

The main issue with this rule is that it forces the use of an additional unnecessary modifier.

internal class A {

    class B

}

is non-compliant with the current implementation of the rule. The visibility of class B is already internal. The false positive forces the use of an additional internal like so -

internal class A {

    internal class B

}

which is pretty cumbersome.

Also, because the inner classes inherit their visibility from the parent class, inner classes cannot have visibility greater than their parent class.

So, afaik -

internal class A {

	public class B

}

class B still has internal visibility.

[arturbosch]

Thanks for the explanation, now I got your point.
I agree we should not flag no modifier at the nested class. I would still flag the public modifier to not confuse the user, maybe even with a new message.

[sridhar-vadlamani-ck]

That would work for me. :)

[BraisGabin]

Is this rule even useful?

Nested classes are often used to implement functionality local to the class it is nested in. Therefore it should not be public to other parts of the code. Prefer keeping nested classes private.

What I understand from that is that any inner class should be private. That's huge, and doesn't allow you to model your public API. For that reason I think that it's only applied to internal classes.

But even with the internal classes it allow nested internal classes. So what this rule forbids is the public classes inside intenal classes... But those don't exist because the visibility is inherited.

So, in my opinion we should:

• Remove this rule.
• Force that ANY class/object/enum/interface (except companion object) inside an internal class should be private.
• Change the description (and probably the name) of the rule to point that it flags the missleading modifiers. And we should check the private classes too.