Change == to error by default (instead of returning false) when comparing elements for which no equality test has been implemented

[lgoettgens]

Closes #1800, resolves oscar-system/Oscar.jl#4107.

This includes only the snippet from #1800 (comment)

cc @joschmitt

[codecov]

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.15%. Comparing base (502745e) to head (2b2576c).
⚠️ Report is 412 commits behind head on master.

Files with missing lines	Patch %	Lines
src/fundamental_interface.jl	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1853      +/-   ##
==========================================
+ Coverage   88.14%   88.15%   +0.01%     
==========================================
  Files         119      126       +7     
  Lines       30019    32076    +2057     
==========================================
+ Hits        26460    28277    +1817     
- Misses       3559     3799     +240     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thofma]

Note that this does not address all of the problems, since it is still possible to compare elements with the same type but different parents.

[fingolfin]

I've now just added this, to get the tests to run; but this is not meant to preempt any discussion, i.e., we can still remove it again, but I wanted to at least know if with this the OscarCI etc. tests complete or (still) don't.

[fingolfin]

In triage nobody voiced objections to this plan, but @fieker correctly reminded that we probably want a function to do comparisons that never fails; my proposal for that is to use isequal for that (see also #1854). I guess that means such an isequal should also be defined here...

Or we define a new function for this.... isequal_nothrow ... dunno (if we define isequal to not throw, this has ramifications for use of e.g. RingElems as dicictionary keys which we may or may not like....)

(And matrices deserve special attention as well as their parents are not rings)

[lgoettgens]

I would prefer to have this in the same breaking release as #1854 (so not in 0.48)

[fingolfin]

This revealed what looks like another genuine bug in Oscar.jl: restrict_automorphism_group calls unique on objects for which we did not implement ==, I think those of type TorQuadModuleMap (CC @simonbrandhorst):

      From worker 3:	Orthogonal groups of ZZ-lattices: Error During Test at /home/oscarci-tester/ssd-data/ssd-runner-26/_work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Oscar/test/Groups/isometry_group.jl:1
      From worker 3:	  Got exception outside of a @test
      From worker 3:	  == is not implemented for the given types
      From worker 3:	  Stacktrace:
      From worker 3:	    [1] error(s::String)
      From worker 3:	      @ Base ./error.jl:35
      From worker 3:	    [2] ==
      From worker 3:	      @ ~/ssd-data/ssd-runner-26/_work/AbstractAlgebra.jl/AbstractAlgebra.jl/src/fundamental_interface.jl:9 [inlined]
      From worker 3:	    [3] isequal
      From worker 3:	      @ ./operators.jl:133 [inlined]
      From worker 3:	    [4] ht_keyindex2_shorthash!(h::Dict{TorQuadModuleMap, Nothing}, key::TorQuadModuleMap)
      From worker 3:	      @ Base ./dict.jl:312
      From worker 3:	    [5] in!
      From worker 3:	      @ ./set.jl:97 [inlined]
      From worker 3:	    [6] unique(itr::Vector{TorQuadModuleMap})
      From worker 3:	      @ Base ./set.jl:176
      From worker 3:	    [7] _unique_dims
      From worker 3:	      @ ./multidimensional.jl:1701 [inlined]
      From worker 3:	    [8] unique
      From worker 3:	      @ ./multidimensional.jl:1699 [inlined]
      From worker 3:	    [9] restrict_automorphism_group(G::AutomorphismGroup{TorQuadModule}, i::TorQuadModuleMap; check::Bool)
      From worker 3:	      @ Oscar ~/ssd-data/ssd-runner-26/_work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Oscar/src/Groups/abelian_aut.jl:576
      From worker 3:	   [10] restrict_automorphism_group
      From worker 3:	      @ ~/ssd-data/ssd-runner-26/_work/AbstractAlgebra.jl/AbstractAlgebra.jl/oscar-dev/Oscar/src/Groups/abelian_aut.jl:558 [inlined]

[fingolfin]

Now I wonder: should we maybe also define hash(::SetElem,::UInt) to throw an error?

[fingolfin]

Triage discussion: Let's try to define hash(::SetElem,::UInt) as raising an error in a separate PR, and see how "bad" the outcome is.

@lgoettgens points out that e.g. unique only uses hash over a certain threshold, so some problems might stay hidden and will only be revealed when a user runs into a certain situation.

My view is that in that case I'd still prefer an error over a potentially wrong result...