From 8ffba9f6d3d07adc01bb68b1b4225af99c115436 Mon Sep 17 00:00:00 2001 From: Chua Chee Seng Date: Fri, 31 Jul 2026 15:32:00 +0800 Subject: [PATCH 1/3] Fix todo-writer misses for override vals, annotated defs, and symbolic names Three classes of declarations were silently skipped by the todo-writer, causing Scaladoc stubs to not be generated for them: 1. `override val` / `override var` declarations were excluded from `findUndocumentedResults` (only Def/Class/Trait were checked). 2. Declarations with an annotation on the same line (e.g. `@inline override def newArray`) were missed because a greedy regex in `declKeywordOnLine` / `declLeadingKeyword` consumed following keywords (like `def`) alongside the annotation, due to `\s` being included in the annotation character class. 3. Methods with symbolic names (e.g. `<:<`, `::`, `+=`) were skipped because `parseDef` only accepted identifier-style name characters (letters, digits, `_`, `$`) and produced empty names for operator names. Fix: extend the kind check to include Val/Var, replace the greedy annotation-stripping regex with the existing `dropLeadingAnnotations` helper in both methods, and add a symbolic-name parsing branch to `parseDef` using a new `isSymbolChar` predicate. Update `DeclStartPattern` with a more precise annotation regex for consistency. Also make `dropLeadingAnnotations` public to allow reuse across files. --- .../main/scala/todowriter/Declaration.scala | 50 +++++++++++-- .../scala/todowriter/ScaladocChecker.scala | 16 ++-- .../scala/todowriter/DeclarationSpec.scala | 65 ++++++++++++++++ .../scala/todowriter/IntegrationSpec.scala | 75 +++++++++++++++++++ 4 files changed, 195 insertions(+), 11 deletions(-) diff --git a/todo-writer/src/main/scala/todowriter/Declaration.scala b/todo-writer/src/main/scala/todowriter/Declaration.scala index 86d539d80d1a..94bd4f37da2a 100644 --- a/todo-writer/src/main/scala/todowriter/Declaration.scala +++ b/todo-writer/src/main/scala/todowriter/Declaration.scala @@ -16,9 +16,18 @@ case class Declaration( ) object Declaration: - /** Regex to detect the start of a declaration (with optional modifiers/annotations). */ + /** Regex to detect the start of a declaration (with optional modifiers/annotations). + * + * The annotation part uses a precise regex that properly handles annotation + * arguments (balanced `(...)` and `[...]` with one level of nesting) without + * including whitespace in the character class. The old regex `@[\w\(\)\s,."]+` + * was too greedy: because `\s` was in the character class it could consume + * following keywords like `override` and `def`, causing failures when used + * with replaceAll. With findFirstMatchIn the old regex still worked (via + * backtracking), but the new regex is both more correct and more efficient. + */ private val DeclStartPattern: Regex = - """(?:@[\w\(\)\s,."]+\s*)*(?:private|protected|final|override|inline|implicit|given|export|opaque|sealed|abstract|lazy|case\s+)*\s*(class|trait|object|def|val|var)\b""".r + """(?:@[\w.]+(?:\[[^\]]*\]|\((?:[^()]|\([^()]*\))*)*\s*)*(?:private|protected|final|override|inline|implicit|given|export|opaque|sealed|abstract|lazy|case\s+)*\s*(class|trait|object|def|val|var)\b""".r /** Regex to parse a def declaration. * @@ -96,6 +105,18 @@ object Declaration: case None => () false + /** Whether a character is a valid Scala operator/symbol character. + * + * Scala method names can be operator names composed of these characters, + * e.g. `<:<`, `+=`, `::`, etc. See the Scala Language Specification. + */ + private def isSymbolChar(c: Char): Boolean = + val symbolChars = Set( + '+', '-', '=', '!', '?', ':', '~', '/', '%', '&', + '*', '<', '>', '|', '^', '\\' + ) + symbolChars.contains(c) + private def parseDef(chunk: String): Declaration = // Normalize chunk: join lines, collapse whitespace val normalized = chunk.linesIterator.mkString(" ").replaceAll("\\s+", " ") @@ -106,8 +127,18 @@ object Declaration: while i < normalized.length && normalized(i).isWhitespace do i += 1 val nameStart = i - while i < normalized.length && - (normalized(i).isLetterOrDigit || normalized(i) == '_' || normalized(i) == '$') do i += 1 + // Scala method names come in two flavours: + // - Identifier names: start with a letter / digit / '_' / '$' + // - Operator (symbolic) names: consist of symbolic characters + // (e.g. <:<, +=, :::, ::-, etc.) + // We branch on the first character so that ':' (a type-annotation + // separator for identifier names) is not mistakenly consumed as part + // of a symbolic name. + if i < normalized.length && (normalized(i).isLetterOrDigit || normalized(i) == '_' || normalized(i) == '$') then + while i < normalized.length && + (normalized(i).isLetterOrDigit || normalized(i) == '_' || normalized(i) == '$') do i += 1 + else + while i < normalized.length && isSymbolChar(normalized(i)) do i += 1 val name = normalized.substring(nameStart, i) while i < normalized.length && normalized(i).isWhitespace do i += 1 @@ -356,8 +387,15 @@ object Declaration: name.substring(1, name.length - 1) else name - /** Remove leading parameter annotations, including annotation arguments. */ - private def dropLeadingAnnotations(str: String): String = + /** Remove leading annotations, including annotation arguments. + * + * This properly handles annotations with string arguments (which may + * contain spaces, commas, parens, etc.) by tracking string boundaries + * and balanced parentheses. This is more robust than a regex with + * whitespace in its character class, which would greedily consume + * following keywords like `def` or `override`. + */ + def dropLeadingAnnotations(str: String): String = var remaining = str.trim var changed = true while changed && remaining.startsWith("@") do diff --git a/todo-writer/src/main/scala/todowriter/ScaladocChecker.scala b/todo-writer/src/main/scala/todowriter/ScaladocChecker.scala index 6140da7a923d..9eb911e467ef 100644 --- a/todo-writer/src/main/scala/todowriter/ScaladocChecker.scala +++ b/todo-writer/src/main/scala/todowriter/ScaladocChecker.scala @@ -166,7 +166,8 @@ object ScaladocChecker: !trimmed.startsWith("*") && !trimmed.startsWith("/*") then val chunk = Declaration.getDeclarationAfter(text, lineStart) val decl = Declaration.parse(chunk) - if (decl.kind == DeclKind.Def || decl.kind == DeclKind.Class || decl.kind == DeclKind.Trait) && + if (decl.kind == DeclKind.Def || decl.kind == DeclKind.Class || decl.kind == DeclKind.Trait || + decl.kind == DeclKind.Val || decl.kind == DeclKind.Var) && decl.name.nonEmpty && declKeywordOnLine(trimmed, decl.kind) && // Do not synthesize docs for private declarations: they are not part @@ -280,10 +281,14 @@ object ScaladocChecker: /** The leading declaration keyword of a trimmed source line, after stripping * annotations and modifiers, or None if the line does not begin a declaration. + * + * Uses Declaration.dropLeadingAnnotations for robust annotation stripping + * (properly handles annotation arguments with spaces, strings, and nested + * parentheses — unlike a greedy regex that includes whitespace in its + * character class and would consume following keywords like `def`). */ private def declLeadingKeyword(trimmed: String): Option[String] = - val stripped = trimmed - .replaceAll("""(?:@[\w\(\)\s,."]+\s*)*""", "") + val stripped = Declaration.dropLeadingAnnotations(trimmed) .replaceAll("""(?:private\[[^\]]*\]|protected\[[^\]]*\]|private|protected|final|override|inline|implicit|sealed|abstract|lazy|case|transparent|opaque|export)\s+""", "") .trim (TermMemberKeywords ++ TemplateKeywords).find(k => @@ -316,9 +321,10 @@ object ScaladocChecker: case DeclKind.Def => "def " case DeclKind.Class => "class " case DeclKind.Trait => "trait " + case DeclKind.Val => "val " + case DeclKind.Var => "var " case _ => return false - val stripped = trimmed - .replaceAll("""(?:@[\w\(\)\s,."]+\s*)*""", "") + val stripped = Declaration.dropLeadingAnnotations(trimmed) .replaceAll("""(?:private\[[^\]]*\]|protected\[[^\]]*\]|private|protected|final|override|inline|implicit|given|export|opaque|sealed|abstract|lazy|case)\s+""", "") .trim stripped.startsWith(keyword) || stripped.startsWith("case " + keyword) diff --git a/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala b/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala index a6b58dcb0d09..562d027d799d 100644 --- a/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala +++ b/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala @@ -252,3 +252,68 @@ class DeclarationSpec extends AnyFlatSpec with Matchers: decl.tparams should be(List("CC", "T")) decl.params should be(List("ord")) } + + // --- Bug fixes ----------------------------------------------------------- + + // Bug 3: symbolic method names (e.g. <:<) were not parsed correctly, + // producing an empty name and causing the declaration to be skipped. + it should "parse def with symbolic operator name" in { + val chunk = "override def <:<(that: ClassManifest[?]): Boolean = (that eq this)" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("<:<") + decl.params should be(List("that")) + decl.returnType should be(Some("Boolean")) + } + + it should "parse def with symbolic operator name and no return type" in { + val chunk = "override def <:<(that: ClassManifest[?]) = (that eq this)" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("<:<") + decl.params should be(List("that")) + } + + it should "parse def with operator name containing =" in { + val chunk = "def +=(other: Int): Int = x + other" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("+=") + decl.params should be(List("other")) + decl.returnType should be(Some("Int")) + } + + it should "not treat colon as part of an alphanumeric method name" in { + val chunk = "def foo: Int = 42" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("foo") + decl.returnType should be(Some("Int")) + } + + // Bug 1: vals were not detected as undocumented declarations. + // Declaration.parse should still correctly parse val/var. + it should "parse override val" in { + val chunk = "override val typeArguments = args.toList" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Val) + decl.name should be("typeArguments") + } + + it should "parse annotated override def with @inline" in { + val chunk = "@inline override def newArray(len: Int): Array[Byte] = new Array[Byte](len)" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("newArray") + decl.params should be(List("len")) + decl.returnType should be(Some("Array[Byte]")) + } + + it should "parse override def preceded by multiple annotations" in { + val chunk = "@SerialVersionUID(1L) @nowarn(\"cat=deprecation\") override def foo(x: Int): Int = x" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("foo") + decl.params should be(List("x")) + decl.returnType should be(Some("Int")) + } diff --git a/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala b/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala index 207758a0fd11..8ab513f55313 100644 --- a/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala +++ b/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala @@ -728,3 +728,78 @@ class IntegrationSpec extends AnyFlatSpec with Matchers: checkResult.hasIssues should be(false) } } + + // --- Regression tests for ticket: todo-writer potential misses --- + + it should "detect undocumented override val (public val in class body)" in { + // Mirrors library/src/scala/reflect/ClassManifestDeprecatedApis.scala: + // /** Returns the runtime class... */ + // override def runtimeClass = clazz + // override val typeArguments = args.toList <-- missed by todo-writer + val content = """package test + | + |/** A class manifest. + | */ + |class AbstractTypeClassManifest { + | /** Returns the runtime class that was supplied as the erasure of the abstract type. */ + | override def runtimeClass: Class[?] = clazz + | override val typeArguments: List[Any] = args.toList + | override def toString(): String = prefix.toString + |} + |""".stripMargin + + withTempFile(content) { path => + val result = ScaladocChecker.checkFile(path) + val synth = result.results.filter(_.scaladoc.synthetic).map(r => (r.declaration.kind, r.declaration.name)) + // typeArguments (a public val) should be detected as undocumented + synth should contain((DeclKind.Val, "typeArguments")) + // toString (a public def) should also be detected + synth should contain((DeclKind.Def, "toString")) + } + } + + it should "detect undocumented @inline override def with annotation on same line" in { + // Mirrors library/src/scala/reflect/Manifest.scala: + // /** Returns the `Class` for the primitive type `byte`. */ + // def runtimeClass: Class[java.lang.Byte] = java.lang.Byte.TYPE + // @inline override def newArray(len: Int): Array[Byte] = new Array[Byte](len) <-- missed + val content = """package test + | + |/** A class. + | */ + |final private[reflect] class ByteManifest extends Base { + | /** Returns the Class for the primitive type byte. */ + | def runtimeClass: Class[Byte] = java.lang.Byte.TYPE + | @inline override def newArray(len: Int): Array[Byte] = new Array[Byte](len) + |} + |""".stripMargin + + withTempFile(content) { path => + val result = ScaladocChecker.checkFile(path) + val synth = result.results.filter(_.scaladoc.synthetic).map(r => (r.declaration.kind, r.declaration.name)) + // @inline override def newArray should be detected as undocumented + synth should contain((DeclKind.Def, "newArray")) + } + } + + it should "detect undocumented def with symbolic operator name <:<" in { + // Mirrors library/src/scala/reflect/Manifest.scala: + // override def newArray(len: Int) = new Array[scala.Any](len) + // override def <:<(that: ClassManifest[?]): Boolean = (that eq this) <-- missed + val content = """package test + | + |/** A class. + | */ + |final private class AnyManifest extends Base { + | override def newArray(len: Int) = new Array[scala.Any](len) + | override def <:<(that: ClassManifest[?]): Boolean = (that eq this) + |} + |""".stripMargin + + withTempFile(content) { path => + val result = ScaladocChecker.checkFile(path) + val synth = result.results.filter(_.scaladoc.synthetic).map(r => (r.declaration.kind, r.declaration.name)) + // override def <:< should be detected as undocumented + synth should contain((DeclKind.Def, "<:<")) + } + } From ab492686362513c0d499220abe7a927243284189 Mon Sep 17 00:00:00 2001 From: Bill Venners Date: Fri, 31 Jul 2026 14:21:44 -0700 Subject: [PATCH 2/3] todo-writer: recognize all Scala operator names LLM-assisted fix and regression tests, reviewed and validated locally. --- .../main/scala/todowriter/Declaration.scala | 13 ++++++------ .../scala/todowriter/DeclarationSpec.scala | 20 +++++++++++++++++++ .../scala/todowriter/IntegrationSpec.scala | 17 ++++++++++++++++ 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/todo-writer/src/main/scala/todowriter/Declaration.scala b/todo-writer/src/main/scala/todowriter/Declaration.scala index 94bd4f37da2a..f6229682d3e8 100644 --- a/todo-writer/src/main/scala/todowriter/Declaration.scala +++ b/todo-writer/src/main/scala/todowriter/Declaration.scala @@ -105,17 +105,18 @@ object Declaration: case None => () false + private val AsciiSymbolChars = "~!@#%^*+-<>?:=&|/\\" + /** Whether a character is a valid Scala operator/symbol character. * * Scala method names can be operator names composed of these characters, - * e.g. `<:<`, `+=`, `::`, etc. See the Scala Language Specification. + * e.g. `<:<`, `#::`, `+=`, `→`, etc. See the Scala Language Specification. */ private def isSymbolChar(c: Char): Boolean = - val symbolChars = Set( - '+', '-', '=', '!', '?', ':', '~', '/', '%', '&', - '*', '<', '>', '|', '^', '\\' - ) - symbolChars.contains(c) + AsciiSymbolChars.contains(c) || + (Character.getType(c) match + case Character.MATH_SYMBOL | Character.OTHER_SYMBOL => true + case _ => false) private def parseDef(chunk: String): Declaration = // Normalize chunk: join lines, collapse whitespace diff --git a/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala b/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala index 562d027d799d..71b911bd7bdf 100644 --- a/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala +++ b/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala @@ -283,6 +283,26 @@ class DeclarationSpec extends AnyFlatSpec with Matchers: decl.returnType should be(Some("Int")) } + it should "parse def with a hash-prefixed operator name" in { + val chunk = "def #::[B](elem: B): List[B] = ???" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("#::") + decl.tparams should be(List("B")) + decl.params should be(List("elem")) + decl.returnType should be(Some("List[B]")) + } + + it should "parse def with a Unicode operator name" in { + val chunk = "def →[B](that: B): (A, B) = ???" + val decl = Declaration.parse(chunk) + decl.kind should be(DeclKind.Def) + decl.name should be("→") + decl.tparams should be(List("B")) + decl.params should be(List("that")) + decl.returnType should be(Some("(A, B)")) + } + it should "not treat colon as part of an alphanumeric method name" in { val chunk = "def foo: Int = 42" val decl = Declaration.parse(chunk) diff --git a/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala b/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala index 8ab513f55313..8470009bdc0e 100644 --- a/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala +++ b/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala @@ -803,3 +803,20 @@ class IntegrationSpec extends AnyFlatSpec with Matchers: synth should contain((DeclKind.Def, "<:<")) } } + + it should "detect undocumented defs with hash-prefixed and Unicode operator names" in { + val content = """package test + | + |class Operators { + | def #::[A](elem: A): List[A] = ??? + | def →[A](that: A): (A, A) = ??? + |} + |""".stripMargin + + withTempFile(content) { path => + val result = ScaladocChecker.checkFile(path) + val synth = result.results.filter(_.scaladoc.synthetic).map(r => (r.declaration.kind, r.declaration.name)) + synth should contain((DeclKind.Def, "#::")) + synth should contain((DeclKind.Def, "→")) + } + } From 40369fa7496ec95165b59ff4cff83ee92c813a03 Mon Sep 17 00:00:00 2001 From: Bill Venners Date: Fri, 31 Jul 2026 14:22:27 -0700 Subject: [PATCH 3/3] todo-writer: handle annotation type arguments LLM-assisted fix and regression tests, reviewed and validated locally. --- .../main/scala/todowriter/Declaration.scala | 60 +++++++++++-------- .../scala/todowriter/DeclarationSpec.scala | 4 ++ .../scala/todowriter/IntegrationSpec.scala | 15 +++++ 3 files changed, 55 insertions(+), 24 deletions(-) diff --git a/todo-writer/src/main/scala/todowriter/Declaration.scala b/todo-writer/src/main/scala/todowriter/Declaration.scala index f6229682d3e8..8aa964bf8945 100644 --- a/todo-writer/src/main/scala/todowriter/Declaration.scala +++ b/todo-writer/src/main/scala/todowriter/Declaration.scala @@ -388,7 +388,7 @@ object Declaration: name.substring(1, name.length - 1) else name - /** Remove leading annotations, including annotation arguments. + /** Remove leading annotations, including type and value arguments. * * This properly handles annotations with string arguments (which may * contain spaces, commas, parens, etc.) by tracking string boundaries @@ -413,31 +413,43 @@ object Declaration: else var i = 1 while i < str.length && (str(i).isLetterOrDigit || str(i) == '_' || str(i) == '.') do i += 1 - while i < str.length && str(i).isWhitespace do i += 1 - if i < str.length && str(i) == '(' then - var depth = 0 - var inString = false - var quoteChar = '\u0000' - var escaped = false + var keepReadingArguments = true + while keepReadingArguments do + while i < str.length && str(i).isWhitespace do i += 1 + if i < str.length && (str(i) == '(' || str(i) == '[') then + val open = str(i) + val close = if open == '(' then ')' else ']' + val end = balancedGroupEnd(str, i, open, close) + if end < 0 then return -1 + i = end + else keepReadingArguments = false + i + + /** Return the index after a balanced parenthesized or bracketed group, or -1 if unbalanced. */ + private def balancedGroupEnd(str: String, start: Int, open: Char, close: Char): Int = + var i = start + var depth = 0 + var inString = false + var quoteChar = '\u0000' + var escaped = false - while i < str.length do - val ch = str(i) - if inString then - if escaped then escaped = false - else if ch == '\\' then escaped = true - else if ch == quoteChar then inString = false - else - if ch == '"' || ch == '\'' then - inString = true - quoteChar = ch - else if ch == '(' then depth += 1 - else if ch == ')' then - depth -= 1 - if depth == 0 then return i + 1 - i += 1 - -1 - else i + while i < str.length do + val ch = str(i) + if inString then + if escaped then escaped = false + else if ch == '\\' then escaped = true + else if ch == quoteChar then inString = false + else + if ch == '"' || ch == '\'' then + inString = true + quoteChar = ch + else if ch == open then depth += 1 + else if ch == close then + depth -= 1 + if depth == 0 then return i + 1 + i += 1 + -1 /** Split a string by commas, but ignore commas inside brackets/parentheses. */ private def splitByCommasTopLevel(str: String): List[String] = diff --git a/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala b/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala index 71b911bd7bdf..9244961330a2 100644 --- a/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala +++ b/todo-writer/src/test/scala/todowriter/DeclarationSpec.scala @@ -337,3 +337,7 @@ class DeclarationSpec extends AnyFlatSpec with Matchers: decl.params should be(List("x")) decl.returnType should be(Some("Int")) } + + it should "strip annotation type and value arguments" in { + Declaration.dropLeadingAnnotations("@ann[String](\"reason\") override def foo") should be("override def foo") + } diff --git a/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala b/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala index 8470009bdc0e..e3792450a70e 100644 --- a/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala +++ b/todo-writer/src/test/scala/todowriter/IntegrationSpec.scala @@ -782,6 +782,21 @@ class IntegrationSpec extends AnyFlatSpec with Matchers: } } + it should "detect undocumented defs preceded by annotations with type arguments" in { + val content = """package test + | + |class Annotated { + | @ann[String]("reason") override def documentedByTheChecker(): Int = 1 + |} + |""".stripMargin + + withTempFile(content) { path => + val result = ScaladocChecker.checkFile(path) + val synth = result.results.filter(_.scaladoc.synthetic).map(r => (r.declaration.kind, r.declaration.name)) + synth should contain((DeclKind.Def, "documentedByTheChecker")) + } + } + it should "detect undocumented def with symbolic operator name <:<" in { // Mirrors library/src/scala/reflect/Manifest.scala: // override def newArray(len: Int) = new Array[scala.Any](len)