make preference loading a bit more type stable #38285
Conversation
| @@ -1556,7 +1557,7 @@ function get_uuid_name(project_toml::String, uuid::UUID) | |||
| return get_uuid_name(project, uuid) | |||
| end | |||
|
|
|||
| function collect_preferences!(project_toml::String, uuid::UUID) | |||
| function collect_preferences(project_toml::String, uuid::UUID) | |||
There was a problem hiding this comment.
This used to be a mutating operation but that got rebased out I suppose. Good catch!
base/loading.jl
Outdated
| new_base = Base._typeddict(base, overrides...) | ||
|
|
||
| for override in overrides | ||
| override isa Dict{String, Any} || continue # error here? |
There was a problem hiding this comment.
You've constrained overrides already, I don't think this check is necessary.
There was a problem hiding this comment.
Good point, agree.
| return nothing | ||
| end | ||
| get_compiletime_preferences(uuid::UUID) = collect(get(COMPILETIME_PREFERENCES, uuid, String[])) | ||
| get_compiletime_preferences(uuid::UUID) = collect(get(Vector{String}, COMPILETIME_PREFERENCES, uuid)) | ||
| get_compiletime_preferences(m::Module) = get_compiletime_preferences(PkgId(m).uuid) | ||
| get_compiletime_preferences(::Nothing) = String[] |
There was a problem hiding this comment.
We don't use nothing as a sentinel value anymore, so you can drop the ::Nothing method here as well.
There was a problem hiding this comment.
Doesn't look like it: https://build.julialang.org/#/builders/20/builds/5814/steps/7/logs/stdio. It happens when PkgId(m) doesn't identify a uuid, just a name.
There was a problem hiding this comment.
Ah, good call. Thank God for tests!
| get_preferences_hash(m::Module, prefs_list::Vector{String}) = get_preferences_hash(PkgId(m).uuid, prefs_list) | ||
| get_preferences_hash(::Nothing, prefs_list::Vector{String}) = UInt64(0x6e65726566657250) |
There was a problem hiding this comment.
It is quite valuable to have you comb over this after me, because I forget that I have eliminated the nothing sentinel value from the control flow.
There was a problem hiding this comment.
I can't comment on the functionality the way @staticfloat can, but the inference improvements seem solid 👍
Aside from the preferences system (xref #38285) and TOML parsing, these appear to trigger the only remaining sources of vulnerability to invalidation of `Base.require` among Julia's *exported* names, which are the main names we can expect to be specialized by packages.
Also, while at it:
get(f, d, v)instead ofget(d, v, f())in some placesgetinstead ofhaskey+ reindex.