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

Custom label #715

Merged
merged 6 commits into from
Jan 20, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@
enabled: true
configuration:
maxLambdaLength: 10 # max length of lambda without parameters
# Checks that using unnecessary, custom label
- name: CUSTOM_LABEL
enabled: true
# Checks that property in constructor doesn't contains comment
- name: KDOC_NO_CONSTRUCTOR_PROPERTY
enabled: true
Expand Down
6 changes: 4 additions & 2 deletions diktat-rules/src/main/kotlin/generated/WarningNames.kt
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,10 @@ public object WarningNames {

public const val RUN_BLOCKING_INSIDE_ASYNC: String = "RUN_BLOCKING_INSIDE_ASYNC"

public const val TOO_MANY_LINES_IN_LAMBDA: String = "TOO_MANY_LINES_IN_LAMBDA"

public const val CUSTOM_LABEL: String = "CUSTOM_LABEL"

public const val SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY: String =
"SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY"

Expand Down Expand Up @@ -235,6 +239,4 @@ public object WarningNames {
public const val OBJECT_IS_PREFERRED: String = "OBJECT_IS_PREFERRED"

public const val INVERSE_FUNCTION_PREFERRED: String = "INVERSE_FUNCTION_PREFERRED"

public const val TOO_MANY_LINES_IN_LAMBDA: String = "TOO_MANY_LINES_IN_LAMBDA"
}
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ enum class Warnings(
WRONG_OVERLOADING_FUNCTION_ARGUMENTS(false, "5.2.3", "use default argument instead of function overloading"),
RUN_BLOCKING_INSIDE_ASYNC(false, "5.2.4", "avoid using runBlocking in asynchronous code"),
TOO_MANY_LINES_IN_LAMBDA(false, "5.2.5", "long lambdas should have a parameter name instead of it"),
CUSTOM_LABEL(false, "5.2.6", "avoid using expression with custom label"),

// ======== chapter 6 ========
SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY(true, "6.1.1", "if a class has single constructor, it should be converted to a primary constructor"),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class BlockStructureBraces(private val configRules: List<RulesConfig>) : Rule("b
FUN, CLASS_INITIALIZER, SECONDARY_CONSTRUCTOR -> checkFun(node, configuration)
IF -> checkIf(node, configuration)
WHEN -> checkWhen(node, configuration)
FOR, WHILE, DO_WHILE -> checkLoop(node, configuration)
in loopType -> checkLoop(node, configuration)
TRY -> checkTry(node, configuration)
else -> return
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import com.pinterest.ktlint.core.ast.ElementType.WHILE_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
import com.pinterest.ktlint.core.ast.isPartOfComment
import com.pinterest.ktlint.core.ast.prevSibling
import org.cqfn.diktat.ruleset.utils.loopType
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.CompositeElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
Expand Down Expand Up @@ -50,7 +51,7 @@ class BracesInConditionalsAndLoopsRule(private val configRules: List<RulesConfig
when (node.elementType) {
IF -> checkIfNode(node)
WHEN -> checkWhenBranches(node)
FOR, WHILE, DO_WHILE -> checkLoop(node)
in loopType -> checkLoop(node)
else -> return
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package org.cqfn.diktat.ruleset.rules

import org.cqfn.diktat.common.config.rules.RulesConfig
import org.cqfn.diktat.ruleset.constants.EmitType
import org.cqfn.diktat.ruleset.constants.Warnings.CUSTOM_LABEL

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.BREAK
import com.pinterest.ktlint.core.ast.ElementType.CALL_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.CONTINUE
import com.pinterest.ktlint.core.ast.ElementType.LABEL_QUALIFIER
import com.pinterest.ktlint.core.ast.ElementType.REFERENCE_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.RETURN
import org.cqfn.diktat.ruleset.utils.loopType
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.psi.psiUtil.parents

/**
* Rule that checks using custom label
*/
class CustomLabel(private val configRules: List<RulesConfig>) : Rule("custom-label") {
private var isFixMode: Boolean = false
private val forEachReference = listOf("forEach", "forEachIndexed")
private val labels = listOf("@loop", "@forEach", "@forEachIndexed")
private val stopWords = listOf(RETURN, BREAK, CONTINUE)
private lateinit var emitWarn: EmitType

override fun visit(node: ASTNode,
autoCorrect: Boolean,
emit: EmitType) {
emitWarn = emit
isFixMode = autoCorrect

if (node.elementType == LABEL_QUALIFIER && node.text !in labels && node.treeParent.elementType in stopWords) {
val nestedCount = node.parents().count {
it.elementType in loopType ||
(it.elementType == CALL_EXPRESSION && it.findChildByType(REFERENCE_EXPRESSION)?.text in forEachReference)
kentr0w marked this conversation as resolved.
Show resolved Hide resolved
}
if (nestedCount == 1) {
CUSTOM_LABEL.warn(configRules, emitWarn, isFixMode, node.text, node.startOffset, node)
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,7 @@ class DiktatRuleSetProvider(private var diktatConfigFile: String = DIKTAT_ANALYS
::PropertyAccessorFields,
::AbstractClassesRule,
::SingleInitRule,
::CustomLabel,
::VariableGenericTypeDeclarationRule,
::LongNumericalValuesSeparatedRule,
::NestedFunctionBlock,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,14 @@
package org.cqfn.diktat.ruleset.utils

import com.pinterest.ktlint.core.ast.ElementType.BLOCK_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.DO_WHILE
import com.pinterest.ktlint.core.ast.ElementType.EOL_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.FOR
import com.pinterest.ktlint.core.ast.ElementType.KDOC
import com.pinterest.ktlint.core.ast.ElementType.LBRACE
import com.pinterest.ktlint.core.ast.ElementType.RBRACE
import com.pinterest.ktlint.core.ast.ElementType.SEMICOLON
import com.pinterest.ktlint.core.ast.ElementType.WHILE
import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE

/**
Expand All @@ -24,6 +27,7 @@ internal const val SET_PREFIX = "set"
val emptyBlockList = listOf(LBRACE, WHITE_SPACE, SEMICOLON, RBRACE)

val commentType = listOf(BLOCK_COMMENT, EOL_COMMENT, KDOC)
val loopType = listOf(FOR, WHILE, DO_WHILE)
val copyrightWords = setOf("copyright", "版权")

internal const val EMPTY_BLOCK_TEXT = "{}"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -568,7 +568,7 @@ fun ASTNode.findLBrace(): ASTNode? = when (this.elementType) {
ElementType.THEN, ElementType.ELSE, ElementType.FUN, ElementType.TRY, ElementType.CATCH, ElementType.FINALLY ->
this.findChildByType(ElementType.BLOCK)?.findChildByType(LBRACE)
ElementType.WHEN -> this.findChildByType(LBRACE)!!
ElementType.FOR, ElementType.WHILE, ElementType.DO_WHILE ->
in loopType ->
this.findChildByType(ElementType.BODY)
?.findChildByType(ElementType.BLOCK)
?.findChildByType(LBRACE)
Expand Down
3 changes: 3 additions & 0 deletions diktat-rules/src/main/resources/diktat-analysis-huawei.yml
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@
enabled: true
configuration:
maxLambdaLength: 10 # max length of lambda without parameters
# Checks that using unnecessary, custom label
- name: CUSTOM_LABEL
enabled: true
# Checks that property in constructor doesn't contains comment
- name: KDOC_NO_CONSTRUCTOR_PROPERTY
enabled: true
Expand Down
3 changes: 3 additions & 0 deletions diktat-rules/src/main/resources/diktat-analysis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,9 @@
enabled: true
configuration:
maxLambdaLength: 10 # max length of lambda without parameters
# Checks that using unnecessary, custom label
- name: CUSTOM_LABEL
enabled: true
# 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
@@ -0,0 +1,75 @@
package org.cqfn.diktat.ruleset.chapter5

import org.cqfn.diktat.ruleset.constants.Warnings.CUSTOM_LABEL
import org.cqfn.diktat.ruleset.rules.CustomLabel
import org.cqfn.diktat.ruleset.rules.DIKTAT_RULE_SET_ID
import org.cqfn.diktat.util.LintTestBase

import com.pinterest.ktlint.core.LintError
import generated.WarningNames
import org.junit.jupiter.api.Tag
import org.junit.jupiter.api.Test

class CustomLabelsTest : LintTestBase(::CustomLabel) {
private val ruleId = "$DIKTAT_RULE_SET_ID:custom-label"

@Test
@Tag(WarningNames.CUSTOM_LABEL)
fun `should trigger custom label`() {
lintMethod(
"""
fun foo() {
run qwe@ {
q.forEach {
return@qwe
}
}
q.forEachIndexed { index, i ->
return@forEachIndexed
}
loop@ for(i: Int in q) {
println(i)
break@loop
kentr0w marked this conversation as resolved.
Show resolved Hide resolved
}
qq@ for(i: Int in q) {
println(i)
break@qq
}
q.run {
it.map {
it.foreach{
break@forEach
}
}
}
}
""".trimMargin(),
LintError(4, 39, ruleId, "${CUSTOM_LABEL.warnText()} @qwe", false),
LintError(16, 34, ruleId, "${CUSTOM_LABEL.warnText()} @qq", false)
)
}

@Test
@Tag(WarningNames.CUSTOM_LABEL)
fun `should not trigger custom label in nested expression`() {
lintMethod(
"""
fun foo() {
qq@ for(i: Int in q) {
for (j: Int in q) {
println(i)
break@qq
}
}

q.forEach outer@ {
it.forEach {
if(it == 21)
return@outer
}
}
}
""".trimMargin()
)
}
}
1 change: 1 addition & 0 deletions info/available-rules.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
| 5 | 5.2.3 | WRONG_OVERLOADING_FUNCTION_ARGUMENTS | Check: function has overloading instead use default arguments | no | no |
| 5 | 5.2.4 | RUN_BLOCKING_INSIDE_ASYNC | Check: using runBlocking inside async block code | no | no | - |
| 5 | 5.2.5 | TOO_MANY_LINES_IN_LAMBDA | Check: that the long lambda has parameters | no | maxLambdaLength |
| 5 | 5.2.6 | CUSTOM_LABEL | Check: that using unnecessary, custom label | no | no| - |
| 6 | 6.1.1 | SINGLE_CONSTRUCTOR_SHOULD_BE_PRIMARY | Check: warns if there is only one secondary constructor in a class<br>Fix: converts it to a primary constructor | yes | no | Support more complicated logic of constructor conversion |
| 6 | 6.1.2 | USE_DATA_CLASS | Check: if class can be made as data class | no | no | yes |
| 6 | 6.1.3 | EMPTY_PRIMARY_CONSTRUCTOR | Check: if there is empty primary constructor | yes | no | yes |
Expand Down
6 changes: 4 additions & 2 deletions info/guide/guide-TOC.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,10 @@ I [Preface](#c0)
* [5.2 Function arguments](#c5.2)
* [5.2.1 The lambda parameter of the function should be placed at the end of the argument list](#r5.2.1)
* [5.2.2 Number of function parameters should be limited to five](#r5.2.2)
* [5.2.3 Use default values for function arguments instead of overloading them](#r5.2.3)
* [5.2.5 Long lambdas should have explicit parameters](#r5.2.4)
* [5.2.3 Use default values for function arguments instead of overloading them](#r5.2.3)
* [5.2.4 Synchronizing code inside asynchronous code](#r5.2.4)
* [5.2.5 Long lambdas should have explicit parameters](#r5.2.5)
* [5.2.6 Avoid using unnecessary, custom label](#r5.2.6)

[6. Classes, interfaces, and extension functions](#c6)
* [6.1 Classes](#c6.1)
Expand Down
26 changes: 26 additions & 0 deletions info/guide/guide-chapter-5.md
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,29 @@ GlobalScope.async {
#### <a name="r5.2.5"></a> 5.2.5 Long lambdas should have explicit parameters
The lambda without parameters shouldn't be too long.
If a lambda is too long, it can confuse the user. Lambda without parameters should consist of 10 lines (non-empty and non-comment) in total.

#### <a name="r5.2.6"></a> 5.2.6 Avoid using unnecessary, custom label
kentr0w marked this conversation as resolved.
Show resolved Hide resolved
Expressions with unnecessary, custom labels generally increase complexity and worsen the maintainability of the code.
kentr0w marked this conversation as resolved.
Show resolved Hide resolved

**Invalid example**:
```kotlin
run lab@ {
list.forEach {
return@lab
}
}
```

**Valid example**:
```kotlin
list.forEachIndexed { index, i ->
return@forEachIndexed
}

lab@ for(i: Int in q) {
for (j: Int in q) {
println(i)
break@lab
}
}
```