Skip to content

Commit

Permalink
Improving variable search mechanism (#352)
Browse files Browse the repository at this point in the history
### What's done:
1) Adding mechanism for searching assignments of VARs
2) Moving all usages to new utilities of searching variables
  • Loading branch information
orchestr7 authored Oct 12, 2020
1 parent 1303570 commit d88c1ba
Show file tree
Hide file tree
Showing 15 changed files with 382 additions and 159 deletions.
5 changes: 5 additions & 0 deletions diktat-rules/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-all</artifactId>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,12 @@ import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType
import org.cqfn.diktat.common.config.rules.RulesConfig
import org.cqfn.diktat.ruleset.constants.Warnings.SAY_NO_TO_VAR
import org.cqfn.diktat.ruleset.utils.findAllNodesWithSpecificType
import org.cqfn.diktat.ruleset.utils.getAllUsages
import org.cqfn.diktat.ruleset.utils.search.findAllVariablesWithAssignments
import org.cqfn.diktat.ruleset.utils.search.findAllVariablesWithUsages
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtLambdaExpression
import org.jetbrains.kotlin.psi.KtLoopExpression
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.psiUtil.getParentOfType

/**
Expand All @@ -29,22 +28,33 @@ class ImmutableValNoVarRule(private val configRules: List<RulesConfig>) : Rule("
emitWarn = emit
isFixMode = autoCorrect

if (node.elementType == ElementType.FUN) {
node.findAllNodesWithSpecificType(ElementType.PROPERTY)
.map { it.psi as KtProperty }
// we can force to be immutable only variables that are from local context (not from class and not from file-level)
.filter { it.isLocal && it.name != null && it.parent is KtBlockExpression }
.filter { it.isVar }
.forEach { property ->
val usedInAccumulators = property.getAllUsages().any {
it.getParentOfType<KtLoopExpression>(true) != null ||
it.getParentOfType<KtLambdaExpression>(true) != null
}

if (!usedInAccumulators) {
SAY_NO_TO_VAR.warn(configRules, emitWarn, isFixMode, property.text, property.node.startOffset, property.node)
}
}
if (node.elementType == ElementType.FILE) {
// we will raise warning for cases when var property has no assignments
val varNoAssignments = node
.findAllVariablesWithAssignments { it.name != null && it.isVar }
.filter { it.value.isEmpty() }

varNoAssignments.forEach { (property, usages) ->
// FixMe: raise another warning and fix the code (change to val) for variables without assignment
}

// we can force to be immutable only variables that are from local context (not from class and not from file-level)
val usages = node
.findAllVariablesWithUsages { it.isLocal && it.name != null && it.parent is KtBlockExpression && it.isVar }
.filter { !varNoAssignments.containsKey(it.key) }

usages.forEach { (property, usages) ->
val usedInAccumulators = usages.any {
it.getParentOfType<KtLoopExpression>(true) != null ||
it.getParentOfType<KtLambdaExpression>(true) != null
}

if (!usedInAccumulators) {
SAY_NO_TO_VAR.warn(configRules, emitWarn, isFixMode, property.text, property.node.startOffset, property.node)
}
}

return
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ class FileSize(private val configRules: List<RulesConfig>) : Rule("file-size") {
checkFileSize(node, configuration.maxSize)
}
}
return
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class FileStructureRule(private val configRules: List<RulesConfig>) : Rule("file
if (checkFileHasCode(node)) {
checkCodeBlocksOrderAndEmptyLines(node)
}
return
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,14 @@ package org.cqfn.diktat.ruleset.rules.identifiers

import com.pinterest.ktlint.core.Rule
import com.pinterest.ktlint.core.ast.ElementType.FILE
import com.pinterest.ktlint.core.ast.ElementType.PROPERTY
import com.pinterest.ktlint.core.ast.isPartOfComment
import com.pinterest.ktlint.core.ast.lineNumber
import org.cqfn.diktat.common.config.rules.RulesConfig
import org.cqfn.diktat.ruleset.constants.Warnings.LOCAL_VARIABLE_EARLY_DECLARATION
import org.cqfn.diktat.ruleset.utils.containsOnlyConstants
import org.cqfn.diktat.ruleset.utils.findAllNodesWithSpecificType
import org.cqfn.diktat.ruleset.utils.getAllUsages
import org.cqfn.diktat.ruleset.utils.getDeclarationScope
import org.cqfn.diktat.ruleset.utils.lastLineNumber
import org.cqfn.diktat.ruleset.utils.search.findAllVariablesWithUsages
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.com.intellij.psi.PsiElement
import org.jetbrains.kotlin.com.intellij.psi.PsiWhiteSpace
Expand Down Expand Up @@ -57,7 +55,7 @@ class LocalVariablesRule(private val configRules: List<RulesConfig>) : Rule("loc

if (node.elementType == FILE) {
// collect all local properties and associate with corresponding references
val propertiesToUsages = collectPropertiesWithUsages(node)
val propertiesToUsages = collectLocalPropertiesWithUsages(node)

// find all usages which include more than one property
val multiPropertyUsages = groupPropertiesByUsages(propertiesToUsages)
Expand All @@ -73,16 +71,14 @@ class LocalVariablesRule(private val configRules: List<RulesConfig>) : Rule("loc
}
}

private fun collectPropertiesWithUsages(node: ASTNode) = node
.findAllNodesWithSpecificType(PROPERTY)
.map { it.psi as KtProperty }
.filter { it.isLocal && it.name != null && it.parent is KtBlockExpression }
.filter {
it.isVar && it.initializer == null ||
(it.initializer?.containsOnlyConstants() ?: false) ||
(it.initializer as? KtCallExpression).isWhitelistedMethod()
private fun collectLocalPropertiesWithUsages(node: ASTNode) = node
.findAllVariablesWithUsages { propertyNode ->
propertyNode.isLocal && propertyNode.name != null && propertyNode.parent is KtBlockExpression &&
(propertyNode.isVar && propertyNode.initializer == null ||
(propertyNode.initializer?.containsOnlyConstants() ?: false) ||
(propertyNode.initializer as? KtCallExpression).isWhitelistedMethod())
}
.associateWith { it.getAllUsages() }

.filterNot { it.value.isEmpty() }

private fun groupPropertiesByUsages(propertiesToUsages: Map<KtProperty, List<KtNameReferenceExpression>>) = propertiesToUsages
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,6 @@ fun ASTNode.findChildBefore(beforeThisNodeType: IElementType, childNodeType: IEl
.find { it.elementType == childNodeType }
?.let { return it }

log.warn("Not able to find a node with type $childNodeType before $beforeThisNodeType")
return null
}

Expand All @@ -160,7 +159,6 @@ fun ASTNode.findChildAfter(afterThisNodeType: IElementType, childNodeType: IElem
}
}

log.warn("Not able to find a node with type $childNodeType after $afterThisNodeType")
return null
}

Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
package org.cqfn.diktat.ruleset.utils.search

import com.pinterest.ktlint.core.ast.ElementType
import org.cqfn.diktat.ruleset.utils.findAllNodesWithSpecificType
import org.cqfn.diktat.ruleset.utils.getDeclarationScope
import org.cqfn.diktat.ruleset.utils.isGoingAfter
import org.jetbrains.kotlin.com.intellij.lang.ASTNode
import org.jetbrains.kotlin.psi.KtClassBody
import org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtNameReferenceExpression
import org.jetbrains.kotlin.psi.KtProperty
import org.jetbrains.kotlin.psi.KtBlockExpression
import org.jetbrains.kotlin.psi.KtFunctionLiteral
import org.jetbrains.kotlin.psi.KtDotQualifiedExpression
import org.jetbrains.kotlin.psi.KtFile
import org.jetbrains.kotlin.psi.psiUtil.getChildrenOfType
import org.jetbrains.kotlin.psi.psiUtil.getParentOfType
import org.jetbrains.kotlin.psi.psiUtil.parents
import org.jetbrains.kotlin.psi.psiUtil.referenceExpression

/**
* @param node - root node of a type File that is used to search all declared properties (variables)
* it should be ONLY node of File elementType
* @param filterForVariables - condition to filter
*/
abstract class VariablesSearch(val node: ASTNode, private val filterForVariables: (KtProperty) -> Boolean) {

/**
* to complete implementation of a search mechanism you need to specify what and how you will search in current scope
* [this] - scope where to search the usages/assignments/e.t.c of the variable (can be of types KtBlockExpression/KtFile/KtClassBody)
*/
protected abstract fun KtElement.getAllSearchResults(property: KtProperty): List<KtNameReferenceExpression>

/**
* method collects all declared variables and it's usages
*
* @return a map of a property to it's usages
*/
fun collectVariables(): Map<KtProperty, List<KtNameReferenceExpression>> {
require(node.elementType == ElementType.FILE) {
"To collect all variables in a file you need to provide file root node"
}
return node
.findAllNodesWithSpecificType(ElementType.PROPERTY)
.map { it.psi as KtProperty }
.filter(filterForVariables)
.associateWith { it.getSearchResults() }
}

@Suppress("UnsafeCallOnNullableType")
fun KtProperty.getSearchResults(): List<KtNameReferenceExpression> {
return this
.getDeclarationScope()
// if declaration scope is not null - then we have found out the block where this variable is stored
// else - it is a global variable on a file level or a property on the class level
.let { declarationScope ->
// searching in the scope with declaration (in the context)
declarationScope?.getAllSearchResults(this)
// searching on the class level in class body
?: (this.getParentOfType<KtClassBody>(true)?.getAllSearchResults(this))
// searching on the file level
?: (this.getParentOfType<KtFile>(true)!!.getAllSearchResults(this))
}
}

/**
* filtering object's fields (expressions) that have same name as variable
*/
protected fun KtNameReferenceExpression.isReferenceToFieldOfObject(): Boolean {
val expression = this
return (expression.parent as? KtDotQualifiedExpression)?.run {
receiverExpression != expression && selectorExpression?.referenceExpression() == expression
} ?: false
}

/**
* filtering local properties from other context (shadowed) and lambda and function arguments with same name
* going through all parent scopes from bottom to top until we will find the scope where the initial variable was declared
* all these scopes are on lower level of inheritance that's why if in one of these scopes we will find any
* variable declaration with the same name - we will understand that it is usage of another variable
*/
protected fun isReferenceToOtherVariableWithSameName(expression: KtNameReferenceExpression,
codeBlock: KtElement, property: KtProperty): Boolean {
return expression.parents
// getting all block expressions/class bodies/file node from bottom to the top
// FixMe: Object companion is not resolved properly yet
.filter { it is KtBlockExpression || it is KtClassBody || it is KtFile }
// until we reached the block that contains the initial declaration
.takeWhile { codeBlock != it }
.any { block ->
// this is not the expression that we needed if:
// 1) there is a new shadowed declaration for this expression (but the declaration should stay on the previous line!)
// 2) or there one of top blocks is a function/lambda that has arguments with the same name
// FixMe: in class or a file the declaration can easily go after the usage (by lines of code)
block.getChildrenOfType<KtProperty>().any { it.nameAsName == property.nameAsName && expression.node.isGoingAfter(it.node) } ||
block.parent
.let { it as? KtFunctionLiteral }
?.valueParameters
?.any { it.nameAsName == property.nameAsName }
?: false
// FixMe: also see very strange behavior of Kotlin in tests (disabled)
}
}
}

/**
* this is a small workaround in case we don't want to make any custom filter while searching variables
*/
@SuppressWarnings("FunctionOnlyReturningConstant")
fun default(node: KtProperty) = true
Loading

0 comments on commit d88c1ba

Please sign in to comment.