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

TOML: Improve type-stability #55016

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

topolarity
Copy link
Member

@topolarity topolarity commented Jul 3, 2024

Dependent on #55017. This changes the output of the TOML parser to provide specialized Vector{T} less aggressively.

Specifically, combinatorially expensive types like Vector{Vector{Float64}} or Vector{Union{Float64,Int64}} are instead returned as Vector{Any}, while vectors of homogeneous leaf types (e.g. Vector{Float64}) are still supported as before.

This change makes the TOML parser fully type-stable.

topolarity and others added 2 commits July 2, 2024 17:20
This allows the `Dates` calls to be statically inferred
This changes the output of the TOML parser to provide specialize
`Vector{T}` less aggressively, so that combinatorially expensive types
like `Vector{Vector{Float64}}` or `Vector{Union{Float64,Int64}}` are
instead returned as `Vector{Any}`

Vectors of homogeneous leaf types, like `Vector{Float64}` are still
supported as before.

This change makes the TOML parser fully type-stable, except for its
dynamic usage of Dates.

Co-authored-by: Gabriel Baraldi <[email protected]>
@topolarity topolarity changed the title Improve type-stability of TOML parser TOML: Improve type-stability Jul 3, 2024
@KristofferC
Copy link
Sponsor Member

Probably good to parameterize here then:

julia/base/loading.jl

Lines 267 to 270 in ce4f090

struct TOMLCache
p::TOML.Parser
d::Dict{String, CachedTOMLDict}
end
to not pessimize the uses of it.

However, Pkg.jl also uses this TOMLCache and wants to have Dates enabled while the loading code does not. So somehow those uses have to be "split up" I guess.

@topolarity
Copy link
Member Author

However, Pkg.jl also uses this TOMLCache and wants to have Dates enabled while the loading code does not. So somehow those uses have to be "split up" I guess.

Is it coherent to have them share the cache, if one returns different objects than the other? That sounds problematic..

@KristofferC
Copy link
Sponsor Member

Yes, Pkg should probably have its own completely separate cache and if some project files are parsed twice (in Base and Pkg) then I guess that is whatever.

@KristofferC
Copy link
Sponsor Member

This cache https://github.com/JuliaLang/Pkg.jl/blob/8c996799b0ae3d6cccf8a5f25744deb6640adb9e/src/Registry/registry_instance.jl#L18-L21 should probably thus be lifted up to the Pkg module and be used by all parsed_toml functions in Pkg.

@topolarity
Copy link
Member Author

Actually it looks like that cache is already separate, along with the one in Types.jl

@topolarity
Copy link
Member Author

OK, I've parameterized TOMLCache now and I think this should be ready to go again.

#55017 should be reviewed first, since this just adds one commit on top

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants