Fix Lint::FormatParameterMismatch cop

[andrykonchin]

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 handles such invalid format strings.

Example of such code - tests in RubySpec project

Changes

• check whether current format string is valid in Lint::FormatParameterMismatch cope
• added offense "Format string is invalid because formatting sequence types (numbered, named or unnumbered) are mixed." it if it's invalid
• added #valid? method to Utils::FormatString class to check mixing of formats

Example

Running Rubocop on this code (where numbered format is mixed with unnumbered):

format('%s %2$s', 'foo', 'bar')

leads to the following error

     ArgumentError:
       comparison of Integer with nil failed
     # ./lib/rubocop/cop/utils/format_string.rb:105:in `max'
     # ./lib/rubocop/cop/utils/format_string.rb:105:in `max_digit_dollar_num'
     # ./lib/rubocop/cop/lint/format_parameter_mismatch.rb:118:in `expected_fields_count'
     # ./lib/rubocop/cop/lint/format_parameter_mismatch.rb:96:in `count_format_matches'
     # ./lib/rubocop/cop/lint/format_parameter_mismatch.rb:79:in `count_matches'
     # ./lib/rubocop/cop/lint/format_parameter_mismatch.rb:43:in `offending_node?'
     # ./lib/rubocop/cop/lint/format_parameter_mismatch.rb:31:in `on_send'
     # ./lib/rubocop/cop/commissioner.rb:57:in `block (2 levels) in trigger_responding_cops'
     # ./lib/rubocop/cop/commissioner.rb:136:in `with_cop_error_handling'
     # ./lib/rubocop/cop/commissioner.rb:56:in `block in trigger_responding_cops'
     # ./lib/rubocop/cop/commissioner.rb:55:in `each'
     # ./lib/rubocop/cop/commissioner.rb:55:in `trigger_responding_cops'
     # ./lib/rubocop/cop/commissioner.rb:32:in `block (2 levels) in <class:Commissioner>'
     # ./lib/rubocop/cop/commissioner.rb:44:in `investigate'
     # ./lib/rubocop/rspec/cop_helper.rb:75:in `_investigate'
     # ./lib/rubocop/rspec/cop_helper.rb:22:in `inspect_source'
     # ./lib/rubocop/rspec/expect_offense.rb:127:in `expect_no_offenses'
     # ./spec/rubocop/cop/lint/format_parameter_mismatch_spec.rb:168:in `block (3 levels) in <top (required)>'

---

Before submitting the PR make sure the following are checked:

• Wrote good commit messages.
• Commit message starts with [Fix #issue-number] (if the related issue exists).
• Feature branch is up-to-date with master (if not - rebase it).
• Squashed related commits together.
• Added tests.
• Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
• The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
• Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

[andrykonchin]

Please let me know if this change isn't welcome.

[marcandre]

Looks like a great PR. I was curious: wouldn't it be best to consider this an offense? Or do you feel it would be better in a different cop?

[andrykonchin]

Oh, well. I didn't think about this. Yes, it definitely makes sense to add an offense in this case.

Regarding adding new cope. It looks reasonable to add a new cope and not to change/expand semantic and responsibility of the current one. On the other hand adding new cope may have some performance impact and lead to code duplication - it will be very similar to the Lint::FormatParameterMismatch.

Anyway I would like to add a new offense if it's welcome.

[andrykonchin]

Just fixed conflict with master in the changelog file.

[marcandre]

Anyway I would like to add a new offense if it's welcome.

Given the fact that these offenses are going to be extra super rare, that you're already doing all the work, and that this cop is already checking for errors with format, I would say go for it. We could always add a setting if someone comes with a valid user case.

[andrykonchin]

Got it.

So what is the best way to add new offense from your point of view? To create a new cop or update this one?

[marcandre]

I would update this one :-)

[andrykonchin]

Done

[bbatsov]

The cop documentation has to be updated as well - now it doesn't mention anywhere this class of problems.

[andrykonchin]

Updated both legacy and asciidoc documentation.

[andrykonchin]

Could anybody please re-run specs on CI?

The only failing environment is JRuby. Specs are passing when I run them repeatedly but fails with some particular seed 57643. It isn't JRuby specific issue and specs fail with this seed on MRI as well (Ruby 2.7)

[marcandre]

I can't wait to set the defaults properly. These error message are completely un-informative "expect(cop.offenses.empty?).to be(false) got true".

I don't see a way to restart the CI. Just amend your last commit (a change in the datestamp is sufficient) and force push again...

[andrykonchin]

The only checks failed are:

• ci/circleci: ruby-head-ascii_spec
• ci/circleci: ruby-head-spec

I have no idea what does cause it and not sure whether it's related to my changes in this PR - locally rake default command passes successfully.

[andrykonchin]

I noticed that these checks fail on all the PR merged recently. So I believe that changes in this PR are safe to merge as well.

cc @marcandre @bbatsov

[bbatsov]

Thanks for tackling this!