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

Update type mutability check in IO.jl #409

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mrufsvold
Copy link

Fix bug (#408) that causes loading from a bin directory to fail due to deprecated type mutability check.

Changes:

  • Update IO.jl to use ismutabletype()
  • Change Julia compat bound to 1.6

@mrufsvold mrufsvold changed the title Main Update type mutability check in IO.jl Feb 22, 2022
@jpsamaroo jpsamaroo linked an issue Feb 22, 2022 that may be closed by this pull request
@jpsamaroo jpsamaroo added the bug label Feb 22, 2022
@jpsamaroo jpsamaroo requested review from joshday and quinnj and removed request for joshday February 23, 2022 03:24
@jpsamaroo
Copy link
Collaborator

LGTM! I'd like another maintainer to confirm that the Julia 1.6 compat bound looks good before merging.

Project.toml Outdated
@@ -36,7 +36,7 @@ RecipesBase = "0.7,1"
StatsBase = "0.32,0.33"
TextParse = "0.9.1,1"
WeakRefStrings = "0.6"
julia = "1"
julia = "1.6"
Copy link
Collaborator

Choose a reason for hiding this comment

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

ismutabletype requires Julia 1.7

Copy link
Author

Choose a reason for hiding this comment

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

Does it make sense to define a mutability check function that checks the Julia version and runs a compatible version of the test?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The source of ismutabletype looks pretty simple. We just need something like:

ismutabletype = if VERSION < v"1.7"
    function ismutabletype(@nospecialize(t::Type))
        t = unwrap_unionall(t)
        return isa(t, DataType) && t.name.flags & 0x2 == 0x2
    end
else
    Base.ismutabletype
end

Copy link
Author

Choose a reason for hiding this comment

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

done

@mrufsvold
Copy link
Author

mrufsvold commented Mar 28, 2022

Yo, sorry I dropped off the face of the planet. I removed the 1.6 requirement from compat, and created the version check for 1.7.

I basically just copied your code except that I defined gave the new function the name _ismutabletype so that I did not get an error about redefining a constant.

I only have 1.7 installed on my system, so I didn't test this on earlier versions of Julia.

tagging @joshday and @jpsamaroo for visibility

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.

load("bin") broken by drop of DataType mutable field
4 participants