-
Notifications
You must be signed in to change notification settings - Fork 316
Don't leak an expression body's indent into later declarations #2365
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,8 +119,34 @@ private constructor(internal val formatParts: List<String>, internal val args: L | |
| /** | ||
| * Returns a copy of the code block without leading and trailing no-arg placeholders (`⇥`, `⇤`, | ||
| * `«`, `»`). | ||
| * | ||
| * With [keepBalanced], placeholders are taken back from either end until nothing left in the | ||
| * block is unmatched, which would otherwise leak the indent or the statement into whatever is | ||
| * emitted next. A body ending inside `withIndent` loses the `⇤` that closed it, e.g.: | ||
| * ``` | ||
| * ["return ", "%S", "⇥", ".trim()", "⇤"] | ||
| * trim() -> ["return ", "%S", "⇥", ".trim()"] | ||
| * trim(keepBalanced = true) -> ["return ", "%S", "⇥", ".trim()", "⇤"] | ||
| * ``` | ||
| * | ||
| * Both ends move when both are unmatched, e.g.: | ||
| * ``` | ||
| * ["⇥", "body1", "⇤", "⇥", "body2", "⇤"] | ||
| * trim() -> ["body1", "⇤", "⇥", "body2"] | ||
| * trim(keepBalanced = true) -> ["⇥", "body1", "⇤", "⇥", "body2", "⇤"] | ||
| * ``` | ||
| * | ||
| * A block that was already unbalanced before trimming is left to the plain behavior, since taking | ||
| * one end back cannot balance it and moves the following declaration further out, e.g.: | ||
| * ``` | ||
| * ["⇥", "return ", "%S", "⇤", "⇥", ".trim()"] | ||
| * trim() -> ["return ", "%S", "⇤", "⇥", ".trim()"] | ||
| * trim(keepBalanced = true) -> ["return ", "%S", "⇤", "⇥", ".trim()"] | ||
| * ``` | ||
| * | ||
| * [trimTrailingNewLine] relies on the default, which always drops both runs. | ||
| */ | ||
| internal fun trim(): CodeBlock { | ||
| internal fun trim(keepBalanced: Boolean = false): CodeBlock { | ||
| var start = 0 | ||
| var end = formatParts.size | ||
| while (start < end && formatParts[start] in NO_ARG_PLACEHOLDERS) { | ||
|
|
@@ -129,6 +155,79 @@ private constructor(internal val formatParts: List<String>, internal val args: L | |
| while (start < end && formatParts[end - 1] in NO_ARG_PLACEHOLDERS) { | ||
| end-- | ||
| } | ||
| if (keepBalanced && (start > 0 || end < formatParts.size)) { | ||
| // Both stripped runs are placeholders by definition, so they are the first `start` and the | ||
| // last `formatParts.size - end` entries of this list, and the kept range is what sits | ||
| // between them. Everything below walks placeholders rather than format parts, which keeps | ||
| // the balance check to a single pass over the parts. | ||
| val placeholders = CharArray(formatParts.size) | ||
| var placeholderCount = 0 | ||
| for (formatPart in formatParts) { | ||
| if (formatPart.length == 1 && formatPart[0].isSingleCharNoArgPlaceholder) { | ||
| placeholders[placeholderCount++] = formatPart[0] | ||
| } | ||
| } | ||
| val keptEnd = placeholderCount - (formatParts.size - end) | ||
| if (isBalanced(placeholders, placeholderCount)) { | ||
| // Track the lowest point reached, not just the running total. Two unrelated halves can | ||
| // cancel out to a total of zero and still emit an unindent ahead of its indent. | ||
| var indentLow = 0 | ||
| var indentTotal = 0 | ||
| var statementLow = 0 | ||
| var statementTotal = 0 | ||
| for (i in start..<keptEnd) { | ||
| when (placeholders[i]) { | ||
| '⇥' -> indentTotal++ | ||
| '⇤' -> indentTotal-- | ||
| '«' -> statementTotal++ | ||
| '»' -> statementTotal-- | ||
| } | ||
| if (indentTotal < indentLow) indentLow = indentTotal | ||
| if (statementTotal < statementLow) statementLow = statementTotal | ||
| } | ||
| var first = start | ||
| while (first > 0 && (indentLow < 0 || statementLow < 0)) { | ||
| when (placeholders[first - 1]) { | ||
| '⇥' -> { | ||
| indentLow++ | ||
| indentTotal++ | ||
| } | ||
| '⇤' -> { | ||
| indentLow-- | ||
| indentTotal-- | ||
| } | ||
| '«' -> { | ||
| statementLow++ | ||
| statementTotal++ | ||
| } | ||
| '»' -> { | ||
| statementLow-- | ||
| statementTotal-- | ||
| } | ||
| } | ||
| first-- | ||
| if (indentLow >= 0 && statementLow >= 0) { | ||
| start = first | ||
| break | ||
| } | ||
| } | ||
| var last = keptEnd | ||
| while (last < placeholderCount && (indentTotal > 0 || statementTotal > 0)) { | ||
| when (placeholders[last]) { | ||
| '⇥' -> indentTotal++ | ||
| '⇤' -> indentTotal-- | ||
| '«' -> statementTotal++ | ||
| '»' -> statementTotal-- | ||
| } | ||
| if (indentTotal < 0 || statementTotal < 0) break | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a test case where this statement will evaluate to true? All tests still pass if I comment it out. |
||
| last++ | ||
| if (indentTotal == 0 && statementTotal == 0) { | ||
| end += last - keptEnd | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } | ||
| return when { | ||
| start > 0 || end < formatParts.size -> CodeBlock(formatParts.subList(start, end), args) | ||
| else -> this | ||
|
|
@@ -148,6 +247,21 @@ private constructor(internal val formatParts: List<String>, internal val args: L | |
|
|
||
| internal fun hasStatements() = formatParts.any { "«" in it } | ||
|
|
||
| private fun isBalanced(placeholders: CharArray, count: Int): Boolean { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's move this helper inside |
||
| var indent = 0 | ||
| var statement = 0 | ||
| for (i in 0..<count) { | ||
| when (placeholders[i]) { | ||
| '⇥' -> indent++ | ||
| '⇤' -> indent-- | ||
| '«' -> statement++ | ||
| '»' -> statement-- | ||
| } | ||
| if (indent < 0 || statement < 0) return false | ||
| } | ||
| return indent == 0 && statement == 0 | ||
| } | ||
|
|
||
| internal fun hasUnmatchedClosingStatement(): Boolean { | ||
| var openCount = 0 | ||
| for (formatPart in formatParts) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -494,6 +494,13 @@ class CodeBlockTest { | |
| assertThat(CodeBlock.of("«»⇥⇤").trim()).isEqualTo(CodeBlock.of("")) | ||
| } | ||
|
|
||
| @Test | ||
| fun trimKeepsAStatementAndItsIndentTogether() { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: maybe |
||
| val codeBlock = CodeBlock.of("«⇥return %S⇤».trim()", "taco") | ||
|
|
||
| assertThat(codeBlock.trim(keepBalanced = true)).isEqualTo(codeBlock) | ||
| } | ||
|
|
||
| @Test | ||
| fun replaceSimple() { | ||
| assertThat(CodeBlock.of("%%⇥%%").replaceAll("%%", "")).isEqualTo(CodeBlock.of("⇥")) | ||
|
|
@@ -631,6 +638,23 @@ class CodeBlockTest { | |
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun ensureEndsWithNewLineKeepsArgsWhenBlockEndsInIndent() { | ||
| val codeBlock = | ||
| CodeBlock.builder().add("%S", "taco\n").indent().add("\nmore").unindent().build() | ||
|
|
||
| assertThat(codeBlock.ensureEndsWithNewLine().toString()) | ||
| .isEqualTo( | ||
| """ | ||
| |""${'"'} | ||
| ||taco | ||
| ||""${'"'}.trimMargin() | ||
| | more | ||
| |""" | ||
| .trimMargin() | ||
| ) | ||
| } | ||
|
|
||
| @Test | ||
| fun `N escapes keywords`() { | ||
| val funSpec = FunSpec.builder("object").build() | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a while to fully grasp what's going on here, and something that I think could help is making the naming a bit less ambiguous, I've got a few suggestions:
startandendabove tokeepFromandkeepUntil. Simpler naming was fine while this method was trivial, now it heavily clashes withfirst,lastandkeptEndmaking the whole thing hard to grok.keptEndtokeepPlaceholdersUntil.keepPlaceholdersFrominstead of usingkeepFrom. While it's convenient that they have the same value, IMO it's better to have a separate pointer for a separate array.firstandlastinto a single variable calledpointeror justi, but don't feel strongly here.