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
@@ -0,0 +1,47 @@
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 org.jetbrains.kotlin.lexer.KtTokens
import org.jetbrains.kotlin.psi.KtClass
import org.jetbrains.kotlin.psi.psiUtil.visibilityModifierType

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(this)

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

private fun isInternal(klass: KtClass): Boolean {
return klass.visibilityModifierType() == KtTokens.INTERNAL_KEYWORD
}
Copy link
Member

Choose a reason for hiding this comment

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

Move this to Junk.kt right under the following function.

fun KtModifierListOwner.isPublic()

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


private fun isPrivate(klass: KtClass): Boolean {
return klass.visibilityModifierType() == KtTokens.PRIVATE_KEYWORD
}

Copy link
Member

Choose a reason for hiding this comment

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

Same here

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

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 class VisibilityVisitor(val rule: NestedClassesVisibility) : DetektVisitor() {
Copy link
Member

Choose a reason for hiding this comment

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

You can also use an inner class here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thx!

override fun visitClass(klass: KtClass) {
if (!rule.isInternal(klass) && !rule.isPrivate(klass)) {
rule.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
}