Skip to content

Commit

Permalink
Improve type stability in Artifacts code (JuliaLang#52759)
Browse files Browse the repository at this point in the history
This was in an attempt to fix
JuliaLang#52711 which ultimately proved
unsuccessful. This might however still be useful in other scenarios.

```
using JET
 report_opt(Tuple{typeof(Artifacts._artifact_str), Module, String, Base.SubString{String}, String, Base.Dict{String, Any}, Base.SHA1, Base.BinaryPlatforms.Platform, Any}; target_modules=[Artifacts])
```

is quite a bit cleaner with this.
  • Loading branch information
KristofferC committed Jan 8, 2024
1 parent b7c24ed commit cc156d9
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 40 deletions.
5 changes: 3 additions & 2 deletions base/parse.jl
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,9 @@ function tryparse(::Type{T}, s::AbstractString; base::Union{Nothing,Integer} = n
end

function parse(::Type{T}, s::AbstractString; base::Union{Nothing,Integer} = nothing) where {T<:Integer}
convert(T, tryparse_internal(T, s, firstindex(s), lastindex(s),
base===nothing ? 0 : check_valid_base(base), true))
v = tryparse_internal(T, s, firstindex(s), lastindex(s), base===nothing ? 0 : check_valid_base(base), true)
v === nothing && error("should not happoen")
convert(T, v)
end
tryparse(::Type{Union{}}, slurp...; kwargs...) = error("cannot parse a value as Union{}")

Expand Down
87 changes: 49 additions & 38 deletions stdlib/Artifacts/src/Artifacts.jl
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,8 @@ function parse_mapping(mapping::String, name::String, override_file::String)
end
return mapping
end
function parse_mapping(mapping::Dict, name::String, override_file::String)
return Dict(k => parse_mapping(v, name, override_file) for (k, v) in mapping)
function parse_mapping(mapping::Dict{String, Any}, name::String, override_file::String)
return Dict{String, Any}(k => parse_mapping(v, name, override_file) for (k, v) in mapping)
end
# Fallthrough for invalid Overrides.toml files
parse_mapping(mapping, name::String, override_file::String) = nothing
Expand Down Expand Up @@ -96,7 +96,7 @@ overriding to another artifact by its content-hash.
const ARTIFACT_OVERRIDES = Ref{Union{Dict{Symbol,Any},Nothing}}(nothing)
function load_overrides(;force::Bool = false)::Dict{Symbol, Any}
if ARTIFACT_OVERRIDES[] !== nothing && !force
return ARTIFACT_OVERRIDES[]
return ARTIFACT_OVERRIDES[]::Dict{Symbol,Any}
end

# We organize our artifact location overrides into two camps:
Expand All @@ -106,13 +106,8 @@ function load_overrides(;force::Bool = false)::Dict{Symbol, Any}
# Overrides per UUID/bound name are intercepted upon Artifacts.toml load, and new
# entries within the "hash" overrides are generated on-the-fly. Thus, all redirects
# mechanistically happen through the "hash" overrides.
overrides = Dict{Symbol,Any}(
# Overrides by UUID
:UUID => Dict{Base.UUID,Dict{String,Union{String,SHA1}}}(),

# Overrides by hash
:hash => Dict{SHA1,Union{String,SHA1}}(),
)
overrides_uuid = Dict{Base.UUID,Dict{String,Union{String,SHA1}}}()
overrides_hash = Dict{SHA1,Union{String,SHA1}}()

for override_file in reverse(artifacts_dirs("Overrides.toml"))
!isfile(override_file) && continue
Expand All @@ -131,20 +126,19 @@ function load_overrides(;force::Bool = false)::Dict{Symbol, Any}
# Next, determine if this is a hash override or a UUID/name override
if isa(mapping, String) || isa(mapping, SHA1)
# if this mapping is a direct mapping (e.g. a String), store it as a hash override
local hash_str
hash = tryparse(Base.SHA1, k)
if hash === nothing
@error("Invalid override in '$(override_file)': Invalid SHA1 hash '$(k)'")
continue
end

# If this mapping is the empty string, un-override it
if mapping == ""
delete!(overrides[:hash], hash)
if mapping isa String && isempty(mapping)
delete!(overrides_hash, hash)
else
overrides[:hash][hash] = mapping
overrides_hash[hash] = mapping
end
elseif isa(mapping, Dict)
elseif isa(mapping, Dict{String, Any})
# Convert `k` into a uuid
uuid = tryparse(Base.UUID, k)
if uuid === nothing
Expand All @@ -153,19 +147,18 @@ function load_overrides(;force::Bool = false)::Dict{Symbol, Any}
end

# If this mapping is itself a dict, store it as a set of UUID/artifact name overrides
ovruuid = overrides[:UUID]::Dict{Base.UUID,Dict{String,Union{String,SHA1}}}
if !haskey(ovruuid, uuid)
ovruuid[uuid] = Dict{String,Union{String,SHA1}}()
if !haskey(overrides_uuid, uuid)
overrides_uuid[uuid] = Dict{String,Union{String,SHA1}}()
end

# For each name in the mapping, update appropriately
for (name, override_value) in mapping
# If the mapping for this name is the empty string, un-override it
if override_value == ""
delete!(ovruuid[uuid], name)
if override_value isa String && isempty(override_value)
delete!(overrides_uuid[uuid], name)
else
# Otherwise, store it!
ovruuid[uuid][name] = override_value
overrides_uuid[uuid][name] = override_value::Union{Base.SHA1, String}
end
end
else
Expand All @@ -174,6 +167,14 @@ function load_overrides(;force::Bool = false)::Dict{Symbol, Any}
end
end

overrides = Dict{Symbol,Any}(
# Overrides by UUID
:UUID => overrides_uuid,

# Overrides by hash
:hash => overrides_hash
)

ARTIFACT_OVERRIDES[] = overrides
return overrides
end
Expand All @@ -190,11 +191,13 @@ Query the loaded `<DEPOT>/artifacts/Overrides.toml` settings for artifacts that
redirected to a particular path or another content-hash.
"""
function query_override(hash::SHA1; overrides::Dict{Symbol,Any} = load_overrides())
return map_override_path(get(overrides[:hash], hash, nothing))
overrides_hash = overrides[:hash]::Dict{SHA1,Union{String,SHA1}}
return map_override_path(get(overrides_hash, hash, nothing))
end
function query_override(pkg::Base.UUID, artifact_name::String; overrides::Dict{Symbol,Any} = load_overrides())
if haskey(overrides[:UUID], pkg)
return map_override_path(get(overrides[:UUID][pkg], artifact_name, nothing))
overrides_uuid = overrides[:UUID]::Dict{Base.UUID,Dict{String,Union{String,SHA1}}}
if haskey(overrides_uuid, pkg)
return map_override_path(get(overrides_uuid[pkg], artifact_name, nothing))
end
return nothing
end
Expand Down Expand Up @@ -284,7 +287,7 @@ function unpack_platform(entry::Dict{String,Any}, name::String,
delete!(tags, "os")
delete!(tags, "arch")
delete!(tags, "git-tree-sha1")
return Platform(entry["arch"], entry["os"], tags)
return Platform(entry["arch"]::String, entry["os"]::String, tags)
end

function pack_platform!(meta::Dict, p::AbstractPlatform)
Expand Down Expand Up @@ -326,8 +329,11 @@ function process_overrides(artifact_dict::Dict, pkg_uuid::Base.UUID)
# Insert just-in-time hash overrides by looking up the names of anything we need to
# override for this UUID, and inserting new overrides for those hashes.
overrides = load_overrides()
if haskey(overrides[:UUID], pkg_uuid)
pkg_overrides = overrides[:UUID][pkg_uuid]::Dict{String, <:Any}
overrides_uuid = overrides[:UUID]::Dict{Base.UUID,Dict{String,Union{String,SHA1}}}
overrides_hash = overrides[:hash]::Dict{SHA1,Union{String,SHA1}}

if haskey(overrides_uuid, pkg_uuid)
pkg_overrides = overrides_uuid[pkg_uuid]::Dict{String, <:Any}

for name in keys(artifact_dict)
# Skip names that we're not overriding
Expand All @@ -336,14 +342,16 @@ function process_overrides(artifact_dict::Dict, pkg_uuid::Base.UUID)
end

# If we've got a platform-specific friend, override all hashes:
if isa(artifact_dict[name], Array)
for entry in artifact_dict[name]
hash = SHA1(entry["git-tree-sha1"])
overrides[:hash][hash] = overrides[:UUID][pkg_uuid][name]
artifact_dict_name = artifact_dict[name]
if isa(artifact_dict_name, Array)
for entry in artifact_dict_name
entry = entry::Dict{String,Any}
hash = SHA1(entry["git-tree-sha1"]::String)
overrides_hash[hash] = overrides_uuid[pkg_uuid][name]
end
elseif isa(artifact_dict[name], Dict)
hash = SHA1(artifact_dict[name]["git-tree-sha1"])
overrides[:hash][hash] = overrides[:UUID][pkg_uuid][name]
elseif isa(artifact_dict_name, Dict{String, Any})
hash = SHA1(artifact_dict_name["git-tree-sha1"]::String)
overrides_hash[hash] = overrides_uuid[pkg_uuid][name]
end
end
end
Expand Down Expand Up @@ -388,7 +396,7 @@ function artifact_meta(name::String, artifact_dict::Dict, artifacts_toml::String
if isa(meta, Vector)
dl_dict = Dict{AbstractPlatform,Dict{String,Any}}()
for x in meta
x::Dict{String}
x = x::Dict{String, Any}
dl_dict[unpack_platform(x, name, artifacts_toml)] = x
end
meta = select_platform(dl_dict, platform)
Expand All @@ -399,9 +407,12 @@ function artifact_meta(name::String, artifact_dict::Dict, artifacts_toml::String
end

# This is such a no-no, we are going to call it out right here, right now.
if meta !== nothing && !haskey(meta, "git-tree-sha1")
@error("Invalid artifacts file at $(artifacts_toml): artifact '$name' contains no `git-tree-sha1`!")
return nothing
if meta !== nothing
meta = meta::Dict{String, Any}
if !haskey(meta, "git-tree-sha1")
@error("Invalid artifacts file at $(artifacts_toml): artifact '$name' contains no `git-tree-sha1`!")
return nothing
end
end

# Return the full meta-dict.
Expand Down

0 comments on commit cc156d9

Please sign in to comment.