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

Enable Aqua.jl's ambiguity check #936

Merged
merged 3 commits into from
Sep 22, 2023

Conversation

lgoettgens
Copy link
Member

@lgoettgens lgoettgens commented Sep 11, 2023

Parts of this have already been merged as #937.

  • 470bde7 removes (::Type{<:AbstractString})(::GapObj). This led to ambiguities with Test.GenericString, LazyString, Core.Compiler.LazyString. The only real removals here are for SubString and SubstitutionString as the other subtypes of AbstractString either have their specialized function or didn't work anyway due to ambiguities.
    It furthermore removes the unclear gap_to_julia(AbstractString, ...) (as it didn't specify what exactly to return). The implementation chose to return a simple String. I made that a bit more clear. Even the documentation says that one should put String as the first argument (cf.
    | `IsStringRep` | `String` | `Symbol`, `Vector{T}` |
    ).
  • 1291ab0 fixes all calls of it and the corresponding tests.

Due to the AbstractString stuff above, I would technically consider this breaking. (E.g. SubString(GAP.evalstr("\"foo\"")) used to work, but now needs to be SubString(String(GAP.evalstr("\"foo\""))).)

Once this is merged, GAP.jl fulfills all of Aqua.jl's tests! 🎉

@lgoettgens
Copy link
Member Author

There seems to be more behind this as initially expectes. I continue later.

@@ -133,15 +133,15 @@ gap_to_julia(::Type{T}, x::GapInt) where {T<:Integer} = T(x)
gap_to_julia(::Type{Rational{T}}, x::GapInt) where {T<:Integer} = Rational{T}(x)

## Floats
gap_to_julia(::Type{T}, obj::GapObj) where {T<:AbstractFloat} = T(obj)
gap_to_julia(::Type{T}, obj::GapObj) where {T<:AbstractFloat} = applicable(T, obj) ? T(obj) : T(Float64(obj))
Copy link
Member

Choose a reason for hiding this comment

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

I am really uncomfortable with using applicable here, IMHO this results in difficult to understand and handle code.

I think a more appropriate fix is to just do

Suggested change
gap_to_julia(::Type{T}, obj::GapObj) where {T<:AbstractFloat} = applicable(T, obj) ? T(obj) : T(Float64(obj))
gap_to_julia(::Type{Float64}, obj::GapObj) = Float64(obj)

and anyone who wants to convert to stuff to Float32 or BigFloat has to deal with it by themselves (e.g. for a GAP big int it's a bad idea to first convert to Float64 in that case).

Copy link
Member Author

Choose a reason for hiding this comment

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

After looking again through the Float stuff, it seems that it caused no ambiguities, and it is fundamentally different to the removed String stuff, as it does not use a magic convert. So this doesn't need to be changed at all.

Copy link
Member

Choose a reason for hiding this comment

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

OK, great! (Sorry, didn't see this comment before)

src/gap_to_julia.jl Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Sep 13, 2023

Codecov Report

Merging #936 (6e57e62) into master (b216185) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #936      +/-   ##
==========================================
- Coverage   75.64%   75.62%   -0.02%     
==========================================
  Files          51       51              
  Lines        4171     4169       -2     
==========================================
- Hits         3155     3153       -2     
  Misses       1016     1016              
Files Changed Coverage
src/constructors.jl ø
src/gap_to_julia.jl 100.00%

@lgoettgens lgoettgens marked this pull request as ready for review September 18, 2023 07:30
@fingolfin fingolfin merged commit 6bafdba into oscar-system:master Sep 22, 2023
14 checks passed
@lgoettgens lgoettgens deleted the lg/ambiguities branch September 22, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants