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.

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

Piracy fix for kwcall #171

Merged
merged 2 commits into from
Sep 5, 2023
Merged

Piracy fix for kwcall #171

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afc5558
Piracy fix for kwcall
lgoettgens Sep 4, 2023
9ac8a95
Fix for older julias
lgoettgens Sep 5, 2023
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
4 changes: 3 additions & 1 deletion src/piracy.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
module Piracy

using ..Aqua: is_kwcall

if VERSION >= v"1.6-"
using Test: is_in_mods
else
Expand Down Expand Up @@ -163,7 +165,7 @@ function is_pirate(meth::Method; treat_as_own = Union{Function,Type}[])
signature = Base.unwrap_unionall(meth.sig)

function_type_index = 1
if signature.parameters[1] === typeof(Core.kwcall)
if is_kwcall(signature)
# kwcall is a special case, since it is not a real function
# but a wrapper around a function, the third parameter is the original
# function, its positional arguments follow.
Expand Down
19 changes: 19 additions & 0 deletions src/utils.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,3 +157,22 @@ function format_diff(
$text_b
"""
end

function is_kwcall(signature::DataType)
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would make sense to use https://github.com/FluxML/MacroTools.jl or https://github.com/invenia/ExprTools.jl instead of rolling this on our own?

julia> ex = :(
                  function Base.f(x::T, y::T; z::Bool=false) where T
                      x + y
                  end
              );

julia> ExprTools.splitdef(ex)
Dict{Symbol, Any} with 6 entries:
  :args        => Any[:(x::T), :(y::T)]
  :body        => quote…
  :kwargs      => Any[:($(Expr(:kw, :(z::Bool), false)))]
  :head        => :function
  :whereparams => Any[:T]
  :name        => :(Base.f)

julia> MacroTools.splitdef(ex)
Dict{Symbol, Any} with 5 entries:
  :name        => :(Base.f)
  :args        => Any[:(x::T), :(y::T)]
  :kwargs      => Any[:($(Expr(:kw, :(z::Bool), false)))]
  :body        => quote…
  :whereparams => (:T,)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, but of course that's for expressions, not methods/signatures, so it might not be helpful.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, these ones unfortunately do not offer what we need here. I couldn't find any good way to test this in other packages. Most just ignore old julia versions, where Core.kwcall doesn't exist, or use some regex with the function name of the kwsorter. The first is not an option here (unless we drop support for julia-pre-1.9, or have different semantics for the tests depending on the julia version). The second option relies on weirder internals, where we won't get good error messages in future changes (as regexes and strings just tell "no", not fail with some type error etc.). Thus, I think the current proposal here is the best we can currently do. I already added a try-catch with some additional debug information for people to open issues here.

@static if VERSION < v"1.9"
try
return length(signature.parameters) >= 3 &&
signature <: Tuple{Function,Any,Any,Vararg} &&
(
signature.parameters[3] <: Type ||
isconcretetype(signature.parameters[3])
) &&
signature.parameters[1] === Core.kwftype(signature.parameters[3])
catch err
@warn "Please open an issue on JuliaTesting/Aqua.jl for \"is_kwcall\" and the following data:" signature err
return false
end
else
return signature.parameters[1] === typeof(Core.kwcall)
end
end