Skip to content

Commit

Permalink
Bugfix/new lines rule dot qualified expression and safe access expres…
Browse files Browse the repository at this point in the history
…sion (#1337)

### Whats added:
 * corrected logic fix and warn long Dot Qualified Expression and Safe Access Expression and Postfix Expression
 * commented on the choice of different logics with these expressions
 * added fix test in NewLineRuleFixTest
 * corrected and added warnings in NewLineRuleWarnTest
 
  ### Issue (#865)
  • Loading branch information
Arrgentum authored Jun 15, 2022
1 parent b3d32e2 commit 3dcd13e
Show file tree
Hide file tree
Showing 24 changed files with 342 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ class DiktatGradlePluginTest {
@Test
fun `check default extension properties`() {
val diktatExtension = project.extensions.getByName("diktat") as DiktatExtension
val actualInputs = project.tasks.named("diktatCheck", DiktatJavaExecTaskBase::class.java).get().actualInputs
val actualInputs = project.tasks
.named("diktatCheck", DiktatJavaExecTaskBase::class.java)
.get()
.actualInputs
Assertions.assertFalse(diktatExtension.debug)
Assertions.assertIterableEquals(project.fileTree("src").files, actualInputs.files)
Assertions.assertTrue(actualInputs.files.isNotEmpty())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,10 @@ class HeaderCommentRule(configRules: List<RulesConfig>) : DiktatRule(

// Check if provided copyright node differs only in the first date from pattern
private fun isCopyRightTextMatchesPattern(copyrightNode: ASTNode?, copyrightPattern: String): Boolean {
val copyrightText = copyrightNode?.text?.replace("/*", "")?.replace("*/", "")?.replace("*", "")
val copyrightText = copyrightNode?.text
?.replace("/*", "")
?.replace("*/", "")
?.replace("*", "")

val datesInPattern = hyphenRegex.find(copyrightPattern)?.value
val datesInCode = copyrightText?.let { hyphenRegex.find(it)?.value }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,10 @@ class CommentsFormatting(configRules: List<RulesConfig>) : DiktatRule(
} else if (node.treeParent.elementType != FILE && node.treeParent.treePrev != null &&
node.treeParent.treePrev.treePrev != null) {
// When comment inside of a PROPERTY
node.treeParent.treePrev.treePrev.elementType == LBRACE
node.treeParent
.treePrev
.treePrev
.elementType == LBRACE
} else {
node.treeParent.getAllChildrenWithType(node.elementType).first() == node
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,9 +222,15 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
propertyInLocalKdoc: ASTNode?
) {
val kdocText = if (prevComment.elementType == KDOC) {
prevComment.text.removePrefix("/**").removeSuffix("*/").trim('\n', ' ')
prevComment.text
.removePrefix("/**")
.removeSuffix("*/")
.trim('\n', ' ')
} else {
prevComment.text.removePrefix("/*").removeSuffix("*/").trim('\n', ' ')
prevComment.text
.removePrefix("/*")
.removeSuffix("*/")
.trim('\n', ' ')
}
// if property is documented with KDoc, which has a property tag inside, then it can contain some additional more complicated
// structure, that will be hard to move automatically
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,10 +109,11 @@ class CollapseIfStatementsRule(configRules: List<RulesConfig>) : DiktatRule(

private fun takeCommentsBeforeNestedIf(node: ASTNode): List<ASTNode> {
val thenNode = (node.psi as KtIfExpression).then?.node
return thenNode?.children()?.takeWhile { it.elementType != IF }?.filter {
it.elementType == EOL_COMMENT ||
it.elementType == BLOCK_COMMENT
}
return thenNode?.children()
?.takeWhile { it.elementType != IF }
?.filter {
it.elementType == EOL_COMMENT || it.elementType == BLOCK_COMMENT
}
?.toList() ?: emptyList()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,10 @@ class LineLength(configRules: List<RulesConfig>) : DiktatRule(
}
// check, that space to split is a part of text - not code
// If the space split is part of the code, then there is a chance of breaking the code when fixing, that why we should ignore it
val isSpaceIsWhiteSpace = node.psi.findElementAt(delimiterIndex)!!.node.isWhiteSpace()
val isSpaceIsWhiteSpace = node.psi
.findElementAt(delimiterIndex)!!
.node
.isWhiteSpace()
if (isSpaceIsWhiteSpace) {
return None()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,10 @@ class StringTemplateFormatRule(configRules: List<RulesConfig>) : DiktatRule(
?.elementType == ARRAY_ACCESS_EXPRESSION

return if (onlyOneRefExpr && !isArrayAccessExpression) {
(!(node.treeNext.text.first().isLetterOrDigit() // checking if first letter is valid
(!(node.treeNext
.text
.first()
.isLetterOrDigit() // checking if first letter is valid
|| node.treeNext.text.startsWith("_")) || node.treeNext.elementType == CLOSING_QUOTE)
} else if (!isArrayAccessExpression) {
node.hasAnyChildOfTypes(FLOAT_CONSTANT, INTEGER_CONSTANT) // it also fixes "${1.0}asd" cases
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,9 @@ class FileStructureRule(configRules: List<RulesConfig>) : DiktatRule(
) {
findAllReferences(node)
packageName = (node.findChildByType(PACKAGE_DIRECTIVE)?.psi as KtPackageDirective).qualifiedName
node.findChildByType(IMPORT_LIST)?.getChildren(TokenSet.create(IMPORT_DIRECTIVE))?.toList()
node.findChildByType(IMPORT_LIST)
?.getChildren(TokenSet.create(IMPORT_DIRECTIVE))
?.toList()
?.forEach { import ->
val ktImportDirective = import.psi as KtImportDirective
val importName = ktImportDirective.importPath?.importedName?.asString()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,13 +232,17 @@ class IndentationRule(configRules: List<RulesConfig>) : DiktatRule(
/**
* If it is triple-quoted string template we need to indent all its parts
*/
@Suppress("LOCAL_VARIABLE_EARLY_DECLARATION")
private fun fixStringLiteral(
whiteSpace: PsiWhiteSpace,
expectedIndent: Int,
actualIndent: Int
) {
val textIndent = " ".repeat(expectedIndent + INDENT_SIZE)
val templateEntries = whiteSpace.node.treeNext.firstChildNode.getAllChildrenWithType(LITERAL_STRING_TEMPLATE_ENTRY)
val templateEntries = whiteSpace.node
.treeNext
.firstChildNode
.getAllChildrenWithType(LITERAL_STRING_TEMPLATE_ENTRY)
templateEntries.forEach { node ->
if (!node.text.contains("\n")) {
fixFirstTemplateEntries(node, textIndent, actualIndent)
Expand All @@ -263,8 +267,14 @@ class IndentationRule(configRules: List<RulesConfig>) : DiktatRule(
) {
val correctedText = StringBuilder()
// shift of the node depending on its initial string template indent
val nodeStartIndent = if (node.firstChildNode.text.takeWhile { it == ' ' }.count() - actualIndent - INDENT_SIZE > 0) {
node.firstChildNode.text.takeWhile { it == ' ' }.count() - actualIndent - INDENT_SIZE
val nodeStartIndent = if (node.firstChildNode
.text
.takeWhile { it == ' ' }
.count() - actualIndent - INDENT_SIZE > 0) {
node.firstChildNode
.text
.takeWhile { it == ' ' }
.count() - actualIndent - INDENT_SIZE
} else {
0
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ 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.findAllDescendantsWithSpecificType
import org.cqfn.diktat.ruleset.utils.findAllNodesWithCondition
import org.cqfn.diktat.ruleset.utils.findParentNodeWithSpecificType
import org.cqfn.diktat.ruleset.utils.getFilePath
import org.cqfn.diktat.ruleset.utils.getFirstChildWithType
import org.cqfn.diktat.ruleset.utils.getIdentifierName
import org.cqfn.diktat.ruleset.utils.getRootNode
import org.cqfn.diktat.ruleset.utils.hasParent
Expand Down Expand Up @@ -124,22 +126,82 @@ class NewlinesRule(configRules: List<RulesConfig>) : DiktatRule(
private val configuration by lazy {
NewlinesRuleConfiguration(configRules.getRuleConfig(WRONG_NEWLINES)?.configuration ?: emptyMap())
}

override fun logic(node: ASTNode) {
when (node.elementType) {
SEMICOLON -> handleSemicolon(node)
OPERATION_REFERENCE, EQ -> handleOperatorWithLineBreakAfter(node)
// this logic regulates indentation with elements - so that the symbol and the subsequent expression are on the same line
in lineBreakBeforeOperators -> handleOperatorWithLineBreakBefore(node)
LPAR -> handleOpeningParentheses(node)
COMMA -> handleComma(node)
COLON -> handleColon(node)
BLOCK -> handleLambdaBody(node)
RETURN -> handleReturnStatement(node)
SUPER_TYPE_LIST, VALUE_PARAMETER_LIST, VALUE_ARGUMENT_LIST -> handleList(node)
// this logic splits long expressions into multiple lines
DOT_QUALIFIED_EXPRESSION, SAFE_ACCESS_EXPRESSION, POSTFIX_EXPRESSION -> handDotQualifiedAndSafeAccessExpression(node)
else -> {
}
}
}

@Suppress("GENERIC_VARIABLE_WRONG_DECLARATION", "MagicNumber")
private fun handDotQualifiedAndSafeAccessExpression(node: ASTNode) {
val listParentTypesNoFix = listOf(PACKAGE_DIRECTIVE, IMPORT_DIRECTIVE, VALUE_PARAMETER_LIST,
VALUE_ARGUMENT_LIST, DOT_QUALIFIED_EXPRESSION, SAFE_ACCESS_EXPRESSION, POSTFIX_EXPRESSION)
if (isNotFindParentNodeWithSpecificManyType(node, listParentTypesNoFix)) {
val listDot = node.findAllNodesWithCondition(
withSelf = true,
excludeChildrenCondition = { !isDotQuaOrSafeAccessOrPostfixExpression(it) }
) {
isDotQuaOrSafeAccessOrPostfixExpression(it) && it.elementType != POSTFIX_EXPRESSION
}.reversed()
if (listDot.size > 3) {
val without = listDot.filterNot {
val whiteSpaceBeforeDotOrSafeAccess = it.findChildByType(DOT)?.treePrev ?: it.findChildByType(SAFE_ACCESS)?.treePrev
whiteSpaceBeforeDotOrSafeAccess?.elementType == WHITE_SPACE && whiteSpaceBeforeDotOrSafeAccess.text.lines().size > 1
}
if (without.size > 1 || (without.size == 1 && without[0] != listDot[0])) {
WRONG_NEWLINES.warnAndFix(configRules, emitWarn, isFixMode, "should be split before second and other dot/safe access", node.startOffset, node) {
fixDotQualifiedExpression(listDot)
}
}
}
}
}

/**
* Return false, if you find parent with types in list else return true
*/
private fun isNotFindParentNodeWithSpecificManyType(node: ASTNode, list: List<IElementType>): Boolean {
list.forEach { elem ->
node.findParentNodeWithSpecificType(elem)?.let {
return false
}
}
return true
}

/**
* Fix Dot Qualified Expression and Safe Access Expression -
* 1) Append new White Space node before second and subsequent node Dot or Safe Access
* in Dot Qualified Expression? Safe Access Expression and Postfix Expression
* 2) If before first Dot or Safe Access node stay White Space node with \n - remove this node
*/
private fun fixDotQualifiedExpression(list: List<ASTNode>) {
list.forEachIndexed { index, astNode ->
val dotNode = astNode.getFirstChildWithType(DOT) ?: astNode.getFirstChildWithType(SAFE_ACCESS)
val nodeBeforeDot = dotNode?.treePrev
if (index > 0) {
astNode.appendNewlineMergingWhiteSpace(nodeBeforeDot, dotNode)
}
}
}

private fun isDotQuaOrSafeAccessOrPostfixExpression(node: ASTNode) =
node.elementType == DOT_QUALIFIED_EXPRESSION || node.elementType == SAFE_ACCESS_EXPRESSION || node.elementType == POSTFIX_EXPRESSION

/**
* Check that EOL semicolon is used only in enums
*/
Expand Down Expand Up @@ -294,9 +356,13 @@ class NewlinesRule(configRules: List<RulesConfig>) : DiktatRule(
}
}

@Suppress("TOO_LONG_FUNCTION")
private fun handleLambdaBody(node: ASTNode) {
if (node.treeParent.elementType == FUNCTION_LITERAL) {
val isSingleLineLambda = node.treeParent.text.lines().size == 1
val isSingleLineLambda = node.treeParent
.text
.lines()
.size == 1
val arrowNode = node.siblings(false).find { it.elementType == ARROW }
if (!isSingleLineLambda && arrowNode != null) {
// lambda with explicit arguments
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,12 +337,18 @@ class WhiteSpaceRule(configRules: List<RulesConfig>) : DiktatRule(
else -> {
}
}
val isDeclaration = node.treeParent.elementType == VALUE_PARAMETER_LIST && node.treeParent.treeParent.elementType.let {
it == PRIMARY_CONSTRUCTOR || it == FUN || it == CALL_EXPRESSION
}
val isCall = node.treeParent.elementType == VALUE_ARGUMENT_LIST && node.treeParent.treeParent.elementType.let {
it == CONSTRUCTOR_DELEGATION_CALL || it == CALL_EXPRESSION
}
val isDeclaration = node.treeParent.elementType == VALUE_PARAMETER_LIST && node.treeParent
.treeParent
.elementType
.let {
it == PRIMARY_CONSTRUCTOR || it == FUN || it == CALL_EXPRESSION
}
val isCall = node.treeParent.elementType == VALUE_ARGUMENT_LIST && node.treeParent
.treeParent
.elementType
.let {
it == CONSTRUCTOR_DELEGATION_CALL || it == CALL_EXPRESSION
}
if (isDeclaration || isCall) {
handleToken(node, 0, 0)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ class NullChecksRule(configRules: List<RulesConfig>) : DiktatRule(
private fun isComplexIfStatement(parentIf: ASTNode): Boolean {
val parentIfPsi = parentIf.psi
require(parentIfPsi is KtIfExpression)
return (parentIfPsi.`else`?.node?.firstChildNode?.elementType == IF_KEYWORD)
return (parentIfPsi.`else`
?.node
?.firstChildNode
?.elementType == IF_KEYWORD)
}

private fun conditionInIfStatement(node: ASTNode) {
Expand Down Expand Up @@ -218,7 +221,8 @@ class NullChecksRule(configRules: List<RulesConfig>) : DiktatRule(
.treeParent
.findChildByType(type)
?.let { it.findChildByType(BLOCK) ?: it }
?.findAllNodesWithCondition { it.elementType == BREAK }?.isNotEmpty()
?.findAllNodesWithCondition { it.elementType == BREAK }
?.isNotEmpty()
?: false

private fun ASTNode.extractLinesFromBlock(type: IElementType): List<String>? =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,10 @@ class RunInScript(private val configRules: List<RulesConfig>) : Rule(NAME_ID) {

private fun checkScript(node: ASTNode) {
val isLambdaArgument = node.firstChildNode.hasChildOfType(LAMBDA_ARGUMENT)
val isLambdaInsideValueArgument = node.firstChildNode.findChildByType(VALUE_ARGUMENT_LIST)?.findChildByType(VALUE_ARGUMENT)?.findChildByType(LAMBDA_EXPRESSION) != null
val isLambdaInsideValueArgument = node.firstChildNode
.findChildByType(VALUE_ARGUMENT_LIST)
?.findChildByType(VALUE_ARGUMENT)
?.findChildByType(LAMBDA_EXPRESSION) != null
if (!isLambdaArgument && !isLambdaInsideValueArgument) {
warnRunInScript(node)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ class InlineClassesRule(configRules: List<RulesConfig>) : DiktatRule(
// Fixme: In Kotlin 1.4.30 inline classes may be used with internal constructors. When it will be released need to check it
if (hasValidProperties(classPsi) &&
!isExtendingClass(classPsi.node) &&
classPsi.node.getFirstChildWithType(MODIFIER_LIST)?.getChildren(null)?.all { it.elementType in goodModifiers } != false) {
classPsi.node
.getFirstChildWithType(MODIFIER_LIST)
?.getChildren(null)
?.all { it.elementType in goodModifiers } != false) {
// Fixme: since it's an experimental feature we shouldn't do fixer
INLINE_CLASS_CAN_BE_USED.warn(configRules, emitWarn, isFixMode, "class ${classPsi.name}", classPsi.node.startOffset, classPsi.node)
}
Expand All @@ -56,8 +59,14 @@ class InlineClassesRule(configRules: List<RulesConfig>) : DiktatRule(
return !classPsi.getProperties().single().isVar
} else if (classPsi.getProperties().isEmpty() && classPsi.hasExplicitPrimaryConstructor()) {
return classPsi.primaryConstructorParameters.size == 1 &&
!classPsi.primaryConstructorParameters.first().node.hasChildOfType(VAR_KEYWORD) &&
classPsi.primaryConstructor?.visibilityModifierType()?.value?.let { it == "public" } ?: true
!classPsi.primaryConstructorParameters
.first()
.node
.hasChildOfType(VAR_KEYWORD) &&
classPsi.primaryConstructor
?.visibilityModifierType()
?.value
?.let { it == "public" } ?: true
}
return false
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -549,15 +549,17 @@ fun ASTNode.leaveExactlyNumNewLines(num: Int) {
}

/**
* If [whiteSpaceNode] is not null and has type [WHITE_SPACE], prepend a line break to it's text.
* Otherwise, insert a new node with a line break before [beforeNode]
* If [whiteSpaceNode] is not null and has type [WHITE_SPACE] and this [WHITE_SPACE] contains 1 line, prepend a line break to it's text.
* If [whiteSpaceNode] is null or has`t type [WHITE_SPACE], insert a new node with a line break before [beforeNode]
*
* @param whiteSpaceNode a node that can possibly be modified
* @param beforeNode a node before which a new WHITE_SPACE node will be inserted
*/
fun ASTNode.appendNewlineMergingWhiteSpace(whiteSpaceNode: ASTNode?, beforeNode: ASTNode?) {
if (whiteSpaceNode != null && whiteSpaceNode.elementType == WHITE_SPACE) {
(whiteSpaceNode as LeafPsiElement).rawReplaceWithText("\n${whiteSpaceNode.text}")
if (whiteSpaceNode.text.lines().size == 1) {
(whiteSpaceNode as LeafPsiElement).rawReplaceWithText("\n${whiteSpaceNode.text}")
}
} else {
addChild(PsiWhiteSpaceImpl("\n"), beforeNode)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ class KotlinParser {
.createBlockCodeFragment(text, null)
.node
.findChildByType(BLOCK)!!
.findChildByType(ERROR_ELEMENT)!!.findChildByType(IMPORT_LIST)!!
.findChildByType(ERROR_ELEMENT)!!
.findChildByType(IMPORT_LIST)!!
}
} else {
ktPsiFactory
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ val JAVA = arrayOf("abstract", "assert", "boolean",
"try", "void", "volatile", "while")

@Suppress("VARIABLE_NAME_INCORRECT_FORMAT")
val KOTLIN = KtTokens.KEYWORDS.types.map { line -> line.toString() }
val KOTLIN = KtTokens.KEYWORDS
.types
.map { line -> line.toString() }
.plus(KtTokens.SOFT_KEYWORDS.types.map { line -> line.toString() })

val loggerPropertyRegex = "(log|LOG|logger)".toRegex()
Expand Down
Loading

0 comments on commit 3dcd13e

Please sign in to comment.