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

Refactored warn and warnAndFix in Warnings #1751

Merged
merged 8 commits into from
Sep 26, 2023
Merged
Show file tree
Hide file tree
Changes from 7 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
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 }
nulls marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
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