Skip to content

Commit

Permalink
[TOML] remove Dates hack, replace with explicit usage (#54755)
Browse files Browse the repository at this point in the history
This hack will cease functioning soon (#54739), so it must be removed so
that packages can stop relying on it.

As an aid, now provide some basic conversion and printing support for
the Base.TOML variants of this as well such that users can round-trip
the contents (if they are well-formed as a date/time).
  • Loading branch information
vtjnash authored Jun 13, 2024
1 parent e8ae5da commit 2ce12e9
Show file tree
Hide file tree
Showing 5 changed files with 46 additions and 24 deletions.
35 changes: 17 additions & 18 deletions base/toml_parser.jl
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ module TOML

using Base: IdSet

# In case we do not have the Dates stdlib available
# we parse DateTime into these internal structs,
# unless a different DateTime library is passed to the Parser constructor
# note that these do not do any argument checking
struct Date
year::Int
Expand Down Expand Up @@ -85,12 +85,10 @@ mutable struct Parser
# Filled in in case we are parsing a file to improve error messages
filepath::Union{String, Nothing}

# Gets populated with the Dates stdlib if it exists
# Optionally populate with the Dates stdlib to change the type of Date types returned
Dates::Union{Module, Nothing}
end

const DATES_PKGID = Base.PkgId(Base.UUID("ade2ca70-3891-5945-98fb-dc099432e06a"), "Dates")

function Parser(str::String; filepath=nothing)
root = TOMLDict()
l = Parser(
Expand All @@ -109,7 +107,7 @@ function Parser(str::String; filepath=nothing)
IdSet{TOMLDict}(), # defined_tables
root,
filepath,
isdefined(Base, :maybe_root_module) ? Base.maybe_root_module(DATES_PKGID) : nothing,
nothing
)
startup(l)
return l
Expand Down Expand Up @@ -151,8 +149,6 @@ end
# Errors #
##########

throw_internal_error(msg) = error("internal TOML parser error: $msg")

# Many functions return a ParserError. We want this to bubble up
# all the way and have this error be returned to the user
# if the parse is called with `raise=false`. This macro
Expand Down Expand Up @@ -900,7 +896,7 @@ end
function parse_float(l::Parser, contains_underscore)::Err{Float64}
s = take_string_or_substring(l, contains_underscore)
v = Base.tryparse(Float64, s)
v === nothing && return(ParserError(ErrGenericValueError))
v === nothing && return ParserError(ErrGenericValueError)
return v
end

Expand All @@ -909,8 +905,8 @@ function parse_int(l::Parser, contains_underscore, base=nothing)::Err{Int64}
v = try
Base.parse(Int64, s; base=base)
catch e
e isa Base.OverflowError && return(ParserError(ErrOverflowError))
error("internal parser error: did not correctly discredit $(repr(s)) as an int")
e isa Base.OverflowError && return ParserError(ErrOverflowError)
rethrow()
end
return v
end
Expand All @@ -932,8 +928,8 @@ for (name, T1, T2, n1, n2) in (("integer", Int64, Int128, 17, 33),
Base.parse(BigInt, s; base)
end
catch e
e isa Base.OverflowError && return(ParserError(ErrOverflowError))
error("internal parser error: did not correctly discredit $(repr(s)) as an int")
e isa Base.OverflowError && return ParserError(ErrOverflowError)
rethrow()
end
return v
end
Expand Down Expand Up @@ -1030,8 +1026,9 @@ function try_return_datetime(p, year, month, day, h, m, s, ms)
if Dates !== nothing
try
return Dates.DateTime(year, month, day, h, m, s, ms)
catch
return ParserError(ErrParsingDateTime)
catch ex
ex isa ArgumentError && return ParserError(ErrParsingDateTime)
rethrow()
end
else
return DateTime(year, month, day, h, m, s, ms)
Expand All @@ -1043,8 +1040,9 @@ function try_return_date(p, year, month, day)
if Dates !== nothing
try
return Dates.Date(year, month, day)
catch
return ParserError(ErrParsingDateTime)
catch ex
ex isa ArgumentError && return ParserError(ErrParsingDateTime)
rethrow()
end
else
return Date(year, month, day)
Expand All @@ -1065,8 +1063,9 @@ function try_return_time(p, h, m, s, ms)
if Dates !== nothing
try
return Dates.Time(h, m, s, ms)
catch
return ParserError(ErrParsingDateTime)
catch ex
ex isa ArgumentError && return ParserError(ErrParsingDateTime)
rethrow()
end
else
return Time(h, m, s, ms)
Expand Down
5 changes: 5 additions & 0 deletions stdlib/Dates/src/types.jl
Original file line number Diff line number Diff line change
Expand Up @@ -492,3 +492,8 @@ end

Base.OrderStyle(::Type{<:AbstractTime}) = Base.Ordered()
Base.ArithmeticStyle(::Type{<:AbstractTime}) = Base.ArithmeticWraps()

# minimal Base.TOML support
Date(d::Base.TOML.Date) = Date(d.year, d.month, d.day)
Time(t::Base.TOML.Time) = Time(t.hour, t.minute, t.second, t.ms)
DateTime(dt::Base.TOML.DateTime) = DateTime(Date(dt.date), Time(dt.time))
3 changes: 2 additions & 1 deletion stdlib/TOML/Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,13 @@ version = "1.0.3"
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"

[compat]
Dates = "1.11.0"
julia = "1.6"

[extras]
Downloads = "f43a241f-c20a-4ad4-852c-f6b1247861c6"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
Tar = "a4e569a6-e804-4fa4-b0f3-eef7a1d5b13e"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"
p7zip_jll = "3f19e933-33d8-53b3-aaab-bd5110c3b7a0"

[targets]
Expand Down
21 changes: 17 additions & 4 deletions stdlib/TOML/src/TOML.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ and to serialize Julia data structures to TOML format.
"""
module TOML

using Dates

module Internals
# The parser is defined in Base
using Base.TOML: Parser, parse, tryparse, ParserError, isvalid_barekey_char, reinit!
Expand Down Expand Up @@ -36,6 +38,17 @@ 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(args...; kwargs...)
parser.Dates = Dates
return parser
end

"""
parsefile(f::AbstractString)
parsefile(p::Parser, f::AbstractString)
Expand All @@ -46,7 +59,7 @@ Parse file `f` and return the resulting table (dictionary). Throw a
See also [`TOML.tryparsefile`](@ref).
"""
parsefile(f::AbstractString) =
Internals.parse(Parser(readstring(f); filepath=abspath(f)))
Internals.parse(DTParser(readstring(f); filepath=abspath(f)))
parsefile(p::Parser, f::AbstractString) =
Internals.parse(Internals.reinit!(p, readstring(f); filepath=abspath(f)))

Expand All @@ -60,7 +73,7 @@ Parse file `f` and return the resulting table (dictionary). Return a
See also [`TOML.parsefile`](@ref).
"""
tryparsefile(f::AbstractString) =
Internals.tryparse(Parser(readstring(f); filepath=abspath(f)))
Internals.tryparse(DTParser(readstring(f); filepath=abspath(f)))
tryparsefile(p::Parser, f::AbstractString) =
Internals.tryparse(Internals.reinit!(p, readstring(f); filepath=abspath(f)))

Expand All @@ -74,7 +87,7 @@ Throw a [`ParserError`](@ref) upon failure.
See also [`TOML.tryparse`](@ref).
"""
parse(str::AbstractString) =
Internals.parse(Parser(String(str)))
Internals.parse(DTParser(String(str)))
parse(p::Parser, str::AbstractString) =
Internals.parse(Internals.reinit!(p, String(str)))
parse(io::IO) = parse(read(io, String))
Expand All @@ -90,7 +103,7 @@ Return a [`ParserError`](@ref) upon failure.
See also [`TOML.parse`](@ref).
"""
tryparse(str::AbstractString) =
Internals.tryparse(Parser(String(str)))
Internals.tryparse(DTParser(String(str)))
tryparse(p::Parser, str::AbstractString) =
Internals.tryparse(Internals.reinit!(p, String(str)))
tryparse(io::IO) = tryparse(read(io, String))
Expand Down
6 changes: 5 additions & 1 deletion stdlib/TOML/src/print.jl
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,8 @@ function print_toml_escaped(io::IO, s::AbstractString)
end

const MbyFunc = Union{Function, Nothing}
const TOMLValue = Union{AbstractVector, AbstractDict, Dates.DateTime, Dates.Time, Dates.Date, Bool, Integer, AbstractFloat, AbstractString}
const TOMLValue = Union{AbstractVector, AbstractDict, Bool, Integer, AbstractFloat, AbstractString,
Dates.DateTime, Dates.Time, Dates.Date, Base.TOML.DateTime, Base.TOML.Time, Base.TOML.Date}


########
Expand Down Expand Up @@ -89,6 +90,9 @@ function printvalue(f::MbyFunc, io::IO, value::AbstractVector, sorted::Bool)
end

function printvalue(f::MbyFunc, io::IO, value::TOMLValue, sorted::Bool)
value isa Base.TOML.DateTime && (value = Dates.DateTime(value))
value isa Base.TOML.Time && (value = Dates.Time(value))
value isa Base.TOML.Date && (value = Dates.Date(value))
value isa Dates.DateTime ? Base.print(io, Dates.format(value, Dates.dateformat"YYYY-mm-dd\THH:MM:SS.sss\Z")) :
value isa Dates.Time ? Base.print(io, Dates.format(value, Dates.dateformat"HH:MM:SS.sss")) :
value isa Dates.Date ? Base.print(io, Dates.format(value, Dates.dateformat"YYYY-mm-dd")) :
Expand Down

5 comments on commit 2ce12e9

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

The package evaluation job you requested has completed - possible new issues were detected.
The full report is available.

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like we have a large number of new failures. 2 sources that I noticed include: error when assigning to a global that hasn’t been declared and the FieldError. We should add a Test shim (similar to LoadError) for FieldError.

@vtjnash
Copy link
Member Author

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

Please sign in to comment.