diff --git a/CHANGELOG.md b/CHANGELOG.md index 73f478c19c2..c57f066eaa4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ * [#8081](https://github.com/rubocop-hq/rubocop/issues/8081): Fix a false positive for `Lint/SuppressedException` when empty rescue block in `do` block. ([@koic][]) * [#8096](https://github.com/rubocop-hq/rubocop/issues/8096): Fix a false positive for `Lint/SuppressedException` when empty rescue block in defs. ([@koic][]) * [#8108](https://github.com/rubocop-hq/rubocop/issues/8108): Fix infinite loop in `Layout/HeredocIndentation` auto-correct. ([@jonas054][]) +* [#8042](https://github.com/rubocop-hq/rubocop/pull/8042): Fix raising error in `Lint::FormatParameterMismatch` when it handles invalid format strings and add new offense. ([@andrykonchin][]) ## 0.85.0 (2020-06-01) @@ -4571,3 +4572,4 @@ [@jschneid]: https://github.com/jschneid [@ric2b]: https://github.com/ric2b [@burnettk]: https://github.com/burnettk +[@andrykonchin]: https://github.com/andrykonchin diff --git a/docs/modules/ROOT/pages/cops_lint.adoc b/docs/modules/ROOT/pages/cops_lint.adoc index afc2248b5da..a23ce2c6a20 100644 --- a/docs/modules/ROOT/pages/cops_lint.adoc +++ b/docs/modules/ROOT/pages/cops_lint.adoc @@ -1100,6 +1100,10 @@ This lint sees if there is a mismatch between the number of expected fields for format/sprintf/#% and what is actually passed as arguments. +In addition it checks whether different formats are used in the same +format string. Do not mix numbered, unnumbered, and named formats in +the same format string. + === Examples [source,ruby] @@ -1116,6 +1120,20 @@ format('A value: %s and another: %i', a_value) format('A value: %s and another: %i', a_value, another) ---- +[source,ruby] +---- +# bad + +format('Unnumbered format: %s and numbered: %2$s', a_value, another) +---- + +[source,ruby] +---- +# good + +format('Numbered format: %1$s and numbered %2$s', a_value, another) +---- + == Lint/HeredocMethodCallPosition |=== diff --git a/lib/rubocop/cop/lint/format_parameter_mismatch.rb b/lib/rubocop/cop/lint/format_parameter_mismatch.rb index cee8f1184de..e5cf62c0ada 100644 --- a/lib/rubocop/cop/lint/format_parameter_mismatch.rb +++ b/lib/rubocop/cop/lint/format_parameter_mismatch.rb @@ -7,6 +7,10 @@ module Lint # expected fields for format/sprintf/#% and what is actually # passed as arguments. # + # In addition it checks whether different formats are used in the same + # format string. Do not mix numbered, unnumbered, and named formats in + # the same format string. + # # @example # # # bad @@ -18,16 +22,37 @@ module Lint # # good # # format('A value: %s and another: %i', a_value, another) + # + # @example + # + # # bad + # + # format('Unnumbered format: %s and numbered: %2$s', a_value, another) + # + # @example + # + # # good + # + # format('Numbered format: %1$s and numbered %2$s', a_value, another) class FormatParameterMismatch < Cop # http://rubular.com/r/CvpbxkcTzy MSG = "Number of arguments (%i) to `%s` doesn't " \ 'match the number of fields (%i).' + MSG_INVALID = 'Format string is invalid because formatting sequence types ' \ + '(numbered, named or unnumbered) are mixed.' KERNEL = 'Kernel' SHOVEL = '<<' STRING_TYPES = %i[str dstr].freeze def on_send(node) + return unless format_string?(node) + + if invalid_format_string?(node) + add_offense(node, location: :selector, message: MSG_INVALID) + return + end + return unless offending_node?(node) add_offense(node, location: :selector) @@ -35,9 +60,15 @@ def on_send(node) private + def format_string?(node) + called_on_string?(node) && method_with_format_args?(node) + end + + def invalid_format_string?(node) + !RuboCop::Cop::Utils::FormatString.new(node.source).valid? + end + def offending_node?(node) - return false unless called_on_string?(node) - return false unless method_with_format_args?(node) return false if splat_args?(node) num_of_format_args, num_of_expected_fields = count_matches(node) diff --git a/lib/rubocop/cop/utils/format_string.rb b/lib/rubocop/cop/utils/format_string.rb index 5242fe5b2ba..383d9e72ddc 100644 --- a/lib/rubocop/cop/utils/format_string.rb +++ b/lib/rubocop/cop/utils/format_string.rb @@ -97,6 +97,10 @@ def format_sequences @format_sequences ||= parse end + def valid? + !mixed_formats? + end + def named_interpolation? format_sequences.any?(&:name) end @@ -114,6 +118,20 @@ def parse ) end end + + def mixed_formats? + formats = format_sequences.map do |seq| + if seq.name + :named + elsif seq.max_digit_dollar_num + :numbered + else + :unnumbered + end + end + + formats.uniq.size > 1 + end end end end diff --git a/spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb b/spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb index ffe1745570d..c0d9601f837 100644 --- a/spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb +++ b/spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb @@ -171,6 +171,15 @@ end end + context 'when format is invalid' do + it 'registers an offense' do + expect_offense(<<~RUBY) + format('%s %2$s', 'foo', 'bar') + ^^^^^^ Format string is invalid because formatting sequence types (numbered, named or unnumbered) are mixed. + RUBY + end + end + # Regression: https://github.com/rubocop-hq/rubocop/issues/3869 context 'when passed an empty array' do it 'does not register an offense' do diff --git a/spec/rubocop/cop/utils/format_string_spec.rb b/spec/rubocop/cop/utils/format_string_spec.rb index 8b9e64b0063..269f92f50bf 100644 --- a/spec/rubocop/cop/utils/format_string_spec.rb +++ b/spec/rubocop/cop/utils/format_string_spec.rb @@ -80,4 +80,36 @@ def format_sequences(string) it_behaves_like 'named format sequence', '%+08.2f' it_behaves_like 'named format sequence', '%+08.2f' end + + describe '#valid?' do + it 'returns true when there are only unnumbered formats' do + fs = described_class.new('%s %d') + expect(fs.valid?).to eq true + end + + it 'returns true when there are only numbered formats' do + fs = described_class.new('%1$s %2$d') + expect(fs.valid?).to eq true + end + + it 'returns true when there are only named formats' do + fs = described_class.new('%{foo}s') + expect(fs.valid?).to eq true + end + + it 'returns false when there are unnumbered and numbered formats' do + fs = described_class.new('%s %1$d') + expect(fs.valid?).to eq false + end + + it 'returns false when there are unnumbered and named formats' do + fs = described_class.new('%s %{foo}d') + expect(fs.valid?).to eq false + end + + it 'returns false when there are numbered and named formats' do + fs = described_class.new('%1$s %{foo}d') + expect(fs.valid?).to eq false + end + end end