Don't render broken links to symbols excluded from documentation (#4448)

dokka-integration-tests/gradle/src/testExternalProjectKotlinxDatetime/kotlin/DatetimeGradleIntegrationTest.kt

@@ -68,7 +68,7 @@ class DatetimeGradleIntegrationTest : AbstractGradleIntegrationTest(), TestOutpu
         projectOutputLocation.allHtmlFiles().forEach { file ->
             assertContainsNoErrorClass(file)
             assertNoUnresolvedLinks(file)
-            // assertNoHrefToMissingLocalFileOrDirectory(file)
+            assertNoHrefToMissingLocalFileOrDirectory(file)
             assertNoEmptyLinks(file)
             assertNoEmptySpans(file)
         }

dokka-integration-tests/gradle/src/testExternalProjectKotlinxIo/kotlin/IoGradleIntegrationTest.kt

@@ -68,7 +68,7 @@ class IoGradleIntegrationTest : AbstractGradleIntegrationTest(), TestOutputCopie
         projectOutputLocation.allHtmlFiles().forEach { file ->
             assertContainsNoErrorClass(file)
             assertNoUnresolvedLinks(file)
-            // assertNoHrefToMissingLocalFileOrDirectory(file)
+            assertNoHrefToMissingLocalFileOrDirectory(file)
             assertNoEmptyLinks(file)
             assertNoEmptySpans(file)
         }

dokka-subprojects/plugin-all-modules-page/src/main/kotlin/org/jetbrains/dokka/allModulesPage/ExternalModuleLinkResolver.kt

@@ -72,21 +72,14 @@ public class DefaultExternalModuleLinkResolver(
         val resolvedLinks = elps.mapNotNull { locProviderWithMdl ->
             locProviderWithMdl.locationProvider.resolve(dri)?.let { it to locProviderWithMdl.moduleDescription }
         }
-        val validLink = resolvedLinks.firstOrNull {
-            // this is a temporary hack for the first case (short-term solution) of #3368:
-            // a situation where 2 (or more) local modules have the same package name.
-            // TODO #3368 currently, it does not work for external modules with the same package name
-            if (resolvedLinks.size > 1) {
-                val moduleDescription = it.second
-                val resolvedLinkWithoutRelativePath = it.first.removePrefix(
-                   "file:/" + moduleDescription.relativePathToOutputDirectory.toRelativeOutputDir()
-                        .toString()
-                )
-
-                val partialModuleOutput = moduleDescription.sourceOutputDirectory
-                val absolutePath = File(partialModuleOutput.absolutePath + resolvedLinkWithoutRelativePath)
-                absolutePath.isFile
-            } else true
+        // A link is resolvable from a module's package-list as long as the package is documented,
+        // but the symbol itself might be excluded from the documentation (suppressed, internal,
+        // deprecated when `skipDeprecated` is on, a private constructor, etc.), so the target page
+        // is never generated. Verifying that the page actually exists prevents rendering broken
+        // links to non-existent pages (#4448). It also disambiguates between local modules that
+        // share a package name (#3368).
+        val validLink = resolvedLinks.firstOrNull { (link, moduleDescription) ->
+            pointsToExistingPage(link, moduleDescription)
         }?.first ?: return null
 
         // relativization [fileContext] path over output path (or `fileContext.relativeTo(outputPath)`)
@@ -105,6 +98,22 @@ public class DefaultExternalModuleLinkResolver(
             .joinToString("/") + validLink.removePrefix("file:")
     }
 
+    /**
+     * Checks that the file backing a [resolvedLink] (produced by an [ExternalLocationProvider]
+     * from a module's package-list) actually exists in the module's partial output.
+     *
+     * The anchor (`#...`) is dropped first because member links resolve to `index.html#anchor`,
+     * and only the `.html` page on disk should be checked for existence.
+     */
+    private fun pointsToExistingPage(resolvedLink: String, moduleDescription: DokkaModuleDescription): Boolean {
+        val relativePathPrefix = "file:/" + moduleDescription.relativePathToOutputDirectory.toRelativeOutputDir().toString()
+        val resolvedLinkWithoutRelativePath = resolvedLink
+            .substringBefore('#')
+            .removePrefix(relativePathPrefix)
+        val absolutePath = File(moduleDescription.sourceOutputDirectory.absolutePath + resolvedLinkWithoutRelativePath)
+        return absolutePath.isFile
+    }
+
     override fun resolveLinkToModuleIndex(moduleName: String): String? =
         context.configuration.modules.firstOrNull { it.name == moduleName }
             ?.let { module ->

dokka-subprojects/plugin-all-modules-page/src/main/kotlin/org/jetbrains/dokka/allModulesPage/ResolveLinkCommandHandler.kt

@@ -15,7 +15,7 @@ import org.jsoup.nodes.Element
 import org.jsoup.parser.Tag
 import java.io.File
 
-public class ResolveLinkCommandHandler(context: DokkaContext) : CommandHandler {
+public class ResolveLinkCommandHandler(private val context: DokkaContext) : CommandHandler {
 
     private val externalModuleLinkResolver =
         context.plugin<AllModulesPagePlugin>().querySingle { externalModuleLinkResolver }
@@ -24,6 +24,10 @@ public class ResolveLinkCommandHandler(context: DokkaContext) : CommandHandler {
         command as ResolveLinkCommand
         val link = externalModuleLinkResolver.resolve(command.dri, output)
         if (link == null) {
+            context.logger.warn(
+                "Couldn't resolve link to `${command.dri}`: the target is not part of the documentation " +
+                    "(it may be suppressed, internal, deprecated, or an undocumented external symbol)"
+            )
             val children = body.childNodes().toList()
             val attributes = Attributes().apply {
                 put("data-unresolved-link", command.dri.toString())

dokka-subprojects/plugin-all-modules-page/src/test/kotlin/org/jetbrains/dokka/allModulesPage/templates/ResolveLinkCommandResolutionTest.kt

@@ -41,6 +41,44 @@ class ResolveLinkCommandResolutionTest : MultiModuleAbstractTest() {
         }
 
         val contentFile = setup(outputDirectory, link)
+        // The page the link points to must actually exist, otherwise it is treated as a
+        // broken link to a symbol excluded from the documentation (see #4448).
+        outputDirectory.resolve("module2/package2/-sample/index.html")
+            .also { assertTrue(it.parentFile.mkdirs()) }
+            .writeText("<html></html>")
+        val configuration = createConfiguration(outputDirectory)
+
+        testFromData(configuration, useOutputLocationFromConfig = true) {
+            finishProcessingSubmodules = {
+                assertHtmlEqualsIgnoringWhitespace(expected, contentFile.readText())
+            }
+        }
+    }
+
+    @Test
+    fun `should not resolve link to a symbol excluded from the documentation`(@TempDir outputDirectory: File) {
+        // The package is documented (present in the package-list), but the symbol itself is
+        // excluded, so its page is never generated. The link must not be rendered as a broken
+        // link to a non-existent page (#4448).
+        val testedDri = DRI(
+            packageName = "package2",
+            classNames = "Excluded",
+        )
+        val link = createHTML().templateCommand(ResolveLinkCommand(testedDri)) {
+            span {
+                +"Excluded"
+            }
+        }
+
+        val expected = createHTML().span {
+            attributes["data-unresolved-link"] = testedDri.toString()
+            span {
+                +"Excluded"
+            }
+        }
+
+        val contentFile = setup(outputDirectory, link)
+        // Note: no page file is created for `package2/-excluded`, mimicking an excluded symbol.
         val configuration = createConfiguration(outputDirectory)
 
         testFromData(configuration, useOutputLocationFromConfig = true) {

dokka-subprojects/plugin-all-modules-page/src/test/kotlin/org/jetbrains/dokka/allModulesPage/templates/ResolveLinkGfmCommandResolutionTest.kt

@@ -61,6 +61,11 @@ class ResolveLinkGfmCommandResolutionTest : MultiModuleAbstractTest() {
 
         indexMd.writeText(indexMdContent)
         packageList.writeText(mockedPackageListForPackages(RecognizedLinkFormat.DokkaGFM, "package2"))
+        // The page the link points to must actually exist, otherwise it is treated as a
+        // broken link to a symbol excluded from the documentation (see #4448).
+        innerModule2.resolve("package2/-sample/index.md")
+            .also { assertTrue(it.parentFile.mkdirs()) }
+            .writeText("")
 
         testFromData(
             configuration,

dokka-subprojects/plugin-base/src/main/kotlin/org/jetbrains/dokka/base/renderers/html/HtmlRenderer.kt

@@ -823,10 +823,16 @@ public open class HtmlRenderer(
                 buildText(node.children, pageContext, sourceSetRestriction)
             }
         } ?: if (isPartial) {
+            // The link may still be resolved against another module during multi-module
+            // assembly, so defer the decision (and any warning) to ResolveLinkCommandHandler.
             templateCommand(ResolveLinkCommand(node.address)) {
                 buildText(node.children, pageContext, sourceSetRestriction)
             }
         } else {
+            context.logger.warn(
+                "Couldn't resolve link to `${node.address}`: the target is not part of the documentation " +
+                    "(it may be suppressed, internal, deprecated, or an undocumented external symbol)"
+            )
             span {
                 attributes["data-unresolved-link"] = node.address.toString().htmlEscape()
                 buildText(node.children, pageContext, sourceSetRestriction)

dokka-subprojects/plugin-base/src/test/kotlin/markdown/LinkToExcludedSymbolTest.kt

@@ -0,0 +1,79 @@
+/*
+ * Copyright 2014-2024 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license.
+ */
+
+package markdown
+
+import org.jetbrains.dokka.DokkaException
+import org.jetbrains.dokka.base.testApi.testRunner.BaseAbstractTest
+import utils.TestOutputWriterPlugin
+import kotlin.test.Test
+import kotlin.test.assertFailsWith
+import kotlin.test.assertFalse
+import kotlin.test.assertTrue
+
+/**
+ * A KDoc link to a symbol that is excluded from the documentation (suppressed, `internal`,
+ * deprecated when `skipDeprecated` is on, etc.) resolves to a valid `DRI` during analysis,
+ * but the symbol has no page. Such a link must not be rendered as a working `<a>` link to a
+ * non-existent page, and Dokka should warn the developer about it (#4448).
+ */
+class LinkToExcludedSymbolTest : BaseAbstractTest() {
+
+    private val configuration = dokkaConfiguration {
+        sourceSets {
+            sourceSet {
+                sourceRoots = listOf("src/")
+            }
+        }
+    }
+
+    private val source = """
+        |/src/main/kotlin/test/Test.kt
+        |package test
+        |
+        |internal class Hidden
+        |
+        |/**
+        | * See [Hidden] for details.
+        | */
+        |public class Public
+        """.trimMargin()
+
+    @Test
+    fun `link to excluded symbol is rendered as unresolved span and warns`() {
+        val writerPlugin = TestOutputWriterPlugin()
+        testInline(source, configuration, pluginOverrides = listOf(writerPlugin)) {
+            renderingStage = { _, _ ->
+                val content = writerPlugin.writer.contents.getValue("root/test/-public/index.html")
+                assertTrue(
+                    content.contains("""data-unresolved-link="test/Hidden///PointingToDeclaration/""""),
+                    "Expected an unresolved-link span for the excluded symbol, but got:\n$content"
+                )
+                assertFalse(
+                    content.contains(Regex("""<a [^>]*href="[^"]*Hidden""")),
+                    "A link to an excluded symbol must not be rendered as a working <a> link:\n$content"
+                )
+                assertTrue(
+                    logger.warnMessages.any { it.contains("test/Hidden") && it.contains("Couldn't resolve link") },
+                    "Expected a warning about the unresolved link, but got: ${logger.warnMessages}"
+                )
+            }
+        }
+    }
+
+    @Test
+    fun `link to excluded symbol fails the build when failOnWarning is enabled`() {
+        val failOnWarningConfiguration = dokkaConfiguration {
+            failOnWarning = true
+            sourceSets {
+                sourceSet {
+                    sourceRoots = listOf("src/")
+                }
+            }
+        }
+        assertFailsWith<DokkaException> {
+            testInline(source, failOnWarningConfiguration) {}
+        }
+    }
+}