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.

Sign up for GitHub

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

Wrong newlines in functional style #346

Merged
merged 14 commits into from
Oct 7, 2020
Merged

Wrong newlines in functional style #346

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
1e05aab
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Sep 30, 2020
ea8adb1
Merge branch 'master' into bugfix/wrong-newlines-functional-style(#321)
aktsay6 Sep 30, 2020
f09e1de
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Sep 30, 2020
fa74792
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Sep 30, 2020
512e042
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 2, 2020
5deffa9
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 2, 2020
05fd630
Merge branch 'master' into bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 2, 2020
415afbb
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 2, 2020
9f3d5fd
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 5, 2020
3df1006
Merge branch 'master' into bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 6, 2020
3814a79
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 7, 2020
9c817af
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 7, 2020
8c52a0f
bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 7, 2020
ccbecac
Merge branch 'master' into bugfix/wrong-newlines-functional-style(#321)
aktsay6 Oct 7, 2020
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
65 changes: 59 additions & 6 deletions diktat-rules/src/main/kotlin/org/cqfn/diktat/ruleset/rules/files/NewlinesRule.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.pinterest.ktlint.core.ast.ElementType.ANDAND
import com.pinterest.ktlint.core.ast.ElementType.ARROW
import com.pinterest.ktlint.core.ast.ElementType.BINARY_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.BLOCK
import com.pinterest.ktlint.core.ast.ElementType.BLOCK_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.CALLABLE_REFERENCE_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.CALL_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.COLON
Expand All @@ -16,12 +17,14 @@ import com.pinterest.ktlint.core.ast.ElementType.DOT
import com.pinterest.ktlint.core.ast.ElementType.DOT_QUALIFIED_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.ELVIS
import com.pinterest.ktlint.core.ast.ElementType.ENUM_ENTRY
import com.pinterest.ktlint.core.ast.ElementType.EOL_COMMENT
import com.pinterest.ktlint.core.ast.ElementType.EQ
import com.pinterest.ktlint.core.ast.ElementType.FUN
import com.pinterest.ktlint.core.ast.ElementType.FUNCTION_LITERAL
import com.pinterest.ktlint.core.ast.ElementType.IDENTIFIER
import com.pinterest.ktlint.core.ast.ElementType.IF
import com.pinterest.ktlint.core.ast.ElementType.IMPORT_DIRECTIVE
import com.pinterest.ktlint.core.ast.ElementType.KDOC
import com.pinterest.ktlint.core.ast.ElementType.LAMBDA_ARGUMENT
import com.pinterest.ktlint.core.ast.ElementType.LPAR
import com.pinterest.ktlint.core.ast.ElementType.MINUS
Expand All @@ -34,15 +37,18 @@ import com.pinterest.ktlint.core.ast.ElementType.PACKAGE_DIRECTIVE
import com.pinterest.ktlint.core.ast.ElementType.PLUS
import com.pinterest.ktlint.core.ast.ElementType.PLUSEQ
import com.pinterest.ktlint.core.ast.ElementType.PRIMARY_CONSTRUCTOR
import com.pinterest.ktlint.core.ast.ElementType.REFERENCE_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.RETURN
import com.pinterest.ktlint.core.ast.ElementType.RETURN_KEYWORD
import com.pinterest.ktlint.core.ast.ElementType.SAFE_ACCESS
import com.pinterest.ktlint.core.ast.ElementType.SAFE_ACCESS_EXPRESSION
import com.pinterest.ktlint.core.ast.ElementType.SECONDARY_CONSTRUCTOR
import com.pinterest.ktlint.core.ast.ElementType.SEMICOLON
import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT
import com.pinterest.ktlint.core.ast.ElementType.VALUE_ARGUMENT_LIST
import com.pinterest.ktlint.core.ast.ElementType.VALUE_PARAMETER_LIST
import com.pinterest.ktlint.core.ast.ElementType.WHITE_SPACE
import com.pinterest.ktlint.core.ast.isLeaf
import com.pinterest.ktlint.core.ast.nextCodeSibling
import com.pinterest.ktlint.core.ast.parent
import com.pinterest.ktlint.core.ast.prevCodeSibling
Expand All @@ -52,16 +58,25 @@ import org.cqfn.diktat.ruleset.constants.Warnings.WRONG_NEWLINES
import org.cqfn.diktat.ruleset.utils.appendNewlineMergingWhiteSpace
import org.cqfn.diktat.ruleset.utils.emptyBlockList
import org.cqfn.diktat.ruleset.utils.extractLineOfText
import org.cqfn.diktat.ruleset.utils.getAllLeafsWithSpecificType
import org.cqfn.diktat.ruleset.utils.findAllNodesWithSpecificType
import org.cqfn.diktat.ruleset.utils.isBeginByNewline
import org.cqfn.diktat.ruleset.utils.isEol
import org.cqfn.diktat.ruleset.utils.isFollowedByNewline
import org.cqfn.diktat.ruleset.utils.isSingleLineIfElse
import org.cqfn.diktat.ruleset.utils.leaveOnlyOneNewLine
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.LeafPsiElement
import org.jetbrains.kotlin.com.intellij.psi.impl.source.tree.PsiWhiteSpaceImpl
import org.jetbrains.kotlin.com.intellij.psi.tree.IElementType
import org.jetbrains.kotlin.com.intellij.psi.tree.TokenSet
import org.jetbrains.kotlin.ir.expressions.IrStatementOrigin
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
import org.jetbrains.kotlin.psi.KtExpression
import org.jetbrains.kotlin.psi.KtExpressionImpl
import org.jetbrains.kotlin.psi.KtPostfixExpression
import org.jetbrains.kotlin.psi.KtQualifiedExpression
import org.jetbrains.kotlin.psi.KtSafeQualifiedExpression
import org.jetbrains.kotlin.psi.psiUtil.children
import org.jetbrains.kotlin.psi.psiUtil.parents
import org.jetbrains.kotlin.psi.psiUtil.siblings
Expand Down Expand Up @@ -89,6 +104,7 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"

private val expressionTypes = TokenSet.create(DOT_QUALIFIED_EXPRESSION, SAFE_ACCESS_EXPRESSION, CALLABLE_REFERENCE_EXPRESSION, BINARY_EXPRESSION)
private val chainExpressionTypes = TokenSet.create(DOT_QUALIFIED_EXPRESSION, SAFE_ACCESS_EXPRESSION)
private val dropChainValues = TokenSet.create(EOL_COMMENT, WHITE_SPACE, BLOCK_COMMENT, KDOC)
}

private lateinit var emitWarn: ((offset: Int, errorMessage: String, canBeAutoCorrected: Boolean) -> Unit)
Expand Down Expand Up @@ -141,6 +157,7 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
}
}

@Suppress("ComplexMethod")
private fun handleOperatorWithLineBreakBefore(node: ASTNode) {
if (node.isDotFromPackageOrImport()) {
return
Expand All @@ -150,6 +167,7 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
val isSingleLineIfElse = parent({ it.elementType == IF }, true)?.isSingleLineIfElse() ?: false
// to follow functional style these operators should be started by newline
(isFollowedByNewline() || !isBeginByNewline()) && !isSingleLineIfElse
&& (!isFirstCall() || !isMultilineLambda(treeParent))
} else {
// unless statement is simple and on single line, these operators cannot have newline after
isFollowedByNewline() && !isSingleDotStatementOnSingleLine()
Expand Down Expand Up @@ -278,6 +296,24 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
}
}


private fun ASTNode.getOrderedCallExpressions(psi: PsiElement, result: MutableList<ASTNode>) {
// if statements here have the only right order - don't change it

if (psi.children.isNotEmpty() && (psi.children[0].node.elementType != DOT_QUALIFIED_EXPRESSION
&& psi.children[0].node.elementType != SAFE_ACCESS_EXPRESSION )) {
result.add(psi.children[0].node.siblings(true)
.dropWhile { it.elementType in dropChainValues }
.first()) // node treeNext is ".", "?.", "!!", "::"
} else if (psi.children.isNotEmpty()) {
getOrderedCallExpressions(psi.children[0], result)

result.add(psi.children[0].node.siblings(true)
.dropWhile { it.elementType in dropChainValues }
.first()) // node treeNext is ".", "?.", "!!", "::"
}
}

/**
* This function is needed because many operators are represented as a single child of [OPERATION_REFERENCE] node
* e.g. [ANDAND] is a single child of [OPERATION_REFERENCE]
Expand All @@ -294,25 +330,42 @@ class NewlinesRule(private val configRules: List<RulesConfig>) : Rule("newlines"
private fun ASTNode.isDotFromPackageOrImport() = elementType == DOT &&
parent({ it.elementType == IMPORT_DIRECTIVE || it.elementType == PACKAGE_DIRECTIVE }, true) != null

/**
* taking all expressions inside complex expression until we reach lambda arguments
*/
private fun ASTNode.isCallsChain() = getParentExpressions()
.lastOrNull()
?.run {
mutableListOf<ASTNode>().also {
getAllLeafsWithSpecificType(DOT, it)
getAllLeafsWithSpecificType(SAFE_ACCESS, it)
getOrderedCallExpressions(psi, it)
}
}
?.dropWhile { !it.treeParent.textContains('(') && !it.treeParent.textContains('{') }
// fixme: we can't distinguish fully qualified names from chain of calls for now
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// fixme: we can't distinguish fully qualified names from chain of calls for now
// fixme: we can't distinguish fully qualified names (like java.lang) from chain of property accesses (like list.size) for now

Just to be sure we don't forget what this was about

?.filter { it.getParentExpressions().count() > 1 }
?.count()
?.let { it > 1 }
?: false

/**
* taking all expressions inside complex expression until we reach lambda arguments
*/
private fun ASTNode.getParentExpressions() =
parents().takeWhile { it.elementType in chainExpressionTypes && it.elementType != LAMBDA_ARGUMENT }

private fun isMultilineLambda(node: ASTNode): Boolean =
node.findAllNodesWithSpecificType(LAMBDA_ARGUMENT).firstOrNull()?.text?.count { it == '\n' } ?: -1 > 0
Copy link
Member

Choose a reason for hiding this comment

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

Won't findAllNodesWithSpecificType trigger on some nested lambdas or lambdas further in a call chain?


/**
* Getting the first call expression in call chain
*/
private fun ASTNode.isFirstCall() = getParentExpressions()
.lastOrNull()
?.run {
val firstCallee = mutableListOf<ASTNode>().also {
getOrderedCallExpressions(psi, it)
}.first()
findAllNodesWithSpecificType(firstCallee.elementType, false).first() === this@isFirstCall
} ?: false


/**
* This method should be called on OPERATION_REFERENCE in the middle of BINARY_EXPRESSION
*/
Expand Down
69 changes: 69 additions & 0 deletions diktat-rules/src/test/kotlin/org/cqfn/diktat/ruleset/chapter3/files/NewlinesRuleWarnTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -502,4 +502,73 @@ class NewlinesRuleWarnTest : LintTestBase(::NewlinesRule) {
LintError(2, 5, ruleId, singleReturnWarn, true)
)
}

@Test
@Tag(WarningNames.WRONG_NEWLINES)
fun `should not trigger`() {
lintMethod(
"""
|fun foo(): String {
|
| val a = java.lang.Boolean.getBoolean(properties.getProperty("parallel.mode"))
|
| allProperties?.filter {
| predicate(it)
| val x = listOf(1,2,3).filter { it < 3 }
| x == 0
| }
| .foo()
| .bar()
|
| allProperties?.filter {
| predicate(it)
| }
| .foo()
| .bar()
| .let {
| it.some()
| }
|
| allProperties
| ?.filter {
| predicate(it)
| }
| .foo()
| .bar()
aktsay6 marked this conversation as resolved.
Show resolved Hide resolved
| .let {
| mutableListOf().also {
|
| }
| }
|}
""".trimMargin()
)
}

@Test
@Tag(WarningNames.WRONG_NEWLINES)
fun `should trigger on non-multiline lambdas`() {
Copy link
Member

Choose a reason for hiding this comment

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

Please add example like

list.filter {

}
.map(::foo).filter {
    bar()
}

It should be considered incorrect, when multiline lambda is not the first call.

lintMethod(
"""
|fun foo(): String {
| allProperties.filter { predicate(it) }
| .foo()
| .bar()
|
| allProperties?.filter { predicate(it) }
| .foo()
| .bar()
|
| list.foo()
| .bar()
| .filter {
| baz()
| }
|}
""".trimMargin(),
LintError(2, 22, ruleId,"${WRONG_NEWLINES.warnText()} should follow functional style at .", true),
LintError(6, 22, ruleId,"${WRONG_NEWLINES.warnText()} should follow functional style at ?.", true),
LintError(10, 13, ruleId,"${WRONG_NEWLINES.warnText()} should follow functional style at .", true)
)
}
}