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

objects of type BigFloat cannot be finalized because they are not mutable #55965

Open
lgoettgens opened this issue Oct 2, 2024 · 5 comments
Open
Assignees
Labels
bignums BigInt and BigFloat invalid Indicates that an issue or pull request is no longer relevant

Comments

@lgoettgens
Copy link
Contributor

This error occurs in our nightly CI at https://github.com/Nemocas/AbstractAlgebra.jl since #55906 is merged.

Apparently, this PR changed ismutable(BigFloat(2)) from true to false.
I would have expected for a change like that a "minor change" or even a "breaking" label, and a pkgeval before merge.

@vtjnash
Copy link
Member

vtjnash commented Oct 3, 2024

I am not quite certain what you'd expect here. The Number types are all documented as being immutable, and now #55906 aligns the reflection query to be consistent with that

@vtjnash vtjnash added the invalid Indicates that an issue or pull request is no longer relevant label Oct 3, 2024
@lgoettgens
Copy link
Contributor Author

The test that got changed in https://github.com/JuliaLang/julia/pull/55906/files#diff-d550de483dd02fa6687b5042154db6ac1133df4890fb1cf92220e51ec83ebfe2 also expected BigFloat to be mutable. The change (changing the example to BigInt) will possibly break as well, once BigInt is changed to be actually mutable as well.

Furthermore, the test was originally about #26939, as issue explicitly about BigFloat, so I don't know if this test change in this way is actually sensible here.

What I consider the breaking change here is that the following piece of code used to work but no longer does. It doesn't use any internals. (Note that the WeakKeyDict docstring doesn't restrict the allowed key types at all)

julia> d26939 = WeakKeyDict()
WeakKeyDict{Any, Any}()

julia> d26939[big"1.0" + 1.1] = 1
ERROR: objects of type BigFloat cannot be finalized because they are not mutable

@nsajko
Copy link
Contributor

nsajko commented Oct 4, 2024

I think that's just how WeakKeyDict works, its doc string says that it's based on non-owning/weak references. See an example independent of BigFloat which fails, too:

struct S end
d = WeakKeyDict()
d[S()] = 1

So the key must be a mutable struct instance. And it's always been like that, at least up to v1.8.

@KristofferC
Copy link
Member

I think that's just how WeakKeyDict works, it's doc string says that it's based on non-owning/weak references.

I think everyone agrees on that. The discussion is about if making a mutable struct immutable should be considered "breaking" since code that used to work (putting the object into a weak key dict) now errors.

@vtjnash
Copy link
Member

vtjnash commented Oct 4, 2024

Unfortunately that test hadn't worked correctly since 2644f79 in 2018, so this was a necessary fix to make the test itself test what it was supposed to test (that finalizer ordering is correct with respect to WeakRefs)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bignums BigInt and BigFloat invalid Indicates that an issue or pull request is no longer relevant
Projects
None yet
Development

No branches or pull requests

4 participants