Update Aqua.jl to support piracy tests

[lgoettgens]

Cf. #883 (comment)

cc @fingolfin

[codecov]

Codecov Report

Merging #889 (b651f1e) into master (b9c5c67) will decrease coverage by 0.11%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
- Coverage   75.82%   75.72%   -0.11%     
==========================================
  Files          51       51              
  Lines        4158     4197      +39     
==========================================
+ Hits         3153     3178      +25     
- Misses       1005     1019      +14     

see 2 files with indirect coverage changes

[lgoettgens]

It seems strange to me that the errors only occur for some julia versions...
Do you have any idea, after working on the changes in Aqua?

[fingolfin]

Upon reflection, these are genuine type piracy reports, because GapObj is actually defined in GAP_jll not in GAP. So the real (?) bug seems to be that Aqua does not detect this in Julia 1.9, only in older Julia versions... (And I did not notice any problems so far because I run 1.9 on my computer by default).

Besides looking into that bug (?) in Aqua (?) (or Julia?) I wonder how to best address this rather special situation (GapObj is defined in GAP_jll purely for technical reasons, it "belongs" to GAP.jl for all practical purposes).

One hackish way would be to add a is_foreign(type,pkg) method which returns false for GapObj + GAP. Of course that'd be type piracy of its own. So cleaner would be to add a feature to Aqua that allows to override the "foreign" status for certain types. Alas, I am not sure it'd be worth the effort as long as GAP is the only package needing it

[lgoettgens]

Upon reflection, these are genuine type piracy reports, because GapObj is actually defined in GAP_jll not in GAP. So the real (?) bug seems to be that Aqua does not detect this in Julia 1.9, only in older Julia versions... (And I did not notice any problems so far because I run 1.9 on my computer by default).

Same for me. That should be investigated.

Besides looking into that bug (?) in Aqua (?) (or Julia?) I wonder how to best address this rather special situation (GapObj is defined in GAP_jll purely for technical reasons, it "belongs" to GAP.jl for all practical purposes).

This scenario is even mentioned in the julia docs (last paragraph of https://docs.julialang.org/en/v1/manual/style-guide/#Avoid-type-piracy).

One hackish way would be to add a is_foreign(type,pkg) method which returns false for GapObj + GAP. Of course that'd be type piracy of its own. So cleaner would be to add a feature to Aqua that allows to override the "foreign" status for certain types. Alas, I am not sure it'd be worth the effort as long as GAP is the only package needing it

I guess since even the julia docs declare exceptions to the type piracy rules, one it would widen the use-cases of Aqua by adding a kwarg with a list of types and functions, that should be handled as if they were not foreign.

[lgoettgens]

After researching a bit more on this, I found JuliaLang/julia#44661 (comment) and followed the hacks mentioned there.

GapObj has different parentmodules depending on the julia version. In particular, if GAP.use_jl_reinit_foreign_type() is true parentmodule(GapObj) === GAP, and parentmodule(GapObj) === GAP_jll otherwise. Only in the latter case, there are any instances of type piracy.

I would thus suggest to just exclude GapObj from piracy tests once JuliaTesting/Aqua.jl#140 is available. If sometime in the future the distinction using GAP.use_jl_reinit_foreign_type() is dropped, we can remove it from the test exclusion again.

[fingolfin]

GAP.use_jl_reinit_foreign_type is only available (and necessary) in Julia >= 1.9. Until we drop support for older Julia versions, it'll stay around.

Of course now Aqua.jl 0.6.4 is out, so we could just use its new treat_as_own feature?

[lgoettgens]

Of course now Aqua.jl 0.6.4 is out, so we could just use its new treat_as_own feature?

Unfortunately, it is broken due to a dump typo by me... Please see JuliaTesting/Aqua.jl#153 for a fix. As soon as there is a release with this fix, this PR should succeed.