Miscellaneous fixes to the KDoc formatter (#372)

Summary:
This CL fixes a number of bugs, covered by associated unit tests:
   - Line overrun when using closed-open interval notation
   - More gracefully handle unterminated [] references (for example when comment is using it in things like [closed, open) intervals)
   - Recognize and convert accidentally capitalized kdoc tags like See
   - If you have a [ref] which spans a line such that the # ends up as a new line, don't treat this as a "# heading".
   - Make sure we don't line break at an expression starting with ">" since that would turn into a quoted line.
   - If you're using optimal line breaking and there's a really long, unbreakable word in the paragraph, switch that paragraph over to greedy line breaking (to make the paragraph better balanced since the really long word throws the algorithm off.)
   - Fix a few scenarios where markup conversion from <p> and </p> wasn't converting everything.
   - Allow property[name], not just param[name]
   - The --hanging-indent flag now also sets the nested list indent (if >= 3)
- Some minor code cleanup.

Pull Request resolved: #372

Reviewed By: strulovich

Differential Revision: D41919138

Pulled By: cgrushko

fbshipit-source-id: aae46b67dbcc4dcabe78578c7187b1353cbd8968

core/src/main/java/com/facebook/ktfmt/kdoc/KDocFormattingOptions.kt

@@ -60,7 +60,8 @@ class KDocFormattingOptions(
 
  /**
  * How many spaces to use for hanging indents in numbered lists and after block tags. Using 4 or
- * more here will result in subsequent lines being interpreted as block formatted.
+ * more here will result in subsequent lines being interpreted as block formatted by IntelliJ (but
+ * not Dokka).
  */
  var hangingIndent: Int = 2
 

core/src/main/java/com/facebook/ktfmt/kdoc/Paragraph.kt

@@ -101,10 +101,6 @@ class Paragraph(private val task: FormattingTask) {
  return content.isEmpty()
  }
 
- private fun hasClosingPre(): Boolean {
- return content.contains("</pre>", ignoreCase = false) || next?.hasClosingPre() ?: false
- }
-
  fun cleanup() {
  val original = text
 
@@ -164,11 +160,23 @@ class Paragraph(private val task: FormattingTask) {
  }
 
  private fun convertMarkup(s: String): String {
- if (s.none { it == '<' || it == '&' || it == '{' }) return s
+ // Whether the tag starts with a capital letter and needs to be cleaned, e.g. `@See` -> `@see`.
+ // (isKDocTag only allows the first letter to be capitalized.)
+ val convertKDocTag = s.isKDocTag() && s[1].isUpperCase()
+
+ if (!convertKDocTag && s.none { it == '<' || it == '&' || it == '{' }) {
+ return s
+ }
 
  val sb = StringBuilder(s.length)
  var i = 0
  val n = s.length
+
+ if (convertKDocTag) {
+ sb.append('@').append(s[1].lowercaseChar())
+ i += 2
+ }
+
  var code = false
  var brackets = 0
  while (i < n) {
@@ -335,8 +343,12 @@ class Paragraph(private val task: FormattingTask) {
  return reflow(words, lineWidth, hangingIndentSize)
  }
 
- fun reflow(words: List<String>, lineWidth: Int, hangingIndentSize: Int): List<String> {
- if (options.alternate || !options.optimal || hanging && hangingIndentSize > 0) {
+ private fun reflow(words: List<String>, lineWidth: Int, hangingIndentSize: Int): List<String> {
+ if (options.alternate ||
+ !options.optimal ||
+ hanging && hangingIndentSize > 0 ||
+ // An unbreakable long word may make other lines shorter and won't look good
+ words.any { it.length > lineWidth }) {
  // Switch to greedy if explicitly turned on, and for hanging indent
  // paragraphs, since the current implementation doesn't have support
  // for a different maximum length on the first line from the rest
@@ -405,7 +417,8 @@ class Paragraph(private val task: FormattingTask) {
  word.startsWith("```") ||
  word.isDirectiveMarker() ||
  word.startsWith("@") || // interpreted as a tag
- word.isTodo()) {
+ word.isTodo() ||
+ word.startsWith(">")) {
  return false
  }
 
@@ -419,7 +432,14 @@ class Paragraph(private val task: FormattingTask) {
  return true
  }
 
- private fun computeWords(): List<String> {
+ /**
+ * Split [text] up into individual "words"; in the case where some words are not allowed to span
+ * lines, it will combine these into single word. For example, if we have a sentence which ends
+ * with a number, e.g. "the sum is 5.", we want to make sure "5." is never placed at the beginning
+ * of a new line (which would turn it into a list item), so for this we'll compute the word list
+ * "the", "sum", "is 5.".
+ */
+ fun computeWords(): List<String> {
  val words = text.split(Regex("\\s+")).filter { it.isNotBlank() }.map { it.trim() }
  if (words.size == 1) {
  return words
@@ -438,39 +458,53 @@ class Paragraph(private val task: FormattingTask) {
 
  val combined = ArrayList<String>(words.size)
 
- // If this paragraph is a list item or a quoted line, merge the first word
- // with this item such that we never split them apart.
- var start = 0
- var first = words[start++]
- if (quoted || hanging && !text.isKDocTag()) {
- first = first + " " + words[start++]
- }
+ var from = 0
+ val end = words.size
+ while (from < end) {
+ val start =
+ if (from == 0 && (quoted || hanging && !text.isKDocTag())) {
+ from + 2
+ } else {
+ from + 1
+ }
+ var to = words.size
+ for (i in start until words.size) {
+ val next = words[i]
+ if (next.startsWith("[") && !next.startsWith("[[")) {
+ // find end
+ var j = -1
+ for (k in i until words.size) {
+ if (']' in words[k]) {
+ j = k
+ break
+ }
+ }
+ if (j != -1) {
+ // combine everything in the string; we can't break link text
+ if (start == from + 1 && canBreakAt(words[start])) {
+ combined.add(words[from])
+ from = start
+ }
+ // Maybe not break; what if the next word isn't okay?
+ to = j + 1
+ if (to == words.size || canBreakAt(words[to])) {
+ break
+ }
+ } // else: unterminated [, ignore
+ } else if (canBreakAt(next)) {
+ to = i
+ break
+ }
+ }
 
- combined.add(first)
- var prev = first
- var insideSquareBrackets = words[start - 1].startsWith("[")
- for (i in start until words.size) {
- val word = words[i]
-
- // We also cannot break up a URL text across lines, which will alter the
- // rendering of the docs.
- if (prev.startsWith("[")) insideSquareBrackets = true
- if (prev.contains("]")) insideSquareBrackets = false
-
- // Can we start a new line with this without interpreting it in a special
- // way?
- if (!canBreakAt(word) || insideSquareBrackets) {
- // Combine with previous word with a single space; the line breaking
- // algorithm won't know that it's more than one word.
- val joined = "$prev $word"
- combined.removeLast()
- combined.add(joined)
- prev = joined
- } else {
- combined.add(word)
- prev = word
+ if (to == from + 1) {
+ combined.add(words[from])
+ } else if (to > from) {
+ combined.add(words.subList(from, to).joinToString(" "))
  }
+ from = to
  }
+
  return combined
  }
 
@@ -503,7 +537,7 @@ class Paragraph(private val task: FormattingTask) {
  }
 
  fun search(pi0: Int, pj0: Int, pi1: Int, pj1: Int) {
- val stack = java.util.ArrayDeque<Quadruple>()
+ val stack = ArrayDeque<Quadruple>()
  stack.add(Quadruple(pi0, pj0, pi1, pj1))
 
  while (stack.isNotEmpty()) {

core/src/main/java/com/facebook/ktfmt/kdoc/ParagraphListBuilder.kt

@@ -48,6 +48,7 @@ class ParagraphListBuilder(
  private fun closeParagraph(): Paragraph {
  val text = paragraph.text
  when {
+ paragraph.preformatted -> {}
  text.isKDocTag() -> {
  paragraph.doc = true
  paragraph.hanging = true
@@ -142,14 +143,15 @@ class ParagraphListBuilder(
  newParagraph()
  var j = i
  var foundClose = false
- var customize = true
+ var allowCustomize = true
  while (j < lines.size) {
  val l = lines[j]
  val lineWithIndentation = lineContent(l)
  if (lineWithIndentation.contains("```") &&
  lineWithIndentation.trimStart().startsWith("```")) {
- // Don't convert <pre> tags if we already have nested ``` content; that will lead to trouble
- customize = false
+ // Don't convert <pre> tags if we already have nested ``` content; that will lead to
+ // trouble
+ allowCustomize = false
  }
  val done = (includeStart || j > i) && until(lineWithIndentation)
  if (!includeEnd && done) {
@@ -175,7 +177,7 @@ class ParagraphListBuilder(
  if (!foundClose && expectClose) {
  // Just add a single line as preformatted and then treat the rest in the
  // normal way
- customize = false
+ allowCustomize = false
  j = lines.size
  }
 
@@ -185,7 +187,7 @@ class ParagraphListBuilder(
  appendText(lineWithIndentation)
  paragraph.preformatted = true
  paragraph.allowEmpty = true
- if (customize) {
+ if (allowCustomize) {
  customize(index, paragraph)
  }
  newParagraph()
@@ -254,8 +256,11 @@ class ParagraphListBuilder(
  newParagraph(i - 1).block = true
  appendText(lineWithoutIndentation)
  newParagraph(i).block = true
- } else if (lineWithoutIndentation.startsWith(
- "#")) { // not isHeader() because <h> is handled separately
+ } else if (lineWithoutIndentation.startsWith("#")
+ // "## X" is a header, "##X" is not
+ &&
+ lineWithoutIndentation.firstOrNull { it != '#' }?.equals(' ') ==
+ true) { // not isHeader() because <h> is handled separately
  // ## Header
  newParagraph(i - 1).block = true
  appendText(lineWithoutIndentation)
@@ -496,17 +501,28 @@ class ParagraphListBuilder(
  return ParagraphList(paragraphs)
  }
 
- private fun addPlainText(i: Int, text: String, braceBalance: Int = 0): Int {
- val s =
- if (options.convertMarkup &&
- (text.startsWith("<p>", true) || text.startsWith("<p/>", true))) {
- paragraph.separate = true
- text.substring(text.indexOf('>') + 1).trim()
- } else {
- text
- }
- .let { if (options.collapseSpaces) it.collapseSpaces() else it }
+ private fun convertPrefix(text: String): String {
+ return if (options.convertMarkup &&
+ (text.startsWith("<p>", true) || text.startsWith("<p/>", true))) {
+ paragraph.separate = true
+ text.substring(text.indexOf('>') + 1).trim()
+ } else {
+ text
+ }
+ }
 
+ private fun convertSuffix(trimmedPrefix: String): String {
+ return if (options.convertMarkup &&
+ (trimmedPrefix.endsWith("<p/>", true) || (trimmedPrefix.endsWith("</p>", true)))) {
+ trimmedPrefix.substring(0, trimmedPrefix.length - 4).trimEnd().removeSuffix("*").trimEnd()
+ } else {
+ trimmedPrefix
+ }
+ }
+
+ private fun addPlainText(i: Int, text: String, braceBalance: Int = 0): Int {
+ val trimmed = convertSuffix(convertPrefix(text))
+ val s = trimmed.let { if (options.collapseSpaces) it.collapseSpaces() else it }
  appendText(s)
  appendText(" ")
 

core/src/main/java/com/facebook/ktfmt/kdoc/Utilities.kt

@@ -114,17 +114,21 @@ fun String.isLine(minCount: Int = 3): Boolean {
 
 fun String.isKDocTag(): Boolean {
  // Not using a hardcoded list here since tags can change over time
- if (startsWith("@")) {
+ if (startsWith("@") && length > 1) {
  for (i in 1 until length) {
  val c = this[i]
  if (c.isWhitespace()) {
  return i > 2
  } else if (!c.isLetter() || !c.isLowerCase()) {
- if (c == '[' && startsWith("@param")) {
+ if (c == '[' && (startsWith("@param") || startsWith("@property"))) {
  // @param is allowed to use brackets -- see
  // https://kotlinlang.org/docs/kotlin-doc.html#param-name
  // Example: @param[foo] The description of foo
  return true
+ } else if (i == 1 && c.isLetter() && c.isUpperCase()) {
+ // Allow capitalized tgs, such as @See -- this is normally a typo; convertMarkup
+ // should also fix these.
+ return true
  }
  return false
  }

core/src/main/java/com/facebook/ktfmt/kdoc/format.md

@@ -0,0 +1 @@
+kdoc-formatter --greedy --max-line-width=100 --max-comment-width=100 .