Skip to content

Commit

Permalink
Refactored warn and warnAndFix in Warnings (#1751)
Browse files Browse the repository at this point in the history
- removed `isFixMode` from `warn`
- removed `canBeAutoCorrected` from `warnAndFix`
  • Loading branch information
nulls authored Sep 26, 2023
1 parent 3c4e688 commit aa26135
Show file tree
Hide file tree
Showing 46 changed files with 173 additions and 122 deletions.
Original file line number Diff line number Diff line change
@@ -1,20 +1,13 @@
package com.saveourtool.diktat.ruleset.constants

import com.saveourtool.diktat.api.DiktatErrorEmitter
import com.saveourtool.diktat.common.config.rules.Rule
import com.saveourtool.diktat.common.config.rules.RulesConfig
import com.saveourtool.diktat.common.config.rules.isRuleEnabled
import com.saveourtool.diktat.ruleset.generation.EnumNames
import com.saveourtool.diktat.ruleset.utils.isSuppressed
import org.jetbrains.kotlin.com.intellij.lang.ASTNode

typealias EmitType = ((offset: Int,
errorMessage: String,
canBeAutoCorrected: Boolean) -> Unit)

typealias ListOfList = MutableList<MutableList<ASTNode>>

typealias ListOfPairs = MutableList<Pair<ASTNode, String>>

/**
* This class represent individual inspections of diktat code style.
* A [Warnings] entry contains rule name, warning message and is used in code check.
Expand Down Expand Up @@ -155,7 +148,7 @@ enum class Warnings(

// FixMe: change float literal to BigDecimal? Or kotlin equivalent?
FLOAT_IN_ACCURATE_CALCULATIONS(false, "4.1.1", "floating-point values shouldn't be used in accurate calculations"),
AVOID_NULL_CHECKS(false, "4.3.3", "Try to avoid explicit null-checks"),
AVOID_NULL_CHECKS(true, "4.3.3", "Try to avoid explicit null-checks"),

// ======== chapter 5 ========
TOO_LONG_FUNCTION(false, "5.1.1", "function is too long: split it or make more primitive"),
Expand Down Expand Up @@ -201,53 +194,95 @@ enum class Warnings(
*/
fun warnText() = "[${ruleName()}] ${this.warn}:"

/**
* @param configRules list of [RulesConfig]
* @param emit function that will be called on warning
* @param freeText text that will be added to the warning message
* @param offset offset from the beginning of the file
* @param node the [ASTNode] on which the warning was triggered
* @param shouldBeAutoCorrected should be auto corrected or not
* @param isFixMode whether autocorrect mode is on
* @param autoFix function that will be called to autocorrect the warning
*/
@Suppress("LongParameterList", "TOO_MANY_PARAMETERS")
fun warnOnlyOrWarnAndFix(
configRules: List<RulesConfig>,
emit: DiktatErrorEmitter,
freeText: String,
offset: Int,
node: ASTNode,
shouldBeAutoCorrected: Boolean,
isFixMode: Boolean,
autoFix: () -> Unit,
) {
if (shouldBeAutoCorrected) {
warnAndFix(configRules, emit, isFixMode, freeText, offset, node, autoFix)
} else {
warn(configRules, emit, freeText, offset, node)
}
}

/**
* @param configRules list of [RulesConfig]
* @param emit function that will be called on warning
* @param isFixMode whether autocorrect mode is on
* @param freeText text that will be added to the warning message
* @param offset offset from the beginning of the file
* @param node the [ASTNode] on which the warning was triggered
* @param canBeAutoCorrected whether this warning can be autocorrected
* @param autoFix function that will be called to autocorrect the warning
*/
@Suppress("LongParameterList", "TOO_MANY_PARAMETERS")
fun warnAndFix(configRules: List<RulesConfig>,
emit: EmitType,
isFixMode: Boolean,
freeText: String,
offset: Int,
node: ASTNode,
canBeAutoCorrected: Boolean = this.canBeAutoCorrected,
autoFix: () -> Unit) {
warn(configRules, emit, canBeAutoCorrected, freeText, offset, node)
if (canBeAutoCorrected) {
fix(configRules, isFixMode, node, autoFix)
fun warnAndFix(
configRules: List<RulesConfig>,
emit: DiktatErrorEmitter,
isFixMode: Boolean,
freeText: String,
offset: Int,
node: ASTNode,
autoFix: () -> Unit,
) {
require(canBeAutoCorrected) {
"warnAndFix is called, but canBeAutoCorrected is false"
}
doWarn(configRules, emit, freeText, offset, node, true)
fix(configRules, isFixMode, node, autoFix)
}

/**
* @param configs list of [RulesConfig]
* @param emit function that will be called on warning
* @param autoCorrected whether this warning can be autocorrected
* @param freeText text that will be added to the warning message
* @param offset offset from the beginning of the file
* @param node the [ASTNode] on which the warning was triggered
*/
@Suppress("LongParameterList", "TOO_MANY_PARAMETERS")
fun warn(configs: List<RulesConfig>,
emit: EmitType,
autoCorrected: Boolean,
freeText: String,
offset: Int,
node: ASTNode) {
fun warn(
configs: List<RulesConfig>,
emit: DiktatErrorEmitter,
freeText: String,
offset: Int,
node: ASTNode,
) {
doWarn(configs, emit, freeText, offset, node, false)
}

@Suppress("LongParameterList", "TOO_MANY_PARAMETERS")
private fun doWarn(
configs: List<RulesConfig>,
emit: DiktatErrorEmitter,
freeText: String,
offset: Int,
node: ASTNode,
canBeAutoCorrected: Boolean,
) {
if (isRuleFromActiveChapter(configs) && configs.isRuleEnabled(this) && !node.isSuppressed(name, this, configs)) {
val trimmedFreeText = freeText
.lines()
.run { if (size > 1) "${first()}..." else first() }
emit(offset,
"${this.warnText()} $trimmedFreeText",
autoCorrected
emit(
offset,
errorMessage = "${this.warnText()} $trimmedFreeText",
canBeAutoCorrected = canBeAutoCorrected,
)
}
}
Expand All @@ -264,7 +299,7 @@ enum class Warnings(

companion object {
val names by lazy {
values().map { it.name }
entries.map { it.name }
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,8 @@ class FileNaming(configRules: List<RulesConfig>) : DiktatRule(
private fun checkFileNaming(node: ASTNode) {
val (name, extension) = getFileParts(filePath)
if (!name.isPascalCase() || !validExtensions.contains(extension)) {
FILE_NAME_INCORRECT.warnAndFix(configRules, emitWarn, isFixMode, "$name$extension", 0, node) {
// FixMe: we can add an autocorrect here in future, but is there any purpose to change file or class name?
}
// FixMe: we can add an autocorrect here in future, but is there any purpose to change file or class name?
FILE_NAME_INCORRECT.warn(configRules, emitWarn, "$name$extension", 0, node)
}
}

Expand All @@ -59,9 +58,8 @@ class FileNaming(configRules: List<RulesConfig>) : DiktatRule(
if (classes.size == 1) {
val className = classes[0].getFirstChildWithType(IDENTIFIER)!!.text
if (className != fileNameWithoutSuffix) {
FILE_NAME_MATCH_CLASS.warnAndFix(configRules, emitWarn, isFixMode, "$fileNameWithoutSuffix$fileNameSuffix vs $className", 0, fileLevelNode) {
// FixMe: we can add an autocorrect here in future, but is there any purpose to change file name?
}
// FixMe: we can add an autocorrect here in future, but is there any purpose to change file name?
FILE_NAME_MATCH_CLASS.warn(configRules, emitWarn, "$fileNameWithoutSuffix$fileNameSuffix vs $className", 0, fileLevelNode)
}
} else {
// FixMe: need to check that if there are several classes - at least one of them should match
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
if (identifierText.startsWith('`') && identifierText.endsWith('`')) {
val isTestFun = node.elementType == KtNodeTypes.FUN && node.hasTestAnnotation()
if (!isTestFun) {
BACKTICKS_PROHIBITED.warn(configRules, emitWarn, isFixMode, identifierText, identifier.startOffset, identifier)
BACKTICKS_PROHIBITED.warn(configRules, emitWarn, identifierText, identifier.startOffset, identifier)
}
return true
}
Expand All @@ -146,6 +146,7 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
@Suppress(
"SAY_NO_TO_VAR",
"TOO_LONG_FUNCTION",
"LongMethod",
"ComplexMethod",
"UnsafeCallOnNullableType",
)
Expand All @@ -158,13 +159,13 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
val isNonPrivatePrimaryConstructorParameter = (node.psi as? KtParameter)?.run {
hasValOrVar() && getParentOfType<KtPrimaryConstructor>(true)?.valueParameters?.contains(this) == true && !isPrivate()
} ?: false
val canBeAutoCorrected = !(isPublicOrNonLocalProperty || isNonPrivatePrimaryConstructorParameter)
val shouldBeAutoCorrected = !(isPublicOrNonLocalProperty || isNonPrivatePrimaryConstructorParameter)
namesOfVariables
.forEach { variableName ->
// variable should not contain only one letter in it's name. This is a bad example: b512
// but no need to raise a warning here if length of a variable. In this case we will raise IDENTIFIER_LENGTH
if (variableName.text.containsOneLetterOrZero() && variableName.text.length > 1) {
VARIABLE_NAME_INCORRECT.warn(configRules, emitWarn, false, variableName.text, variableName.startOffset, node)
VARIABLE_NAME_INCORRECT.warn(configRules, emitWarn, variableName.text, variableName.startOffset, node)
}
// check if identifier of a property has a confusing name
if (confusingIdentifierNames.contains(variableName.text) && !isValidCatchIdentifier(variableName) &&
Expand All @@ -176,13 +177,29 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
// it should be in UPPER_CASE, no need to raise this warning if it is one-letter variable
if (node.isConstant()) {
if (!variableName.text.isUpperSnakeCase() && variableName.text.length > 1) {
CONSTANT_UPPERCASE.warnAndFix(configRules, emitWarn, isFixMode, variableName.text, variableName.startOffset, node, canBeAutoCorrected) {
CONSTANT_UPPERCASE.warnOnlyOrWarnAndFix(
configRules = configRules,
emit = emitWarn,
freeText = variableName.text,
offset = variableName.startOffset,
node = node,
shouldBeAutoCorrected = shouldBeAutoCorrected,
isFixMode = isFixMode,
) {
(variableName as LeafPsiElement).rawReplaceWithText(variableName.text.toDeterministic { toUpperSnakeCase() })
}
}
} else if (variableName.text != "_" && !variableName.text.isLowerCamelCase()) {
// variable name should be in camel case. The only exception is a list of industry standard variables like i, j, k.
VARIABLE_NAME_INCORRECT_FORMAT.warnAndFix(configRules, emitWarn, isFixMode, variableName.text, variableName.startOffset, node, canBeAutoCorrected) {
VARIABLE_NAME_INCORRECT_FORMAT.warnOnlyOrWarnAndFix(
configRules = configRules,
emit = emitWarn,
freeText = variableName.text,
offset = variableName.startOffset,
node = node,
shouldBeAutoCorrected = shouldBeAutoCorrected,
isFixMode = isFixMode,
) {
// FixMe: cover fixes with tests
val correctVariableName = variableName.text.toDeterministic { toLowerCamelCase() }
variableName
Expand Down Expand Up @@ -240,7 +257,7 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
else -> ""

}
CONFUSING_IDENTIFIER_NAMING.warn(configRules, emitWarn, false, warnText, variableName.startOffset, variableName)
CONFUSING_IDENTIFIER_NAMING.warn(configRules, emitWarn, warnText, variableName.startOffset, variableName)
}

/**
Expand Down Expand Up @@ -274,9 +291,8 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
private fun checkClassNamings(node: ASTNode): List<ASTNode> {
val genericType: ASTNode? = node.getTypeParameterList()
if (genericType != null && !validGenericTypeName(genericType)) {
GENERIC_NAME.warnAndFix(configRules, emitWarn, isFixMode, genericType.text, genericType.startOffset, genericType) {
// FixMe: should fix generic name here
}
// FixMe: should fix generic name here
GENERIC_NAME.warn(configRules, emitWarn, genericType.text, genericType.startOffset, genericType)
}

val className: ASTNode = node.getIdentifierName() ?: return emptyList()
Expand Down Expand Up @@ -397,9 +413,8 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
if (functionReturnType != null && functionReturnType == PrimitiveType.BOOLEAN.typeName.asString()) {
@Suppress("COLLAPSE_IF_STATEMENTS")
if (allMethodPrefixes.none { functionName.text.startsWith(it) }) {
FUNCTION_BOOLEAN_PREFIX.warnAndFix(configRules, emitWarn, isFixMode, functionName.text, functionName.startOffset, functionName) {
// FixMe: add agressive autofix for this
}
// FixMe: add agressive autofix for this
FUNCTION_BOOLEAN_PREFIX.warn(configRules, emitWarn, functionName.text, functionName.startOffset, functionName)
}
}
}
Expand Down Expand Up @@ -441,7 +456,7 @@ class IdentifierNaming(configRules: List<RulesConfig>) : DiktatRule(
if (it.text != "_" && !it.isTextLengthInRange(MIN_IDENTIFIER_LENGTH..MAX_IDENTIFIER_LENGTH) &&
!isValidOneCharVariable && !isValidCatchIdentifier(it)
) {
IDENTIFIER_LENGTH.warn(configRules, emitWarn, isFixMode, it.text, it.startOffset, it)
IDENTIFIER_LENGTH.warn(configRules, emitWarn, it.text, it.startOffset, it)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ class PackageNaming(configRules: List<RulesConfig>) : DiktatRule(
// all words should contain only ASCII letters or digits
wordsInPackageName
.filter { word -> !areCorrectSymbolsUsed(word.text) }
.forEach { PACKAGE_NAME_INCORRECT_SYMBOLS.warn(configRules, emitWarn, isFixMode, it.text, it.startOffset, it) }
.forEach { PACKAGE_NAME_INCORRECT_SYMBOLS.warn(configRules, emitWarn, it.text, it.startOffset, it) }

// all words should contain only ASCII letters or digits
wordsInPackageName.forEach { correctPackageWordSeparatorsUsed(it) }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package com.saveourtool.diktat.ruleset.rules.chapter2.comments

import com.saveourtool.diktat.common.config.rules.RulesConfig
import com.saveourtool.diktat.ruleset.constants.ListOfPairs
import com.saveourtool.diktat.ruleset.constants.Warnings.COMMENTED_OUT_CODE
import com.saveourtool.diktat.ruleset.rules.DiktatRule
import com.saveourtool.diktat.ruleset.utils.findAllDescendantsWithSpecificType
Expand All @@ -20,6 +19,8 @@ import org.jetbrains.kotlin.psi.KtPsiFactory
import org.jetbrains.kotlin.psi.stubs.elements.KtFileElementType
import org.jetbrains.kotlin.resolve.ImportPath

private typealias ListOfPairs = MutableList<Pair<ASTNode, String>>

/**
* This rule performs checks if there is any commented code.
* No commented out code is allowed, including imports.
Expand Down Expand Up @@ -105,7 +106,6 @@ class CommentsRule(configRules: List<RulesConfig>) : DiktatRule(
COMMENTED_OUT_CODE.warn(
configRules,
emitWarn,
isFixMode,
parsedNode.text.substringBefore("\n").trim(),
offset,
invalidNode
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class HeaderCommentRule(configRules: List<RulesConfig>) : DiktatRule(
val numDeclaredClassesAndObjects = node.getAllChildrenWithType(KtNodeTypes.CLASS).size +
node.getAllChildrenWithType(KtNodeTypes.OBJECT_DECLARATION).size
if (numDeclaredClassesAndObjects != 1) {
HEADER_MISSING_IN_NON_SINGLE_CLASS_FILE.warn(configRules, emitWarn, isFixMode,
HEADER_MISSING_IN_NON_SINGLE_CLASS_FILE.warn(configRules, emitWarn,
"there are $numDeclaredClassesAndObjects declared classes and/or objects", node.startOffset, node)
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
.mapNotNull { it.nameIdentifier?.text }
propertiesInKdoc
.filterNot { it.getSubjectName() == null || it.getSubjectName() in propertyNames }
.forEach { KDOC_EXTRA_PROPERTY.warn(configRules, emitWarn, isFixMode, it.text, it.node.startOffset, node) }
.forEach { KDOC_EXTRA_PROPERTY.warn(configRules, emitWarn, it.text, it.node.startOffset, node) }
}

@Suppress("UnsafeCallOnNullableType", "ComplexMethod")
Expand Down Expand Up @@ -236,7 +236,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
// 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
val isFixable = propertyInLocalKdoc == null
KDOC_NO_CONSTRUCTOR_PROPERTY.warnAndFix(configRules, emitWarn, isFixMode, prevComment.text, prevComment.startOffset, node, isFixable) {
KDOC_NO_CONSTRUCTOR_PROPERTY.warnOnlyOrWarnAndFix(configRules, emitWarn, prevComment.text, prevComment.startOffset, node, shouldBeAutoCorrected = isFixable, isFixMode) {
propertyInClassKdoc?.let {
// local docs should be appended to docs in class
appendKdocTagContent(propertyInClassKdoc, "\n$kdocText")
Expand Down Expand Up @@ -274,7 +274,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
val traversedNodes: MutableSet<String?> = mutableSetOf()
propertiesAndParams.forEach { property ->
if (!traversedNodes.add(property.getSubjectName())) {
KDOC_DUPLICATE_PROPERTY.warn(configRules, emitWarn, isFixMode, property.text, property.node.startOffset, kdoc)
KDOC_DUPLICATE_PROPERTY.warn(configRules, emitWarn, property.text, property.node.startOffset, kdoc)
}
}
}
Expand Down Expand Up @@ -326,7 +326,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}
propertyInClassKdoc?.let {
// if property is documented as `@property`, then we suggest to move docs to the declaration inside the class body
KDOC_NO_CLASS_BODY_PROPERTIES_IN_HEADER.warn(configRules, emitWarn, isFixMode, classElement.text, classElement.startOffset, classElement)
KDOC_NO_CLASS_BODY_PROPERTIES_IN_HEADER.warn(configRules, emitWarn, classElement.text, classElement.startOffset, classElement)
return
}
}
Expand Down Expand Up @@ -359,7 +359,7 @@ class KdocComments(configRules: List<RulesConfig>) : DiktatRule(
}

if (isModifierAccessibleOutsideOrActual && kdoc == null && !isTopLevelFunctionStandard(node)) {
warning.warn(configRules, emitWarn, isFixMode, name!!.text, node.startOffset, node)
warning.warn(configRules, emitWarn, name!!.text, node.startOffset, node)
}
}

Expand Down
Loading

0 comments on commit aa26135

Please sign in to comment.