Don't leak an expression body's indent into later declarations - #2365
Don't leak an expression body's indent into later declarations#2365eyupcanakman wants to merge 1 commit into
Conversation
|
I'm wondering if we should just turn off expression bodies. Seems like we only gain pain by having them enabled. |
d8ef22e to
731768a
Compare
|
Turning them off does fix it, but 101 of the 1172 tests here fail and the two #1300 examples don't move at all since those are property initializers rather than function bodies. Reworked this one to leave a block alone when its indents already don't match. |
A function body that opens or closes an indent had that half trimmed away while its partner stayed, so CodeWriter kept the wrong level for every declaration after it. Deeper cases threw cannot unindent instead. Trimming now takes placeholders back from either end until nothing in the block is left unmatched. Fixes square#1421.
731768a to
1d828b5
Compare
Egorand
left a comment
Egorand
left a comment
There was a problem hiding this comment.
Looks good overall, got some feedback on naming.
| '«' -> statementTotal++ | ||
| '»' -> statementTotal-- | ||
| } | ||
| if (indentTotal < 0 || statementTotal < 0) break |
There was a problem hiding this comment.
Is there a test case where this statement will evaluate to true? All tests still pass if I comment it out.
| while (start < end && formatParts[end - 1] in NO_ARG_PLACEHOLDERS) { | ||
| end-- | ||
| } | ||
| if (keepBalanced && (start > 0 || end < formatParts.size)) { |
There was a problem hiding this comment.
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:
- First, let's rename
startandendabove tokeepFromandkeepUntil. Simpler naming was fine while this method was trivial, now it heavily clashes withfirst,lastandkeptEndmaking the whole thing hard to grok. - Similarly, let's rename
keptEndtokeepPlaceholdersUntil. - Let's also introduce
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. - We can probably squash
firstandlastinto a single variable calledpointeror justi, but don't feel strongly here.
|
|
||
| internal fun hasStatements() = formatParts.any { "«" in it } | ||
|
|
||
| private fun isBalanced(placeholders: CharArray, count: Int): Boolean { |
There was a problem hiding this comment.
Let's move this helper inside trim()?
| } | ||
|
|
||
| @Test | ||
| fun trimKeepsAStatementAndItsIndentTogether() { |
There was a problem hiding this comment.
nit: maybe trimKeepsStatementsAndIndentsBalanced?
3 participants
An expression body that opens or closes an indent leaks it into every declaration after it, so each following function lands two spaces deeper than the last. Deeper nesting throws
cannot unindentinstead.CodeBlock.trim()strips no-arg placeholders from both ends, dropping one half of a pair while its partner stays behind. It now takes them back from either end until nothing is left unmatched.A block that was already unmatched before trimming is left alone, since taking one end back cannot balance it. Markers that open and close out of order are left alone as well, and building that shape takes raw placeholders rather than the builder.
A body that opens an indent before the returned expression emits as a block body now, since the trimmed block no longer starts with the return. Same code, different shape. Most of the diff is tests.
Fixes #1421.
docs/changelog.mdhas been updated if applicable.be added to the changelog.