Skip to content

Commit

Permalink
Fix Lint::FormatParameterMismatch cop
Browse files Browse the repository at this point in the history
The cop failed to process format string in invalid format and raised exception. There are several cases when format string contains valid segments but these segments aren't compatible.

For instance numbered format cannot be mixed with unnumbered or named. Such format string will lead to ArgumentError when code is executed but sometimes it's needed e.g. in tests to check how the code such invalid format strings.
  • Loading branch information
andrykonchin authored and bbatsov committed Jun 7, 2020
1 parent 672e7fa commit 937f549
Show file tree
Hide file tree
Showing 5 changed files with 61 additions and 0 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -4571,3 +4572,4 @@
[@jschneid]: https://github.com/jschneid
[@ric2b]: https://github.com/ric2b
[@burnettk]: https://github.com/burnettk
[@andrykonchin]: https://github.com/andrykonchin
3 changes: 3 additions & 0 deletions lib/rubocop/cop/lint/format_parameter_mismatch.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ def offending_node?(node)
num_of_format_args, num_of_expected_fields = count_matches(node)

return false if num_of_format_args == :unknown
return false if num_of_expected_fields == :unknown

matched_arguments_count?(num_of_expected_fields, num_of_format_args)
end
Expand Down Expand Up @@ -113,6 +114,8 @@ def expected_fields_count(node)
return :unknown unless node.str_type?

format_string = RuboCop::Cop::Utils::FormatString.new(node.source)

return :unknown unless format_string.valid?
return 1 if format_string.named_interpolation?

max_digit_dollar_num = format_string.max_digit_dollar_num
Expand Down
18 changes: 18 additions & 0 deletions lib/rubocop/cop/utils/format_string.rb
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ def format_sequences
@format_sequences ||= parse
end

def valid?
!mixed_formats?
end

def named_interpolation?
format_sequences.any?(&:name)
end
Expand All @@ -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
Expand Down
6 changes: 6 additions & 0 deletions spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,12 @@
end
end

context 'when format is invalid' do
it 'does not register an offense' do
expect_no_offenses("format('%s %2$s', 'foo', 'bar')")
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
Expand Down
32 changes: 32 additions & 0 deletions spec/rubocop/cop/utils/format_string_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -80,4 +80,36 @@ def format_sequences(string)
it_behaves_like 'named format sequence', '%+0<num>8.2f'
it_behaves_like 'named format sequence', '%+08<num>.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

0 comments on commit 937f549

Please sign in to comment.