Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation references are generated for symbols which are not documented #478

Closed
Octogonapus opened this issue Apr 3, 2024 · 12 comments

Comments

@Octogonapus
Copy link
Contributor

Octogonapus commented Apr 3, 2024

Clang.jl generates doc refs which reference symbols that aren't documented. This causes cross reference errors when using Documenter.jl. Ideally, Clang.jl would understand when a symbol is documented, and link to it only if it is documented.

I wrote a test case you can use to check this:

@testset "generating docs from doxygen comments with a missing reference omits that reference" begin
    mktemp() do path, io
        # Generate the bindings
        options = Dict("general" => Dict{String, Any}("output_file_path" => path,
                                                      "extract_c_comment_style" => "doxygen"))
        args = get_default_args()
        push!(args, "-fparse-all-comments")
        ctx = create_context([joinpath(@__DIR__, "include/documentation_missing_ref.h")], args, options)
        build!(ctx)

        # Load into a temporary module to avoid polluting the global namespace
        m = Module()
        Base.include(m, path)
        println(read(path, String))

        # Do some sanity checks on the docstring
        docstring = string(@doc m.foo)
        docstring_has = occursin(docstring)
        @show docstring # TODO assert there is no doc ref to SOME_CONST
    end
end

which needs this file documentation_missing_ref.h:

#define SOME_CONST 1

/**
 * uses SOME_CONST.
 */
void foo() {
}

So that it's clear, the problem is that this code is generated:

"""
    foo()

uses [`SOME_CONST`](@ref).
"""
function foo()
    ccall((:foo, libxxx), Cvoid, ())
end

You can see it includes a reference to SOME_CONST, but that symbol is not documented.

I'm just not sure how to change the implementation to fix this. Maybe another dict is needed for format_inline to track which symbols have been documented? But then you will have an ordering problem.

@JamesWrigley
Copy link
Member

I would suggest using the documentation callback feature to explicitly add docstrings, there's an example of using it for that in the Clang.jl generator: https://github.com/JuliaInterop/Clang.jl/blob/master/gen/generator.jl#L43

@Octogonapus
Copy link
Contributor Author

I see, thanks.

@Octogonapus
Copy link
Contributor Author

Octogonapus commented Apr 5, 2024

Actually it turns out there are some nodes which don't have code generated but are still cross-referenced. Here is an example node:

node = ExprNode{Clang.Generators.MacroFunctionLike, Clang.CLMacroDefinition}(:AWS_STATIC_STRING_FROM_LITERAL, Clang.Generators.MacroFunctionLike(), CLCursor (Clang.CLMacroDefinition) AWS_STATIC_STRING_FROM_LITERAL, Expr[], Expr[], Int64[])

But it's referenced by other generated docs:

"""
    aws_string_destroy_secure(str)

Zeroes out the data bytes of string and then deallocates the memory. Not safe to run on a string created with [`AWS_STATIC_STRING_FROM_LITERAL`](@ref).

### Prototype

void aws_string_destroy_secure(struct aws_string *str);

"""
function aws_string_destroy_secure(str)
    ccall((:aws_string_destroy_secure, libaws_c_common), Cvoid, (Ptr{aws_string},), str)
end

With macro_mode = "basic" there is no code generated. So I tried setting macro_mode = "aggressive" but I get an error:

[ Info: Emit Julia expressions...
ERROR: LoadError: ParseError:
# Error @ none:1:56
RETURN_ARG_COUNT((_1_),(_2_),(_3_),(_4_),(_5_),(count),...)
#                                                      └─┘ ── invalid identifier
Stacktrace:
 [1] #parse#3
   @ ./meta.jl:244 [inlined]
 [2] parse
   @ ./meta.jl:236 [inlined]
 [3] parse(str::String; filename::String, raise::Bool, depwarn::Bool)
   @ Base.Meta ./meta.jl:278
 [4] parse(str::String)
   @ Base.Meta ./meta.jl:276
 [5] macro_emit!(dag::ExprDAG, node::ExprNode{Clang.Generators.MacroFunctionLike, Clang.CLMacroDefinition}, options::Dict{String, Any})
   @ Clang.Generators ~/.julia/packages/Clang/kNxXb/src/generator/macro.jl:381
 [6] (::CodegenMacro)(dag::ExprDAG, options::Dict{String, Any})
   @ Clang.Generators ~/.julia/packages/Clang/kNxXb/src/generator/passes.jl:1198
 [7] build!(ctx::Context, stage::Clang.Generators.BuildStage)
   @ Clang.Generators ~/.julia/packages/Clang/kNxXb/src/generator/context.jl:176
 [8] top-level scope
   @ ~/Documents/code/LibAwsCommon.jl/gen/generator.jl:83
in expression starting at /home/salmon/Documents/code/LibAwsCommon.jl/gen/generator.jl:52

I don't really want to generate code for this macro, either. I just want to avoid the dangling cross-reference.

For context I am working on this PR JuliaServices/LibAwsCommon.jl#8

Do you have any advice for this case?

@Octogonapus Octogonapus reopened this Apr 5, 2024
@JamesWrigley
Copy link
Member

I see 🤔 I think the simplest option would be to use latest master of Clang.jl which will call get_docs() on all nodes (like in the Clang.jl generator), and then do a replace() on the missing symbols in the doc argument that are referenced to turn e.g. [`AWS_STATIC_STRING_FROM_LITERAL`](@ref) into `AWS_STATIC_STRING_FROM_LITERAL`.

@JamesWrigley
Copy link
Member

@Gnimuc, maybe it's worth make a new release? That would also add support for Julia 1.11. If so then I can make a PR to update the changelog beforehand.

@Gnimuc
Copy link
Member

Gnimuc commented Apr 6, 2024

The latest release has some JLL-platform issues on Windows. I'm considering dropping 1.10- in Clang.jl-v0.18.

@JamesWrigley
Copy link
Member

You mean all releases from 1.10 onwards, including 1.11? That would be kinda unfortunate 😅 What're the JLL issues?

@Gnimuc
Copy link
Member

Gnimuc commented Apr 6, 2024

https://github.com/JuliaInterop/Clang.jl/blob/master/Artifacts.toml

This file is copied from BBB upstream and is somewhat outdated. The hash of certain Windows platform tarballs doesn't match anymore.

@Gnimuc
Copy link
Member

Gnimuc commented Apr 6, 2024

however, the mismatch issue doesn't happen on CI... I did reproduce it on a clean Windows machine.

@Gnimuc
Copy link
Member

Gnimuc commented Apr 20, 2024

anything else need to be done here?

@Octogonapus
Copy link
Contributor Author

I will try doing #478 (comment)

@Octogonapus
Copy link
Contributor Author

That is working for me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants