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

Implement MultilineRawStringIndentation #5058

Merged
merged 7 commits into from Aug 1, 2022
3 changes: 3 additions & 0 deletions detekt-core/src/main/resources/default-detekt-config.yml
Expand Up @@ -602,6 +602,9 @@ style:
active: true
MultilineLambdaItParameter:
active: false
MultilineRawStringIndentation:
active: false
indentSize: 4
NestedClassesVisibility:
active: true
NewLineAtEndOfFile:
Expand Down
@@ -0,0 +1,186 @@
package io.gitlab.arturbosch.detekt.rules.style

import io.github.detekt.psi.getLineAndColumnInPsiFile
import io.github.detekt.psi.toFilePath
import io.gitlab.arturbosch.detekt.api.CodeSmell
import io.gitlab.arturbosch.detekt.api.Config
import io.gitlab.arturbosch.detekt.api.Debt
import io.gitlab.arturbosch.detekt.api.Entity
import io.gitlab.arturbosch.detekt.api.Issue
import io.gitlab.arturbosch.detekt.api.Location
import io.gitlab.arturbosch.detekt.api.Rule
import io.gitlab.arturbosch.detekt.api.Severity
import io.gitlab.arturbosch.detekt.api.SourceLocation
import io.gitlab.arturbosch.detekt.api.TextLocation
import io.gitlab.arturbosch.detekt.api.config
import io.gitlab.arturbosch.detekt.api.internal.Configuration
import org.jetbrains.kotlin.com.intellij.psi.PsiFile
import org.jetbrains.kotlin.psi.KtElement
import org.jetbrains.kotlin.psi.KtStringTemplateExpression

/**
* This rule ensures that raw strings have a consistent indentation.
*
* The content of a multi line raw string should have the same indentation as the enclosing expression plus the
* configured indentSize. The closing triple-quotes (`"""`) must have the same indentation as the enclosing expression.
*
* <noncompliant>
* val a = """
* Hello World!
* How are you?
* """.trimMargin()
*
* val a = """
* Hello World!
* How are you?
* """.trimMargin()
* </noncompliant>
*
* <compliant>
* val a = """
* Hello World!
* How are you?
* """.trimMargin()
*
* val a = """
* Hello World!
* How are you?
* """.trimMargin()
* </compliant>
*/
class MultilineRawStringIndentation(config: Config) : Rule(config) {
override val issue = Issue(
javaClass.simpleName,
Severity.Style,
"The indentation of the raw String should be consistent",
Debt.FIVE_MINS
)

@Configuration("indentation size")
marschwar marked this conversation as resolved.
Show resolved Hide resolved
private val indentSize by config(4)

@Suppress("ReturnCount")
override fun visitStringTemplateExpression(expression: KtStringTemplateExpression) {
super.visitStringTemplateExpression(expression)

val text = expression.text
val lineCount = text.lines().count()
if (lineCount <= 1) return
if (!expression.isTrimmed()) return
if (!text.matches(rawStringRegex)) return

val lineAndColumn = getLineAndColumnInPsiFile(expression.containingFile, expression.textRange) ?: return

expression.checkIndentation(
baseIndent = lineAndColumn.lineContent?.countIndent() ?: return,
firstLineNumber = lineAndColumn.line,
lastLineNumber = lineAndColumn.line + lineCount - 1
)
}

private fun KtStringTemplateExpression.checkIndentation(
baseIndent: Int,
firstLineNumber: Int,
lastLineNumber: Int,
) {
checkContent(desiredIndent = baseIndent + indentSize, (firstLineNumber + 1)..(lastLineNumber - 1))
checkClosing(baseIndent, lastLineNumber)
}

private fun KtStringTemplateExpression.checkContent(
desiredIndent: Int,
lineNumberRange: IntRange,
) {
data class LineInformation(val lineNumber: Int, val line: String, val currentIndent: Int)

val indentation = lineNumberRange
.map { lineNumber ->
val line = containingFile.getLine(lineNumber)
LineInformation(lineNumber, line, line.countIndent())
}

if (indentation.isNotEmpty()) {
indentation
.filter { (_, line, currentIndent) -> line.isNotEmpty() && currentIndent < desiredIndent }
.onEach { (lineNumber, line, currentIndent) ->
val location = containingFile.getLocation(
SourceLocation(lineNumber, if (line.isBlank()) 1 else currentIndent + 1),
SourceLocation(lineNumber, line.length + 1)
)

report(this, location, message(desiredIndent, currentIndent))
Comment on lines +105 to +111
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually want to report one violation for each line that is incorrectly indented? Consider a code block like in detekt tests. If you get the indentation wrong, it is most likely wrong for entire block. Would it not be enough to mark the multi line string as a violation as a whole? That most likely would make the rule much easier to implement.

Specifically I do not like the fact that

val a = """
Hello world!
How are you?
""".trimIndent()

produces 2 violations, while

val a = """
        Hello world!
        How are you?
""".trimIndent()

produces one

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes... this was a difficult decission. I was thinking about a long raw string with only one/few lines with the wrong identation. In that case I wanted to point out exactly which line was the wrong one. The problem is that if all of them are wrong I end up raising all of them

The second case was easy because if it happens I know that all the lines are wrong so I just raise the error once.


I feel that this is a tradeoff:

  • If I raise only one the cases where all of them are wrong will be easier,
  • But if only some of them are wrong this approuch would be better.

🤷 I'm fine with both of them to be honest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. I think the code could have been easier to write and to maintain if it were just one violation per multi line string, but I see your point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm merging it like this. We can always revisit this once we get the users feedback.

}
.ifEmpty {
if (indentation.none { (_, _, currentIndent) -> currentIndent == desiredIndent }) {
val location = containingFile.getLocation(
SourceLocation(lineNumberRange.first, desiredIndent + 1),
SourceLocation(lineNumberRange.last, indentation.last().line.length + 1),
)

report(
this,
location,
message(desiredIndent, indentation.minOf { (_, _, indent) -> indent }),
)
}
}
}
}

private fun KtStringTemplateExpression.checkClosing(
desiredIndent: Int,
lineNumber: Int,
) {
val currentIndent = containingFile.getLine(lineNumber).countIndent()
if (currentIndent != desiredIndent) {
val location = if (currentIndent < desiredIndent) {
containingFile.getLocation(
SourceLocation(lineNumber, currentIndent + 1),
SourceLocation(lineNumber, currentIndent + "\"\"\"".length + 1),
)
} else {
containingFile.getLocation(
SourceLocation(lineNumber, desiredIndent + 1),
SourceLocation(lineNumber, currentIndent + 1),
)
}

report(this, location, message(desiredIndent, currentIndent))
}
}
}

private fun Rule.report(element: KtElement, location: Location, message: String) {
report(CodeSmell(issue, Entity.from(element, location), message))
}

private fun message(desiredIntent: Int, currentIndent: Int): String {
return "The indentation should be $desiredIntent but it is $currentIndent."
}

private val rawStringRegex = "\"{3}\n.*\n *\"{3}".toRegex(RegexOption.DOT_MATCHES_ALL)

private fun String.countIndent() = this.takeWhile { it == ' ' }.count()

private fun PsiFile.getLine(line: Int): String {
return text.lineSequence().drop(line - 1).first()
}

private fun PsiFile.getLocation(start: SourceLocation, end: SourceLocation): Location {
val lines = this.text.lines()
var startOffset = 0
for (i in 1 until start.line) {
startOffset += lines[i - 1].length + 1
}
var endOffset = startOffset
for (i in start.line until end.line) {
endOffset += lines[i - 1].length + 1
}
this.text.lines()
return Location(
start,
end,
TextLocation(startOffset + start.column - 1, endOffset + end.column - 1),
toFilePath()
)
}
Expand Up @@ -96,6 +96,7 @@ class StyleGuideProvider : DefaultRuleSetProvider {
RedundantHigherOrderMapUsage(config),
UseIfEmptyOrIfBlank(config),
MultilineLambdaItParameter(config),
MultilineRawStringIndentation(config),
UseIsNullOrEmpty(config),
UseOrEmpty(config),
UseAnyOrNoneInsteadOfFind(config),
Expand Down
Expand Up @@ -50,14 +50,7 @@ class TrimMultilineRawString(val config: Config) : Rule(config) {

if (expression.text.lines().count() <= 1) return

val nextCall = expression.getQualifiedExpressionForSelectorOrThis()
.getQualifiedExpressionForReceiver()
?.selectorExpression
?.asKtCallExpression()
?.calleeExpression
?.text

if (nextCall !in trimFunctions) {
if (!expression.isTrimmed()) {
report(
CodeSmell(
issue,
Expand All @@ -69,6 +62,17 @@ class TrimMultilineRawString(val config: Config) : Rule(config) {
}
}

private fun KtExpression.asKtCallExpression(): KtCallExpression? = this as? KtCallExpression
fun KtStringTemplateExpression.isTrimmed(): Boolean {
fun KtExpression.asKtCallExpression(): KtCallExpression? = this as? KtCallExpression

val nextCall = getQualifiedExpressionForSelectorOrThis()
.getQualifiedExpressionForReceiver()
?.selectorExpression
?.asKtCallExpression()
?.calleeExpression
?.text

return nextCall in trimFunctions
}

private val trimFunctions = listOf("trimIndent", "trimMargin")