From 5164ad9c90a851b99c8b76dcd6fb33fcc40264b7 Mon Sep 17 00:00:00 2001 From: David Torosyan Date: Fri, 7 Jan 2022 10:50:32 -0800 Subject: [PATCH] Consolidate emitQualifiedExpression functions Summary: ## Context Before, we had two paths for `emitQualifiedExpression`: * `emitQualifiedExpressionOnePerLine` - breaks expressions with `UNIFIED` * `emitQualifiedExpressionSeveralInOneLine` - breaks grouped expressions with `INDEPENDENT`, and then the remaining with `UNIFIED` This had a few problems: 1. The `OnePerLine` was really a misnomer, since it didn't actually put them one-per-line if it all fit on one line 2. It was hard to modify the logic, since it's not obvious how to keep changes in sync I noticed that they could be combined if `OnePerLine` was treated as a special case of the `SeveralInOneLine` - when there are no grouped expressions. ## New problems Combining these functions has two issues: 1. Now we're keeping track of `textLength`, which further complicates the larger function 2. We have this `simple` boolean which helps deal with the differences between the two cases, mostly having to do with indentation. Both of these had to be included to avoid causing any behavior changes, but I want to deal with them by removing them in the next diff, since they add complexity and (I think) little value. ## Next steps Now that the two code paths are consolidated, I can start changing behavior as described in T80900127! Reviewed By: strulovich, hick209 Differential Revision: D33399531 fbshipit-source-id: 8f86d1cd0c9aa89cae04afbae8b3f10fde6edfef --- .../ktfmt/format/KotlinInputAstVisitor.kt | 144 +++++++++--------- 1 file changed, 68 insertions(+), 76 deletions(-) diff --git a/core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt b/core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt index 79a426a3..b7394bf0 100644 --- a/core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt +++ b/core/src/main/java/com/facebook/ktfmt/format/KotlinInputAstVisitor.kt @@ -479,24 +479,28 @@ class KotlinInputAstVisitor( return } - emitQualifiedExpression(expression) + emitQualifiedExpression(breakIntoChunks(expression)) } - private fun emitQualifiedExpression(expression: KtExpression) { + /** + * Decomposes a qualified expression into chunks. + * + * So this expression: + * ``` + * rainbow.red.orange.shine().yellow + * ``` + * + * Becomes: + * ``` + * chunks: [ + * chunk(expressions=[rainbow, red, orange, shine()], shouldKeepOnSameLine=true), + * chunk(expressions=[yellow], shouldKeepOnSameLine=false) + * ] + * ``` + */ + private fun breakIntoChunks(expression: KtExpression): List { val parts = breakIntoParts(expression) - val chunks = breakIntoChunks(parts) - val hasPrefixes = chunks.first().shouldKeepOnSameLine - val hasTrailingLambda = parts.last().isLambda() - - if (hasPrefixes || hasTrailingLambda) { - emitQualifiedExpressionSeveralInOneLine(chunks) - } else { - emitQualifiedExpressionOnePerLine(parts) - } - } - - private fun breakIntoChunks(parts: List): List { val prefixes = mutableSetOf() // Check if the dot chain has a prefix that looks like a type name, so we can @@ -632,59 +636,6 @@ class KotlinInputAstVisitor( return simpleNames } - /** - * Output a "regular" chain of dereferences, possibly in builder-style. Break before every dot. - * - * Example: - * ``` - * fieldName - * .field1 - * .field2 - * .method1() - * .method2() - * .apply { - * // ... - * } - * ``` - */ - private fun emitQualifiedExpressionOnePerLine(items: Collection) { - var needDot = false - val trailingDereferences = items.size > 1 - builder.block(expressionBreakIndent) { - // don't break after the first element if it is every small, unless the - // chain starts with another expression - var length = 0 - for (item in items) { - val extractCallExpression = extractCallExpression(item) - - if (needDot) { - if (length > options.continuationIndent || - !extractCallExpression?.lambdaArguments.isNullOrEmpty()) { - builder.breakOp(Doc.FillMode.UNIFIED, "", ZERO) - } - builder.token((item as KtQualifiedExpression).operationSign.value) - length++ - } - emitSelectorUpToParenthesis(item) - - // Emit parenthesis and lambda. - extractCallExpression?.apply { - visitCallElement( - null, - typeArgumentList, - valueArgumentList, - lambdaArguments, - argumentsIndent = - if (trailingDereferences || needDot) expressionBreakIndent else ZERO, - lambdaIndent = - if (trailingDereferences || needDot) ZERO else expressionBreakNegativeIndent) - } - length += item.text.length - needDot = true - } - } - } - /** * Output a chain of dereferences, some of which should be grouped together. * @@ -700,20 +651,52 @@ class KotlinInputAstVisitor( * .method1() * ``` */ - private fun emitQualifiedExpressionSeveralInOneLine( + private fun emitQualifiedExpression( chunks: List, ) { - // is the last expression a lambda? + // Keeps track of how much text we've emitted so far, and is used to avoid line breaks that are + // shorter than the indentation length. For example: + // ``` + // user.field1 + // .field2 + // ... + // .field9 + // ``` + // Since `user` is less than or equal to 4 characters (the indentation length here), `field1` is + // kept on the same line. + var textLength = 0 + + // is the last expression a lambda? e.g.`foo.bar.apply { ... }` val hasTrailingLambda = chunks.last().expressions.last().isLambda() - // are there method invocations or field accesses after the prefix? + + // Do we have expressions that should be split up following expressions that should be kept on + // the same line? + // e.g + // ``` + // rainbow.red.orange.shine() // <-- chunk with shouldKeepOnSameLine = true + // .yellow // <--| + // .green // <--| + // .blue // <--|-- chunk with shouldKeepOnSameLine = false + // ``` val trailingDereferences = chunks.first().shouldKeepOnSameLine && !chunks.last().shouldKeepOnSameLine + // Do we have any expressions that should be kept on the same line? + // True example: `rainbow.red.orange.shine()` + // False example: `rainbow.red.orange` + // Note that in this example the function invocation is what causes the chunk to be a "prefix", + // but there are other ways defined in [breakIntoChunks]. + val hasPrefixes = chunks.first().shouldKeepOnSameLine + + // simple chain with no meaningful groups or trailing lambda, e.g `rainbow.red.orange` + val simple = !hasPrefixes && !hasTrailingLambda + // how to indent function arguments if the line is not broken - val argsIndentElse = if (trailingDereferences) expressionBreakIndent else ZERO + val argsIndentElse = if (trailingDereferences || simple) expressionBreakIndent else ZERO // how to indent lambdas if the line is not broken val lambdaIndentElse = - if (trailingDereferences && !hasTrailingLambda) ZERO else expressionBreakNegativeIndent + if ((trailingDereferences && !hasTrailingLambda) || simple) ZERO + else expressionBreakNegativeIndent builder.block(expressionBreakIndent) { // trailing lambdas get their own block, so wrap everything before it in a block @@ -737,10 +720,17 @@ class KotlinInputAstVisitor( // for everything after the very first element, emit a break and a dot if (chunkIndex > 0 || itemIndex > 0) { - val fillMode = - if (chunk.shouldKeepOnSameLine) Doc.FillMode.INDEPENDENT else Doc.FillMode.UNIFIED - builder.breakOp(fillMode, "", ZERO, Optional.of(nameTag)) - builder.token((item as KtQualifiedExpression).operationSign.value) + + // break if there's a lambda, or the line is long enough + if (!simple || textLength > options.continuationIndent || item.isLambda()) { + val fillMode = + if (chunk.shouldKeepOnSameLine) Doc.FillMode.INDEPENDENT else Doc.FillMode.UNIFIED + builder.breakOp(fillMode, "", ZERO, Optional.of(nameTag)) + } + + val operator = (item as KtQualifiedExpression).operationSign.value + builder.token(operator) + textLength += operator.length } // emit the reference or method name @@ -770,6 +760,8 @@ class KotlinInputAstVisitor( argumentsIndent = Indent.If.make(nameTag, expressionBreakIndent, argsIndentElse), lambdaIndent = Indent.If.make(nameTag, ZERO, lambdaIndentElse)) } + + textLength += item.text.length } } }