From 5c9fb73fe115d9a7a21c8ceccf2f937d002869c4 Mon Sep 17 00:00:00 2001 From: Sukera Date: Thu, 19 May 2022 11:53:31 +0200 Subject: [PATCH 01/10] Initial refactor of error messages in Printf stdlib --- stdlib/Printf/src/Printf.jl | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index 1d04e7146e28b..d4294086ee973 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -79,16 +79,18 @@ char(::Type{Val{c}}) where {c} = c # parse format string function Format(f::AbstractString) - isempty(f) && throw(ArgumentError("empty format string")) + isempty(f) && throw(ArgumentError("Format string must be non-empty")) bytes = codeunits(f) len = length(bytes) pos = 1 b = 0x00 + + # skip ahead to first format specifier while pos <= len b = bytes[pos] pos += 1 if b == UInt8('%') - pos > len && throw(ArgumentError("invalid format string: '$f'")) + pos > len && throw(ArgumentError("Format string is invalid - first format specifier incomplete at end of string: '$f'")) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] @@ -120,7 +122,7 @@ function Format(f::AbstractString) else break end - pos > len && throw(ArgumentError("incomplete format string: '$f'")) + pos > len && throw(ArgumentError("Format string is invalid - last format specifier is incomplete: '$f'")) b = bytes[pos] pos += 1 end @@ -139,7 +141,7 @@ function Format(f::AbstractString) precision = 0 parsedprecdigits = false if b == UInt8('.') - pos > len && throw(ArgumentError("incomplete format string: '$f'")) + pos > len && throw(ArgumentError("Format string is invalid - precision specifier is missing precision: '$f'")) parsedprecdigits = true b = bytes[pos] pos += 1 @@ -158,16 +160,16 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == prev - pos > len && throw(ArgumentError("invalid format string: '$f'")) + pos > len && throw(ArgumentError("Format string is invalid - unterminated length modifier at end of string: '$f'")) b = bytes[pos] pos += 1 end - elseif b in b"Ljqtz" + elseif b in b"Ljqtz" # what is q? Possibly quad? b = bytes[pos] pos += 1 end # parse type - !(b in b"diouxXDOUeEfFgGaAcCsSpn") && throw(ArgumentError("invalid format string: '$f', invalid type specifier: '$(Char(b))'")) + !(b in b"diouxXDOUeEfFgGaAcCsSpn") && throw(ArgumentError("Format string is invalid - '$(Char(b))' is not a valid type specifier: '$f'")) type = Val{Char(b)} if type <: Ints && precision > 0 zero = false @@ -184,7 +186,7 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == UInt8('%') - pos > len && throw(ArgumentError("invalid format string: '$f'")) + pos > len && throw(ArgumentError("Format string is invalid - last format specifier is incomplete at end of string: '$f'")) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] @@ -409,11 +411,13 @@ const __BIG_FLOAT_MAX__ = 8192 if len > siz maxout = max(__BIG_FLOAT_MAX__, ceil(Int, precision(x) * log(2) / log(10)) + 25) + # TODO: `error` is kind of generic, is there a more appropriate one to throw? len > maxout && error("Over $maxout bytes $len needed to output BigFloat $x") resize!(buf, len + 1) len = _snprintf(pointer(buf, pos), len + 1, str, x) end + # TODO: when will this actually be thrown? What more informative error would be appropriate? len > 0 || throw(ArgumentError("invalid printf formatting $str for BigFloat")) return pos + len end @@ -805,7 +809,7 @@ plength(::Spec{PositionCounter}, x) = 0 end @noinline argmismatch(a, b) = - throw(ArgumentError("mismatch between # of format specifiers and provided args: $a != $b")) + throw(ArgumentError("Number of format specifiers and number of provided args differ: $a != $b")) """ Printf.format(f::Printf.Format, args...) => String @@ -892,7 +896,8 @@ macro printf(io_or_fmt, args...) return esc(:($Printf.format(stdout, $fmt, $(args...)))) else io = io_or_fmt - isempty(args) && throw(ArgumentError("must provide required format string")) + isempty(args) && throw(ArgumentError("No arguments provided to `@printf` - use like `@printf [io] .")) + args[1] isa String || throw(ArgumentError("First argument after `IO` has to be a format string.")) fmt = Format(args[1]) return esc(:($Printf.format($io, $fmt, $(Base.tail(args)...)))) end From 59da89fcbb4ea5d16b206e09d72a847cbb0d69de Mon Sep 17 00:00:00 2001 From: Sukera Date: Thu, 19 May 2022 12:04:13 +0200 Subject: [PATCH 02/10] Add additional error messages to `@sprintf` and clarify top-level `@printf` macro usage errors --- stdlib/Printf/src/Printf.jl | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index d4294086ee973..e4d4a2d90c9ec 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -894,12 +894,15 @@ macro printf(io_or_fmt, args...) if io_or_fmt isa String fmt = Format(io_or_fmt) return esc(:($Printf.format(stdout, $fmt, $(args...)))) - else + elseif io_or_fmt isa IO io = io_or_fmt - isempty(args) && throw(ArgumentError("No arguments provided to `@printf` - use like `@printf [io] .")) - args[1] isa String || throw(ArgumentError("First argument after `IO` has to be a format string.")) - fmt = Format(args[1]) + isempty(args) && throw(ArgumentError("No format string provided to `@printf` - use like `@printf [io] [].")) + fmt_str = first(args) + fmt_str isa String || throw(ArgumentError("First argument after `IO` has to be a format string")) + fmt = Format(fmt_str) return esc(:($Printf.format($io, $fmt, $(Base.tail(args)...)))) + else + throw(ArgumentError("First argument to `@printf` has to be either an `<: IO` to print to or a format `String`.")) end end @@ -915,6 +918,7 @@ julia> @sprintf "this is a %s %15.1f" "test" 34.567 ``` """ macro sprintf(fmt, args...) + fmt isa String || throw(ArgumentError("First argument has to be a format string.")) f = Format(fmt) return esc(:($Printf.format($f, $(args...)))) end From ac5f8b2a9e79a4ef2672c6b44794052df9f85d6a Mon Sep 17 00:00:00 2001 From: Sukera Date: Thu, 19 May 2022 12:25:35 +0200 Subject: [PATCH 03/10] Fix erronous check for IO on `@printf` argument --- stdlib/Printf/src/Printf.jl | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index e4d4a2d90c9ec..47149a1113b73 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -894,15 +894,13 @@ macro printf(io_or_fmt, args...) if io_or_fmt isa String fmt = Format(io_or_fmt) return esc(:($Printf.format(stdout, $fmt, $(args...)))) - elseif io_or_fmt isa IO + else io = io_or_fmt isempty(args) && throw(ArgumentError("No format string provided to `@printf` - use like `@printf [io] [].")) fmt_str = first(args) - fmt_str isa String || throw(ArgumentError("First argument after `IO` has to be a format string")) + fmt_str isa String || throw(ArgumentError("First argument after io has to be a format string")) fmt = Format(fmt_str) return esc(:($Printf.format($io, $fmt, $(Base.tail(args)...)))) - else - throw(ArgumentError("First argument to `@printf` has to be either an `<: IO` to print to or a format `String`.")) end end From 7586bed36c9809156b1b99372d04a693e5009c51 Mon Sep 17 00:00:00 2001 From: Sukera Date: Fri, 20 May 2022 10:40:10 +0200 Subject: [PATCH 04/10] Introduce message feedback --- stdlib/Printf/src/Printf.jl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index 47149a1113b73..4724ad7786c12 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -79,7 +79,7 @@ char(::Type{Val{c}}) where {c} = c # parse format string function Format(f::AbstractString) - isempty(f) && throw(ArgumentError("Format string must be non-empty")) + isempty(f) && throw(ArgumentError("Format string must not be empty")) bytes = codeunits(f) len = length(bytes) pos = 1 @@ -898,7 +898,7 @@ macro printf(io_or_fmt, args...) io = io_or_fmt isempty(args) && throw(ArgumentError("No format string provided to `@printf` - use like `@printf [io] [].")) fmt_str = first(args) - fmt_str isa String || throw(ArgumentError("First argument after io has to be a format string")) + fmt_str isa String || throw(ArgumentError("First argument after `io` must be a format string")) fmt = Format(fmt_str) return esc(:($Printf.format($io, $fmt, $(Base.tail(args)...)))) end @@ -916,7 +916,7 @@ julia> @sprintf "this is a %s %15.1f" "test" 34.567 ``` """ macro sprintf(fmt, args...) - fmt isa String || throw(ArgumentError("First argument has to be a format string.")) + fmt isa String || throw(ArgumentError("First argument must be a format string.")) f = Format(fmt) return esc(:($Printf.format($f, $(args...)))) end From 636b0bea81e1dc05d3adaf10c6f345cba53e6f87 Mon Sep 17 00:00:00 2001 From: Sukera Date: Fri, 20 May 2022 13:15:46 +0200 Subject: [PATCH 05/10] Add pretty printing of invalid printf formats --- stdlib/Printf/src/Printf.jl | 52 ++++++++++++++++++++++++++++------ stdlib/Printf/test/runtests.jl | 20 ++++++------- 2 files changed, 53 insertions(+), 19 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index 4724ad7786c12..1b9906e423f6d 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -77,9 +77,43 @@ end base(T) = T <: HexBases ? 16 : T <: Val{'o'} ? 8 : 10 char(::Type{Val{c}}) where {c} = c +struct InvalidFormatStringError <: Exception + message::String + format::String + position::Int +end + +function showerror(io::IO, err::InvalidFormatStringError) + io_has_color = get(io, :color, false) + + println(io, err.message, ':') + print(io, "\t'", @view err.format[begin:prevind(err.format, err.position)]) + # err.position may be greater than the last character + invalid_text = if checkbounds(Bool, err.position, err.format) + format_str[pos] + else + "" + end + + if io_has_color + printstyled(io, invalid_text, color=:red) + else + print(io, invalid_text) + end + + println(io, @view err.format[min(end+1, pos):end], '\'') + + arrow = '\t' * ('-'^pos) * "^\n" + if io_has_color + printstyled(io, arrow, color=:red) + else + print(io, arrow) + end +end + # parse format string function Format(f::AbstractString) - isempty(f) && throw(ArgumentError("Format string must not be empty")) + isempty(f) && throw(InvalidFormatStringError("Format string must not be empty", f, 1)) bytes = codeunits(f) len = length(bytes) pos = 1 @@ -90,7 +124,7 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == UInt8('%') - pos > len && throw(ArgumentError("Format string is invalid - first format specifier incomplete at end of string: '$f'")) + pos > len && throw(InvalidFormatStringError("First format specifier incomplete at end of string", f, pos)) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] @@ -122,7 +156,7 @@ function Format(f::AbstractString) else break end - pos > len && throw(ArgumentError("Format string is invalid - last format specifier is incomplete: '$f'")) + pos > len && throw(InvalidFormatStringError("Last format specifier is incomplete", f, pos)) b = bytes[pos] pos += 1 end @@ -141,7 +175,7 @@ function Format(f::AbstractString) precision = 0 parsedprecdigits = false if b == UInt8('.') - pos > len && throw(ArgumentError("Format string is invalid - precision specifier is missing precision: '$f'")) + pos > len && throw(InvalidFormatStringError("Precision specifier is missing precision", f, pos)) parsedprecdigits = true b = bytes[pos] pos += 1 @@ -160,7 +194,7 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == prev - pos > len && throw(ArgumentError("Format string is invalid - unterminated length modifier at end of string: '$f'")) + pos > len && throw(InvalidFormatStringError("Unterminated length modifier at end of string", f, pos)) b = bytes[pos] pos += 1 end @@ -169,7 +203,7 @@ function Format(f::AbstractString) pos += 1 end # parse type - !(b in b"diouxXDOUeEfFgGaAcCsSpn") && throw(ArgumentError("Format string is invalid - '$(Char(b))' is not a valid type specifier: '$f'")) + !(b in b"diouxXDOUeEfFgGaAcCsSpn") && throw(InvalidFormatStringError("'$(Char(b))' is not a valid type specifier", f, pos)) type = Val{Char(b)} if type <: Ints && precision > 0 zero = false @@ -186,7 +220,7 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == UInt8('%') - pos > len && throw(ArgumentError("Format string is invalid - last format specifier is incomplete at end of string: '$f'")) + pos > len && throw(InvalidFormatStringError("Last format specifier is incomplete at end of string", f, pos)) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] @@ -898,7 +932,7 @@ macro printf(io_or_fmt, args...) io = io_or_fmt isempty(args) && throw(ArgumentError("No format string provided to `@printf` - use like `@printf [io] [].")) fmt_str = first(args) - fmt_str isa String || throw(ArgumentError("First argument after `io` must be a format string")) + fmt_str isa String || throw(ArgumentError("First argument to `@printf` after `io` must be a format string")) fmt = Format(fmt_str) return esc(:($Printf.format($io, $fmt, $(Base.tail(args)...)))) end @@ -916,7 +950,7 @@ julia> @sprintf "this is a %s %15.1f" "test" 34.567 ``` """ macro sprintf(fmt, args...) - fmt isa String || throw(ArgumentError("First argument must be a format string.")) + fmt isa String || throw(ArgumentError("First argument to `@sprintf` must be a format string.")) f = Format(fmt) return esc(:($Printf.format($f, $(args...)))) end diff --git a/stdlib/Printf/test/runtests.jl b/stdlib/Printf/test/runtests.jl index e80cbe9626823..fc9b3e9f97962 100644 --- a/stdlib/Printf/test/runtests.jl +++ b/stdlib/Printf/test/runtests.jl @@ -339,10 +339,10 @@ end @test Printf.@sprintf("1%%2%%3") == "1%2%3" @test Printf.@sprintf("GAP[%%]") == "GAP[%]" @test Printf.@sprintf("hey there") == "hey there" - @test_throws ArgumentError Printf.Format("") - @test_throws ArgumentError Printf.Format("%+") - @test_throws ArgumentError Printf.Format("%.") - @test_throws ArgumentError Printf.Format("%.0") + @test_throws InvalidFormatStringError Printf.Format("") + @test_throws InvalidFormatStringError Printf.Format("%+") + @test_throws InvalidFormatStringError Printf.Format("%.") + @test_throws InvalidFormatStringError Printf.Format("%.0") @test isempty(Printf.Format("%%").formats) @test Printf.@sprintf("%d%d", 1, 2) == "12" @test (Printf.@sprintf "%d%d" [1 2]...) == "12" @@ -355,10 +355,10 @@ end @test (Printf.@sprintf("%d\u0f00%d", 1, 2)) == "1\u0f002" @test (Printf.@sprintf("%d\U0001ffff%d", 1, 2)) == "1\U0001ffff2" @test (Printf.@sprintf("%d\u2203%d\u0203", 1, 2)) == "1\u22032\u0203" - @test_throws ArgumentError Printf.Format("%y%d") - @test_throws ArgumentError Printf.Format("%\u00d0%d") - @test_throws ArgumentError Printf.Format("%\u0f00%d") - @test_throws ArgumentError Printf.Format("%\U0001ffff%d") + @test_throws InvalidFormatStringError Printf.Format("%y%d") + @test_throws InvalidFormatStringError Printf.Format("%\u00d0%d") + @test_throws InvalidFormatStringError Printf.Format("%\u0f00%d") + @test_throws InvalidFormatStringError Printf.Format("%\U0001ffff%d") @test Printf.@sprintf("%10.5d", 4) == " 00004" @test (Printf.@sprintf "%d" typemax(Int64)) == "9223372036854775807" @@ -528,13 +528,13 @@ end @test Printf.@sprintf( "%0-5d", -42) == "-42 " @test Printf.@sprintf( "%0-15d", 42) == "42 " @test Printf.@sprintf( "%0-15d", -42) == "-42 " - @test_throws ArgumentError Printf.Format("%d %") + @test_throws InvalidFormatStringError Printf.Format("%d %") @test Printf.@sprintf("%lld", 18446744065119617025) == "18446744065119617025" @test Printf.@sprintf("%+8lld", 100) == " +100" @test Printf.@sprintf("%+.8lld", 100) == "+00000100" @test Printf.@sprintf("%+10.8lld", 100) == " +00000100" - @test_throws ArgumentError Printf.Format("%_1lld") + @test_throws InvalidFormatStringError Printf.Format("%_1lld") @test Printf.@sprintf("%-1.5lld", -100) == "-00100" @test Printf.@sprintf("%5lld", 100) == " 100" @test Printf.@sprintf("%5lld", -100) == " -100" From d5d09e3d2b87cef408870e642a5c6c9833d898b3 Mon Sep 17 00:00:00 2001 From: Sukera Date: Fri, 20 May 2022 16:39:33 +0200 Subject: [PATCH 06/10] Add format string error indicator --- stdlib/Printf/src/Printf.jl | 39 +++++++++++++++++----------------- stdlib/Printf/test/runtests.jl | 20 ++++++++--------- 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index 1b9906e423f6d..7a0c944ed6f6b 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -80,20 +80,16 @@ char(::Type{Val{c}}) where {c} = c struct InvalidFormatStringError <: Exception message::String format::String - position::Int + start_color::Int + end_color::Int end -function showerror(io::IO, err::InvalidFormatStringError) +function Base.showerror(io::IO, err::InvalidFormatStringError) io_has_color = get(io, :color, false) - println(io, err.message, ':') - print(io, "\t'", @view err.format[begin:prevind(err.format, err.position)]) - # err.position may be greater than the last character - invalid_text = if checkbounds(Bool, err.position, err.format) - format_str[pos] - else - "" - end + println(io, "InvalidFormatStringError: ", err.message) + print(io, "\t\"", @view(err.format[begin:prevind(err.format, err.start_color)])) + invalid_text = @view err.format[err.start_color:err.end_color] if io_has_color printstyled(io, invalid_text, color=:red) @@ -101,9 +97,11 @@ function showerror(io::IO, err::InvalidFormatStringError) print(io, invalid_text) end - println(io, @view err.format[min(end+1, pos):end], '\'') + # +1 is okay, since all format characters are single bytes + println(io, @view(err.format[err.end_color+1:end]), "\"") - arrow = '\t' * ('-'^pos) * "^\n" + arrow_error = '-'^(length(invalid_text)-1) + arrow = "\t" * ' '^err.start_color * arrow_error * "^\n" if io_has_color printstyled(io, arrow, color=:red) else @@ -113,18 +111,20 @@ end # parse format string function Format(f::AbstractString) - isempty(f) && throw(InvalidFormatStringError("Format string must not be empty", f, 1)) + isempty(f) && throw(InvalidFormatStringError("Format string must not be empty", f, 1, 1)) bytes = codeunits(f) len = length(bytes) pos = 1 b = 0x00 + local last_percent_pos # skip ahead to first format specifier while pos <= len b = bytes[pos] pos += 1 if b == UInt8('%') - pos > len && throw(InvalidFormatStringError("First format specifier incomplete at end of string", f, pos)) + last_percent_pos = pos-1 + pos > len && throw(InvalidFormatStringError("Format specifier incomplete", f, last_percent_pos, last_percent_pos)) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] @@ -156,7 +156,7 @@ function Format(f::AbstractString) else break end - pos > len && throw(InvalidFormatStringError("Last format specifier is incomplete", f, pos)) + pos > len && throw(InvalidFormatStringError("Format specifier is incomplete", f, last_percent_pos, pos-1)) b = bytes[pos] pos += 1 end @@ -175,7 +175,7 @@ function Format(f::AbstractString) precision = 0 parsedprecdigits = false if b == UInt8('.') - pos > len && throw(InvalidFormatStringError("Precision specifier is missing precision", f, pos)) + pos > len && throw(InvalidFormatStringError("Precision specifier is missing precision", f, last_percent_pos, pos-1)) parsedprecdigits = true b = bytes[pos] pos += 1 @@ -194,7 +194,7 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == prev - pos > len && throw(InvalidFormatStringError("Unterminated length modifier at end of string", f, pos)) + pos > len && throw(InvalidFormatStringError("Unterminated length modifier", f, last_percent_pos, pos-1)) b = bytes[pos] pos += 1 end @@ -203,7 +203,7 @@ function Format(f::AbstractString) pos += 1 end # parse type - !(b in b"diouxXDOUeEfFgGaAcCsSpn") && throw(InvalidFormatStringError("'$(Char(b))' is not a valid type specifier", f, pos)) + !(b in b"diouxXDOUeEfFgGaAcCsSpn") && throw(InvalidFormatStringError("'$(Char(b))' is not a valid type specifier", f, last_percent_pos, pos-1)) type = Val{Char(b)} if type <: Ints && precision > 0 zero = false @@ -220,7 +220,8 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 if b == UInt8('%') - pos > len && throw(InvalidFormatStringError("Last format specifier is incomplete at end of string", f, pos)) + last_percent_pos = pos-1 + pos > len && throw(InvalidFormatStringError("Format specifier is incomplete", f, last_percent_pos, last_percent_pos)) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] diff --git a/stdlib/Printf/test/runtests.jl b/stdlib/Printf/test/runtests.jl index fc9b3e9f97962..27ba14a202c38 100644 --- a/stdlib/Printf/test/runtests.jl +++ b/stdlib/Printf/test/runtests.jl @@ -339,10 +339,10 @@ end @test Printf.@sprintf("1%%2%%3") == "1%2%3" @test Printf.@sprintf("GAP[%%]") == "GAP[%]" @test Printf.@sprintf("hey there") == "hey there" - @test_throws InvalidFormatStringError Printf.Format("") - @test_throws InvalidFormatStringError Printf.Format("%+") - @test_throws InvalidFormatStringError Printf.Format("%.") - @test_throws InvalidFormatStringError Printf.Format("%.0") + @test_throws Printf.InvalidFormatStringError Printf.Format("") + @test_throws Printf.InvalidFormatStringError Printf.Format("%+") + @test_throws Printf.InvalidFormatStringError Printf.Format("%.") + @test_throws Printf.InvalidFormatStringError Printf.Format("%.0") @test isempty(Printf.Format("%%").formats) @test Printf.@sprintf("%d%d", 1, 2) == "12" @test (Printf.@sprintf "%d%d" [1 2]...) == "12" @@ -355,10 +355,10 @@ end @test (Printf.@sprintf("%d\u0f00%d", 1, 2)) == "1\u0f002" @test (Printf.@sprintf("%d\U0001ffff%d", 1, 2)) == "1\U0001ffff2" @test (Printf.@sprintf("%d\u2203%d\u0203", 1, 2)) == "1\u22032\u0203" - @test_throws InvalidFormatStringError Printf.Format("%y%d") - @test_throws InvalidFormatStringError Printf.Format("%\u00d0%d") - @test_throws InvalidFormatStringError Printf.Format("%\u0f00%d") - @test_throws InvalidFormatStringError Printf.Format("%\U0001ffff%d") + @test_throws Printf.InvalidFormatStringError Printf.Format("%y%d") + @test_throws Printf.InvalidFormatStringError Printf.Format("%\u00d0%d") + @test_throws Printf.InvalidFormatStringError Printf.Format("%\u0f00%d") + @test_throws Printf.InvalidFormatStringError Printf.Format("%\U0001ffff%d") @test Printf.@sprintf("%10.5d", 4) == " 00004" @test (Printf.@sprintf "%d" typemax(Int64)) == "9223372036854775807" @@ -528,13 +528,13 @@ end @test Printf.@sprintf( "%0-5d", -42) == "-42 " @test Printf.@sprintf( "%0-15d", 42) == "42 " @test Printf.@sprintf( "%0-15d", -42) == "-42 " - @test_throws InvalidFormatStringError Printf.Format("%d %") + @test_throws Printf.InvalidFormatStringError Printf.Format("%d %") @test Printf.@sprintf("%lld", 18446744065119617025) == "18446744065119617025" @test Printf.@sprintf("%+8lld", 100) == " +100" @test Printf.@sprintf("%+.8lld", 100) == "+00000100" @test Printf.@sprintf("%+10.8lld", 100) == " +00000100" - @test_throws InvalidFormatStringError Printf.Format("%_1lld") + @test_throws Printf.InvalidFormatStringError Printf.Format("%_1lld") @test Printf.@sprintf("%-1.5lld", -100) == "-00100" @test Printf.@sprintf("%5lld", 100) == " 100" @test Printf.@sprintf("%5lld", -100) == " -100" From d6fed5d9eb4319818c6a349cf68f4feb743cff5c Mon Sep 17 00:00:00 2001 From: Sukera Date: Fri, 20 May 2022 16:52:27 +0200 Subject: [PATCH 07/10] Fix out-of-bounds bug with length modifiers at the end of a formatstring --- stdlib/Printf/src/Printf.jl | 6 ++++-- stdlib/Printf/test/runtests.jl | 6 ++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index 7a0c944ed6f6b..03e57ce6a91b3 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -124,7 +124,7 @@ function Format(f::AbstractString) pos += 1 if b == UInt8('%') last_percent_pos = pos-1 - pos > len && throw(InvalidFormatStringError("Format specifier incomplete", f, last_percent_pos, last_percent_pos)) + pos > len && throw(InvalidFormatStringError("Format specifier is incomplete", f, last_percent_pos, last_percent_pos)) if bytes[pos] == UInt8('%') # escaped '%' b = bytes[pos] @@ -191,14 +191,16 @@ function Format(f::AbstractString) # parse length modifier (ignored) if b == UInt8('h') || b == UInt8('l') prev = b + pos > len && throw(InvalidFormatStringError("Length modifier is missing type specifier", f, last_percent_pos, pos-1)) b = bytes[pos] pos += 1 if b == prev - pos > len && throw(InvalidFormatStringError("Unterminated length modifier", f, last_percent_pos, pos-1)) + pos > len && throw(InvalidFormatStringError("Length modifier is missing type specifier", f, last_percent_pos, pos-1)) b = bytes[pos] pos += 1 end elseif b in b"Ljqtz" # what is q? Possibly quad? + pos > len && throw(InvalidFormatStringError("Length modifier is missing type specifier", f, last_percent_pos, pos-1)) b = bytes[pos] pos += 1 end diff --git a/stdlib/Printf/test/runtests.jl b/stdlib/Printf/test/runtests.jl index 27ba14a202c38..f97b572684097 100644 --- a/stdlib/Printf/test/runtests.jl +++ b/stdlib/Printf/test/runtests.jl @@ -782,4 +782,10 @@ end @test (Printf.@sprintf("%s%n", "1234", x); x[] == 4) end +@testset "length modifiers" begin + @test_throws Printf.InvalidFormatStringError Printf.@sprintf("%h", 1) + @test_throws Printf.InvalidFormatStringError Printf.@sprintf("%hh", 1) + @test_throws Printf.InvalidFormatStringError Printf.@sprintf("%z", 1) +end + end # @testset "Printf" From 27bb52f9f78b06402f6b9f3e9e2684281f3aa836 Mon Sep 17 00:00:00 2001 From: Sukera Date: Fri, 20 May 2022 19:17:51 +0200 Subject: [PATCH 08/10] Fix throwing test for length modifier --- stdlib/Printf/test/runtests.jl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/stdlib/Printf/test/runtests.jl b/stdlib/Printf/test/runtests.jl index f97b572684097..d881c892dedc0 100644 --- a/stdlib/Printf/test/runtests.jl +++ b/stdlib/Printf/test/runtests.jl @@ -783,9 +783,9 @@ end end @testset "length modifiers" begin - @test_throws Printf.InvalidFormatStringError Printf.@sprintf("%h", 1) - @test_throws Printf.InvalidFormatStringError Printf.@sprintf("%hh", 1) - @test_throws Printf.InvalidFormatStringError Printf.@sprintf("%z", 1) + @test_throws Printf.InvalidFormatStringError Printf.Format("%h") + @test_throws Printf.InvalidFormatStringError Printf.Format("%hh") + @test_throws Printf.InvalidFormatStringError Printf.Format("%z") end end # @testset "Printf" From 0f872557a2852d0b9791dfe1866ab45cadc06af0 Mon Sep 17 00:00:00 2001 From: Sukera Date: Tue, 31 May 2022 20:09:03 +0200 Subject: [PATCH 09/10] Incorporate feedback & clarify mpfr-based printing errors --- stdlib/Printf/src/Printf.jl | 36 +++++++++++++++------------------- stdlib/Printf/test/runtests.jl | 4 ++-- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/stdlib/Printf/src/Printf.jl b/stdlib/Printf/src/Printf.jl index 03e57ce6a91b3..ce52ed959971a 100644 --- a/stdlib/Printf/src/Printf.jl +++ b/stdlib/Printf/src/Printf.jl @@ -88,20 +88,16 @@ function Base.showerror(io::IO, err::InvalidFormatStringError) io_has_color = get(io, :color, false) println(io, "InvalidFormatStringError: ", err.message) - print(io, "\t\"", @view(err.format[begin:prevind(err.format, err.start_color)])) + print(io, " \"", @view(err.format[begin:prevind(err.format, err.start_color)])) invalid_text = @view err.format[err.start_color:err.end_color] - if io_has_color - printstyled(io, invalid_text, color=:red) - else - print(io, invalid_text) - end + printstyled(io, invalid_text, color=:red) # +1 is okay, since all format characters are single bytes println(io, @view(err.format[err.end_color+1:end]), "\"") arrow_error = '-'^(length(invalid_text)-1) - arrow = "\t" * ' '^err.start_color * arrow_error * "^\n" + arrow = " " * ' '^err.start_color * arrow_error * "^\n" if io_has_color printstyled(io, arrow, color=:red) else @@ -199,7 +195,7 @@ function Format(f::AbstractString) b = bytes[pos] pos += 1 end - elseif b in b"Ljqtz" # what is q? Possibly quad? + elseif b in b"Ljqtz" # q was a synonym for ll above, see `man 3 printf`. Not to be used. pos > len && throw(InvalidFormatStringError("Length modifier is missing type specifier", f, last_percent_pos, pos-1)) b = bytes[pos] pos += 1 @@ -433,6 +429,10 @@ _snprintf(ptr, siz, str, arg) = @ccall "libmpfr".mpfr_snprintf(ptr::Ptr{UInt8}, siz::Csize_t, str::Ptr{UInt8}; arg::Ref{BigFloat})::Cint +# Arbitrary constant for a maximum number of bytes we want to output for a BigFloat. +# 8KiB seems like a reasonable default. Larger BigFloat representations should probably +# use a custom printing routine. Printing values with results larger than this ourselves +# seems like a dangerous thing to do. const __BIG_FLOAT_MAX__ = 8192 @inline function fmt(buf, pos, arg, spec::Spec{T}) where {T <: Floats} @@ -444,19 +444,15 @@ const __BIG_FLOAT_MAX__ = 8192 GC.@preserve buf begin siz = length(buf) - pos + 1 str = string(spec; modifier="R") - len = _snprintf(pointer(buf, pos), siz, str, x) - if len > siz - maxout = max(__BIG_FLOAT_MAX__, - ceil(Int, precision(x) * log(2) / log(10)) + 25) - # TODO: `error` is kind of generic, is there a more appropriate one to throw? - len > maxout && - error("Over $maxout bytes $len needed to output BigFloat $x") - resize!(buf, len + 1) - len = _snprintf(pointer(buf, pos), len + 1, str, x) + required_length = _snprintf(pointer(buf, pos), siz, str, x) + if required_length > siz + required_length > __BIG_FLOAT_MAX__ && + throw(ArgumentError("The given BigFloat requires $required_length bytes to be printed, which is more than the maximum of $__BIG_FLOAT_MAX__ bytes supported.")) + resize!(buf, required_length + 1) + required_length = _snprintf(pointer(buf, pos), required_length + 1, str, x) end - # TODO: when will this actually be thrown? What more informative error would be appropriate? - len > 0 || throw(ArgumentError("invalid printf formatting $str for BigFloat")) - return pos + len + required_length > 0 || throw(ArgumentError("The given BigFloat would produce less than the maximum allowed number of bytes $__BIG_FLOAT_MAX__, but still couldn't be printed fully for an unknown reason.")) + return pos + required_length end end x = Float64(x) diff --git a/stdlib/Printf/test/runtests.jl b/stdlib/Printf/test/runtests.jl index d881c892dedc0..40a6a763e4eac 100644 --- a/stdlib/Printf/test/runtests.jl +++ b/stdlib/Printf/test/runtests.jl @@ -444,8 +444,8 @@ end @test (Printf.@sprintf("%f", parse(BigFloat, "1e400"))) == "10000000000000000000000000000000000000000000000000000000000000000000000000000025262527574416492004687051900140830217136998040684679611623086405387447100385714565637522507383770691831689647535911648520404034824470543643098638520633064715221151920028135130764414460468236314621044034960475540018328999334468948008954289495190631358190153259681118693204411689043999084305348398480210026863210192871358464.000000" - # Check that does not attempt to output incredibly large amounts of digits - @test_throws ErrorException Printf.@sprintf("%f", parse(BigFloat, "1e99999")) + # Check that Printf does not attempt to output more than 8KiB worth of digits + @test_throws ArgumentError Printf.@sprintf("%f", parse(BigFloat, "1e99999")) # Check bug with precision > length of string @test Printf.@sprintf("%4.2s", "a") == " a" From e3866506bf242122b849544038da0dc31f559754 Mon Sep 17 00:00:00 2001 From: Sukera Date: Fri, 10 Jun 2022 16:49:52 +0200 Subject: [PATCH 10/10] Add NEWS.md entry --- NEWS.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/NEWS.md b/NEWS.md index e4ef4fef7002c..f49bdf60e828d 100644 --- a/NEWS.md +++ b/NEWS.md @@ -90,6 +90,9 @@ Standard library changes #### Printf +* Error messages for bad format strings have been improved, to make it clearer what & where in the + format string is wrong. ([#45366]) + #### Random * `randn` and `randexp` now work for any `AbstractFloat` type defining `rand` ([#44714]).