Fix missing doc check for methods from other packages (#1861)

CHANGELOG.md

@@ -20,6 +20,7 @@
   - KaTeX has been updated from `v0.13.11` to `v0.13.24`.
 
 * ![Bugfix][badge-bugfix] When including docstrings for an alias, Documenter now correctly tries to include the exactly matching docstring first, before checking for signature subtypes. ([#1842][github-1842])
+* ![Bugfix][badge-bugfix] When checking for missing docstrings, Documenter now correctly handles docstrings for methods that extend bindings from other modules that have not been imported into the current module. ([#1695][github-1695], [#1857][github-1857], [#1861][github-1861])
 
 ## Version `v0.27.19`
 
@@ -1020,6 +1021,7 @@
 [github-1689]: https://github.com/JuliaDocs/Documenter.jl/pull/1689
 [github-1691]: https://github.com/JuliaDocs/Documenter.jl/pull/1691
 [github-1693]: https://github.com/JuliaDocs/Documenter.jl/issues/1693
+[github-1695]: https://github.com/JuliaDocs/Documenter.jl/issues/1695
 [github-1696]: https://github.com/JuliaDocs/Documenter.jl/pull/1696
 [github-1698]: https://github.com/JuliaDocs/Documenter.jl/issues/1698
 [github-1699]: https://github.com/JuliaDocs/Documenter.jl/pull/1699
@@ -1079,6 +1081,8 @@
 [github-1842]: https://github.com/JuliaDocs/Documenter.jl/pull/1842
 [github-1844]: https://github.com/JuliaDocs/Documenter.jl/pull/1844
 [github-1846]: https://github.com/JuliaDocs/Documenter.jl/pull/1846
+[github-1857]: https://github.com/JuliaDocs/Documenter.jl/issues/1857
+[github-1861]: https://github.com/JuliaDocs/Documenter.jl/pull/1861
 <!-- end of issue link definitions -->
 
 [julia-38079]: https://github.com/JuliaLang/julia/issues/38079

src/DocChecks.jl

@@ -60,9 +60,11 @@ function missingdocs(doc::Documents.Document)
                 println(b, "    $binding", sig ≡ Union{} ? "" : " :: $sig")
             end
         end
-        println(b, """\n
-            These are docstrings in the checked modules (configured with the modules keyword)
-            that are not included in @docs or @autodocs blocks.""")
+        println(b)
+        print(b, """
+        These are docstrings in the checked modules (configured with the modules keyword)
+        that are not included in @docs or @autodocs blocks.
+        """)
         @docerror(doc, :missing_docs, String(take!(b)))
     end
 end
@@ -76,12 +78,19 @@ function allbindings(checkdocs::Symbol, mods)
 end
 
 function allbindings(checkdocs::Symbol, mod::Module, out = Dict{Utilities.Binding, Set{Type}}())
-    for (obj, doc) in meta(mod)
-        isa(obj, IdDict{Any,Any}) && continue
-        name = nameof(obj)
-        isexported = Base.isexported(mod, name)
+    for (binding, doc) in meta(mod)
+        # The keys of the docs meta dictonary should always be Docs.Binding objects in
+        # practice. However, the key type is Any, so it is theoretically possible that
+        # some non-binding metadata gets added to the dict. So on the off-chance that has
+        # happened, we simply ignore those entries.
+        isa(binding, Docs.Binding) || continue
+        # We only consider a name exported only if it actually exists in the module, either
+        # by virtue of being defined there, or if it has been brought into the scope with
+        # import/using.
+        name = nameof(binding)
+        isexported = (binding == Utilities.Binding(mod, name)) && Base.isexported(mod, name)
         if checkdocs === :all || (isexported && checkdocs === :exports)
-            out[Utilities.Binding(mod, name)] = Set(sigs(doc))
+            out[binding] = Set(sigs(doc))
         end
     end
     out

test/docchecks.jl

@@ -3,40 +3,139 @@ module DocCheckTests
 using Test
 
 using Markdown
-using Documenter.DocChecks: linkcheck
+using Documenter.DocChecks: linkcheck, allbindings
 using Documenter.Documents
 
+# The following modules set up a few docstrings for allbindings tests
+module Dep1
+    "dep1_private"
+    dep1_private() = nothing
+    "dep1_private_2"
+    dep1_private_2() = nothing
+    "dep1_exported"
+    dep1_exported() = nothing
+    export dep1_exported
+    "dep1_reexported"
+    dep1_reexported() = nothing
+    # test for shadowing exports
+    "bar"
+    bar() = nothing
+end
+module Dep2
+    # This module extends a function from Dep1, but creates a new local binding
+    # for it, to reproduce the case reported in
+    # https://github.com/JuliaDocs/Documenter.jl/issues/1695
+    using ..Dep1: Dep1
+    const dep1_private = Dep1.dep1_private
+    "Dep2: dep1_private"
+    dep1_private(::Int) = nothing
+end
+module TestModule
+    # Standard case of attaching a docstring to a local binding
+    "local_binding"
+    local_binding() = nothing
+    "local_binding"
+    local_binding_exported() = nothing
+    export local_binding_exported
+
+    # These extend functions from another module (package). The bindings should
+    # all be Dep1.XXX, rather than TestModule.XXX
+    using ..Dep1
+    "TestModule : dep1_private"
+    Dep1.dep1_private(::Any) = nothing
+    "TestModule : dep1_exported"
+    Dep1.dep1_exported(::Any) = nothing
+    import ..Dep1: dep1_private_2
+    "TestModule : dep1_private_2"
+    dep1_private_2(::Any) = nothing
+    # Re-export of a binding from another module
+    import ..Dep1: dep1_reexported
+    "TestModule : dep1_reexported"
+    dep1_reexported(::Any) = nothing
+    export dep1_reexported
+    # This also extends Dep1.dep1_private, but the docstring should get attached
+    # to the Dep2.dep1_private binding because of the assignment.
+    using ..Dep2: Dep2
+    "TestModuleDep2: Dep2.dep1_private"
+    Dep2.dep1_private(::Any, ::Any) = nothing
+
+    # This tests the case where there is an undocumented but exported local function
+    # that shares the name with a documented function from another module, potentially
+    # confusing the isexported check.
+    const bar = nothing
+    export bar
+    "Dep1.bar"
+    Dep1.bar(::Any) = nothing
+end
+
 @testset "DocChecks" begin
     @testset "linkcheck" begin
-        src = md"""
-            [HTTP (HTTP/1.1) success](http://www.google.com)
-            [HTTPS (HTTP/2) success](https://www.google.com)
-            [FTP success](ftp://ftp.iana.org/tz/data/etcetera)
-            [FTP (no proto) success](ftp.iana.org/tz/data/etcetera)
-            [Redirect success](google.com)
-            [HEAD fail GET success](https://codecov.io/gh/invenia/LibPQ.jl)
-            """
-
-        Documents.walk(Dict{Symbol, Any}(), src) do block
-            doc = Documents.Document(; linkcheck=true, linkcheck_timeout=20)
-            result = linkcheck(block, doc)
-            @test doc.internal.errors == Set{Symbol}()
-            result
-        end
+        if haskey(ENV, "DOCUMENTER_TEST_LINKCHECK")
+            src = md"""
+                [HTTP (HTTP/1.1) success](http://www.google.com)
+                [HTTPS (HTTP/2) success](https://www.google.com)
+                [FTP success](ftp://ftp.iana.org/tz/data/etcetera)
+                [FTP (no proto) success](ftp.iana.org/tz/data/etcetera)
+                [Redirect success](google.com)
+                [HEAD fail GET success](https://codecov.io/gh/invenia/LibPQ.jl)
+                """
+
+            Documents.walk(Dict{Symbol, Any}(), src) do block
+                doc = Documents.Document(; linkcheck=true, linkcheck_timeout=20)
+                result = linkcheck(block, doc)
+                @test doc.internal.errors == Set{Symbol}()
+                result
+            end
+
+            src = Markdown.parse("[FILE failure](file://$(@__FILE__))")
+            doc = Documents.Document(; linkcheck=true)
+            Documents.walk(Dict{Symbol, Any}(), src) do block
+                linkcheck(block, doc)
+            end
+            @test doc.internal.errors == Set{Symbol}([:linkcheck])
 
-        src = Markdown.parse("[FILE failure](file://$(@__FILE__))")
-        doc = Documents.Document(; linkcheck=true)
-        Documents.walk(Dict{Symbol, Any}(), src) do block
-            linkcheck(block, doc)
+            src = Markdown.parse("[Timeout](http://httpbin.org/delay/3)")
+            doc = Documents.Document(; linkcheck=true, linkcheck_timeout=0.1)
+            Documents.walk(Dict{Symbol, Any}(), src) do block
+                linkcheck(block, doc)
+            end
+            @test doc.internal.errors == Set{Symbol}([:linkcheck])
+        else
+            @info "DOCUMENTER_TEST_LINKCHECK not set, skipping online linkcheck tests."
+            @test_broken false
         end
-        @test doc.internal.errors == Set{Symbol}([:linkcheck])
+    end
+
+    @testset "allbindings" begin
+        # dep1_private has not been imported into TestModule, so the binding does not
+        # resolve to the Deps1 binding.
+        @test Docs.Binding(TestModule, :dep1_private) != Docs.Binding(Dep1, :dep1_private)
+        @test Docs.Binding(TestModule, :dep1_private) != Docs.Binding(Dep2, :dep1_private)
+        # These three bindings are imported into the TestModule scope, so the Binding objects
+        # automatically resolve to the Dep1.X bindings.
+        @test Docs.Binding(TestModule, :dep1_private_2) == Docs.Binding(Dep1, :dep1_private_2)
+        @test Docs.Binding(TestModule, :dep1_exported) == Docs.Binding(Dep1, :dep1_exported)
+        @test Docs.Binding(TestModule, :dep1_reexported) == Docs.Binding(Dep1, :dep1_reexported)
+        # There is a TestModule.bar, but it's not the same as Dep1.bar, but the latter has
+        # a docstring in TestModule.
+        @test Docs.Binding(TestModule, :bar) != Docs.Binding(Dep1, :bar)
 
-        src = Markdown.parse("[Timeout](http://httpbin.org/delay/3)")
-        doc = Documents.Document(; linkcheck=true, linkcheck_timeout=0.1)
-        Documents.walk(Dict{Symbol, Any}(), src) do block
-            linkcheck(block, doc)
+        let bindings = allbindings(:all, TestModule)
+            @test length(bindings) == 8
+            @test Docs.Binding(TestModule, :local_binding) in keys(bindings)
+            @test Docs.Binding(TestModule, :local_binding_exported) in keys(bindings)
+            @test Docs.Binding(Dep1, :dep1_private) in keys(bindings)
+            @test Docs.Binding(Dep1, :dep1_private_2) in keys(bindings)
+            @test Docs.Binding(Dep1, :dep1_exported) in keys(bindings)
+            @test Docs.Binding(Dep1, :dep1_reexported) in keys(bindings)
+            @test Docs.Binding(Dep1, :bar) in keys(bindings)
+            @test Docs.Binding(Dep2, :dep1_private) in keys(bindings)
+        end
+        let bindings = allbindings(:exports, TestModule)
+            @test length(bindings) == 2
+            @test Docs.Binding(TestModule, :local_binding_exported) in keys(bindings)
+            @test Docs.Binding(Dep1, :dep1_reexported) in keys(bindings)
         end
-        @test doc.internal.errors == Set{Symbol}([:linkcheck])
     end
 end
 

test/missingdocs/make.jl

@@ -1,3 +1,8 @@
+isdefined(@__MODULE__, :TestUtilities) || include("../TestUtilities.jl")
+module MissingDocsTests
+using Test, ..TestUtilities
+using Documenter
+
 module MissingDocs
     export f
 
@@ -8,15 +13,27 @@ module MissingDocs
     g(x) = x
 end
 
-using Documenter
+@testset "missing docs" begin
+    for sym in [:none, :exports, :all]
+        @quietly @test makedocs(
+            root = dirname(@__FILE__),
+            source = joinpath("src", string(sym)),
+            build = joinpath("build", string(sym)),
+            modules = MissingDocs,
+            checkdocs = sym,
+            sitename = "MissingDocs Checks",
+        ) === nothing
+    end
 
-for sym in [:none, :exports]
-    makedocs(
+    @quietly @test_throws ErrorException makedocs(
         root = dirname(@__FILE__),
-        source = joinpath("src", string(sym)),
-        build = joinpath("build", string(sym)),
+        source = joinpath("src", "none"),
+        build = joinpath("build", "error"),
         modules = MissingDocs,
-        checkdocs = sym,
+        checkdocs = :all,
+        strict = true,
         sitename = "MissingDocs Checks",
     )
 end
+
+end

test/missingdocs/src/all/index.md

@@ -0,0 +1,6 @@
+# MissingDocs All
+
+```@docs
+Main.MissingDocs.f
+Main.MissingDocs.g
+```

test/runtests.jl

@@ -9,7 +9,7 @@ include("TestUtilities.jl"); using .TestUtilities
 
     # Test missing docs
     @info "Building missingdocs/make.jl"
-    @quietly include("missingdocs/make.jl")
+    include("missingdocs/make.jl")
 
     # Error reporting.
     @info "Building errors/make.jl"
@@ -21,11 +21,7 @@ include("TestUtilities.jl"); using .TestUtilities
     include("markdown2.jl")
 
     # DocChecks tests
-    if haskey(ENV, "DOCUMENTER_TEST_LINKCHECK")
-        include("docchecks.jl")
-    else
-        @info "DOCUMENTER_TEST_LINKCHECK not set, skipping online linkcheck tests."
-    end
+    include("docchecks.jl")
 
     # NavNode tests.
     include("navnode.jl")