Avoid method ambiguities in TypedefMutualRef's #458

JamesWrigley · 2023-12-21T19:31:28Z

emit!() for TypedefMutualRef will create some specializations of Base.unsafe_convert(), and previously they didn't declare a type for the Ref/Ptr value being passed to them, which would cause a method ambiguity with Base (detected by Aqua.jl).

The problem is that the typedef will of course be declared in the C header before the struct it references, so the Base.unsafe_convert() specializations couldn't use the actual struct type in their signature because it won't have been created yet. This commit fixes that with the notion of a partially emitted node that contains premature exprs that haven't been emitted yet. Now emit!() for TypedefMutualRef will record the Base.unsafe_convert() methods as premature exprs and mark itself as a partially emitted node. Then later, emit!() for the StructDefinition will look for any partially emitted nodes that are marked for it and emit their premature expressions itself.

Before:

mutable struct __JL_foo_struct
end

function Base.unsafe_load(x::Ptr{__JL_foo_struct})
    unsafe_load(Ptr{foo_struct}(x))
end

function Base.getproperty(x::Ptr{__JL_foo_struct}, f::Symbol)
    getproperty(Ptr{foo_struct}(x), f)
end

function Base.setproperty!(x::Ptr{__JL_foo_struct}, f::Symbol, v)
    setproperty!(Ptr{foo_struct}(x), f, v)
end

Base.unsafe_convert(::Type{Ptr{__JL_foo_struct}}, x::Ref) = Base.unsafe_convert(Ptr{__JL_foo_struct}, Base.unsafe_convert(Ptr{foo_struct}, x))

Base.unsafe_convert(::Type{Ptr{__JL_foo_struct}}, x::Ptr) = Ptr{__JL_foo_struct}(x)

const foo = Ptr{__JL_foo_struct}

struct foo_struct
    bar::foo
end

After:

mutable struct __JL_foo_struct
end

function Base.unsafe_load(x::Ptr{__JL_foo_struct})
    unsafe_load(Ptr{foo_struct}(x))
end

function Base.getproperty(x::Ptr{__JL_foo_struct}, f::Symbol)
    getproperty(Ptr{foo_struct}(x), f)
end

function Base.setproperty!(x::Ptr{__JL_foo_struct}, f::Symbol, v)
    setproperty!(Ptr{foo_struct}(x), f, v)
end

const foo = Ptr{__JL_foo_struct}

struct foo_struct
    bar::foo
end
Base.unsafe_convert(::Type{Ptr{__JL_foo_struct}}, x::Ref{foo_struct}) = Base.unsafe_convert(Ptr{__JL_foo_struct}, Base.unsafe_convert(Ptr{foo_struct}, x))

Base.unsafe_convert(::Type{Ptr{__JL_foo_struct}}, x::Ptr{foo_struct}) = Ptr{__JL_foo_struct}(x)

I'm not sure why there's no newline after the struct definition 🤔

`emit!()` for `TypedefMutualRef` will create some specializations of `Base.unsafe_convert()`, and previously they didn't declare a type for the `Ref`/`Ptr` value being passed to them, which would cause a method ambiguity with Base (detected by Aqua.jl). The problem is that the typedef will of course be declared in the C header before the struct it references, so the `Base.unsafe_convert()` specializations couldn't use the actual struct type in their signature because it won't have been created yet. This commit fixes that with the notion of a *partially emitted node* that contains *premature exprs* that haven't been emitted yet. Now `emit!()` for `TypedefMutualRef` will record the `Base.unsafe_convert()` methods as premature exprs and mark itself as a partially emitted node. Then later, `emit!()` for the `StructDefinition` will look for any partially emitted nodes that are marked for it and emit their premature expressions itself.

JamesWrigley · 2023-12-21T19:44:49Z

I'll try fixing the docs, but I have no idea what's going on with the Windows CI 👀 AFAICT it seems unrelated?

Useful if some codebase is missing docstrings for a few definitions.

It looks like this also automatically the version of Clang 15 used.

JamesWrigley · 2023-12-21T22:58:00Z

Well, that was a bit of a rabbit hole 🙃 TL;DR:

One reason the docs weren't building is because of missing docstrings, so I added support for using callback_documentation as a fallback even when extract_c_comment_style is set (5c78b0a).
Then I regenerated the bindings in 25acb38, but my system seems to have pulled in a newer version of clang 15? Not sure how important that is, if you like I can remove all the non-docstring hunks from the commit.
And finally, fixed the remaining issues with the docs (bd2fc8a).

JamesWrigley · 2023-12-21T23:33:11Z

Hmph, now I know why the windows tests are failing... 7zip was recently moved to a different directory, which is used by BinDeps.jl, which is used by CMake.jl, which is used by our tests: JuliaLang/julia#48931
That was backported to 1.9, which is why the Julia 1 tests are failing too and not just nightly. Relevant issue: JuliaPackaging/CMake.jl#23

CMake.jl is currently broken on 1.9 because the location of 7z.exe changed: - JuliaPackaging/CMake.jl#23 - JuliaLang/julia#48931 - JuliaInterop#458 (comment)

JamesWrigley · 2023-12-22T00:31:25Z

Ok, got there in the end 🎉 Main fix was switching to CMake_jll in 31876f0, and then some other minor fixes in 3afee34 and 61f1c48.

Gnimuc · 2023-12-23T04:58:05Z

lib/15/LibClang.jl

@@ -6937,7 +7112,7 @@ end

 const CINDEX_VERSION_MAJOR = 0

-const CINDEX_VERSION_MINOR = 62
+const CINDEX_VERSION_MINOR = 63


although these low-level APIs are not exported by Clang.jl, but I think we should bump Clang.jl's minor version as well.

Sure, fixed in 423820b.

Gnimuc · 2023-12-23T05:05:13Z

src/type.jl

@@ -70,7 +70,7 @@ getPointeeType(t::CLType)::CLType = clang_getPointeeType(t)
    getTypeDeclaration(t::CXType) -> CXCursor
    getTypeDeclaration(t::CLType) -> CLCursor
 Return the cursor for the declaration of the given type. To get the type of the cursor,
-see [`type`](@ref). Wrapper for libclang's [`clang_getTypeDeclaration`](@ref).
+see `typeof`. Wrapper for libclang's [`clang_getTypeDeclaration`](@ref).


Suggested change

see `typeof`. Wrapper for libclang's [`clang_getTypeDeclaration`](@ref).

see [`getCursorType`](@ref). Wrapper for libclang's [`clang_getTypeDeclaration`](@ref).

Oops 😅 Fixed in 49abae7.

Gnimuc · 2023-12-23T05:08:40Z

Thanks for fixing this! I'll give it a test and update the test-suite: https://github.com/Gnimuc/GeneratorScripts

Note that :missing_docs is now warn-only because it was complaining about everything in Clang.Generators, which we don't want/need to document. I didn't have any luck ignoring those with a `Filter` either.

This was polluting the `readdir()` results and causing the BB build to wrongly be marked as failed and fall back to the native build method with `build_libbitfield_native()`. Also added a full backtrace to the warning logs when build errors do occur in either functions.

CMake.jl is currently broken on 1.9 because the location of 7z.exe changed: - JuliaPackaging/CMake.jl#23 - JuliaLang/julia#48931 - JuliaInterop#458 (comment)

This seems to be required on Windows. Also cleaned up the whitespace.

CMake.jl is currently broken on 1.9 because the location of 7z.exe changed: - JuliaPackaging/CMake.jl#23 - JuliaLang/julia#48931 - #458 (comment)

JuliaInterop/Clang.jl#458

JamesWrigley added 2 commits December 21, 2023 23:40

Add support for calling callback_documentation as a fallback

5c78b0a

Useful if some codebase is missing docstrings for a few definitions.

Regenerate the libclang bindings with a default docstring callback

25acb38

It looks like this also automatically the version of Clang 15 used.

Gnimuc reviewed Dec 23, 2023

View reviewed changes

JamesWrigley added 5 commits December 23, 2023 12:10

Get the docs building

49abae7

Note that :missing_docs is now warn-only because it was complaining about everything in Clang.Generators, which we don't want/need to document. I didn't have any luck ignoring those with a `Filter` either.

Use CMake_jll instead of CMake.jl

c7e31d4

CMake.jl is currently broken on 1.9 because the location of 7z.exe changed: - JuliaPackaging/CMake.jl#23 - JuliaLang/julia#48931 - JuliaInterop#458 (comment)

Include missing header in test header

9ee5f43

This seems to be required on Windows. Also cleaned up the whitespace.

Bump the version number because of the new libclang bindings

423820b

JamesWrigley force-pushed the method-ambiguity branch from 61f1c48 to 423820b Compare December 23, 2023 11:10

Gnimuc merged commit 8b698dc into JuliaInterop:master Dec 24, 2023
11 checks passed

Gnimuc pushed a commit that referenced this pull request Dec 24, 2023

Use CMake_jll instead of CMake.jl

45ba45a

CMake.jl is currently broken on 1.9 because the location of 7z.exe changed: - JuliaPackaging/CMake.jl#23 - JuliaLang/julia#48931 - #458 (comment)

Gnimuc added a commit to Gnimuc/GeneratorScripts that referenced this pull request Dec 24, 2023

Test against Clang 0.17.x

99fb1b4

JuliaInterop/Clang.jl#458

JamesWrigley deleted the method-ambiguity branch December 24, 2023 12:45

JamesWrigley mentioned this pull request Dec 26, 2023

Remove deprecated use of JLL do-method #459

Merged

JamesWrigley mentioned this pull request Jan 30, 2024

Bump Clang_jll's version #465

Merged

JamesWrigley mentioned this pull request Mar 6, 2024

Documentation test fixes #473

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid method ambiguities in TypedefMutualRef's #458

Avoid method ambiguities in TypedefMutualRef's #458

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 22, 2023

Gnimuc Dec 23, 2023

JamesWrigley Dec 23, 2023

Gnimuc Dec 23, 2023

JamesWrigley Dec 23, 2023

Gnimuc commented Dec 23, 2023

	see `typeof`. Wrapper for libclang's [`clang_getTypeDeclaration`](@ref).
	see [`getCursorType`](@ref). Wrapper for libclang's [`clang_getTypeDeclaration`](@ref).

Avoid method ambiguities in TypedefMutualRef's #458

Avoid method ambiguities in TypedefMutualRef's #458

Conversation

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 21, 2023

JamesWrigley commented Dec 22, 2023

Gnimuc Dec 23, 2023

Choose a reason for hiding this comment

JamesWrigley Dec 23, 2023

Choose a reason for hiding this comment

Gnimuc Dec 23, 2023

Choose a reason for hiding this comment

JamesWrigley Dec 23, 2023

Choose a reason for hiding this comment

Gnimuc commented Dec 23, 2023