From 36d21ffb7a07ab29662f7f9f3f061920c51a7527 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Fri, 19 Aug 2022 11:07:23 +0800 Subject: [PATCH 1/2] Disallow mixing of sequential and named `sprintf` parameters --- spec/std/sprintf_spec.cr | 48 ++++++++++++++++++ src/kernel.cr | 4 +- src/string/formatter.cr | 103 ++++++++++++++++++++++----------------- 3 files changed, 109 insertions(+), 46 deletions(-) diff --git a/spec/std/sprintf_spec.cr b/spec/std/sprintf_spec.cr index 68a567dca123..a4de6a9060db 100644 --- a/spec/std/sprintf_spec.cr +++ b/spec/std/sprintf_spec.cr @@ -35,6 +35,24 @@ describe "::sprintf" do assert_sprintf "%%*%%", [1, 2, 3], "%*%" end + it "doesn't accept modifiers for %%" do + expect_raises(ArgumentError) { sprintf("%0%") } + expect_raises(ArgumentError) { sprintf("%+%") } + expect_raises(ArgumentError) { sprintf("%-%") } + expect_raises(ArgumentError) { sprintf("% %") } + expect_raises(ArgumentError) { sprintf("%#%") } + expect_raises(ArgumentError) { sprintf("%.0%") } + expect_raises(ArgumentError) { sprintf("%*%", 1) } + + expect_raises(ArgumentError) { sprintf("%0%") } + expect_raises(ArgumentError) { sprintf("%+%") } + expect_raises(ArgumentError) { sprintf("%-%") } + expect_raises(ArgumentError) { sprintf("% %") } + expect_raises(ArgumentError) { sprintf("%#%") } + expect_raises(ArgumentError) { sprintf("%.0%") } + expect_raises(ArgumentError) { sprintf("%*%", 1) } + end + context "integers" do context "base specifier" do it "supports base 2" do @@ -437,6 +455,13 @@ describe "::sprintf" do expect_raises(ArgumentError, "One hash or named tuple required") { sprintf("change %{this}", "this") } end + it "doesn't raise if 1-element list of hash or named tuple given" do + assert_sprintf "change %{this}", [{"this" => "nothing"}], "change nothing" + assert_sprintf "change %{this}", [{this: "nothing"}], "change nothing" + assert_sprintf "change %{this}", { {"this" => "nothing"} }, "change nothing" + assert_sprintf "change %{this}", { {this: "nothing"} }, "change nothing" + end + it "raises on unbalanced curly" do expect_raises(ArgumentError, "Malformed name - unmatched parenthesis") { sprintf("change %{this", {"this" => 1}) } end @@ -444,6 +469,14 @@ describe "::sprintf" do it "doesn't raise on balanced curly with null byte" do assert_sprintf "change %{this\u{0}}", {"this\u{0}" => 1}, "change 1" end + + it "raises if sequential parameters also given" do + expect_raises(ArgumentError, "Cannot mix named parameters with sequential ones") { sprintf("%{this}%d", {"this" => 1}) } + end + + it "doesn't raise if formatted substitution also given" do + assert_sprintf "%{foo}%s", {"foo" => "x", "bar" => "y"}, "xy" + end end context "formatted substitution" do @@ -451,5 +484,20 @@ describe "::sprintf" do assert_sprintf "change %.2f", {"this" => 23.456}, "change 23.46" assert_sprintf "change %.2f", {this: 23.456}, "change 23.46" end + + it "raises if sequential parameters also given" do + expect_raises(ArgumentError, "Cannot mix named parameters with sequential ones") { sprintf("%d%d", {"this" => 1}) } + end + + it "doesn't raise if plain substitution also given" do + assert_sprintf "%s%{bar}", {"foo" => "x", "bar" => "y"}, "xy" + end + end + + context "sequential parameters" do + it "raises if named parameters also given" do + expect_raises(ArgumentError, "Cannot mix sequential parameters with named ones") { sprintf("%d%{this}", 1) } + expect_raises(ArgumentError, "Cannot mix sequential parameters with named ones") { sprintf("%d%d", 1) } + end end end diff --git a/src/kernel.cr b/src/kernel.cr index efa06b3d6676..6653536f285c 100644 --- a/src/kernel.cr +++ b/src/kernel.cr @@ -166,6 +166,9 @@ end # %[flags][width][.precision]type # ``` # +# The percent sign itself is escaped by `%%`. No format modifiers are allowed, +# and no arguments are consumed. +# # The field type controls how the corresponding argument value is to be # interpreted, while the flags modify that interpretation. # @@ -208,7 +211,6 @@ end # s | Argument is a string to be substituted. If the format # | sequence contains a precision, at most that many characters # | will be copied. -# % | A percent sign itself will be displayed. No argument taken. # ``` # # Flags modify the behavior of the format specifiers: diff --git a/src/string/formatter.cr b/src/string/formatter.cr index b39f16f30f1c..dc12526f870d 100644 --- a/src/string/formatter.cr +++ b/src/string/formatter.cr @@ -2,8 +2,19 @@ require "c/stdio" # :nodoc: struct String::Formatter(A) + private enum Mode + None + + # `%s`, index type is `Nil` + Sequential + + # `%{a}` or `%s`, index type is `String` + Named + end + @format_buf : Pointer(UInt8)? @temp_buf : Pointer(UInt8)? + @arg_mode : Mode = :none def initialize(string, @args : A, @io : IO) @reader = Char::Reader.new(string) @@ -18,7 +29,7 @@ struct String::Formatter(A) when '%' consume_percent else - char char + @io << char end next_char end @@ -32,33 +43,27 @@ struct String::Formatter(A) when '<' next_char consume_formatted_substitution + when '%' + @io << '%' else flags = consume_flags - consume_type flags + consume_type flags, nil end end private def consume_substitution key = consume_substitution_key '}' - arg = current_arg - if arg.is_a?(Hash) || arg.is_a?(NamedTuple) - @io << arg[key] - else - raise ArgumentError.new "One hash or named tuple required" - end + # note: "`@io << (arg_at(key))` has no type" without this `arg` variable + arg = arg_at(key) + @io << arg end private def consume_formatted_substitution key = consume_substitution_key '>' + target_arg = arg_at(key) next_char - arg = current_arg - if arg.is_a?(Hash) || arg.is_a?(NamedTuple) - target_arg = arg[key] - else - raise ArgumentError.new "One hash or named tuple required" - end flags = consume_flags - consume_type flags, target_arg, true + consume_type flags, key end private def consume_substitution_key(end_char) @@ -146,10 +151,9 @@ struct String::Formatter(A) end private def consume_dynamic_value - value = current_arg + value = arg_at(nil) if value.is_a?(Int) next_char - next_arg value.to_i else raise ArgumentError.new("Expected dynamic value '*' to be an Int - #{value.inspect} (#{value.class.inspect})") @@ -174,48 +178,44 @@ struct String::Formatter(A) {num, size} end - private def consume_type(flags, arg = nil, arg_specified = false) + private def consume_type(flags, index) + arg = arg_at(index) + case char = current_char when 'c' - char flags, arg, arg_specified + char flags, arg when 's' - string flags, arg, arg_specified + string flags, arg when 'b' flags.base = 2 flags.type = char - int flags, arg, arg_specified + int flags, arg when 'o' flags.base = 8 flags.type = char - int flags, arg, arg_specified + int flags, arg when 'd', 'i' flags.base = 10 - int flags, arg, arg_specified + int flags, arg when 'x', 'X' flags.base = 16 flags.type = char - int flags, arg, arg_specified + int flags, arg when 'a', 'A', 'e', 'E', 'f', 'g', 'G' flags.type = char - float flags, arg, arg_specified - when '%' - char '%' + float flags, arg else raise ArgumentError.new("Malformed format string - %#{char.inspect}") end end - def char(flags, arg, arg_specified) : Nil - arg = next_arg unless arg_specified - + def char(flags, arg) : Nil pad 1, flags if flags.left_padding? @io << arg pad 1, flags if flags.right_padding? end - def string(flags, arg, arg_specified) : Nil - arg = next_arg unless arg_specified - + def string(flags, arg) : Nil if precision = flags.precision arg = arg.to_s[0...precision] end @@ -225,9 +225,7 @@ struct String::Formatter(A) pad arg.to_s.size, flags if flags.right_padding? end - def int(flags, arg, arg_specified) : Nil - arg = next_arg unless arg_specified - + def int(flags, arg) : Nil raise ArgumentError.new("Expected an integer, not #{arg.inspect}") unless arg.responds_to?(:to_i) int = arg.is_a?(Int) ? arg : arg.to_i @@ -284,9 +282,7 @@ struct String::Formatter(A) end # We don't actually format the float ourselves, we delegate to snprintf - def float(flags, arg, arg_specified) : Nil - arg = next_arg unless arg_specified - + def float(flags, arg) : Nil if arg.responds_to?(:to_f64) float = arg.is_a?(Float64) ? arg : arg.to_f64 @@ -347,16 +343,33 @@ struct String::Formatter(A) pad size, flags end - def char(char) : Nil - @io << char + private def arg_at(index : Nil) + args_are :sequential + arg = @args.fetch(@arg_index) { raise ArgumentError.new("Too few arguments") } + @arg_index += 1 + arg end - private def current_arg - @args.fetch(@arg_index) { raise ArgumentError.new("Too few arguments") } + private def arg_at(index : String) + args_are :named + args = @args + # note: "index '0' out of bounds for empty tuple" without the `is_a?` check + # TODO: use `Tuple()` once support for 1.0.0 is dropped + if args.size == 1 && !args.is_a?(Tuple(*typeof(Tuple.new))) + arg = args[0] + if arg.is_a?(Hash) || arg.is_a?(NamedTuple) + return arg[index] + end + end + raise ArgumentError.new "One hash or named tuple required" end - def next_arg - current_arg.tap { @arg_index += 1 } + private def args_are(mode : Mode) + if @arg_mode.none? + @arg_mode = mode + elsif mode != @arg_mode + raise ArgumentError.new "Cannot mix #{@arg_mode.to_s.downcase} parameters with #{mode.to_s.downcase} ones" + end end private def current_char From 79faeca214e252da1924958fce5447962923ef40 Mon Sep 17 00:00:00 2001 From: Quinton Miller Date: Wed, 24 Aug 2022 05:08:09 +0800 Subject: [PATCH 2/2] fixup --- src/string/formatter.cr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/string/formatter.cr b/src/string/formatter.cr index dc12526f870d..a576c95ef4eb 100644 --- a/src/string/formatter.cr +++ b/src/string/formatter.cr @@ -60,7 +60,7 @@ struct String::Formatter(A) private def consume_formatted_substitution key = consume_substitution_key '>' - target_arg = arg_at(key) + args_are :named next_char flags = consume_flags consume_type flags, key