Skip to content

Fixes for nightly #176

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 6, 2025
Merged

Fixes for nightly #176

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3f7a07
Fixes for nightly
serenity4 May 31, 2025
be6499d
Refactor kwarg extraction logic
serenity4 Jun 1, 2025
08fc5d9
Apply suggestions from code review
mortenpi Jun 6, 2025
27c92b4
Update Project.toml
mortenpi Jun 6, 2025
bcb5b8d
Update src/utilities.jl
mortenpi Jun 6, 2025
a66676c
Update src/utilities.jl
mortenpi Jun 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/utilities.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,10 +452,10 @@ kws = keywords(f, first(methods(f)))
```
"""
function keywords(func, m::Method)
table = methods(func).mt
# table is a MethodTable object. For some reason, the :kwsorter field is not always
# defined. An undefined kwsorter seems to imply that there are no methods in the
# MethodTable with keyword arguments.
table::Core.MethodTable = VERSION ≥ v"1.13.0-DEV.647" ? Core.GlobalMethods : methods(func).mt
# For some reason, the :kwsorter field is not always defined.
# An undefined kwsorter seems to imply that there are no methods
# in the MethodTable with keyword arguments.
if !(Base.fieldindex(Core.MethodTable, :kwsorter, false) > 0) || isdefined(table, :kwsorter)
Copy link
Member

@mortenpi mortenpi Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that removing the fieldindex part is safe? I am not sure why it's here in the first place.

Copy link
Contributor Author

@serenity4 serenity4 Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure either. If isdefined(table, :kwsorter) returns true, fieldindex should have returned a strictly positive index anyways.

Perhaps this is a remnant of a time where we used to only check statically for the type (with fieldindex), and later we wanted to check for instances and added the isdefined check, without removing the former.

Copy link
Member

@mortenpi mortenpi Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it got added very intentionally in https://github.com/JuliaDocs/DocStringExtensions.jl/pull/137/files But the theory is that it's not relevant for <= 1.4?

Copy link
Member

@mortenpi mortenpi Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It got added here for "1.0 support".

#137 (comment)

cc @MichaelHatherly -- any memories from 3 years ago? 😄

Copy link
Member

@mortenpi mortenpi Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just thinking a bit more: I think Base.fieldindex(Core.MethodTable, :kwsorter, false) > 0 is always true (specifically, Base.fieldindex(Core.MethodTable, :kwsorter, false) == 5) on < 1.4. So I think #176 (comment) just has no effect in the refactored code. But doesn't hurt either.

# Fetching method keywords stolen from base/replutil.jl:572-576 (commit 3b45cdc9aab0):
kwargs = VERSION < v"1.4.0-DEV.215" ? Base.kwarg_decl(m, typeof(table.kwsorter)) : Base.kwarg_decl(m)
Expand Down
12 changes: 7 additions & 5 deletions test/tests.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,15 +40,15 @@ end
# Its signature is kwarg_decl(m::Method, kwtype::DataType). The second argument
# should be the type of the kwsorter from the corresponding MethodTable.
@test isa(methods(M.j_1), Base.MethodList)
@test isdefined(methods(M.j_1), :mt)
local mt = methods(M.j_1).mt
get_mt(func) = VERSION ≥ v"1.13.0-DEV.647" ? Core.GlobalMethods : methods(func).mt
local mt = get_mt(M.j_1)
@test isa(mt, Core.MethodTable)
if Base.fieldindex(Core.MethodTable, :kwsorter, false) > 0
@test isdefined(mt, :kwsorter)
end
# .kwsorter is not always defined -- namely, it seems when none of the methods
# have keyword arguments:
@test isdefined(methods(M.f).mt, :kwsorter) === false
@test isdefined(get_mt(M.f), :kwsorter) === false
# M.j_1 has two methods. Fetch the single argument one..
local m = which(M.j_1, (Any,))
@test isa(m, Method)
Expand All @@ -61,7 +61,7 @@ end
# that does not have any arguments
m = which(M.j_1, (Any,Any)) # fetch the no-keyword method
if VERSION < v"1.4.0-DEV.215"
@test Base.kwarg_decl(m, typeof(methods(M.j_1).mt.kwsorter)) == Tuple{}()
@test Base.kwarg_decl(m, typeof(get_mt(M.j_1).kwsorter)) == Tuple{}()
else
@test Base.kwarg_decl(m) == []
end
Expand All @@ -83,7 +83,9 @@ end
DSE.format(IMPORTS, buf, doc)
str = String(take!(buf))
@test occursin("\n - `Base`\n", str)
@test occursin("\n - `Core`\n", str)
if VERSION < v"1.13-DEV"
@test occursin("\n - `Core`\n", str)
end

# Module exports.
DSE.format(EXPORTS, buf, doc)
Expand Down
Loading