Bad interaction between `RSpec/LetBeforeExamples` and `RSpec/ScatteredLet`

[marcandre]

On this source file

# running:
> rubocop -A spec/rubocop/ast/node_pattern_spec.rb --only RSpec/LetBeforeExamples,RSpec/ScatteredLet

Inspecting 1 file
C

Offenses:

spec/rubocop/ast/node_pattern_spec.rb:1632:9: C: [Corrected] RSpec/LetBeforeExamples: Move let before the examples in the group.
        let(:captured_vals) { ['world', :hello] }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/rubocop/ast/node_pattern_spec.rb:1632:9: C: [Corrected] RSpec/ScatteredLet: Group all let/let! blocks in the example group together.
        let(:captured_vals) { ['world', :hello] }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/rubocop/ast/node_pattern_spec.rb:1640:9: C: [Corrected] RSpec/LetBeforeExamples: Move let before the examples in the group.
        let(:captured_vals) { [:hello, 3] }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
spec/rubocop/ast/node_pattern_spec.rb:1640:9: C: [Corrected] RSpec/ScatteredLet: Group all let/let! blocks in the example group together.
        let(:captured_vals) { [:hello, 3] }
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

These lets are duplicated by the autocorrection:

diff --git a/spec/rubocop/ast/node_pattern_spec.rb b/spec/rubocop/ast/node_pattern_spec.rb
index f3903ac..17fa0d9 100644
--- a/spec/rubocop/ast/node_pattern_spec.rb
+++ b/spec/rubocop/ast/node_pattern_spec.rb
@@ -1626,18 +1626,20 @@ RSpec.describe RuboCop::AST::NodePattern do
 
       context 'with an ellipsis inside and outside' do
         let(:pattern) { '(array <(str $_) (sym $_) ...> ...)' }
+        let(:captured_vals) { ['world', :hello] }
 
+        let(:captured_vals) { ['world', :hello] }
         it_behaves_like 'multiple capture'
 
-        let(:captured_vals) { ['world', :hello] }
       end
 
       context 'doubled with ellipsis' do
         let(:pattern) { '(array <(sym $_) ...> <(int $_) ...>)' }
+        let(:captured_vals) { [:hello, 3] }
 
+        let(:captured_vals) { [:hello, 3] }
         it_behaves_like 'multiple capture'
 
-        let(:captured_vals) { [:hello, 3] }
       end

[pirj]

Interesting!

The same line of code triggers two cops. Both cops auto-correct, but one the same code one after moves the offensive line to "after the first let", and another to "before the first example". Internally, they both call corrector.remove(node) for the offensive let and both call insert_before/insert_after resulting in a duplicate let.

Exactly the same happens if you call the same sequence from inside a single cop:

corrector.remove(range)
corrector.insert_after(position_one, source)
corrector.remove(range)
corrector.insert_before(position_two, source)

No warnings/errors even when wrapped inside corrector.transaction do, neither for removing the same range (because it's defined as replace(range, '')), nor clobbering.

I guess one solution would be to get the source not from the node itself, since e.g. node.loc.expression is not affected in any way by replace(range, ''), but from the already rewritten source, that would insert an empty string on the second insert. But TreeRewriter does not apply incrementally, it accumulates changes and applies them all in one go on process, so this doesn't seem possible.

Those two remove operate on exactly the same range, so crossing_deletions policy is not violated. different_replacements policy is not violated either, both times it's a replacement with an empty string.

Given all this, I'm not sure if this can be quickfixed in RuboCop itself without involving performance-impacting changes (process rewrites after each cop's correction, re-detect offences for previously triggered cops after correction).

I don't have an idea how to fix this on cops' level either. @bquorning @Darhazer wdyt?

Just in case - it's not a regression, checked on RuboCop 0.68.1, it behaves exactly the same way.

[marcandre]

Interesting, I probably should have added a swallowed_deletion policy, I'll keep that in mind whenever I finish my refactor. In any case, RuboCop's policy for :crossing_deletions (which is worse imo) is to :accept... Not too sure why, or what the effect would be if that were changed to :raise. It would probably be a good idea, as we silently rescue those exceptions anyways, and the impacted cop is retried afterwards anyways.

Shouldn't ScatteredLet simply be turned off when LetBeforeExamples is on? Otherwise maybe they should be combined into LetPosition?

[pirj]

RuboCop's policy for :crossing_deletions (which is worse imo) is to :accept

Can't say anything about that. I can imagine two cops, one operating on the statement and its first argument, and another - at all arguments. Should a warning be issued each time they cross? Or should the auto-correction of one of them be ignored? Or should detection and correction of the second be done at the second pass? 🤷‍♂️

Shouldn't ScatteredLet simply be turned off when LetBeforeExamples is on? Otherwise maybe they should be combined into LetPosition?

Makes sense.

As per the upcoming cop naming guideline, we name cops to indicate what is wrong, not how it should be or what is the subject of the offence. ScatteredLet sounds right. But LetBeforeExamples doesn't, it says how it should be, not what's wrong. And a better name could be LetAfterExample, or ... ScatteredLet.

Now they check different things:

let(:a) { }
before { } # or any other statement which doesn't even have to be part of RSpec DSL
let(:b) { }
^^^ ScatteredLet

it { }
let(:a) { }
^^^ LetBeforeExamples

The following would be considered valid if ScatteredLet is enabled, but LetBeforeExamples is not:

        it { expect(subject.foo).to eq(a) }
        let(:a) { a }
        let(:b) { b }

The only downside of combining those two cops is that this style won't be tolerated anymore. I can think of a default AboveExamples: true option for ScatteredLet that would be possible to set to false and get the same behaviour as previously with LetBeforeExamples disabled.

@bquorning @Darhazer Please let me know what you think, I can handle the merger.

[marcandre]

I opened rubocop-hq/rubocop#8619, maybe it's best to do nothing and the problem should resolve itself once that is resolved.