diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c14d3f99c8c..71c118a9c775 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## master (unreleased) +### New features + +* [#2416](https://github.com/bbatsov/rubocop/pull/2416): New cop `Style/ConditionalAssignment` checks for assignment of the same variable in all branches of conditionals and replaces them with a single assignment to the return of the conditional. ([@rrosenblum][]) + ## 0.35.1 (10/11/2015) ### Bug Fixes diff --git a/config/enabled.yml b/config/enabled.yml index 10671e1723ce..1385fd90244a 100644 --- a/config/enabled.yml +++ b/config/enabled.yml @@ -153,6 +153,13 @@ Style/CommentIndentation: Description: 'Indentation of comments.' Enabled: true +Style/ConditionalAssignment: + Description: >- + Use the return value of `if` and `case` statements for + assignment to a variable instead of assigning that variable + inside of each branch. + Enabled: true + Style/ConstantName: Description: 'Constants should use SCREAMING_SNAKE_CASE.' StyleGuide: 'https://github.com/bbatsov/ruby-style-guide#screaming-snake-case' diff --git a/lib/rubocop.rb b/lib/rubocop.rb index ca96725f8164..ee7280eb0f96 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -168,6 +168,7 @@ require 'rubocop/cop/style/command_literal' require 'rubocop/cop/style/comment_annotation' require 'rubocop/cop/style/comment_indentation' +require 'rubocop/cop/style/conditional_assignment' require 'rubocop/cop/style/copyright' require 'rubocop/cop/style/constant_name' require 'rubocop/cop/style/def_with_parentheses' diff --git a/lib/rubocop/cop/lint/useless_setter_call.rb b/lib/rubocop/cop/lint/useless_setter_call.rb index 25ee73706338..95d87ed5c5c1 100644 --- a/lib/rubocop/cop/lint/useless_setter_call.rb +++ b/lib/rubocop/cop/lint/useless_setter_call.rb @@ -30,11 +30,11 @@ class UselessSetterCall < Cop def on_method_def(_node, _method_name, _args, body) return unless body - if body.type == :begin - expression = body.children - else - expression = body - end + expression = if body.type == :begin + body.children + else + body + end last_expr = expression.is_a?(Array) ? expression.last : expression diff --git a/lib/rubocop/cop/style/command_literal.rb b/lib/rubocop/cop/style/command_literal.rb index a47678472fd6..f295618a44a7 100644 --- a/lib/rubocop/cop/style/command_literal.rb +++ b/lib/rubocop/cop/style/command_literal.rb @@ -95,11 +95,11 @@ def preferred_delimiters def autocorrect(node) return if contains_backtick?(node) - if backtick_literal?(node) - replacement = ['%x', ''].zip(preferred_delimiters).map(&:join) - else - replacement = %w(` `) - end + replacement = if backtick_literal?(node) + ['%x', ''].zip(preferred_delimiters).map(&:join) + else + %w(` `) + end lambda do |corrector| corrector.replace(node.loc.begin, replacement.first) diff --git a/lib/rubocop/cop/style/conditional_assignment.rb b/lib/rubocop/cop/style/conditional_assignment.rb new file mode 100644 index 000000000000..de0c2579b605 --- /dev/null +++ b/lib/rubocop/cop/style/conditional_assignment.rb @@ -0,0 +1,247 @@ +# encoding: utf-8 + +module RuboCop + module Cop + module Style + # Check for `if` and `case` statements where each branch is used for + # assignment to the same variable when using the return of the + # condition can be used instead. + # + # @example + # # bad + # if foo + # bar = 1 + # else + # bar = 2 + # end + # + # case foo + # when 'a' + # bar += 1 + # else + # bar += 2 + # end + # + # if foo + # some_method + # bar = 1 + # else + # some_other_method + # bar = 2 + # end + # + # # good + # bar = if foo + # 1 + # else + # 2 + # end + # + # bar += case foo + # when 'a' + # 1 + # else + # 2 + # end + # + # bar << if foo + # some_method + # 1 + # else + # some_other_method + # 2 + # end + class ConditionalAssignment < Cop + include IfNode + + MSG = 'Use the return of the conditional for variable assignment.' + ASSIGNMENT_TYPES = [:casgn, :cvasgn, :gvasgn, :ivasgn, :lvasgn, + :and_asgn, :or_asgn, :op_asgn].freeze + IF = 'if'.freeze + ELSIF = 'elsif'.freeze + UNLESS = 'unless'.freeze + + def on_if(node) + return if ternary_op?(node) + return if elsif?(node) + _condition, if_branch, else_branch = *node + elsif_branches, else_branch = expand_elses(else_branch) + return if [if_branch, *elsif_branches, else_branch].any?(&:nil?) + *_, if_branch = *if_branch if if_branch.begin_type? + return unless assignment_type?(if_branch) + return unless types_match?(if_branch, *elsif_branches, else_branch) + return unless variables_match?(if_branch, *elsif_branches, + else_branch) + if_variable, = *if_branch + operator = operator(if_branch) + return if correction_exceeds_line_limit?(node, if_variable, operator) + + add_offense(node, :expression) + end + + def on_case(node) + return unless node.loc.else + _condition, *when_branches, else_branch = *node + *_, else_branch = *else_branch if else_branch.begin_type? + when_branches = expand_when_branches(when_branches) + return unless assignment_type?(else_branch) + return unless types_match?(*when_branches, else_branch) + return unless variables_match?(*when_branches, else_branch) + + variable, = *else_branch + operator = operator(else_branch) + return if correction_exceeds_line_limit?(node, variable, operator) + + add_offense(node, :expression) + end + + def autocorrect(node) + case node.loc.keyword.source + when IF + if_correction(node) + when UNLESS + unless_correction(node) + else + case_correction(node) + end + end + + private + + def expand_elses(branch) + elsif_branches = expand_elsif(branch) + else_branch = elsif_branches.any? ? elsif_branches.pop : branch + if else_branch && else_branch.begin_type? + *_, else_branch = *else_branch + end + [elsif_branches, else_branch] + end + + def expand_elsif(node, elsif_branches = []) + return [] if node.nil? || !node.if_type? + _condition, elsif_branch, else_branch = *node + if elsif_branch && elsif_branch.begin_type? + *_, elsif_branch = *elsif_branch + end + elsif_branches << elsif_branch + if else_branch && else_branch.if_type? + expand_elsif(else_branch, elsif_branches) + else + elsif_branches << else_branch + end + elsif_branches + end + + def variables_match?(*branches) + first_variable, = *branches.first + branches.all? { |branch| branch.children[0] == first_variable } + end + + def types_match?(*nodes) + first_type = nodes.first.type + nodes.all? { |node| node.type == first_type } + end + + def assignment_type?(branch) + return true if ASSIGNMENT_TYPES.include?(branch.type) + + if branch.send_type? + _variable, method, = *branch + return true if method == :<< + end + + false + end + + def expand_when_branches(when_branches) + when_branches.map do |branch| + when_branch = branch.children[1] + *_, when_branch = *when_branch if when_branch.begin_type? + when_branch + end + end + + def correction_exceeds_line_limit?(node, variable, operator) + return false unless config.for_cop('Metrics/LineLength')['Enabled'] + assignment = "#{variable} #{operator} " + assignment_regex = /#{variable}\s*#{operator}\s*/ + max_line_length = config.for_cop('Metrics/LineLength')['Max'] + lines = node.loc.expression.source.lines.map do |line| + line.chomp.sub(assignment_regex, '') + end + longest_line = lines.max_by(&:length) + (longest_line + assignment).length > max_line_length + end + + def if_correction(node) + _condition, if_branch, else_branch = *node + *_, if_branch = *if_branch if if_branch.begin_type? + variable, *_operator, if_assignment = *if_branch + variable, alternate = *variable + variable ||= alternate + elsif_branches, else_branch = expand_elses(else_branch) + _else_variable, *_operator, else_assignment = *else_branch + + lambda do |corrector| + corrector.insert_before(node.loc.expression, + "#{variable} #{operator(if_branch)} ") + corrector.replace(if_branch.loc.expression, + if_assignment.loc.expression.source) + correct_branches(corrector, elsif_branches) + corrector.replace(else_branch.loc.expression, + else_assignment.loc.expression.source) + end + end + + def case_correction(node) + _condition, *when_branches, else_branch = *node + *_, else_branch = *else_branch if else_branch.begin_type? + when_branches = expand_when_branches(when_branches) + + variable, *_operator, else_assignment = *else_branch + variable, alternate = *variable + variable ||= alternate + + lambda do |corrector| + corrector.insert_before(node.loc.expression, + "#{variable} #{operator(else_branch)} ") + correct_branches(corrector, when_branches) + corrector.replace(else_branch.loc.expression, + else_assignment.loc.expression.source) + end + end + + def correct_branches(corrector, branches) + branches.each do |branch| + *_, assignment = *branch + corrector.replace(branch.loc.expression, + assignment.loc.expression.source) + end + end + + def unless_correction(node) + _condition, else_branch, if_branch = *node + *_, if_branch = *if_branch if if_branch.begin_type? + *_, else_branch = *else_branch if else_branch.begin_type? + variable, *_operator, if_assignment = *if_branch + variable, alternate = *variable + variable ||= alternate + _else_variable, *_operator, else_assignment = *else_branch + + lambda do |corrector| + corrector.insert_before(node.loc.expression, + "#{variable} #{operator(if_branch)} ") + corrector.replace(if_branch.loc.expression, + if_assignment.loc.expression.source) + corrector.replace(else_branch.loc.expression, + else_assignment.loc.expression.source) + end + end + + def operator(node) + node.send_type? ? node.loc.selector.source : node.loc.operator.source + end + end + end + end +end diff --git a/lib/rubocop/cop/style/copyright.rb b/lib/rubocop/cop/style/copyright.rb index d70e11fc0b31..54c25f91264f 100644 --- a/lib/rubocop/cop/style/copyright.rb +++ b/lib/rubocop/cop/style/copyright.rb @@ -75,11 +75,11 @@ def autocorrect(token) "match Notice /#{notice}/" unless autocorrect_notice =~ regex lambda do |corrector| - if token.nil? - range = Parser::Source::Range.new('', 0, 0) - else - range = token.pos - end + range = if token.nil? + Parser::Source::Range.new('', 0, 0) + else + token.pos + end corrector.insert_before(range, "#{autocorrect_notice}\n") end end diff --git a/lib/rubocop/cop/style/next.rb b/lib/rubocop/cop/style/next.rb index d3d348a292e3..25c066c9b9c4 100644 --- a/lib/rubocop/cop/style/next.rb +++ b/lib/rubocop/cop/style/next.rb @@ -102,13 +102,13 @@ def offense_location(offense_node) def autocorrect(node) lambda do |corrector| cond, if_body, else_body = *node - if if_body.nil? - opposite_kw = 'if' - body = else_body - else - opposite_kw = 'unless' - body = if_body - end + body = if if_body.nil? + opposite_kw = 'if' + else_body + else + opposite_kw = 'unless' + if_body + end next_code = 'next ' << opposite_kw << ' ' << cond.loc.expression.source << "\n" corrector.insert_before(node.loc.expression, next_code) diff --git a/lib/rubocop/cop/style/regexp_literal.rb b/lib/rubocop/cop/style/regexp_literal.rb index 20f3d85f2b9d..151af720eacb 100644 --- a/lib/rubocop/cop/style/regexp_literal.rb +++ b/lib/rubocop/cop/style/regexp_literal.rb @@ -91,11 +91,11 @@ def preferred_delimiters def autocorrect(node) return if contains_slash?(node) - if slash_literal?(node) - replacement = ['%r', ''].zip(preferred_delimiters).map(&:join) - else - replacement = %w(/ /) - end + replacement = if slash_literal?(node) + ['%r', ''].zip(preferred_delimiters).map(&:join) + else + %w(/ /) + end lambda do |corrector| corrector.replace(node.loc.begin, replacement.first) diff --git a/lib/rubocop/formatter/formatter_set.rb b/lib/rubocop/formatter/formatter_set.rb index e237f2683396..62cfdeb1b8a8 100644 --- a/lib/rubocop/formatter/formatter_set.rb +++ b/lib/rubocop/formatter/formatter_set.rb @@ -65,13 +65,13 @@ def add_formatter(formatter_type, output_path = nil) builtin_formatter_class(formatter_type) end - if output_path - dir_path = File.dirname(output_path) - FileUtils.mkdir_p(dir_path) unless File.exist?(dir_path) - output = File.open(output_path, 'w') - else - output = $stdout - end + output = if output_path + dir_path = File.dirname(output_path) + FileUtils.mkdir_p(dir_path) unless File.exist?(dir_path) + File.open(output_path, 'w') + else + $stdout + end self << formatter_class.new(output) end diff --git a/lib/rubocop/node_pattern.rb b/lib/rubocop/node_pattern.rb index b45477c3e396..578de410889c 100644 --- a/lib/rubocop/node_pattern.rb +++ b/lib/rubocop/node_pattern.rb @@ -356,15 +356,15 @@ def def_node_matcher(method_name, pattern_str) # yield all descendants which match. def def_node_search(method_name, pattern_str) compiler = RuboCop::NodePattern::Compiler.new(pattern_str, 'node') - if method_name.to_s.end_with?('?') - on_match = 'return true' - prelude = '' - else - yieldval = compiler.emit_capture_list - yieldval = 'node' if yieldval.empty? - on_match = "yield(#{yieldval})" - prelude = "return enum_for(:#{method_name},node0) unless block_given?" - end + prelude = if method_name.to_s.end_with?('?') + on_match = 'return true' + '' + else + yieldval = compiler.emit_capture_list + yieldval = 'node' if yieldval.empty? + on_match = "yield(#{yieldval})" + "return enum_for(:#{method_name},node0) unless block_given?" + end src = <<-END def #{method_name}(node0#{compiler.emit_trailing_param_list}) #{prelude} diff --git a/lib/rubocop/string_util.rb b/lib/rubocop/string_util.rb index 83f2553d18f4..c6b03b3008ff 100644 --- a/lib/rubocop/string_util.rb +++ b/lib/rubocop/string_util.rb @@ -19,13 +19,13 @@ def self.distance(*args) end def initialize(a, b) - if a.size < b.size - @shorter = a - @longer = b - else - @shorter = b - @longer = a - end + @longer = if a.size < b.size + @shorter = a + b + else + @shorter = b + a + end end def distance diff --git a/spec/rubocop/cli_spec.rb b/spec/rubocop/cli_spec.rb index 8141aecbc01a..7e55e40f6dee 100644 --- a/spec/rubocop/cli_spec.rb +++ b/spec/rubocop/cli_spec.rb @@ -850,18 +850,18 @@ def abs(path) 'C: 4: 8: Prefer {...} over do...end for single-line blocks.', 'C: 5: 34: Do not use semicolons to terminate expressions.'] - if RUBY_VERSION >= '2' - src += ['def self.some_method(foo, bar: 1)', - ' log.debug(foo)', - 'end'] - corrected += ['def self.some_method(foo, bar: 1)', - ' log.debug(foo)', - 'end'] - offenses += ['W: 6: 27: Unused method argument - bar.'] - summary = '1 file inspected, 5 offenses detected' - else - summary = '1 file inspected, 4 offenses detected' - end + summary = if RUBY_VERSION >= '2' + src += ['def self.some_method(foo, bar: 1)', + ' log.debug(foo)', + 'end'] + corrected += ['def self.some_method(foo, bar: 1)', + ' log.debug(foo)', + 'end'] + offenses += ['W: 6: 27: Unused method argument - bar.'] + '1 file inspected, 5 offenses detected' + else + '1 file inspected, 4 offenses detected' + end create_file('example.rb', src) expect(cli.run(%w(-a -f simple))).to eq(1) expect($stderr.string).to eq('') diff --git a/spec/rubocop/cop/style/conditional_assignment_spec.rb b/spec/rubocop/cop/style/conditional_assignment_spec.rb new file mode 100644 index 000000000000..9e32e100953b --- /dev/null +++ b/spec/rubocop/cop/style/conditional_assignment_spec.rb @@ -0,0 +1,960 @@ +# encoding: utf-8 + +require 'spec_helper' + +describe RuboCop::Cop::Style::ConditionalAssignment do + subject(:cop) { described_class.new(config) } + let(:config) do + RuboCop::Config.new('Style/ConditionalAssignment' => { + 'Enabled' => true + }, + 'Metrics/LineLength' => { + 'Max' => 80, + 'Enabled' => true + }) + end + + it 'allows if else without variable assignment' do + source = ['if foo', + ' 1', + 'else', + ' 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment to the result of a ternary operation' do + source = 'bar = foo? ? "a" : "b"' + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows modifier if' do + inspect_source(cop, 'return if a == 1') + + expect(cop.messages).to be_empty + end + + it 'allows modifier if inside of if else' do + source = ['if foo', + ' a unless b', + 'else', + ' c unless d', + 'end'] + + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + context 'empty branch' do + it 'allows an empty if statement' do + source = ['if foo', + ' # comment', + 'else', + ' do_something', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows an empty elsif statement' do + source = ['if foo', + ' bar = 1', + 'elsif baz', + ' # empty', + 'else', + ' bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows if elsif without else' do + source = ['if foo', + " bar = 'some string'", + 'elsif bar', + " bar = 'another string'", + 'end'] + + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment in if without an else' do + source = ['if foo', + ' bar = 1', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment in unless without an else' do + source = ['unless foo', + ' bar = 1', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment in case when without an else' do + source = ['case foo', + 'when "a"', + ' bar = 1', + 'when "b"', + ' bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows an empty when branch' do + source = ['case foo', + 'when "a"', + ' # empty', + 'when "b"', + ' bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + end + + it 'allows assignment of different variables in if else' do + source = ['if foo', + ' bar = 1', + 'else', + ' baz = 1', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows method calls in if else' do + source = ['if foo', + ' bar', + 'else', + ' baz', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows if elsif else with the same assignment only in if else' do + source = ['if foo', + ' bar = 1', + 'elsif foobar', + ' baz = 2', + 'else', + ' bar = 1', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows if elsif else with the same assignment only in if elsif' do + source = ['if foo', + ' bar = 1', + 'elsif foobar', + ' bar = 2', + 'else', + ' baz = 1', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows if elsif else with the same assignment only in elsif else' do + source = ['if foo', + ' bar = 1', + 'elsif foobar', + ' baz = 2', + 'else', + ' baz = 1', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment using different operators in if else' do + source = ['if foo', + ' bar = 1', + 'else', + ' bar << 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment using different operators in if elsif else' do + source = ['if foo', + ' bar = 1', + 'elsif foobar', + ' bar += 2', + 'else', + ' bar << 3', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment of different variables in case when else' do + source = ['case foo', + 'when "a"', + ' bar = 1', + 'else', + ' baz = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + context 'correction would exceed max line length' do + it 'allows assignment to the same variable in if else if the ' \ + 'correction would create a line longer than the configured LineLength' do + source = ['if foo', + " #{'a' * 78}", + ' bar = 1', + 'else', + ' bar = 2', + 'end'] + + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + it 'allows assignment to the same variable in case when else if the ' \ + 'correction would create a line longer than the configured LineLength' do + source = ['case foo', + 'when foobar', + " #{'a' * 78}", + ' bar = 1', + 'else', + ' bar = 2', + 'end'] + + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + end + + shared_examples 'all variable types' do |variable| + it 'registers an offense assigning any variable type in if else' do + source = ['if foo', + " #{variable} = 1", + 'else', + " #{variable} = 2", + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense assigning any variable type in case when' do + source = ['case foo', + 'when "a"', + " #{variable} = 1", + 'else', + " #{variable} = 2", + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + end + + it_behaves_like('all variable types', 'bar') + it_behaves_like('all variable types', 'BAR') + it_behaves_like('all variable types', '@bar') + it_behaves_like('all variable types', '@@bar') + it_behaves_like('all variable types', '$BAR') + + shared_examples 'all assignment types' do |assignment| + it "registers and offense for assignment using #{assignment} in if else" do + source = ['if foo', + " bar #{assignment} 1", + 'else', + " bar #{assignment} 2", + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it "registers an offense for assignment using #{assignment} in case when" do + source = ['case foo', + 'when "a"', + " bar #{assignment} 1", + 'else', + " bar #{assignment} 2", + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + end + + it_behaves_like('all assignment types', '=') + it_behaves_like('all assignment types', '||=') + it_behaves_like('all assignment types', '&&=') + it_behaves_like('all assignment types', '+=') + it_behaves_like('all assignment types', '<<') + it_behaves_like('all assignment types', '-=') + + it 'registers an offense for assignment in if elsif else' do + source = ['if foo', + ' bar = 1', + 'elsif baz', + ' bar = 2', + 'else', + ' bar = 3', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense for assignment in if elsif else' do + source = ['if foo', + ' bar = 1', + 'elsif baz', + ' bar = 2', + 'elsif foobar', + ' bar = 3', + 'else', + ' bar = 4', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + context 'assignment as the last statement' do + it 'registers an offense in if else with more than variable assignment' do + source = ['if foo', + ' method_call', + ' bar = 1', + 'else', + ' method_call', + ' bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense in if elsif else with more than ' \ + 'variable assignment' do + source = ['if foo', + ' method_call', + ' bar = 1', + 'elsif foobar', + ' method_call', + ' bar = 2', + 'else', + ' method_call', + ' bar = 3', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense in if elsif else with some branches only ' \ + 'containing variable assignment and others containing more than ' \ + 'variable assignment' do + source = ['if foo', + ' bar = 1', + 'elsif foobar', + ' method_call', + ' bar = 2', + 'elsif baz', + ' bar = 3', + 'else', + ' method_call', + ' bar = 4', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense in unless else with more than ' \ + 'variable assignment' do + source = ['unless foo', + ' method_call', + ' bar = 1', + 'else', + ' method_call', + ' bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense in case when else with more than ' \ + 'variable assignment' do + source = ['case foo', + 'when foobar', + ' method_call', + ' bar = 1', + 'else', + ' method_call', + ' bar = 2', + 'end'] + + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + end + + it 'registers an offense for assignment in if then else' do + source = ['if foo then bar = 1', + 'else bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense for assignment in if elsif else' do + source = ['if foo', + ' bar = 1', + 'elsif foobar', + ' bar = 2', + 'elsif baz', + ' bar = 3', + 'else', + ' bar = 4', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense for assignment in unless else' do + source = ['unless foo', + ' bar = 1', + 'else', + ' bar = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense for assignment in case when then else' do + source = ['case foo', + 'when bar then baz = 1', + 'else baz = 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'registers an offense for assignment in case with when when else' do + source = ['case foo', + 'when foobar', + ' bar = 1', + 'when baz', + ' bar = 2', + 'else', + ' bar = 3', + 'end'] + inspect_source(cop, source) + + expect(cop.messages) + .to eq(['Use the return of the conditional for variable assignment.']) + end + + it 'allows different assignment types in case with when when else' do + source = ['case foo', + 'when foobar', + ' bar = 1', + 'else', + ' bar << 2', + 'end'] + inspect_source(cop, source) + + expect(cop.messages).to be_empty + end + + context 'auto-correct' do + it 'corrects assignment in if else' do + source = ['if foo', + ' bar = 1', + 'else', + ' bar = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = if foo', + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + + it 'corrects assignment in if elsif else' do + source = ['if foo', + ' bar = 1', + 'elsif baz', + ' bar = 2', + 'else', + ' bar = 3', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = if foo', + ' 1', + 'elsif baz', + ' 2', + 'else', + ' 3', + 'end'].join("\n")) + end + + shared_examples '2 character assignment types' do |asgn| + it "corrects assignment using #{asgn} in if elsif else" do + source = ['if foo', + " bar #{asgn} 1", + 'elsif baz', + " bar #{asgn} 2", + 'else', + " bar #{asgn} 3", + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(["bar #{asgn} if foo", + ' 1', + 'elsif baz', + ' 2', + 'else', + ' 3', + 'end'].join("\n")) + end + + it "corrects assignment using #{asgn} in case when else" do + source = ['case foo', + 'when bar', + " baz #{asgn} 1", + 'else', + " baz #{asgn} 2", + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(["baz #{asgn} case foo", + 'when bar', + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + + it "corrects assignment using #{asgn} in unless else" do + source = ['unless foo', + " bar #{asgn} 1", + 'else', + " bar #{asgn} 2", + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(["bar #{asgn} unless foo", + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + end + + it_behaves_like('2 character assignment types', '+=') + it_behaves_like('2 character assignment types', '-=') + it_behaves_like('2 character assignment types', '<<') + + shared_examples '3 character assignment types' do |asgn| + it "corrects assignment using #{asgn} in if elsif else" do + source = ['if foo', + " bar #{asgn} 1", + 'elsif baz', + " bar #{asgn} 2", + 'else', + " bar #{asgn} 3", + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(["bar #{asgn} if foo", + ' 1', + 'elsif baz', + ' 2', + 'else', + ' 3', + 'end'].join("\n")) + end + + it "corrects assignment using #{asgn} in case when else" do + source = ['case foo', + 'when bar', + " baz #{asgn} 1", + 'else', + " baz #{asgn} 2", + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(["baz #{asgn} case foo", + 'when bar', + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + + it "corrects assignment using #{asgn} in unless else" do + source = ['unless foo', + " bar #{asgn} 1", + 'else', + " bar #{asgn} 2", + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(["bar #{asgn} unless foo", + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + end + + it_behaves_like('3 character assignment types', '&&=') + it_behaves_like('3 character assignment types', '||=') + + it 'corrects assignment in if elsif else with multiple elsifs' do + source = ['if foo', + ' bar = 1', + 'elsif baz', + ' bar = 2', + 'elsif foobar', + ' bar = 3', + 'else', + ' bar = 4', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = if foo', + ' 1', + 'elsif baz', + ' 2', + 'elsif foobar', + ' 3', + 'else', + ' 4', + 'end'].join("\n")) + end + + it 'corrects assignment in unless else' do + source = ['unless foo', + ' bar = 1', + 'else', + ' bar = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = unless foo', + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + + it 'corrects assignment in case when else' do + source = ['case foo', + 'when bar', + ' baz = 1', + 'else', + ' baz = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['baz = case foo', + 'when bar', + ' 1', + 'else', + ' 2', + 'end'].join("\n")) + end + + it 'corrects assignment in case when else with multiple whens' do + source = ['case foo', + 'when bar', + ' baz = 1', + 'when foobar', + ' baz = 2', + 'else', + ' baz = 3', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['baz = case foo', + 'when bar', + ' 1', + 'when foobar', + ' 2', + 'else', + ' 3', + 'end'].join("\n")) + end + + context 'assignment from a method' do + it 'corrects if else' do + source = ['if foo?(scope.node)', + ' bar << foobar(var, all)', + 'else', + ' bar << baz(var, all)', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar << if foo?(scope.node)', + ' foobar(var, all)', + 'else', + ' baz(var, all)', + 'end'].join("\n")) + end + + it 'corrects unless else' do + source = ['unless foo?(scope.node)', + ' bar << foobar(var, all)', + 'else', + ' bar << baz(var, all)', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar << unless foo?(scope.node)', + ' foobar(var, all)', + 'else', + ' baz(var, all)', + 'end'].join("\n")) + end + + it 'corrects case when' do + source = ['case foo', + 'when foobar', + ' bar << foobar(var, all)', + 'else', + ' bar << baz(var, all)', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar << case foo', + 'when foobar', + ' foobar(var, all)', + 'else', + ' baz(var, all)', + 'end'].join("\n")) + end + end + + context 'then' do + it 'corrects if then elsif then else' do + source = ['if cond then bar = 1', + 'elsif cond then bar = 2', + 'else bar = 3', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = if cond then 1', + 'elsif cond then 2', + 'else 3', + 'end'].join("\n")) + end + + it 'corrects case when then else' do + source = ['case foo', + 'when baz then bar = 1', + 'else bar = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = case foo', + 'when baz then 1', + 'else 2', + 'end'].join("\n")) + end + end + + it 'preserves comments during correction in if else' do + source = ['if foo', + ' # comment in if', + ' bar = 1', + 'else', + ' # comment in else', + ' bar = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = if foo', + ' # comment in if', + ' 1', + 'else', + ' # comment in else', + ' 2', + 'end'].join("\n")) + end + + it 'preserves comments during correction in case when else' do + source = ['case foo', + 'when foobar', + ' # comment in when', + ' bar = 1', + 'else', + ' # comment in else', + ' bar = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = case foo', + 'when foobar', + ' # comment in when', + ' 1', + 'else', + ' # comment in else', + ' 2', + 'end'].join("\n")) + end + + context 'assignment as the last statement' do + it 'preserves all code before the variable assignment in a branch' do + source = ['if foo', + ' # comment in if', + ' method_call', + ' bar = 1', + 'elsif foobar', + ' method_call', + ' bar = 2', + 'elsif baz', + ' bar = 3', + 'else', + ' bar = 4', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = if foo', + ' # comment in if', + ' method_call', + ' 1', + 'elsif foobar', + ' method_call', + ' 2', + 'elsif baz', + ' 3', + 'else', + ' 4', + 'end'].join("\n")) + end + + it 'preserves all code before variable assignment in unless else' do + source = ['unless foo', + ' method_call', + ' bar = 1', + 'else', + ' method_call', + ' bar = 2', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = unless foo', + ' method_call', + ' 1', + 'else', + ' method_call', + ' 2', + 'end'].join("\n")) + end + + it 'preserves all code before varialbe assignment in case when else' do + source = ['case foo', + 'when foobar', + ' method_call', + ' bar = 1', + 'when baz', + ' # comment in when', + ' bar = 2', + 'else', + ' method_call', + ' bar = 3', + 'end'] + + new_source = autocorrect_source(cop, source) + + expect(new_source).to eq(['bar = case foo', + 'when foobar', + ' method_call', + ' 1', + 'when baz', + ' # comment in when', + ' 2', + 'else', + ' method_call', + ' 3', + 'end'].join("\n")) + end + end + end +end