From d2b1557f335f5e2bf2027949214b2367c0e54d2c Mon Sep 17 00:00:00 2001 From: Cody Tapscott Date: Tue, 2 Jul 2024 16:31:10 -0400 Subject: [PATCH 1/3] TOML: Make `Dates` a type parameter This allows the `Dates` calls to be statically inferred --- base/loading.jl | 6 +++--- base/toml_parser.jl | 30 ++++++++++++++---------------- stdlib/TOML/src/TOML.jl | 3 +-- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 358ffae36a844..08b7c230e129b 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -264,11 +264,11 @@ const LOADING_CACHE = Ref{Union{LoadingCache, Nothing}}(nothing) LoadingCache() = LoadingCache(load_path(), Dict(), Dict(), Dict(), Set(), Dict(), Dict(), Dict()) -struct TOMLCache - p::TOML.Parser +struct TOMLCache{Dates} + p::TOML.Parser{Dates} d::Dict{String, CachedTOMLDict} end -const TOML_CACHE = TOMLCache(TOML.Parser(), Dict{String, Dict{String, Any}}()) +const TOML_CACHE = TOMLCache(TOML.Parser(), Dict{String, CachedTOMLDict}()) parsed_toml(project_file::AbstractString) = parsed_toml(project_file, TOML_CACHE, require_lock) function parsed_toml(project_file::AbstractString, toml_cache::TOMLCache, toml_lock::ReentrantLock) diff --git a/base/toml_parser.jl b/base/toml_parser.jl index d50ca3b423c26..10a281fd719fa 100644 --- a/base/toml_parser.jl +++ b/base/toml_parser.jl @@ -38,7 +38,7 @@ const TOMLDict = Dict{String, Any} # Parser # ########## -mutable struct Parser +mutable struct Parser{Dates} str::String # 1 character look ahead current_char::Char @@ -84,14 +84,11 @@ mutable struct Parser # Filled in in case we are parsing a file to improve error messages filepath::Union{String, Nothing} - - # Optionally populate with the Dates stdlib to change the type of Date types returned - Dates::Union{Module, Nothing} end -function Parser(str::String; filepath=nothing) +function Parser{Dates}(str::String; filepath=nothing) where {Dates} root = TOMLDict() - l = Parser( + l = Parser{Dates}( str, # str EOF_CHAR, # current_char firstindex(str), # pos @@ -106,12 +103,12 @@ function Parser(str::String; filepath=nothing) IdSet{Any}(), # static_arrays IdSet{TOMLDict}(), # defined_tables root, - filepath, - nothing + filepath ) startup(l) return l end + function startup(l::Parser) # Populate our one character look-ahead c = eat_char(l) @@ -122,8 +119,12 @@ function startup(l::Parser) end end -Parser() = Parser("") -Parser(io::IO) = Parser(read(io, String)) +Parser{Dates}() where {Dates} = Parser{Dates}("") +Parser{Dates}(io::IO) where {Dates} = Parser{Dates}(read(io, String)) + +Parser() = Parser{nothing}() +Parser(io::IO) = Parser{nothing}(io) +Parser(str::String; filepath=nothing) = Parser{nothing}(str; filepath) function reinit!(p::Parser, str::String; filepath::Union{Nothing, String}=nothing) p.str = str @@ -1021,8 +1022,7 @@ function parse_datetime(l) return try_return_datetime(l, year, month, day, h, m, s, ms) end -function try_return_datetime(p, year, month, day, h, m, s, ms) - Dates = p.Dates +function try_return_datetime(p::Parser{Dates}, year, month, day, h, m, s, ms) where Dates if Dates !== nothing try return Dates.DateTime(year, month, day, h, m, s, ms) @@ -1035,8 +1035,7 @@ function try_return_datetime(p, year, month, day, h, m, s, ms) end end -function try_return_date(p, year, month, day) - Dates = p.Dates +function try_return_date(p::Parser{Dates}, year, month, day) where Dates if Dates !== nothing try return Dates.Date(year, month, day) @@ -1058,8 +1057,7 @@ function parse_local_time(l::Parser) return try_return_time(l, h, m, s, ms) end -function try_return_time(p, h, m, s, ms) - Dates = p.Dates +function try_return_time(p::Parser{Dates}, h, m, s, ms) where Dates if Dates !== nothing try return Dates.Time(h, m, s, ms) diff --git a/stdlib/TOML/src/TOML.jl b/stdlib/TOML/src/TOML.jl index 7414b5dc686f4..787fa11e5ab63 100644 --- a/stdlib/TOML/src/TOML.jl +++ b/stdlib/TOML/src/TOML.jl @@ -44,8 +44,7 @@ const Parser = Internals.Parser Constructor for a TOML `Parser` which returns date and time objects from Dates. """ function DTParser(args...; kwargs...) - parser = Parser(args...; kwargs...) - parser.Dates = Dates + parser = Parser{Dates}(args...; kwargs...) return parser end From b4aee431338bdf1288878ea34c3348c22f8156c9 Mon Sep 17 00:00:00 2001 From: Cody Tapscott Date: Tue, 2 Jul 2024 23:09:40 -0400 Subject: [PATCH 2/3] TOML: Make `TOML.Parser()` use Dates by default This change also adds back the `.Dates` field, which negates the type stability improvements for now but can be removed once we land a fix to Pkg to use the new internal constructor. --- base/loading.jl | 2 +- base/toml_parser.jl | 25 +++++++++++++++---------- stdlib/REPL/src/Pkg_beforeload.jl | 4 +++- stdlib/TOML/src/TOML.jl | 21 ++++++++------------- stdlib/TOML/test/values.jl | 15 +++++++++++++++ 5 files changed, 42 insertions(+), 25 deletions(-) diff --git a/base/loading.jl b/base/loading.jl index 08b7c230e129b..8f706d0f11f3a 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -268,7 +268,7 @@ struct TOMLCache{Dates} p::TOML.Parser{Dates} d::Dict{String, CachedTOMLDict} end -const TOML_CACHE = TOMLCache(TOML.Parser(), Dict{String, CachedTOMLDict}()) +const TOML_CACHE = TOMLCache(TOML.Parser{nothing}(), Dict{String, CachedTOMLDict}()) parsed_toml(project_file::AbstractString) = parsed_toml(project_file, TOML_CACHE, require_lock) function parsed_toml(project_file::AbstractString, toml_cache::TOMLCache, toml_lock::ReentrantLock) diff --git a/base/toml_parser.jl b/base/toml_parser.jl index 10a281fd719fa..cc1455f61928b 100644 --- a/base/toml_parser.jl +++ b/base/toml_parser.jl @@ -84,6 +84,9 @@ mutable struct Parser{Dates} # Filled in in case we are parsing a file to improve error messages filepath::Union{String, Nothing} + + # Optionally populate with the Dates stdlib to change the type of Date types returned + Dates::Union{Module, Nothing} # TODO: remove once Pkg is updated end function Parser{Dates}(str::String; filepath=nothing) where {Dates} @@ -103,7 +106,8 @@ function Parser{Dates}(str::String; filepath=nothing) where {Dates} IdSet{Any}(), # static_arrays IdSet{TOMLDict}(), # defined_tables root, - filepath + filepath, + nothing ) startup(l) return l @@ -122,9 +126,7 @@ end Parser{Dates}() where {Dates} = Parser{Dates}("") Parser{Dates}(io::IO) where {Dates} = Parser{Dates}(read(io, String)) -Parser() = Parser{nothing}() -Parser(io::IO) = Parser{nothing}(io) -Parser(str::String; filepath=nothing) = Parser{nothing}(str; filepath) +# Parser(...) will be defined by TOML stdlib function reinit!(p::Parser, str::String; filepath::Union{Nothing, String}=nothing) p.str = str @@ -1023,9 +1025,10 @@ function parse_datetime(l) end function try_return_datetime(p::Parser{Dates}, year, month, day, h, m, s, ms) where Dates - if Dates !== nothing + if Dates !== nothing || p.Dates !== nothing + mod = Dates !== nothing ? Dates : p.Dates try - return Dates.DateTime(year, month, day, h, m, s, ms) + return mod.DateTime(year, month, day, h, m, s, ms) catch ex ex isa ArgumentError && return ParserError(ErrParsingDateTime) rethrow() @@ -1036,9 +1039,10 @@ function try_return_datetime(p::Parser{Dates}, year, month, day, h, m, s, ms) wh end function try_return_date(p::Parser{Dates}, year, month, day) where Dates - if Dates !== nothing + if Dates !== nothing || p.Dates !== nothing + mod = Dates !== nothing ? Dates : p.Dates try - return Dates.Date(year, month, day) + return mod.Date(year, month, day) catch ex ex isa ArgumentError && return ParserError(ErrParsingDateTime) rethrow() @@ -1058,9 +1062,10 @@ function parse_local_time(l::Parser) end function try_return_time(p::Parser{Dates}, h, m, s, ms) where Dates - if Dates !== nothing + if Dates !== nothing || p.Dates !== nothing + mod = Dates !== nothing ? Dates : p.Dates try - return Dates.Time(h, m, s, ms) + return mod.Time(h, m, s, ms) catch ex ex isa ArgumentError && return ParserError(ErrParsingDateTime) rethrow() diff --git a/stdlib/REPL/src/Pkg_beforeload.jl b/stdlib/REPL/src/Pkg_beforeload.jl index 56c4e2562f7e6..75ae69854a197 100644 --- a/stdlib/REPL/src/Pkg_beforeload.jl +++ b/stdlib/REPL/src/Pkg_beforeload.jl @@ -71,7 +71,9 @@ end function projname(project_file::String) if isfile(project_file) name = try - p = Base.TOML.Parser() + # The `nothing` here means that this TOML parser does not return proper Dates.jl + # objects - but that's OK since we're just checking the name here. + p = Base.TOML.Parser{nothing}() Base.TOML.reinit!(p, read(project_file, String); filepath=project_file) proj = Base.TOML.parse(p) get(proj, "name", nothing) diff --git a/stdlib/TOML/src/TOML.jl b/stdlib/TOML/src/TOML.jl index 787fa11e5ab63..94d2808c0bc24 100644 --- a/stdlib/TOML/src/TOML.jl +++ b/stdlib/TOML/src/TOML.jl @@ -38,15 +38,10 @@ performance if a larger number of small files are parsed. """ const Parser = Internals.Parser -""" - DTParser() - -Constructor for a TOML `Parser` which returns date and time objects from Dates. -""" -function DTParser(args...; kwargs...) - parser = Parser{Dates}(args...; kwargs...) - return parser -end +# Dates-enabled constructors +Parser() = Parser{Dates}() +Parser(io::IO) = Parser{Dates}(io) +Parser(str::String; filepath=nothing) = Parser{Dates}(str; filepath) """ parsefile(f::AbstractString) @@ -58,7 +53,7 @@ Parse file `f` and return the resulting table (dictionary). Throw a See also [`TOML.tryparsefile`](@ref). """ parsefile(f::AbstractString) = - Internals.parse(DTParser(readstring(f); filepath=abspath(f))) + Internals.parse(Parser(readstring(f); filepath=abspath(f))) parsefile(p::Parser, f::AbstractString) = Internals.parse(Internals.reinit!(p, readstring(f); filepath=abspath(f))) @@ -72,7 +67,7 @@ Parse file `f` and return the resulting table (dictionary). Return a See also [`TOML.parsefile`](@ref). """ tryparsefile(f::AbstractString) = - Internals.tryparse(DTParser(readstring(f); filepath=abspath(f))) + Internals.tryparse(Parser(readstring(f); filepath=abspath(f))) tryparsefile(p::Parser, f::AbstractString) = Internals.tryparse(Internals.reinit!(p, readstring(f); filepath=abspath(f))) @@ -86,7 +81,7 @@ Throw a [`ParserError`](@ref) upon failure. See also [`TOML.tryparse`](@ref). """ parse(str::AbstractString) = - Internals.parse(DTParser(String(str))) + Internals.parse(Parser(String(str))) parse(p::Parser, str::AbstractString) = Internals.parse(Internals.reinit!(p, String(str))) parse(io::IO) = parse(read(io, String)) @@ -102,7 +97,7 @@ Return a [`ParserError`](@ref) upon failure. See also [`TOML.parse`](@ref). """ tryparse(str::AbstractString) = - Internals.tryparse(DTParser(String(str))) + Internals.tryparse(Parser(String(str))) tryparse(p::Parser, str::AbstractString) = Internals.tryparse(Internals.reinit!(p, String(str))) tryparse(io::IO) = tryparse(read(io, String)) diff --git a/stdlib/TOML/test/values.jl b/stdlib/TOML/test/values.jl index be2ed3acce5b5..4fc49d47fc98d 100644 --- a/stdlib/TOML/test/values.jl +++ b/stdlib/TOML/test/values.jl @@ -4,16 +4,31 @@ using Test using TOML using TOML: Internals +# Construct an explicit Parser to test the "cached" version of parsing +const test_parser = TOML.Parser() + function testval(s, v) f = "foo = $s" + # First, test with the standard entrypoint parsed = TOML.parse(f)["foo"] return isequal(v, parsed) && typeof(v) == typeof(parsed) + (!isequal(v, parsed) || typeof(v) != typeof(parsed)) && return false + # Next, test with the "cached" (explicit Parser) entrypoint + parsed = TOML.parse(test_parser, f)["foo"] + (!isequal(v, parsed) || typeof(v) != typeof(parsed)) && return false + return true end function failval(s, v) f = "foo = $s" + # First, test with the standard entrypoint err = TOML.tryparse(f); return err isa TOML.Internals.ParserError && err.type == v + (!isa(err, TOML.Internals.ParserError) || err.type != v) && return false + # Next, test with the "cached" (explicit Parser) entrypoint + err = TOML.tryparse(test_parser, f); + (!isa(err, TOML.Internals.ParserError) || err.type != v) && return false + return true end @testset "Numbers" begin From 842f332a9c4a12c957afdc15c609713f89ef9d79 Mon Sep 17 00:00:00 2001 From: Cody Tapscott Date: Tue, 2 Jul 2024 23:42:05 -0400 Subject: [PATCH 3/3] Add converting constructor for `TOMLCache` This is another backwards compatibility concession to Pkg, which we'll remove in the breaking follow-up PR. --- base/loading.jl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/base/loading.jl b/base/loading.jl index 8f706d0f11f3a..e23606b860e2f 100644 --- a/base/loading.jl +++ b/base/loading.jl @@ -268,7 +268,11 @@ struct TOMLCache{Dates} p::TOML.Parser{Dates} d::Dict{String, CachedTOMLDict} end -const TOML_CACHE = TOMLCache(TOML.Parser{nothing}(), Dict{String, CachedTOMLDict}()) +TOMLCache(p::TOML.Parser) = TOMLCache(p, Dict{String, CachedTOMLDict}()) +# TODO: Delete this converting constructor once Pkg stops using it +TOMLCache(p::TOML.Parser, d::Dict{String, Dict{String, Any}}) = TOMLCache(p, convert(Dict{String, CachedTOMLDict}, d)) + +const TOML_CACHE = TOMLCache(TOML.Parser{nothing}()) parsed_toml(project_file::AbstractString) = parsed_toml(project_file, TOML_CACHE, require_lock) function parsed_toml(project_file::AbstractString, toml_cache::TOMLCache, toml_lock::ReentrantLock)