diff --git a/README.md b/README.md index 75817f6a5..01cdc2311 100644 --- a/README.md +++ b/README.md @@ -228,8 +228,8 @@ If you contributed to detekt but your name is not in the list, please feel free - [George Poothicottu Jacob](https://github.com/geojakes) - Bug fix: Rule deactivation - [Mohamed Elmahdi](https://github.com/mohamed-elmahdi) - Rule Improvement: Add descriptive alias - [Michael McCormick](https://github.com/MichaelM97) - Documentation improvement - - [Hans-Martin Schuller](https://github.com/hmSchuller) - Rule Improvement: ForbiddenComment +- [Lukasz Osowicki](https://github.com/lukaszosowicki) - New rule: OutdatedDocumentation ### Mentions diff --git a/detekt-core/src/main/resources/default-detekt-config.yml b/detekt-core/src/main/resources/default-detekt-config.yml index 0c5a76d13..b072cf8ab 100644 --- a/detekt-core/src/main/resources/default-detekt-config.yml +++ b/detekt-core/src/main/resources/default-detekt-config.yml @@ -62,6 +62,10 @@ comments: EndOfSentenceFormat: active: false endOfSentenceFormat: '([.?!][ \t\n\r\f<])|([.?!:]$)' + OutdatedDocumentation: + active: false + matchTypeParameters: true + matchDeclarationsOrder: true UndocumentedPublicClass: active: false excludes: ['**/test/**', '**/androidTest/**', '**/commonTest/**', '**/jvmTest/**', '**/jsTest/**', '**/iosTest/**'] diff --git a/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/CommentSmellProvider.kt b/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/CommentSmellProvider.kt index dc9518126..8d93bd8e9 100644 --- a/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/CommentSmellProvider.kt +++ b/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/CommentSmellProvider.kt @@ -20,6 +20,7 @@ class CommentSmellProvider : DefaultRuleSetProvider { CommentOverPrivateFunction(config), CommentOverPrivateProperty(config), KDocStyle(config), + OutdatedDocumentation(config), UndocumentedPublicClass(config), UndocumentedPublicFunction(config), UndocumentedPublicProperty(config), diff --git a/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentation.kt b/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentation.kt new file mode 100644 index 000000000..3266bc706 --- /dev/null +++ b/detekt-rules-documentation/src/main/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentation.kt @@ -0,0 +1,198 @@ +package io.gitlab.arturbosch.detekt.rules.documentation + +import io.gitlab.arturbosch.detekt.api.CodeSmell +import io.gitlab.arturbosch.detekt.api.Config +import io.gitlab.arturbosch.detekt.api.Debt +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.api.config +import io.gitlab.arturbosch.detekt.api.internal.Configuration +import org.jetbrains.kotlin.kdoc.parser.KDocKnownTag +import org.jetbrains.kotlin.kdoc.psi.api.KDoc +import org.jetbrains.kotlin.kdoc.psi.impl.KDocSection +import org.jetbrains.kotlin.kdoc.psi.impl.KDocTag +import org.jetbrains.kotlin.psi.KtClass +import org.jetbrains.kotlin.psi.KtNamedDeclaration +import org.jetbrains.kotlin.psi.KtNamedFunction +import org.jetbrains.kotlin.psi.KtParameter +import org.jetbrains.kotlin.psi.KtPrimaryConstructor +import org.jetbrains.kotlin.psi.KtSecondaryConstructor +import org.jetbrains.kotlin.psi.psiUtil.PsiChildRange +import org.jetbrains.kotlin.psi.psiUtil.allChildren +import org.jetbrains.kotlin.psi.psiUtil.isPropertyParameter + +/** + * This rule will report any class, function or constructor with KDoc that does not match declaration signature. + * If KDoc is not present or does not contain any @param or @property tags, rule violation will not be reported. + * By default, both type and value parameters need to be matched and declarations orders must be preserved. You can + * turn off these features using configuration options. + * + * + * /** + * * @param someParam + * * @property someProp + * */ + * class MyClass(otherParam: String, val otherProp: String) + * + * /** + * * @param T + * * @param someParam + * */ + * fun myFun(someParam: String) + * + * + * + * + * /** + * * @param someParam + * * @property someProp + * */ + * class MyClass(someParam: String, val someProp: String) + * + * /** + * * @param T + * * @param S + * * @param someParam + * */ + * fun myFun(someParam: String) + * + * + */ +@Suppress("TooManyFunctions") +class OutdatedDocumentation(config: Config = Config.empty) : Rule(config) { + + override val issue = Issue( + javaClass.simpleName, + Severity.Maintainability, + "KDoc should match actual function or class signature", + Debt.TEN_MINS + ) + + @Configuration("if type parameters should be matched") + private val matchTypeParameters: Boolean by config(true) + + @Configuration("if the order of declarations should be preserved") + private val matchDeclarationsOrder: Boolean by config(true) + + override fun visitClass(klass: KtClass) { + super.visitClass(klass) + reportIfDocumentationIsOutdated(klass) { getClassDeclarations(klass) } + } + + override fun visitSecondaryConstructor(constructor: KtSecondaryConstructor) { + super.visitSecondaryConstructor(constructor) + reportIfDocumentationIsOutdated(constructor) { getSecondaryConstructorDeclarations(constructor) } + } + + override fun visitNamedFunction(function: KtNamedFunction) { + super.visitNamedFunction(function) + reportIfDocumentationIsOutdated(function) { getFunctionDeclarations(function) } + } + + private fun getClassDeclarations(klass: KtClass): List { + val ctor = klass.primaryConstructor ?: return emptyList() + val constructorDeclarations = getPrimaryConstructorDeclarations(ctor) + val typeParams = if (matchTypeParameters) { + klass.typeParameters.mapNotNull { it.name.toParamOrNull() } + } else emptyList() + return typeParams + constructorDeclarations + } + + private fun getFunctionDeclarations(function: KtNamedFunction): List { + val typeParams = if (matchTypeParameters) { + function.typeParameters.mapNotNull { it.name.toParamOrNull() } + } else emptyList() + val valueParams = function.valueParameters.mapNotNull { it.name.toParamOrNull() } + return typeParams + valueParams + } + + private fun getPrimaryConstructorDeclarations(constructor: KtPrimaryConstructor): List { + return getDeclarationsForValueParameters(constructor.valueParameters) + } + + private fun getSecondaryConstructorDeclarations(constructor: KtSecondaryConstructor): List { + return getDeclarationsForValueParameters(constructor.valueParameters) + } + + private fun getDeclarationsForValueParameters(valueParameters: List): List { + return valueParameters.mapNotNull { + it.name?.let { name -> + val type = if (it.isPropertyParameter()) DeclarationType.PROPERTY else DeclarationType.PARAM + Declaration(name, type) + } + } + } + + private fun getDocDeclarations(doc: KDoc): List { + return processDocChildren(doc.allChildren) + } + + private fun processDocChildren(children: PsiChildRange): List { + return children + .map { + when (it) { + is KDocSection -> processDocChildren(it.allChildren) + is KDocTag -> processDocTag(it) + else -> emptyList() + } + } + .fold(emptyList()) { acc, declarations -> acc + declarations } + } + + private fun processDocTag(docTag: KDocTag): List { + val knownTag = docTag.knownTag + val subjectName = docTag.getSubjectName() ?: return emptyList() + return when (knownTag) { + KDocKnownTag.PARAM -> listOf(Declaration(subjectName, DeclarationType.PARAM)) + KDocKnownTag.PROPERTY -> listOf(Declaration(subjectName, DeclarationType.PROPERTY)) + else -> emptyList() + } + } + + private fun reportIfDocumentationIsOutdated( + element: KtNamedDeclaration, + elementDeclarationsProvider: () -> List + ) { + val doc = element.docComment ?: return + val docDeclarations = getDocDeclarations(doc) + if (docDeclarations.isNotEmpty()) { + val elementDeclarations = elementDeclarationsProvider() + if (!declarationsMatch(docDeclarations, elementDeclarations)) { + reportCodeSmell(element) + } + } + } + + private fun declarationsMatch(doc: List, element: List): Boolean { + return if (matchDeclarationsOrder) { + doc == element + } else { + doc.sortedBy { it.name } == element.sortedBy { it.name } + } + } + + private fun reportCodeSmell(element: KtNamedDeclaration) { + report( + CodeSmell( + issue, + Entity.atName(element), + "Documentation of ${element.nameAsSafeName} is outdated" + ) + ) + } + + private fun String?.toParamOrNull(): Declaration? { + return this?.let { Declaration(it, DeclarationType.PARAM) } + } + + data class Declaration( + val name: String, + val type: DeclarationType + ) + + enum class DeclarationType { + PARAM, PROPERTY + } +} diff --git a/detekt-rules-documentation/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentationSpec.kt b/detekt-rules-documentation/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentationSpec.kt new file mode 100644 index 000000000..bbf03b58c --- /dev/null +++ b/detekt-rules-documentation/src/test/kotlin/io/gitlab/arturbosch/detekt/rules/documentation/OutdatedDocumentationSpec.kt @@ -0,0 +1,368 @@ +package io.gitlab.arturbosch.detekt.rules.documentation + +import io.gitlab.arturbosch.detekt.test.TestConfig +import io.gitlab.arturbosch.detekt.test.compileAndLint +import org.assertj.core.api.Assertions.assertThat +import org.spekframework.spek2.Spek +import org.spekframework.spek2.style.specification.describe + +class OutdatedDocumentationSpec : Spek({ + val subject by memoized { OutdatedDocumentation() } + + describe("OutdatedDocumentation rule") { + + describe("general") { + it("should not report when doc is missing") { + val withoutDoc = """ + class MyClass(someParam: String, val someProp: String) + """ + assertThat(subject.compileAndLint(withoutDoc)).isEmpty() + } + + it("should not report when doc does not contain any property or param tags") { + val docWithoutParamAndPropertyTags = """ + /** + * Some class description without referring to tags or properties + */ + class MyClass(someParam: String, val someProp: String) + """ + assertThat(subject.compileAndLint(docWithoutParamAndPropertyTags)).isEmpty() + } + } + + describe("class") { + it("should not report when doc match class params") { + val correctParam = """ + /** + * @param someParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(subject.compileAndLint(correctParam)).isEmpty() + } + + it("should report when doc mismatch class param name") { + val incorrectParamName = """ + /** + * @param someParam Description of param + */ + class MyClass(otherParam: String) + """ + assertThat(subject.compileAndLint(incorrectParamName)).hasSize(1) + } + + it("should report when doc mismatch class param list") { + val incorrectListOfParams = """ + /** + * @param someParam Description of param + * @param someSecondParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(subject.compileAndLint(incorrectListOfParams)).hasSize(1) + } + + it("should report when doc mismatch class param list order") { + val incorrectParamOrder = """ + /** + * @param someParam Description of param + * @param otherParam Description of param + */ + class MyClass(otherParam: String, someParam: String) + """ + assertThat(subject.compileAndLint(incorrectParamOrder)).hasSize(1) + } + + it("should not report when doc match class params and props") { + val correctParamAndProp = """ + /** + * @param someParam Description of param + * @property someProp Description of property + */ + class MyClass(someParam: String, val someProp: String) + """ + assertThat(subject.compileAndLint(correctParamAndProp)).isEmpty() + } + + it("should report when doc match class params but mismatch props") { + val correctParamIncorrectProp = """ + /** + * @param someParam Description of param + * @property someProp Description of property + */ + class MyClass(someParam: String, val otherProp: String) + """ + assertThat(subject.compileAndLint(correctParamIncorrectProp)).hasSize(1) + } + + it("should report when doc mismatch class params and match props") { + val incorrectParamCorrectProp = """ + /** + * @param someParam Description of param + * @property someProp Description of property + */ + class MyClass(otherParam: String, val someProp: String) + """ + assertThat(subject.compileAndLint(incorrectParamCorrectProp)).hasSize(1) + } + + it("should report when doc for constructor is incorrect") { + val incorrectConstructorDoc = """ + class MyClass { + /** + * @param someParam + */ + constructor(otherParam: String) + } + """ + assertThat(subject.compileAndLint(incorrectConstructorDoc)).hasSize(1) + } + + it("should report when property is documented as param") { + val propertyAsParam = """ + /** + * @property someParam Description of param + * @param someProp Description of property + */ + class MyClass(someParam: String, val someProp: String) + """ + assertThat(subject.compileAndLint(propertyAsParam)).hasSize(1) + } + + it("should report when declarations order is incorrect") { + val incorrectDeclarationsOrder = """ + /** + * @property someProp Description of property + * @param someParam Description of param + */ + class MyClass(someParam: String, val someProp: String) + """ + assertThat(subject.compileAndLint(incorrectDeclarationsOrder)).hasSize(1) + } + } + + describe("class with type params") { + + it("should not report when doc match class params") { + val correctTypeParam = """ + /** + * @param T Description of type param + * @param someParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(subject.compileAndLint(correctTypeParam)).isEmpty() + } + + it("should report when doc misses type param") { + val missingTypeParam = """ + /** + * @param someParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(subject.compileAndLint(missingTypeParam)).hasSize(1) + } + + it("should report when doc mismatch type param name") { + val incorrectTypeParamName = """ + /** + * @param S Description of type param + * @param someParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(subject.compileAndLint(incorrectTypeParamName)).hasSize(1) + } + + it("should report when doc mismatch type param list") { + val incorrectTypeParamList = """ + /** + * @param T Description of type param + * @param someParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(subject.compileAndLint(incorrectTypeParamList)).hasSize(1) + } + } + + describe("function") { + + it("should not report when doc match function params") { + val correctDoc = """ + /** + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + """ + assertThat(subject.compileAndLint(correctDoc)).isEmpty() + } + + it("should report when doc mismatch function param name") { + val incorrectParamName = """ + /** + * @param someParam Description of param + */ + fun myFun(otherParam: String) {} + """ + assertThat(subject.compileAndLint(incorrectParamName)).hasSize(1) + } + } + + describe("function with type params") { + + it("should not report when doc match function params") { + val correctTypeParam = """ + /** + * @param T Description of type param + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + """ + assertThat(subject.compileAndLint(correctTypeParam)).isEmpty() + } + + it("should report when doc misses type param") { + val missingTypeParam = """ + /** + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + """ + assertThat(subject.compileAndLint(missingTypeParam)).hasSize(1) + } + + it("should report when doc mismatch type param name") { + val incorrectTypeParamName = """ + /** + * @param S Description of type param + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + """ + assertThat(subject.compileAndLint(incorrectTypeParamName)).hasSize(1) + } + + it("should report when doc mismatch type param list") { + val incorrectTypeParamList = """ + /** + * @param T Description of type param + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + """ + assertThat(subject.compileAndLint(incorrectTypeParamList)).hasSize(1) + } + + it("should report when not all type params are first declarations of doc") { + val incorrectTypeParamsOrder = """ + /** + * @param T Description of type param + * @param someParam Description of param + * @param S Description of type param + */ + fun myFun(someParam: String) {} + """ + assertThat(subject.compileAndLint(incorrectTypeParamsOrder)).hasSize(1) + } + } + + describe("advanced scenarios") { + + it("should not report when doc match all signatures") { + val correctClassWithFunction = """ + /** + * @param someParam Description of param + */ + class MyClass(someParam: String) { + /** + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + } + """ + assertThat(subject.compileAndLint(correctClassWithFunction)).isEmpty() + } + + it("should report for every class and function with incorrect doc") { + val incorrectClassWithTwoIncorrectFunctions = """ + /** + * @param someParam Description of param + */ + class MyClass(val someProp: String) { + /** + * @param someParam Description of param + */ + fun myFun(someParam: String, someSecondParam: String) {} + /** + * @param someParam Description of param + */ + fun myOtherFun(otherParam: String) {} + /** + * @param someParam Description of param + */ + class MyNestedClass(otherParam: String) + } + """ + assertThat(subject.compileAndLint(incorrectClassWithTwoIncorrectFunctions)).hasSize(4) + } + } + + describe("configuration matchTypeParameters") { + val configuredSubject by memoized { + OutdatedDocumentation(TestConfig(mapOf("matchTypeParameters" to "false"))) + } + + it("should not report when class type parameters mismatch and configuration is off") { + val incorrectClassTypeParams = """ + /** + * @param someParam Description of param + */ + class MyClass(someParam: String) + """ + assertThat(configuredSubject.compileAndLint(incorrectClassTypeParams)).isEmpty() + } + + it("should not report when function type parameters mismatch and configuration is off") { + val incorrectFunctionTypeParams = """ + /** + * @param someParam Description of param + */ + fun myFun(someParam: String) {} + """ + assertThat(configuredSubject.compileAndLint(incorrectFunctionTypeParams)).isEmpty() + } + } + + describe("configuration matchDeclarationsOrder") { + val configuredSubject by memoized { + OutdatedDocumentation(TestConfig(mapOf("matchDeclarationsOrder" to "false"))) + } + + it("should not report when declarations order mismatch and configuration is off") { + val incorrectDeclarationsOrder = """ + /** + * @param someParam Description of param + * @param otherParam Description of param + */ + class MyClass(otherParam: String, someParam: String) + """ + assertThat(configuredSubject.compileAndLint(incorrectDeclarationsOrder)).isEmpty() + } + + it("should not report when declarations with types order mismatch and configuration is off") { + val incorrectDeclarationsOrderWithType = """ + /** + * @param S Description of type param + * @param someParam Description of param + * @param otherParam Description of param + * @param T Description of param + */ + fun myFun(otherParam: String, someParam: String) {} + """ + assertThat(configuredSubject.compileAndLint(incorrectDeclarationsOrderWithType)).isEmpty() + } + } + } +})