Merge pull request scala#13705 from pikinier20/scaladoc/cats-feedback

Scaladoc: Fix bugs found while setting up cats

Author: pikinier20

scaladoc-testcases/src/tests/packageobjdocs/package.scala

@@ -0,0 +1,10 @@
+package tests
+
+/** Some documentation that should be present in Scaladoc
+ *
+ * It's a test
+ *
+ */
+package object packageobjdocs {
+  def placeholder: Int = 42
+}

scaladoc/src/dotty/tools/scaladoc/ExternalDocLink.scala

@@ -18,31 +18,44 @@ enum DocumentationKind:
   case Scaladoc3 extends DocumentationKind
 
 object ExternalDocLink:
-  def parse(mapping: String): Either[String, ExternalDocLink] =
-    def fail(msg: String) = Left(s"Unable to process external mapping $mapping. $msg")
+  private def fail(mapping: String, msg: String) = Left(s"Unable to process external mapping $mapping. $msg")
+
+  private def tryParse[T](mapping: String, descr: String)(op: => T): Either[String, T] = Try(op) match {
+    case Success(v) => Right(v)
+    case Failure(e) => fail(mapping, s"Unable to parse $descr. Exception $e occured")
+  }
 
-    def tryParse[T](descr: String)(op: => T): Either[String, T] = Try(op) match {
-      case Success(v) => Right(v)
-      case Failure(e) => fail(s"Unable to parse $descr. Exception $e occured")
-    }
+  def parseLegacy(mapping: String): Either[String, ExternalDocLink] =
+    mapping.split("#").toList match
+      case path :: apiUrl :: Nil => for {
+        url <- tryParse(mapping, "url")(URL(apiUrl))
+      } yield ExternalDocLink(
+        List(s"$path.*".r),
+        url,
+        DocumentationKind.Scaladoc2,
+        None
+      )
+      case _ => fail(mapping, "Wrong format of legacy external mapping. path#apiUrl format is accepted.")
+
+  def parse(mapping: String): Either[String, ExternalDocLink] =
 
     def parsePackageList(elements: List[String]) = elements match
-     case List(urlStr) => tryParse("packageList")(Some(URL(urlStr)))
+     case List(urlStr) => tryParse(mapping, "packageList")(Some(URL(urlStr)))
      case Nil => Right(None)
-     case other => fail(s"Provided multiple package lists: $other")
+     case other => fail(mapping, s"Provided multiple package lists: $other")
 
     def doctoolByName(name: String) = name match
       case "javadoc" => Right(DocumentationKind.Javadoc)
       case "scaladoc2" => Right(DocumentationKind.Scaladoc2)
       case "scaladoc3" => Right(DocumentationKind.Scaladoc3)
-      case other => fail(s"Unknown doctool: $other")
+      case other => fail(mapping, s"Unknown doctool: $other")
 
 
     mapping.split("::").toList match
       case regexStr :: docToolStr :: urlStr :: rest =>
         for {
-          regex <- tryParse("regex")(regexStr.r)
-          url <- tryParse("url")(URL(urlStr))
+          regex <- tryParse(mapping, "regex")(regexStr.r)
+          url <- tryParse(mapping, "url")(URL(urlStr))
           doctool <- doctoolByName(docToolStr)
           packageList <- parsePackageList(rest)
         } yield ExternalDocLink(
@@ -52,4 +65,4 @@ object ExternalDocLink:
             packageList
           )
       case _ =>
-        fail("Accepted format: `regexStr::docToolStr::urlStr[::rest]`")
+        fail(mapping, "Accepted format: `regexStr::docToolStr::urlStr[::rest]`")

scaladoc/src/dotty/tools/scaladoc/Scaladoc.scala

@@ -169,6 +169,8 @@ object Scaladoc:
           CommentSyntax.default
         }
       }
+      val legacySourceLinkList = if legacySourceLink.get.nonEmpty then List(legacySourceLink.get) else Nil
+
       val externalMappings =
         externalDocumentationMappings.get.flatMap( s =>
             ExternalDocLink.parse(s).fold(left => {
@@ -178,6 +180,15 @@ object Scaladoc:
           )
         )
 
+      val legacyExternalMappings =
+        legacyExternalDocumentationMappings.get.flatMap { s =>
+          ExternalDocLink.parseLegacy(s).fold(left => {
+              report.warning(left)
+              None
+            }, right => Some(right)
+          )
+        }
+
       val socialLinksParsed =
         socialLinks.get.flatMap { s =>
           SocialLinks.parse(s).fold(left => {
@@ -208,9 +219,9 @@ object Scaladoc:
         projectLogo.nonDefault,
         projectFooter.nonDefault,
         parseSyntax,
-        sourceLinks.get,
+        sourceLinks.get ++ legacySourceLinkList,
         revision.nonDefault,
-        externalMappings,
+        externalMappings ++ legacyExternalMappings,
         socialLinksParsed,
         skipById.get ++ deprecatedSkipPackages.get,
         skipByRegex.get,

scaladoc/src/dotty/tools/scaladoc/ScaladocSettings.scala

@@ -34,13 +34,16 @@ class ScaladocSettings extends SettingGroup with AllScalaSettings:
     StringSetting("-project-version", "project version", "The current version of your project.", "", aliases = List("-doc-version"))
 
   val projectLogo: Setting[String] =
-    StringSetting("-project-logo", "project logo filename", "The file that contains the project's logo (in /images).", "", aliases = List("-doc-logo"))
+    StringSetting("-project-logo", "project logo filename", "Path to the file that contains the project's logo. Provided path can be absolute or relative to the project root directory.", "", aliases = List("-doc-logo"))
 
   val projectFooter: Setting[String] = StringSetting("-project-footer", "project footer", "A footer on every Scaladoc page.", "", aliases = List("-doc-footer"))
 
   val sourceLinks: Setting[List[String]] =
     MultiStringSetting("-source-links", "sources", SourceLinks.usage)
 
+  val legacySourceLink: Setting[String] =
+    StringSetting("-doc-source-url", "sources", "Legacy option from Scala 2. Use -source-links instead.", "")
+
   val syntax: Setting[String] =
     StringSetting("-comment-syntax", "syntax", "Syntax of the comment used", "")
 
@@ -52,6 +55,9 @@ class ScaladocSettings extends SettingGroup with AllScalaSettings:
       "Mapping between regexes matching classpath entries and external documentation. " +
         "'regex::[scaladoc|scaladoc|javadoc]::path' syntax is used")
 
+  val legacyExternalDocumentationMappings: Setting[List[String]] =
+    MultiStringSetting("-doc-external-doc", "legacy-external-mappings", "Legacy option from Scala 2. Mapping betweeen path and external documentation. Use -external-mappings instead.")
+
   val socialLinks: Setting[List[String]] =
     MultiStringSetting("-social-links", "social-links",
       "Links to social sites. '[github|twitter|gitter|discord]::link' syntax is used. " +
@@ -124,4 +130,4 @@ class ScaladocSettings extends SettingGroup with AllScalaSettings:
     StringSetting("-scastie-configuration", "Scastie configuration", "Additional configuration passed to Scastie in code snippets", "")
 
   def scaladocSpecificSettings: Set[Setting[_]] =
-    Set(sourceLinks, syntax, revision, externalDocumentationMappings, socialLinks, skipById, skipByRegex, deprecatedSkipPackages, docRootContent, snippetCompiler, generateInkuire, scastieConfiguration)
+    Set(sourceLinks, legacySourceLink, syntax, revision, externalDocumentationMappings, socialLinks, skipById, skipByRegex, deprecatedSkipPackages, docRootContent, snippetCompiler, generateInkuire, scastieConfiguration)

scaladoc/src/dotty/tools/scaladoc/SourceLinks.scala

@@ -22,10 +22,10 @@ case class TemplateSourceLink(val urlTemplate: String) extends SourceLink:
       "\\{\\{ path \\}\\}".r -> pathString,
       "\\{\\{ line \\}\\}".r -> line.fold("")(_.toString),
       "\\{\\{ ext \\}\\}".r -> Some(
-        pathString).filter(_.lastIndexOf(".") == -1).fold("")(p => p.substring(p.lastIndexOf("."))
+        pathString).filter(_.lastIndexOf(".") != -1).fold("")(p => p.substring(p.lastIndexOf("."))
       ),
       "\\{\\{ path_no_ext \\}\\}".r -> Some(
-        pathString).filter(_.lastIndexOf(".") == -1).fold(pathString)(p => p.substring(0, p.lastIndexOf("."))
+        pathString).filter(_.lastIndexOf(".") != -1).fold(pathString)(p => p.substring(0, p.lastIndexOf("."))
       ),
       "\\{\\{ name \\}\\}".r -> memberName
     )
@@ -43,7 +43,7 @@ case class WebBasedSourceLink(prefix: String, revision: String, subPath: String)
     s"$prefix/$action/$finalRevision$subPath/${pathToString(path)}$linePart"
 
 class SourceLinkParser(revision: Option[String]) extends ArgParser[SourceLink]:
-  val KnownProvider = raw"(\w+):\/\/([^\/#]+)\/([^\/#]+)(\/[^\/#]+)?(#.+)?".r
+  val KnownProvider = raw"(\w+):\/\/([^\/#]+)\/([^\/#]+)(\/[^#]+)?(#.+)?".r
   val BrokenKnownProvider = raw"(\w+):\/\/.+".r
   val ScalaDocPatten = raw"€\{(TPL_NAME|TPL_OWNER|FILE_PATH|FILE_EXT|FILE_LINE|FILE_PATH_EXT)\}".r
   val SupportedScalaDocPatternReplacements = Map(
@@ -63,6 +63,12 @@ class SourceLinkParser(revision: Option[String]) extends ArgParser[SourceLink]:
 
   def parse(string: String): Either[String, SourceLink] =
     val res = string match
+      case scaladocSetting if ScalaDocPatten.findFirstIn(scaladocSetting).nonEmpty =>
+        val all = ScalaDocPatten.findAllIn(scaladocSetting)
+        val (supported, unsupported) = all.partition(SupportedScalaDocPatternReplacements.contains)
+        if unsupported.nonEmpty then Left(s"Unsupported patterns from scaladoc format are used: ${unsupported.mkString(" ")}")
+        else Right(TemplateSourceLink(supported.foldLeft(string)((template, pattern) =>
+          template.replace(pattern, SupportedScalaDocPatternReplacements(pattern)))))
       case KnownProvider(name, organization, repo, rawRevision, rawSubPath) =>
         val subPath = Option(rawSubPath).fold("")("/" + _.drop(1))
         val pathRev = Option(rawRevision).map(_.drop(1)).orElse(revision)
@@ -81,12 +87,6 @@ class SourceLinkParser(revision: Option[String]) extends ArgParser[SourceLink]:
             Left(s"'$other' is not a known provider, please provide full source path template.")
       case BrokenKnownProvider("gitlab" | "github") =>
         Left(s"Does not match known provider syntax: `<name>://organization/repository`")
-      case scaladocSetting if ScalaDocPatten.findFirstIn(scaladocSetting).nonEmpty =>
-        val all = ScalaDocPatten.findAllIn(scaladocSetting)
-        val (supported, unsupported) = all.partition(SupportedScalaDocPatternReplacements.contains)
-        if unsupported.nonEmpty then Left(s"Unsupported patterns from scaladoc format are used: ${unsupported.mkString(" ")}")
-        else Right(TemplateSourceLink(supported.foldLeft(string)((template, pattern) =>
-          template.replace(pattern, SupportedScalaDocPatternReplacements(pattern)))))
       case other =>
         Left("Does not match any implemented source link syntax")
     res match {

scaladoc/src/dotty/tools/scaladoc/api.scala

@@ -195,6 +195,8 @@ extension[T] (member: Member)
 
   def withKind(kind: Kind): Member = member.copy(kind = kind)
 
+  def withDocs(docs: Option[Comment]) = member.copy(docs = docs)
+
   def withNewMembers(newMembers: Seq[Member]): Member =
     member.copy(members = member.members ++ newMembers)
 

scaladoc/src/dotty/tools/scaladoc/tasty/SymOps.scala

@@ -271,7 +271,8 @@ class SymOpsWithLinkCache:
             val extLink = if externalLinkCache.contains(csym.associatedFile)
               then externalLinkCache(csym.associatedFile)
               else {
-                val calculatedLink = Option(csym.associatedFile).map(_.path).flatMap { path =>
+                def calculatePath(file: AbstractFile): String = file.underlyingSource.filter(_ != file).fold("")(f => calculatePath(f) + "/") + file.path
+                val calculatedLink = Option(csym.associatedFile).map(f => calculatePath(f)).flatMap { path =>
                   dctx.externalDocumentationLinks.find(_.originRegexes.exists(r => r.matches(path)))
                 }
                 externalLinkCache += (csym.associatedFile -> calculatedLink)

scaladoc/src/dotty/tools/scaladoc/tasty/TastyParser.scala

@@ -160,7 +160,8 @@ case class ScaladocTastyInspector()(using ctx: DocContext) extends DocTastyInspe
     all.groupBy(_._1).map { case (pckName, members) =>
       val (pcks, rest) = members.map(_._2).partition(_.kind == Kind.Package)
       val basePck = pcks.reduce( (p1, p2) =>
-        p1.withNewMembers(p2.members) // TODO add doc
+        val withNewMembers = p1.withNewMembers(p2.members)
+        if withNewMembers.docs.isEmpty then withNewMembers.withDocs(p2.docs) else withNewMembers
       )
       basePck.withMembers((basePck.members ++ rest).sortBy(_.name))
     }.toList -> rootDoc

scaladoc/test/dotty/tools/scaladoc/ExternalLocationProviderIntegrationTest.scala

@@ -45,6 +45,18 @@ class Scaladoc3ExternalLocationProviderIntegrationTest extends ExternalLocationP
   )
 )
 
+class Scaladoc2LegacyExternalLocationProviderIntegrationTest extends LegacyExternalLocationProviderIntegrationTest(
+  "externalScaladoc2",
+  List(".*scala.*#https://www.scala-lang.org/api/current/"),
+  List(
+    "https://www.scala-lang.org/api/current/scala/util/matching/Regex$$Match.html",
+    "https://www.scala-lang.org/api/current/scala/Predef$.html#String",
+    "https://www.scala-lang.org/api/current/scala/collection/immutable/Map.html",
+    "https://www.scala-lang.org/api/current/scala/collection/IterableOnceOps.html#addString(b:StringBuilder,start:String,sep:String,end:String):StringBuilder",
+    "https://www.scala-lang.org/api/current/scala/collection/IterableOnceOps.html#mkString(start:String,sep:String,end:String):String"
+  )
+)
+
 
 abstract class ExternalLocationProviderIntegrationTest(
   name: String,
@@ -85,3 +97,16 @@ abstract class ExternalLocationProviderIntegrationTest(
     }
   } :: Nil
 
+abstract class LegacyExternalLocationProviderIntegrationTest(
+  name: String,
+  mappings: Seq[String],
+  expectedLinks: Seq[String]
+) extends ExternalLocationProviderIntegrationTest(name, mappings, expectedLinks):
+
+  override def args = super.args.copy(
+      externalMappings = mappings.flatMap( s =>
+        ExternalDocLink.parseLegacy(s).fold(left => None, right => Some(right)
+      )
+    ).toList
+  )
+

scaladoc/test/dotty/tools/scaladoc/PackageDocumentationTest.scala

@@ -0,0 +1,23 @@
+package dotty.tools.scaladoc
+
+import org.junit.Assert._
+import com.vladsch.flexmark.util.{ast => mdu, sequence}
+import com.vladsch.flexmark.{ast => mda}
+import collection.JavaConverters._
+
+
+class PackageDocumentationTest extends ScaladocTest("packageobjdocs"):
+  override def runTest: Unit = withModule { module =>
+    module.members.values.find {
+      case member if member.kind == Kind.Package => true
+      case _ => false
+    }.flatMap(_.docs).map(_.body).fold(throw AssertionError("No package found or documentation is not present")) {
+      case node: mdu.ContentNode =>
+        val text = node.getDescendants().asScala.toList.map {
+          case node: mdu.ContentNode => node.getContentChars().toString()
+          case _ => ""
+        }.mkString
+        assertTrue("Documentation for package is incorrect", text.contains("It's a test"))
+      case _ => throw AssertionError("No documentation node found in package docs")
+    }
+  }

scaladoc/test/dotty/tools/scaladoc/source-links/SourceLinksTest.scala

@@ -33,6 +33,7 @@ class SourceLinkTest:
     Seq(
       "github://lampepfl/dotty",
       "gitlab://lampepfl/dotty",
+      "github://lampepfl/dotty/branch/withslash",
       "https://github.com/scala/scala/blob/2.13.x€{FILE_PATH_EXT}#€{FILE_LINE}"
     ).foreach{ template =>
       test(template)
@@ -45,12 +46,6 @@ class SourceLinkTest:
       val template = s"$provider://ala/ma"
       val res = SourceLinkParser(None).parse(template)
       assertTrue(s"Expected failure containing missing revision: $res", res.left.exists(_.contains("revision")))
-
-      Seq(s"$provider://ala/ma/", s"$provider://ala", s"$provider://ala/ma/develop/on/master").foreach { template =>
-        val res = SourceLinkParser(Some("develop")).parse(template)
-        assertTrue(s"Expected failure syntax info: $res", res.left.exists(_.contains("syntax")))
-      }
-
     }
 
 class SourceLinksTest:
@@ -121,7 +116,7 @@ class SourceLinksTest:
       ("project/Build.scala", 54, edit) -> "https://gitlab.com/lampepfl/dotty/-/edit/develop/project/Build.scala#L54",
     )
 
-    testLink(Seq("€{FILE_PATH}#€{FILE_LINE}"), Some("develop"))(
+    testLink(Seq("€{FILE_PATH}.scala#€{FILE_LINE}"), Some("develop"))(
       "project/Build.scala" -> "/project/Build.scala#",
       ("project/Build.scala", 54) -> "/project/Build.scala#54",
       ("project/Build.scala", edit) -> "/project/Build.scala#",
@@ -135,6 +130,20 @@ class SourceLinksTest:
       ("project/Build.scala", 54, edit) -> "https://github.com/scala/scala/blob/2.13.x/project/Build.scala#L54",
     )
 
+    testLink(Seq("https://github.com/scala/scala/blob/2.13.x€{FILE_PATH}.scala#L€{FILE_LINE}"), Some("develop"))(
+      "project/Build.scala" -> "https://github.com/scala/scala/blob/2.13.x/project/Build.scala#L",
+      ("project/Build.scala", 54) -> "https://github.com/scala/scala/blob/2.13.x/project/Build.scala#L54",
+      ("project/Build.scala", edit) -> "https://github.com/scala/scala/blob/2.13.x/project/Build.scala#L",
+      ("project/Build.scala", 54, edit) -> "https://github.com/scala/scala/blob/2.13.x/project/Build.scala#L54",
+    )
+
+    testLink(Seq("github://lampepfl/dotty/branch/withslash#src/lib"), None)(
+      "project/Build.scala" -> "https://github.com/lampepfl/dotty/blob/branch/withslash/src/lib/project/Build.scala",
+      ("project/Build.scala", 54) -> "https://github.com/lampepfl/dotty/blob/branch/withslash/src/lib/project/Build.scala#L54",
+      ("project/Build.scala", edit) -> "https://github.com/lampepfl/dotty/edit/branch/withslash/src/lib/project/Build.scala",
+      ("project/Build.scala", 54, edit) -> "https://github.com/lampepfl/dotty/edit/branch/withslash/src/lib/project/Build.scala#L54",
+    )
+
   @Test
   def testBasicPrefixedPaths =
     testLink(Seq("src=gitlab://lampepfl/dotty"), Some("develop"))(
@@ -148,7 +157,7 @@ class SourceLinksTest:
   @Test
   def prefixedPaths =
     testLink(Seq(
-     "src/generated=€{FILE_PATH}#€{FILE_LINE}",
+     "src/generated=€{FILE_PATH_EXT}#€{FILE_LINE}",
       "src=gitlab://lampepfl/dotty",
       "github://lampepfl/dotty"
       ), Some("develop"))(