From 73575d189b61ae635c46c0229d306819bdf7fca1 Mon Sep 17 00:00:00 2001 From: Daniel Vandersluis Date: Sat, 29 May 2021 14:26:30 -0400 Subject: [PATCH] [Fix #9836] Fix incorrect corrections for `Layout/HashAlignment` when a `kwsplat` node is on the same line as a `pair` node with table style. Keyword splats were previously treated as pairs using colon style, which some hacks in the Alignment classes to handle them. However, this did not account for all possibilities and would cause previous pairs on the same line as the `kwsplat` to be removed by autocorrection. Instead, now a special `KeywordSplatAlignment` type was added to handle `kwsplat`s. The previously behaviour about `kwsplat`s was retained (they will still be aligned with the beginning of the rest of the hash, regardless of enforced style), and the hacks in `KeyAlignment` and `TableAlignment` for `kwsplat`s were removed. --- .../fix_fix_incorrect_corrections_for.md | 1 + config/default.yml | 2 +- lib/rubocop/cop/layout/hash_alignment.rb | 15 +- .../cop/mixin/hash_alignment_styles.rb | 17 ++- .../rubocop/cop/layout/hash_alignment_spec.rb | 144 +++++++++++++++++- 5 files changed, 171 insertions(+), 8 deletions(-) create mode 100644 changelog/fix_fix_incorrect_corrections_for.md diff --git a/changelog/fix_fix_incorrect_corrections_for.md b/changelog/fix_fix_incorrect_corrections_for.md new file mode 100644 index 00000000000..d2b6db55705 --- /dev/null +++ b/changelog/fix_fix_incorrect_corrections_for.md @@ -0,0 +1 @@ +* [#9836](https://github.com/rubocop/rubocop/issues/9836): Fix incorrect corrections for `Layout/HashAlignment` when a `kwsplat` node is on the same line as a `pair` node with table style. ([@dvandersluis][]) diff --git a/config/default.yml b/config/default.yml index f7e1d07640d..073d1ea1e3b 100644 --- a/config/default.yml +++ b/config/default.yml @@ -772,7 +772,7 @@ Layout/HashAlignment: Enabled: true AllowMultipleStyles: true VersionAdded: '0.49' - VersionChanged: '0.77' + VersionChanged: '<>' # Alignment of entries using hash rocket as separator. Valid values are: # # key - left alignment of keys diff --git a/lib/rubocop/cop/layout/hash_alignment.rb b/lib/rubocop/cop/layout/hash_alignment.rb index a55f5011c89..c70892c3cbd 100644 --- a/lib/rubocop/cop/layout/hash_alignment.rb +++ b/lib/rubocop/cop/layout/hash_alignment.rb @@ -185,7 +185,9 @@ class HashAlignment < Base SeparatorAlignment => 'Align the separators of a hash ' \ 'literal if they span more than one line.', TableAlignment => 'Align the keys and values of a hash ' \ - 'literal if they span more than one line.' }.freeze + 'literal if they span more than one line.', + KeywordSplatAlignment => 'Align keyword splats with the ' \ + 'rest of the hash if it spans more than one line.' }.freeze def on_send(node) return if double_splat?(node) @@ -247,7 +249,14 @@ def check_pairs(node) end def add_offences + kwsplat_offences = offences_by.delete(KeywordSplatAlignment) + register_offences_with_format(kwsplat_offences, KeywordSplatAlignment) + format, offences = offences_by.min_by { |_, v| v.length } + register_offences_with_format(offences, format) + end + + def register_offences_with_format(offences, format) (offences || []).each do |offence| add_offense(offence, message: MESSAGES[format]) do |corrector| delta = column_deltas[alignment_for(offence).first.class][offence] @@ -275,7 +284,9 @@ def ignore_hash_argument?(node) end def alignment_for(pair) - if pair.hash_rocket? + if pair.kwsplat_type? + [KeywordSplatAlignment.new] + elsif pair.hash_rocket? alignment_for_hash_rockets else alignment_for_colons diff --git a/lib/rubocop/cop/mixin/hash_alignment_styles.rb b/lib/rubocop/cop/mixin/hash_alignment_styles.rb index 8282672a05b..273934b04cf 100644 --- a/lib/rubocop/cop/mixin/hash_alignment_styles.rb +++ b/lib/rubocop/cop/mixin/hash_alignment_styles.rb @@ -43,7 +43,7 @@ def separator_delta(pair) end def value_delta(pair) - return 0 if pair.kwsplat_type? || pair.value_on_new_line? + return 0 if pair.value_on_new_line? correct_value_column = pair.loc.operator.end.column + 1 actual_value_column = pair.value.loc.column @@ -108,8 +108,6 @@ def hash_rocket_delta(first_pair, current_pair) end def value_delta(first_pair, current_pair) - return 0 if current_pair.kwsplat_type? - correct_value_column = first_pair.key.loc.column + current_pair.delimiter(true).length + max_key_width @@ -139,6 +137,19 @@ def value_delta(first_pair, current_pair) first_pair.value_delta(current_pair) end end + + # Handles calculation of deltas for `kwsplat` nodes. + # This is a special case that just ensures the kwsplat is aligned with the rest of the hash + # since a `kwsplat` does not have a key, separator or value. + class KeywordSplatAlignment + def deltas(first_pair, current_pair) + if Util.begins_its_line?(current_pair.source_range) + { key: first_pair.key_delta(current_pair) } + else + {} + end + end + end end end end diff --git a/spec/rubocop/cop/layout/hash_alignment_spec.rb b/spec/rubocop/cop/layout/hash_alignment_spec.rb index 2ef6f99e3f4..bf99bfced84 100644 --- a/spec/rubocop/cop/layout/hash_alignment_spec.rb +++ b/spec/rubocop/cop/layout/hash_alignment_spec.rb @@ -540,7 +540,7 @@ def example expect_offense(<<~RUBY) Hash(foo: 'bar', **extra_params - ^^^^^^^^^^^^^^ Align the keys of a hash literal if they span more than one line. + ^^^^^^^^^^^^^^ Align keyword splats with the rest of the hash if it spans more than one line. ) RUBY @@ -555,7 +555,7 @@ def example expect_offense(<<~RUBY) {foo: 'bar', **extra_params - ^^^^^^^^^^^^^^ Align the keys of a hash literal if they span more than one line. + ^^^^^^^^^^^^^^ Align keyword splats with the rest of the hash if it spans more than one line. } RUBY @@ -1127,4 +1127,144 @@ def self.scenarios_order it 'register no offense for yield without args' do expect_no_offenses('yield') end + + context 'with `EnforcedColonStyle`: `table`' do + let(:cop_config) do + { + 'EnforcedColonStyle' => 'table' + } + end + + context 'and misaligned keys' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + foo ab: 1, + c: 2 + ^^^^ Align the keys and values of a hash literal if they span more than one line. + RUBY + + expect_correction(<<~RUBY) + foo ab: 1, + c: 2 + RUBY + end + end + + context 'when the only item is a kwsplat' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + foo({**rest}) + RUBY + end + end + + context 'and a double splat argument after a hash key' do + it 'registers an offense on the misaligned key and corrects' do + expect_offense(<<~RUBY) + foo ab: 1, + c: 2, **rest + ^^^^ Align the keys and values of a hash literal if they span more than one line. + RUBY + + expect_correction(<<~RUBY) + foo ab: 1, + c: 2, **rest + RUBY + end + end + + context 'and aligned keys but a double splat argument after' do + it 'does not register an offense on the `kwsplat`' do + expect_no_offenses(<<~RUBY) + foo a: 1, + b: 2, **rest + RUBY + end + end + + context 'and a misaligned double splat argument' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + foo a: 1, + **rest + ^^^^^^ Align keyword splats with the rest of the hash if it spans more than one line. + RUBY + + expect_correction(<<~RUBY) + foo a: 1, + **rest + RUBY + end + end + end + + context 'with `EnforcedHashRocketStyle`: `table`' do + let(:cop_config) do + { + 'EnforcedHashRocketStyle' => 'table' + } + end + + context 'and misaligned keys' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + foo :ab => 1, + :c => 2 + ^^^^^^^ Align the keys and values of a hash literal if they span more than one line. + RUBY + + expect_correction(<<~RUBY) + foo :ab => 1, + :c => 2 + RUBY + end + end + + context 'when the only item is a kwsplat' do + it 'does not register an offense' do + expect_no_offenses(<<~RUBY) + foo({**rest}) + RUBY + end + end + + context 'and a double splat argument after a hash key' do + it 'registers an offense on the misaligned key and corrects' do + expect_offense(<<~RUBY) + foo :ab => 1, + :c => 2, **rest + ^^^^^^^ Align the keys and values of a hash literal if they span more than one line. + RUBY + + expect_correction(<<~RUBY) + foo :ab => 1, + :c => 2, **rest + RUBY + end + end + + context 'and aligned keys but a double splat argument after' do + it 'does not register an offense on the `kwsplat`' do + expect_no_offenses(<<~RUBY) + foo :a => 1, + :b => 2, **rest + RUBY + end + end + + context 'and a misaligned double splat argument' do + it 'registers an offense and corrects' do + expect_offense(<<~RUBY) + foo :a => 1, + **rest + ^^^^^^ Align keyword splats with the rest of the hash if it spans more than one line. + RUBY + + expect_correction(<<~RUBY) + foo :a => 1, + **rest + RUBY + end + end + end end