Skip to content

Commit

Permalink
Fixed KdocComments for types in generic classes (#1832)
Browse files Browse the repository at this point in the history
### What's done:
- fixed case when `KDOC_EXTRA_PROPERTY` warning generation didn't take into account types in generic classes.
- added configuration `isParamTagsForGenericTypes` for `@param` tags creation for types in generic classes.
- added fixed warnings `KDOC_NO_CONSTRUCTOR_PROPERTY` for cases when configuration option are on, and then generic types must have `@param` tags in class-KDoc. Additionally added logic for replacing incorrect tag with correct one regardless of whether configuration is enabled or not.
- added new warning and fix tests.

Closes #1825
  • Loading branch information
DrAlexD authored Nov 29, 2023
1 parent 626bac9 commit 3ebe006
Show file tree
Hide file tree
Showing 17 changed files with 311 additions and 190 deletions.
1 change: 1 addition & 0 deletions diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@
configuration:
isParamTagsForParameters: false # create param tags for parameters without val or var
isParamTagsForPrivateProperties: false # create param tags for private properties
isParamTagsForGenericTypes: false # create param tags for generic types
# Checks that property in KDoc present in class
- name: KDOC_EXTRA_PROPERTY
enabled: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import org.jetbrains.kotlin.KtNodeTypes.FUN
import org.jetbrains.kotlin.KtNodeTypes.MODIFIER_LIST
import org.jetbrains.kotlin.KtNodeTypes.PRIMARY_CONSTRUCTOR
import org.jetbrains.kotlin.KtNodeTypes.PROPERTY
import org.jetbrains.kotlin.KtNodeTypes.TYPE_PARAMETER
import org.jetbrains.kotlin.KtNodeTypes.TYPE_PARAMETER_LIST
import org.jetbrains.kotlin.KtNodeTypes.TYPE_REFERENCE
import org.jetbrains.kotlin.KtNodeTypes.VALUE_PARAMETER
import org.jetbrains.kotlin.KtNodeTypes.VALUE_PARAMETER_LIST
Expand Down Expand Up @@ -76,8 +78,6 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
when (node.elementType) {
KtFileElementType.INSTANCE -> checkTopLevelDoc(node)
CLASS -> checkClassElements(node)
VALUE_PARAMETER -> checkValueParameter(node)
PRIMARY_CONSTRUCTOR -> checkParameterList(node.findChildByType(VALUE_PARAMETER_LIST))
BLOCK -> checkKdocPresent(node)
else -> {
// this is a generated else block
Expand All @@ -102,28 +102,42 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}
}

private fun checkParameterList(node: ASTNode?) {
val kdocBeforeClass = node
?.parent { it.elementType == CLASS }
?.findChildByType(KDOC) ?: return
private fun checkParameterList(
classNode: ASTNode,
typeParameterListNode: ASTNode?,
valueParameterListNode: ASTNode?
) {
if (typeParameterListNode == null && valueParameterListNode == null) {
return
}

val kdocBeforeClass = classNode.findChildByType(KDOC)
?: return

val parametersInKdoc = kdocBeforeClass
.kDocTags()
.filter { it.knownTag == KDocKnownTag.PROPERTY || it.knownTag == KDocKnownTag.PARAM }

val parametersInConstructor = node
.findChildrenMatching { it.elementType == VALUE_PARAMETER }
.mapNotNull { it.findChildByType(IDENTIFIER)?.text }
val getParametersFromListNode = { parameterListNode: ASTNode? ->
parameterListNode?.let { node ->
val parameterType = if (parameterListNode.elementType == TYPE_PARAMETER_LIST) TYPE_PARAMETER else VALUE_PARAMETER

node.findChildrenMatching { it.elementType == parameterType }
.mapNotNull { it.findChildByType(IDENTIFIER)?.text }
} ?: emptyList()
}

val parametersInTypeParameterList = getParametersFromListNode(typeParameterListNode)
val parametersInValueParameterList = getParametersFromListNode(valueParameterListNode)

parametersInKdoc
.filterNot { it.getSubjectName() == null || it.getSubjectName() in parametersInConstructor }
.forEach { KDOC_EXTRA_PROPERTY.warn(configRules, emitWarn, it.text, it.node.startOffset, node) }
.filter { it.getSubjectName() != null && it.getSubjectName() !in (parametersInTypeParameterList + parametersInValueParameterList) }
.forEach { KDOC_EXTRA_PROPERTY.warn(configRules, emitWarn, it.text, it.node.startOffset, classNode) }
}

@Suppress("UnsafeCallOnNullableType", "ComplexMethod")
private fun checkValueParameter(valueParameterNode: ASTNode) {
if (valueParameterNode.parents().none { it.elementType == PRIMARY_CONSTRUCTOR } ||
valueParameterNode.parents().any { it.elementType == TYPE_REFERENCE }) {
private fun checkValueParameter(classNode: ASTNode, valueParameterNode: ASTNode) {
if (valueParameterNode.parents().any { it.elementType == TYPE_REFERENCE }) {
return
}

Expand All @@ -139,7 +153,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
null
}

val kdocBeforeClass = valueParameterNode.parent { it.elementType == CLASS }!!.findChildByType(KDOC)
val kdocBeforeClass = classNode.findChildByType(KDOC)
val isParamTagNeeded = (!valueParameterNode.hasChildOfType(VAL_KEYWORD) && !valueParameterNode.hasChildOfType(VAR_KEYWORD)) ||
!valueParameterNode.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()

Expand All @@ -155,6 +169,15 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
?: createKdocBasicKdoc(valueParameterNode, isParamTagNeeded)
}

@Suppress("UnsafeCallOnNullableType", "ComplexMethod")
private fun checkTypeParameter(classNode: ASTNode, typeParameterNode: ASTNode) {
val kdocBeforeClass = classNode.findChildByType(KDOC)

kdocBeforeClass?.let {
checkBasicKdocBeforeClass(typeParameterNode, kdocBeforeClass, true)
} ?: createKdocBasicKdoc(typeParameterNode, true)
}

@Suppress("UnsafeCallOnNullableType", "ComplexMethod")
private fun checkBasicKdocBeforeClass(
node: ASTNode,
Expand Down Expand Up @@ -182,10 +205,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}
}
} ?: run {
val isParameter = !node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)
val isPrivateProperty = !node.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()

if (!isParamTagNeeded || (isParameter && configuration.isParamTagsForParameters) || (isPrivateProperty && configuration.isParamTagsForPrivateProperties)) {
if (isNeedToWarn(node, isParamTagNeeded)) {
val warningText = if (isParamTagNeeded) "add param <$parameterName> to KDoc" else "add property <$parameterName> to KDoc"

KDOC_NO_CONSTRUCTOR_PROPERTY.warnAndFix(configRules, emitWarn, isFixMode, warningText, node.startOffset, node) {
Expand All @@ -196,6 +216,17 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}
}

private fun isNeedToWarn(node: ASTNode, isParamTagNeeded: Boolean): Boolean {
val isParameter = node.elementType == VALUE_PARAMETER && !node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)
val isPrivateProperty = node.elementType == VALUE_PARAMETER && !node.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()
val isTypeParameterNode = node.elementType == TYPE_PARAMETER

return !isParamTagNeeded ||
(isParameter && configuration.isParamTagsForParameters) ||
(isPrivateProperty && configuration.isParamTagsForPrivateProperties) ||
(isTypeParameterNode && configuration.isParamTagsForGenericTypes)
}

private fun replaceWrongTagInClassKdoc(
kdocBeforeClass: ASTNode,
parameterName: String,
Expand Down Expand Up @@ -271,10 +302,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(

@Suppress("UnsafeCallOnNullableType")
private fun createKdocBasicKdoc(node: ASTNode, isParamTagNeeded: Boolean) {
val isParameter = !node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)
val isPrivateProperty = !node.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()

if (!isParamTagNeeded || (isParameter && configuration.isParamTagsForParameters) || (isPrivateProperty && configuration.isParamTagsForPrivateProperties)) {
if (isNeedToWarn(node, isParamTagNeeded)) {
val parameterName = node.findChildByType(IDENTIFIER)!!.text
val warningText = if (isParamTagNeeded) "add param <$parameterName> to KDoc" else "add property <$parameterName> to KDoc"

Expand All @@ -295,10 +323,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
prevComment: ASTNode,
isParamTagNeeded: Boolean
) {
val isParameter = !node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)
val isPrivateProperty = !node.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()

if (!isParamTagNeeded || (isParameter && configuration.isParamTagsForParameters) || (isPrivateProperty && configuration.isParamTagsForPrivateProperties)) {
if (isNeedToWarn(node, isParamTagNeeded)) {
val parameterName = node.findChildByType(IDENTIFIER)!!.text
val classNode = node.parent { it.elementType == CLASS }!!

Expand Down Expand Up @@ -409,10 +434,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}
}
?: run {
val isParameter = !node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)
val isPrivateProperty = !node.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()

if (!isParamTagNeeded || (isParameter && configuration.isParamTagsForParameters) || (isPrivateProperty && configuration.isParamTagsForPrivateProperties)) {
if (isNeedToWarn(node, isParamTagNeeded)) {
KDOC_NO_CONSTRUCTOR_PROPERTY_WITH_COMMENT.warnOnlyOrWarnAndFix(configRules, emitWarn, warningText, prevComment.startOffset, node, isFixable, isFixMode) {
val newKdocText = if (isParamTagNeeded) "* @param $parameterName$commentText\n " else "* @property $parameterName$commentText\n "
insertTextInKdoc(kdocBeforeClass, checkOneNewLineAfterKdocClassDescription(kdocBeforeClass, newKdocText, isFirstTagInKdoc))
Expand Down Expand Up @@ -462,10 +484,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}
}
?: run {
val isParameter = !node.hasChildOfType(VAL_KEYWORD) && !node.hasChildOfType(VAR_KEYWORD)
val isPrivateProperty = !node.getFirstChildWithType(MODIFIER_LIST).isAccessibleOutside()

if (!isParamTagNeeded || (isParameter && configuration.isParamTagsForParameters) || (isPrivateProperty && configuration.isParamTagsForPrivateProperties)) {
if (isNeedToWarn(node, isParamTagNeeded)) {
KDOC_NO_CONSTRUCTOR_PROPERTY_WITH_COMMENT.warnAndFix(configRules, emitWarn, isFixMode, warningText, prevComment.startOffset, node) {
val newKdocText = if (isParamTagNeeded) {
"* @param $parameterName ${createClassKdocTextFromEolComment(prevComment)}\n "
Expand Down Expand Up @@ -540,12 +559,25 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
} ?: kdocTagNode.addChild(LeafPsiElement(KDocTokens.TEXT, content), null)
}

private fun checkClassElements(node: ASTNode) {
val modifier = node.getFirstChildWithType(MODIFIER_LIST)
val classBody = node.getFirstChildWithType(CLASS_BODY)
private fun checkClassElements(classNode: ASTNode) {
val modifierList = classNode.getFirstChildWithType(MODIFIER_LIST)
val typeParameterList = classNode.getFirstChildWithType(TYPE_PARAMETER_LIST)
val valueParameterList = classNode.getFirstChildWithType(PRIMARY_CONSTRUCTOR)?.getFirstChildWithType(VALUE_PARAMETER_LIST)
val classBody = classNode.getFirstChildWithType(CLASS_BODY)
val classKdoc = classNode.getFirstChildWithType(KDOC)

checkParameterList(classNode, typeParameterList, valueParameterList)

typeParameterList
?.findChildrenMatching { it.elementType == TYPE_PARAMETER }
?.forEach { checkTypeParameter(classNode, it) }

valueParameterList
?.findChildrenMatching { it.elementType == VALUE_PARAMETER }
?.forEach { checkValueParameter(classNode, it) }

// if parent class is public or internal than we can check it's internal code elements
if (classBody != null && modifier.isAccessibleOutside()) {
if (classBody != null && modifierList.isAccessibleOutside()) {
classBody
.getChildren(statementsToDocument)
.filterNot {
Expand All @@ -555,7 +587,6 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
if (classElement.elementType == PROPERTY) {
// we check if property declared in class body is also documented in class header via
// `@property` tag
val classKdoc = node.getFirstChildWithType(KDOC)
val propertyInClassKdoc = classKdoc?.kDocTags()?.find {
it.knownTag == KDocKnownTag.PROPERTY && it.getSubjectName() == classElement.getIdentifierName()?.text
}
Expand Down Expand Up @@ -614,6 +645,11 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
* Create param tags for private properties
*/
val isParamTagsForPrivateProperties = config["isParamTagsForPrivateProperties"]?.toBoolean() ?: true

/**
* Create param tags for generic types
*/
val isParamTagsForGenericTypes = config["isParamTagsForGenericTypes"]?.toBoolean() ?: true
}

companion object {
Expand Down
1 change: 1 addition & 0 deletions diktat-rules/src/main/resources/diktat-analysis-huawei.yml
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@
configuration:
isParamTagsForParameters: false # create param tags for parameters without val or var
isParamTagsForPrivateProperties: false # create param tags for private properties
isParamTagsForGenericTypes: false # create param tags for generic types
# Checks that property in KDoc present in class
- name: KDOC_EXTRA_PROPERTY
enabled: true
Expand Down
1 change: 1 addition & 0 deletions diktat-rules/src/main/resources/diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -439,6 +439,7 @@
configuration:
isParamTagsForParameters: true # create param tags for parameters without val or var
isParamTagsForPrivateProperties: true # create param tags for private properties
isParamTagsForGenericTypes: true # create param tags for generic types
# Checks that the long lambda has parameters
- name: TOO_MANY_LINES_IN_LAMBDA
enabled: true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -817,4 +817,67 @@ class KdocCommentsWarnTest : LintTestBase(::KdocComments) {
DiktatError(5, 4, ruleId, "${KDOC_DUPLICATE_PROPERTY.warnText()} @param field2"),
)
}

@Test
@Tag(WarningNames.KDOC_EXTRA_PROPERTY)
fun `shouldn't warn extra property on generic type`() {
lintMethod(
"""
|/**
| * S3 implementation of Storage
| *
| * @param s3Operations [S3Operations] to operate with S3
| * @param K type of key
| */
|abstract class AbstractReactiveStorage<K : Any> constructor(
| s3Operations: S3Operations,
|) : ReactiveStorage<K> {
| // abcd
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.KDOC_NO_CONSTRUCTOR_PROPERTY)
fun `should trigger on generic type`() {
lintMethod(
"""
|/**
| * S3 implementation of Storage
| *
| * @param s3Operations [S3Operations] to operate with S3
| */
|abstract class AbstractReactiveStorage<K : Any, P: Any>(
| s3Operations: S3Operations,
|) : ReactiveStorage<K>, AnotherStorage<P> {
| // abcd
|}
""".trimMargin(),
DiktatError(6, 40, ruleId, "${KDOC_NO_CONSTRUCTOR_PROPERTY.warnText()} add param <K> to KDoc", true),
DiktatError(6, 49, ruleId, "${KDOC_NO_CONSTRUCTOR_PROPERTY.warnText()} add param <P> to KDoc", true),
)
}

@Test
@Tag(WarningNames.KDOC_NO_CONSTRUCTOR_PROPERTY)
fun `change property to param in kdoc for generic type`() {
lintMethod(
"""
|/**
| * S3 implementation of Storage
| *
| * @param s3Operations [S3Operations] to operate with S3
| * @property K type of key
| */
|abstract class AbstractReactiveStorage<K : Any, P: Any>(
| s3Operations: S3Operations,
|) : ReactiveStorage<K>, AnotherStorage<P> {
| // abcd
|}
""".trimMargin(),
DiktatError(7, 40, ruleId, "${KDOC_NO_CONSTRUCTOR_PROPERTY.warnText()} change `@property` tag to `@param` tag for <K> to KDoc", true),
DiktatError(7, 49, ruleId, "${KDOC_NO_CONSTRUCTOR_PROPERTY.warnText()} add param <P> to KDoc", true),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,7 @@ class A constructor(
* class
* comment
*
* @param K
* @property openName single-line comment
* @property openLastName
* block
Expand All @@ -196,7 +197,7 @@ class A constructor(
* kdoc property
* comment
*/
open class B constructor(
open class B<K : Any> constructor(
open val openName: String,
open val openLastName: String,
open val openBirthDate: String,
Expand All @@ -212,6 +213,9 @@ open class B constructor(
* class
* comment
*
* @param K
* @param P
* @param G
* @param privateName single-line comment
* @property protectedName single-line comment
* @property internalName single-line comment
Expand Down Expand Up @@ -255,7 +259,7 @@ open class B constructor(
* kdoc property
* comment
*/
class A constructor(
class A<K : Any, P: Any, G: Any> constructor(
private val privateName: String,
protected val protectedName: String,
internal val internalName: String,
Expand Down Expand Up @@ -304,4 +308,4 @@ class A constructor(
* comment
*/
paramAddr: String,
) : B() {}
) : B<K>(), C<P>, D<G> {}
Loading

0 comments on commit 3ebe006

Please sign in to comment.