Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Nested class, interface or enum shouldn't be publicly visible #446

Merged
merged 11 commits into from Oct 5, 2017
2 changes: 2 additions & 0 deletions detekt-cli/src/main/resources/default-detekt-config.yml
Expand Up @@ -349,6 +349,8 @@ style:
active: false
UnusedImports:
active: false
NestedClassesVisibility:
active: false

comments:
active: true
Expand Down
Expand Up @@ -23,10 +23,12 @@ fun KtModifierListOwner.isPublicNotOverridden() =
fun KtModifierListOwner.isPublic(): Boolean {
return this.hasModifier(KtTokens.PUBLIC_KEYWORD)
|| !(this.hasModifier(KtTokens.PRIVATE_KEYWORD)
|| this.hasModifier(KtTokens.PROTECTED_KEYWORD)
|| this.hasModifier(KtTokens.INTERNAL_KEYWORD))
|| this.hasModifier(KtTokens.PROTECTED_KEYWORD)
|| this.hasModifier(KtTokens.INTERNAL_KEYWORD))
}

fun KtModifierListOwner.isInternal() = this.hasModifier(KtTokens.INTERNAL_KEYWORD)

fun KtCallExpression.isUsedForNesting(): Boolean = when (getCallNameExpression()?.text) {
"run", "let", "apply", "with", "use", "forEach" -> true
else -> false
Expand Down
Expand Up @@ -12,6 +12,7 @@ import io.gitlab.arturbosch.detekt.rules.style.ForbiddenImport
import io.gitlab.arturbosch.detekt.rules.style.LoopWithTooManyJumpStatements
import io.gitlab.arturbosch.detekt.rules.style.MagicNumber
import io.gitlab.arturbosch.detekt.rules.style.ModifierOrder
import io.gitlab.arturbosch.detekt.rules.style.NestedClassesVisibility
import io.gitlab.arturbosch.detekt.rules.style.NewLineAtEndOfFile
import io.gitlab.arturbosch.detekt.rules.style.OptionalAbstractKeyword
import io.gitlab.arturbosch.detekt.rules.style.OptionalWhenBraces
Expand Down Expand Up @@ -68,7 +69,8 @@ class StyleGuideProvider : RuleSetProvider {
DataClassContainsFunctionsRule(config),
UseDataClass(config),
UnusedImports(config),
ExpressionBodySyntax(config)
ExpressionBodySyntax(config),
NestedClassesVisibility(config)
))
}
}
@@ -0,0 +1,42 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.DetektVisitor
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.rules.isInternal
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.psiUtil.isPrivate

class NestedClassesVisibility(config: Config = Config.empty) : Rule(config) {
override val issue: Issue = Issue("NestedClassesVisibility",
severity = Severity.Security,
description = "Nested types are often used for implementing private functionality. " +
"Therefore, they shouldn't be externally visible.")

private val visitor = VisibilityVisitor()

override fun visitClass(klass: KtClass) {
super.visitClass(klass)
if (klass.isInternal()) {
klass.declarations.forEach {
it.accept(visitor)
}
}
}

private fun handleClass(klass: KtClass) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could be inlined.

report(CodeSmell(issue, Entity.from(klass)))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we report the issue with a concrete description? Something along the lines of: `issue.copy(description = "Nested types are often used for implementing private functionality. However the visiblilty of ${klass.name} makes it visible externally.")

That mentions the class name and might make the description of the user a bit clearer to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Done

}

private inner class VisibilityVisitor : DetektVisitor() {
override fun visitClass(klass: KtClass) {
if (!klass.isInternal() && !klass.isPrivate()) {
handleClass(klass)
}
}
}
}
Expand Up @@ -46,7 +46,8 @@ enum class Case(val file: String) {
TooManyFunctions("/cases/TooManyFunctions.kt"),
TooManyFunctionsTopLevel("/cases/TooManyFunctionsTopLevel.kt"),
UseDataClass("/cases/UseDataClass.kt"),
UnconditionalJumpStatementInLoop("/cases/UnconditionalJumpStatementInLoop.kt");
UnconditionalJumpStatementInLoop("/cases/UnconditionalJumpStatementInLoop.kt"),
NestedClassesVisibility("/cases/NestedClassesVisibility.kt");

fun path(): Path = Paths.get(resource(file))
}
@@ -0,0 +1,18 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.gitlab.arturbosch.detekt.rules.Case
import io.gitlab.arturbosch.detekt.test.lint
import org.assertj.core.api.Assertions
import org.jetbrains.spek.api.dsl.given
import org.jetbrains.spek.api.dsl.it
import org.jetbrains.spek.subject.SubjectSpek

class NestedClassesVisibilitySpec : SubjectSpek<NestedClassesVisibility>({
subject { NestedClassesVisibility() }

given("several nested classes") {
it("check interfaces,enums,classes") {
Assertions.assertThat(subject.lint(Case.NestedClassesVisibility.path())).hasSize(3)
}
}
})
16 changes: 16 additions & 0 deletions detekt-rules/src/test/resources/cases/NestedClassesVisibility.kt
@@ -0,0 +1,16 @@
package cases

internal class ParentType {
public class NestedType { // report
}

enum class NestedEnum { One, Two } // report

private enum class NestedPrivateEnum { Three } // valid

interface Test {} // report

private interface PrivateTest {} // valid
Copy link
Member

@arturbosch arturbosch Oct 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add some cases for nested classes (nest-level > 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done + fix for enums


internal interface InternalTest {} // valid
}