Merge pull request #3636 from tessi/feature/3570-guard-clauses-on-mul…

…ti-line-statements

[Fix #3570] avoid if-modifiers on multi-line statements

CHANGELOG.md

@@ -10,6 +10,7 @@
 * [#3646](https://github.com/bbatsov/rubocop/pull/3646): Add new `Lint/EmptyWhen` cop. ([@drenmi][])
 * [#3246](https://github.com/bbatsov/rubocop/issues/3246): Add list of all cops to the manual (generated automatically from a rake task). ([@sihu][])
 * [#3647](https://github.com/bbatsov/rubocop/issues/3647): Add `--force-default-config` option. ([@jawshooah][])
+* [#3570](https://github.com/bbatsov/rubocop/issues/3570): Add new `MultilineIfModifier` cop to avoid usage of if/unless-modifiers on multiline statements. ([@tessi][])
 
 ### Bug fixes
 
@@ -2467,3 +2468,4 @@
 [@jessieay]: https://github.com/jessieay
 [@tiagocasanovapt]: https://github.com/tiagocasanovapt
 [@iGEL]: https://github.com/iGEL
+[@tessi]: https://github.com/tessi

config/enabled.yml

@@ -450,6 +450,11 @@ Style/MultilineIfThen:
   StyleGuide: '#no-then'
   Enabled: true
 
+Style/MultilineIfModifier:
+  Description: 'Only use if/unless modifiers on single line statements.'
+  StyleGuide: '#no-multiline-if-modifiers'
+  Enabled: true
+
 Style/MultilineMemoization:
   Description: 'Wrap multiline memoizations in a `begin` and `end` block.'
   Enabled: true

lib/rubocop.rb

@@ -286,6 +286,7 @@
 require 'rubocop/cop/style/multiline_block_layout'
 require 'rubocop/cop/style/multiline_hash_brace_layout'
 require 'rubocop/cop/style/multiline_if_then'
+require 'rubocop/cop/style/multiline_if_modifier'
 require 'rubocop/cop/style/multiline_memoization'
 require 'rubocop/cop/style/multiline_method_call_brace_layout'
 require 'rubocop/cop/style/multiline_method_call_indentation'

lib/rubocop/cop/style/copyright.rb

@@ -16,6 +16,9 @@ module Style
       # an offense is reported.
       #
       class Copyright < Cop
+        AUTOCORRECT_EMPTY_WARNING = 'An AutocorrectNotice must be defined in' \
+                                    'your RuboCop config'.freeze
+
         def message
           "Include a copyright notice matching /#{notice}/" \
           'before any code.'
@@ -68,11 +71,12 @@ def notice_found?(processed_source)
         end
 
         def autocorrect(token)
-          raise Warning, 'An AutocorrectNotice must be defined in ' \
-            'your RuboCop config' if autocorrect_notice.empty?
+          raise Warning, AUTOCORRECT_EMPTY_WARNING if autocorrect_notice.empty?
           regex = Regexp.new(notice)
-          raise Warning, "AutocorrectNotice '#{autocorrect_notice}' must " \
-            "match Notice /#{notice}/" unless autocorrect_notice =~ regex
+          unless autocorrect_notice =~ regex
+            raise Warning, "AutocorrectNotice '#{autocorrect_notice}' must " \
+                           "match Notice /#{notice}/"
+          end
 
           lambda do |corrector|
             range = token.nil? ? range_between(0, 0) : token.pos

lib/rubocop/cop/style/multiline_if_modifier.rb

@@ -0,0 +1,73 @@
+# frozen_string_literal: true
+
+module RuboCop
+  module Cop
+    module Style
+      # Checks for uses of if/unless modifiers with multiple-lines bodies.
+      #
+      # @example
+      #
+      #   # bad
+      #   {
+      #     result: 'this should not happen'
+      #   } unless cond
+      #
+      #   # good
+      #   { result: 'ok' } if cond
+      class MultilineIfModifier < Cop
+        include IfNode
+        include StatementModifier
+        include AutocorrectAlignment
+
+        MSG = 'Favor a normal %s-statement over a modifier' \
+              ' clause in a multiline statement.'.freeze
+
+        def on_if(node)
+          return unless modifier_if?(node)
+
+          _cond, body = if_node_parts(node)
+          return if body.single_line?
+
+          add_offense(node, :expression)
+        end
+
+        private
+
+        def message(node)
+          format(MSG, node.loc.keyword.source)
+        end
+
+        def autocorrect(node)
+          lambda do |corrector|
+            corrector.replace(node.source_range, to_normal_if(node))
+          end
+        end
+
+        def to_normal_if(node)
+          cond, body = if_node_parts(node)
+          indented_body = indented_body(body, node)
+
+          condition = "#{node.loc.keyword.source} #{cond.source}"
+          indented_end = "#{offset(node)}end"
+
+          "#{condition}\n#{indented_body}\n#{indented_end}"
+        end
+
+        def configured_indentation_width
+          super || 2
+        end
+
+        def indented_body(body, node)
+          body_source = "#{offset(node)}#{body.source}"
+          body_source.each_line.map do |line|
+            if line == "\n"
+              line
+            else
+              line.sub(/^\s{#{offset(node).length}}/, indentation(node))
+            end
+          end.join
+        end
+      end
+    end
+  end
+end

spec/rubocop/cop/style/multiline_if_modifier_spec.rb

@@ -0,0 +1,116 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe RuboCop::Cop::Style::MultilineIfModifier do
+  subject(:cop) { described_class.new }
+
+  shared_examples 'offense' do |modifier|
+    it 'registers an offense' do
+      inspect_source(cop, source)
+      expect(cop.messages)
+        .to eq(["Favor a normal #{modifier}-statement over a modifier" \
+                ' clause in a multiline statement.'])
+    end
+  end
+
+  shared_examples 'no offense' do
+    it 'does not register an offense' do
+      inspect_source(cop, source)
+      expect(cop.messages).to be_empty
+    end
+  end
+
+  shared_examples 'autocorrect' do |correct_code|
+    it 'auto-corrects' do
+      corrected = autocorrect_source(cop, source)
+      expect(corrected).to eq(correct_code)
+    end
+  end
+
+  context 'if guard clause' do
+    let(:source) do
+      [
+        '{',
+        '  result: run',
+        '} if cond'
+      ].join("\n")
+    end
+
+    include_examples 'offense', 'if'
+    include_examples 'autocorrect', "if cond\n  {\n    result: run\n  }\nend"
+
+    context 'one liner' do
+      let(:source) { 'run if cond' }
+
+      include_examples 'no offense'
+    end
+
+    context 'multiline condition' do
+      let(:source) { "run if cond &&\n       cond2" }
+
+      include_examples 'no offense'
+    end
+
+    context 'indented offense' do
+      let(:source) do
+        [
+          '  {',
+          '    result: run',
+          '  } if cond'
+        ].join("\n")
+      end
+
+      include_examples 'autocorrect', "  if cond\n" \
+                                      "    {\n" \
+                                      "      result: run\n" \
+                                      "    }\n" \
+                                      '  end'
+    end
+  end
+
+  context 'unless guard clause' do
+    let(:source) do
+      [
+        '{',
+        '  result: run',
+        '} unless cond'
+      ].join("\n")
+    end
+
+    include_examples 'offense', 'unless'
+    include_examples 'autocorrect', "unless cond\n" \
+                                    "  {\n" \
+                                    "    result: run\n" \
+                                    "  }\n" \
+                                    'end'
+
+    context 'one liner' do
+      let(:source) { 'run unless cond' }
+
+      include_examples 'no offense'
+    end
+
+    context 'multiline condition' do
+      let(:source) { "run unless cond &&\n           cond2" }
+
+      include_examples 'no offense'
+    end
+
+    context 'indented offense' do
+      let(:source) do
+        [
+          '  {',
+          '    result: run',
+          '  } unless cond'
+        ].join("\n")
+      end
+
+      include_examples 'autocorrect', "  unless cond\n" \
+                                      "    {\n" \
+                                      "      result: run\n" \
+                                      "    }\n" \
+                                      '  end'
+    end
+  end
+end

spec/rubocop/cop/style/mutable_constant_spec.rb

@@ -96,23 +96,25 @@
   end
 
   context 'when the constant is a frozen string literal' do
-    context 'when the target ruby version >= 3.0' do
-      let(:ruby_version) { 3.0 }
-
-      context 'when the frozen string literal comment is missing' do
-        it_behaves_like :immutable_objects, '"#{a}"'
-      end
-
-      context 'when the frozen string literal comment is true' do
-        let(:prefix) { '# frozen_string_literal: true' }
-        it_behaves_like :immutable_objects, '"#{a}"'
+    if RuboCop::Config::KNOWN_RUBIES.include?(3.0)
+      context 'when the target ruby version >= 3.0' do
+        let(:ruby_version) { 3.0 }
+
+        context 'when the frozen string literal comment is missing' do
+          it_behaves_like :immutable_objects, '"#{a}"'
+        end
+
+        context 'when the frozen string literal comment is true' do
+          let(:prefix) { '# frozen_string_literal: true' }
+          it_behaves_like :immutable_objects, '"#{a}"'
+        end
+
+        context 'when the frozen string literal comment is false' do
+          let(:prefix) { '# frozen_string_literal: false' }
+          it_behaves_like :immutable_objects, '"#{a}"'
+        end
       end
-
-      context 'when the frozen string literal comment is false' do
-        let(:prefix) { '# frozen_string_literal: false' }
-        it_behaves_like :immutable_objects, '"#{a}"'
-      end
-    end if RuboCop::Config::KNOWN_RUBIES.include?(3.0)
+    end
 
     context 'when the target ruby version >= 2.3' do
       let(:ruby_version) { 2.3 }

spec/rubocop/cop/style/redundant_freeze_spec.rb

@@ -46,23 +46,25 @@
   end
 
   context 'when the receiver is a frozen string literal' do
-    context 'when the target ruby version >= 3.0' do
-      let(:ruby_version) { 3.0 }
-
-      context 'when the frozen string literal comment is missing' do
-        it_behaves_like :immutable_objects, '"#{a}"'
-      end
-
-      context 'when the frozen string literal comment is true' do
-        let(:prefix) { '# frozen_string_literal: true' }
-        it_behaves_like :immutable_objects, '"#{a}"'
+    if RuboCop::Config::KNOWN_RUBIES.include?(3.0)
+      context 'when the target ruby version >= 3.0' do
+        let(:ruby_version) { 3.0 }
+
+        context 'when the frozen string literal comment is missing' do
+          it_behaves_like :immutable_objects, '"#{a}"'
+        end
+
+        context 'when the frozen string literal comment is true' do
+          let(:prefix) { '# frozen_string_literal: true' }
+          it_behaves_like :immutable_objects, '"#{a}"'
+        end
+
+        context 'when the frozen string literal comment is false' do
+          let(:prefix) { '# frozen_string_literal: false' }
+          it_behaves_like :immutable_objects, '"#{a}"'
+        end
       end
-
-      context 'when the frozen string literal comment is false' do
-        let(:prefix) { '# frozen_string_literal: false' }
-        it_behaves_like :immutable_objects, '"#{a}"'
-      end
-    end if RuboCop::Config::KNOWN_RUBIES.include?(3.0)
+    end
 
     context 'when the target ruby version >= 2.3' do
       let(:ruby_version) { 2.3 }