From bca8718a167546ee27f32919b79cb5cd69d35203 Mon Sep 17 00:00:00 2001 From: Max Horn Date: Tue, 27 Jun 2023 18:25:36 +0200 Subject: [PATCH] Turn hash for GapObj into error ... instead of providing a trivial (constant) hash. This may break some trivial applications, but in return it helps us identify code that relies on the useless default hash, i.e., it helps us find performance bottlenecks. This is a breaking change so also increase the package version. --- CHANGES.md | 8 ++++++++ Project.toml | 2 +- src/adapter.jl | 7 ++++--- src/constructors.jl | 17 ----------------- test/basics.jl | 12 ++---------- test/constructors.jl | 2 +- test/conversion.jl | 6 +++--- 7 files changed, 19 insertions(+), 35 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 83f743118..27d7df533 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -1,5 +1,13 @@ # Changes in GAP.jl +## Version 0.10.0-DEV (released YYYY-MM-DD) + +- Changed `hash(::GapObj)` to throw an error (no general non-trivial hashing is + possible for general GAP objects) +- Remove support for conversion to `AbstractString` (it was not meaningful anyway, + and hopefully nobody used it; but if you did, just convert to `String` instead + for an identical outcome) + ## Version 0.9.8 (released 2023-09-11) - Allow GAP.Obj(x,true) for recursive conversion (#910. #925) diff --git a/Project.toml b/Project.toml index 4d5ef154f..28631f1cc 100644 --- a/Project.toml +++ b/Project.toml @@ -1,7 +1,7 @@ name = "GAP" uuid = "c863536a-3901-11e9-33e7-d5cd0df7b904" authors = ["Thomas Breuer ", "Sebastian Gutsche ", "Max Horn "] -version = "0.9.8" +version = "0.10.0-DEV" [deps] Artifacts = "56f22d72-fd6d-98f1-02f0-08ddc0907c33" diff --git a/src/adapter.jl b/src/adapter.jl index 1935a02d4..b315a2de2 100644 --- a/src/adapter.jl +++ b/src/adapter.jl @@ -361,11 +361,12 @@ for (left, right) in typecombinations end # Since we define the equality of GAP objects (see above), -# we must provide a safe `hash` method, otherwise `==` results may be wrong. +# we must provide a safe `hash` method, otherwise using GAP objects +# in dictionaries or `Set`s can lead to inconsistent results. # For example, `==` for `Set`s of GAP objects may erroneously return # `false` if the default `hash` is used. -Base.hash(::GapObj, h::UInt) = h -Base.hash(::FFE, h::UInt) = h +Base.hash(::GapObj, h::UInt) = error("hash for GAP objects is not implemented") +Base.hash(::FFE, h::UInt) = error("hash for GAP objects is not implemented") ### RNGs diff --git a/src/constructors.jl b/src/constructors.jl index ca75ebcd0..3a14c0d76 100644 --- a/src/constructors.jl +++ b/src/constructors.jl @@ -389,23 +389,6 @@ Set{Any} with 2 elements: Any[1] Any[2] ``` - -In the following examples, -the order in which the Julia output is shown may vary. - -# Examples -```jldoctest -julia> s = Set{Any}(GAP.Obj([[1], [2], [1]]; recursive=true); recursive=false); - -julia> s == Set{Any}([GAP.Obj([1]), GAP.Obj([2])]) -true - -julia> s = Set{Any}(GAP.Globals.SymmetricGroup(2); recursive=false); - -julia> s == Set{Any}([GAP.evalstr("()"), GAP.evalstr("(1,2)")]) -true - -``` """ Base.Set{T}(obj::GapObj; recursive::Bool = true) where {T} = gap_to_julia(Set{T}, obj; recursive) diff --git a/test/basics.jl b/test/basics.jl index 19fdcfb87..d660e5456 100644 --- a/test/basics.jl +++ b/test/basics.jl @@ -28,21 +28,13 @@ y = GAP.evalstr("[]") @test !(x === y) @test (x == y) - @test hash(x) == 0 + @test_throws ErrorException hash(x) x = GAP.evalstr("Z(2)") y = GAP.evalstr("Z(4)^3") @test !(x === y) @test (x == y) - @test hash(x) == 0 - - # The following would sometimes fail if no dedicated `hash` - # method would be available for GAP objects. - for i in 1:100 - x = Set{Any}([GAP.evalstr("()"), GAP.evalstr("(1,2)")]) - y = Set{Any}([GAP.evalstr("()"), GAP.evalstr("(1,2)")]) - @test (x == y) - end + @test_throws ErrorException hash(x) end @testset "globals" begin diff --git a/test/constructors.jl b/test/constructors.jl index 8ba7ff088..71f57ce85 100644 --- a/test/constructors.jl +++ b/test/constructors.jl @@ -149,7 +149,7 @@ @test (@inferred Set{Vector{Int}}(GAP.evalstr("[[1,2],[2,3,4]]"))) == Set([[1, 2], [2, 3, 4]]) @test (@inferred Set{String}(GAP.evalstr("[\"b\", \"a\", \"b\"]"))) == Set(["b", "a", "b"]) x = GAP.evalstr("SymmetricGroup(3)") - @test (@inferred Set{GAP.GapObj}(x)) == Set{GAP.GapObj}(GAP.Globals.AsSet(x)) + #@test (@inferred Set{GAP.GapObj}(x)) == Set{GAP.GapObj}(GAP.Globals.AsSet(x)) end @testset "Tuples" begin diff --git a/test/conversion.jl b/test/conversion.jl index a8f78f02d..6aa05a93b 100644 --- a/test/conversion.jl +++ b/test/conversion.jl @@ -173,12 +173,12 @@ x = GAP.evalstr("[ [ 1 ], [ 2 ], [ 1 ] ]") y = [GAP.evalstr("[ 1 ]"), GAP.evalstr("[ 2 ]")] @test (@inferred GAP.gap_to_julia(Set{Vector{Int}}, x)) == Set([[1], [2], [1]]) - @test @inferred GAP.gap_to_julia(Set{GAP.GapObj}, x, recursive = false) == Set(y) - @test @inferred GAP.gap_to_julia(Set{Any}, x, recursive = false) == Set(y) + #@test @inferred GAP.gap_to_julia(Set{GAP.GapObj}, x, recursive = false) == Set(y) + #@test @inferred GAP.gap_to_julia(Set{Any}, x, recursive = false) == Set(y) @test (@inferred GAP.gap_to_julia(Set{Any}, x)) == Set([[1], [2], [1]]) x = GAP.evalstr("[ Z(2), Z(3) ]") # a non-collection y = [GAP.evalstr("Z(2)"), GAP.evalstr("Z(3)")] - @test GAP.gap_to_julia(Set{GAP.FFE}, x) == Set(y) + #@test GAP.gap_to_julia(Set{GAP.FFE}, x) == Set(y) end @testset "Tuples" begin