Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 80 additions & 29 deletions todo-writer/src/main/scala/todowriter/Declaration.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -96,6 +105,19 @@ 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.
*/
private def isSymbolChar(c: Char): Boolean =
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
val normalized = chunk.linesIterator.mkString(" ").replaceAll("\\s+", " ")
Expand All @@ -106,8 +128,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
Expand Down Expand Up @@ -356,8 +388,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 type and value 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
Expand All @@ -374,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] =
Expand Down
16 changes: 11 additions & 5 deletions todo-writer/src/main/scala/todowriter/ScaladocChecker.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 =>
Expand Down Expand Up @@ -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)
Expand Down
89 changes: 89 additions & 0 deletions todo-writer/src/test/scala/todowriter/DeclarationSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -252,3 +252,92 @@ 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 "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)
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"))
}

it should "strip annotation type and value arguments" in {
Declaration.dropLeadingAnnotations("@ann[String](\"reason\") override def foo") should be("override def foo")
}
107 changes: 107 additions & 0 deletions todo-writer/src/test/scala/todowriter/IntegrationSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -728,3 +728,110 @@ 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 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)
// 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, "<:<"))
}
}

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, "→"))
}
}