From 52368888a8900bfc51a142612f845d8fe80be325 Mon Sep 17 00:00:00 2001 From: Chris Lowder & Paul Mucur Date: Sat, 21 Mar 2015 18:24:30 +0000 Subject: [PATCH] Permit Weirich-style block syntax * https://github.com/bbatsov/rubocop/issues/1600 * https://github.com/bbatsov/ruby-style-guide/issues/162 * http://devblog.avdi.org/2011/07/26/the-procedurefunction-block-convention-in-ruby/ Add a configuration option to the Style/Blocks cop to permit two styles: * multiline (the current default); * weirich (the semantic rule as described in the links above). With Weirich style enabled, this allows multi-line blocks with braces if the block is considered "functional". The current implementation checks whether the return value of a block is used to classify it as "functional". It performs the following checks: 1. Is the return value of the block being assigned? 2. Is the return value of the block sent a message? 3. Is the return value of the block the last thing in its scope? This should cover the following Weirich-style use cases: # 1 foo = map { |x| x * 2 } # 2 map { |x| x * 2 }.inspect # 3 block do foo map { |x| x * 2 } end # 3 puts map { |x| x * 2 } Add offenses if the return value of a block is used but do...end is used instead of the intention-revealing {...}. Conversely, if the return value of a block is not used, add an offense if {...} is used instead of do...end. Add a configurable whitelist of methods that use the return value of their block without being obvious to the caller. This permits use cases such as let or subject in RSpec. The same whitelist can be used to permit methods that don't use the return value of their block despite being used in assignment such as tap or Active Record's create and new. As DSLs often use do...end (e.g. RSpec.describe), do not add an offense if a block uses do...end even though it could potentially be the return value of its outer scope, e.g. RSpec.describe Foo do it 'blah' do # ... end end --- CHANGELOG.md | 3 + config/default.yml | 13 + lib/rubocop/cop/style/blocks.rb | 98 +++++++- spec/rubocop/cop/style/blocks_spec.rb | 328 +++++++++++++++++++------- 4 files changed, 348 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3f25cf8d1e98..3f8cf4b87492 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### New features +* [#1600](https://github.com/bbatsov/rubocop/issues/1600): Add `multiline` and `weirich` styles to the `Blocks` cop. ([@clowder][], [@mudge][]) * [#1712](https://github.com/bbatsov/rubocop/pull/1712): Set `Offense#corrected?` to `true`, `false`, or `nil` when it was, wasn't, or can't be auto-corrected, respectively. ([@vassilevsky][]) * [#1669](https://github.com/bbatsov/rubocop/pull/1669): Add command-line switch `--display-style-guide`. ([@marxarelli][]) * [#1405](https://github.com/bbatsov/rubocop/issues/1405): Add Rails TimeZone and Date cops. ([@palkan][]) @@ -1309,3 +1310,5 @@ [@zvkemp]: https://github.com/zvkemp [@vassilevsky]: https://github.com/vassilevsky [@gerry3]: https://github.com/gerry3 +[@clowder]: https://github.com/clowder +[@mudge]: https://github.com/mudge diff --git a/config/default.yml b/config/default.yml index 3e105619a761..6a0fc487adb3 100644 --- a/config/default.yml +++ b/config/default.yml @@ -148,6 +148,19 @@ Style/BarePercentLiterals: - percent_q - bare_percent +Style/Blocks: + EnforcedStyle: multiline + SupportedStyles: + - multiline + - weirich + Whitelist: + - let + - let! + - subject + - tap + - new + - create + Style/BracesAroundHashParameters: EnforcedStyle: no_braces SupportedStyles: diff --git a/lib/rubocop/cop/style/blocks.rb b/lib/rubocop/cop/style/blocks.rb index 9e8a3e855937..a1efb245c336 100644 --- a/lib/rubocop/cop/style/blocks.rb +++ b/lib/rubocop/cop/style/blocks.rb @@ -6,11 +6,9 @@ module Style # Check for uses of braces or do/end around single line or # multi-line blocks. class Blocks < Cop + include ConfigurableEnforcedStyle include AutocorrectUnlessChangingAST - MULTI_LINE_MSG = 'Avoid using {...} for multi-line blocks.' - SINGLE_LINE_MSG = 'Prefer {...} over do...end for single-line blocks.' - def on_send(node) _receiver, method_name, *args = *node return unless args.any? @@ -27,17 +25,67 @@ def on_send(node) def on_block(node) return if ignored_node?(node) + if proper_block_style?(node) + correct_style_detected + else + add_offense(node, :begin) + end + end + + private + + def message(node) + block_begin = node.loc.begin.source + block_length = Util.block_length(node) + + case style + when :weirich + if block_begin == '{' + 'Prefer do...end over {...} for procedural blocks.' + else + 'Prefer {...} over do...end for functional blocks.' + end + when :multiline + if block_length > 0 + 'Avoid using {...} for multi-line blocks.' + else + 'Prefer {...} over do...end for single-line blocks.' + end + end + end + + def proper_block_style?(node) + case style + when :weirich + weirich_block_style?(node) + when :multiline + multiline_block_style?(node) + end + end + + def weirich_block_style?(node) block_length = Util.block_length(node) block_begin = node.loc.begin.source - if block_length > 0 && block_begin == '{' - add_offense(node, :begin, MULTI_LINE_MSG) - elsif block_length == 0 && block_begin != '{' - add_offense(node, :begin, SINGLE_LINE_MSG) + return true if block_length == 0 || whitelisted_method?(node) + + if block_begin == '{' + functional_block?(node) + else + !return_value_used?(node) end end - private + def multiline_block_style?(node) + block_length = Util.block_length(node) + block_begin = node.loc.begin.source + + if block_length > 0 + block_begin != '{' + else + block_begin == '{' + end + end def correction(node) lambda do |corrector| @@ -74,6 +122,40 @@ def parentheses?(send_node) def operator?(method_name) method_name =~ /^\W/ end + + def functional_block?(node) + return_value_used?(node) || return_value_of_scope?(node) + end + + def whitelisted_method?(node) + receiver, _args, _body = *node + _, method_name = *receiver + + whitelist.include?(method_name) + end + + def return_value_used?(node) + return unless node.parent + + # If there are parentheses around the block, check if that + # is being used. + if node.parent.begin_type? + return_value_used?(node.parent) + else + Util::ASGN_NODES.include?(node.parent.type) || + node.parent.send_type? + end + end + + def return_value_of_scope?(node) + return unless node.parent + + node.parent.children.last == node + end + + def whitelist + Array(cop_config['Whitelist']).map(&:to_sym) + end end end end diff --git a/spec/rubocop/cop/style/blocks_spec.rb b/spec/rubocop/cop/style/blocks_spec.rb index cdd65450770b..537d2284fe16 100644 --- a/spec/rubocop/cop/style/blocks_spec.rb +++ b/spec/rubocop/cop/style/blocks_spec.rb @@ -2,120 +2,276 @@ require 'spec_helper' -describe RuboCop::Cop::Style::Blocks do - subject(:cop) { described_class.new } +describe RuboCop::Cop::Style::Blocks, :config do + subject(:cop) { described_class.new(config) } - it 'accepts a multiline block with do-end' do - inspect_source(cop, ['each do |x|', - 'end']) - expect(cop.offenses).to be_empty - end + context 'Weirich style' do + let(:cop_config) { { 'EnforcedStyle' => 'weirich' } } - it 'registers an offense for a single line block with do-end' do - inspect_source(cop, 'each do |x| end') - expect(cop.messages) - .to eq(['Prefer {...} over do...end for single-line blocks.']) - end + it 'accepts a multiline block with braces if the return value is ' \ + 'assigned' do + inspect_source(cop, ['foo = map { |x|', + ' x', + '}']) + expect(cop.offenses).to be_empty + end - it 'accepts a single line block with braces' do - inspect_source(cop, 'each { |x| }') - expect(cop.offenses).to be_empty - end + it 'accepts a multiline block with braces if it is the return value ' \ + 'of its scope' do + inspect_source(cop, ['block do', + ' map { |x|', + ' x', + ' }', + 'end']) + expect(cop.offenses).to be_empty + end - it 'auto-corrects do and end for single line blocks to { and }' do - new_source = autocorrect_source(cop, 'block do |x| end') - expect(new_source).to eq('block { |x| }') - end + it 'accepts a multiline block with braces when passed as an argument' do + inspect_source(cop, ['puts map { |x|', + ' x', + '}']) + expect(cop.offenses).to be_empty + end - it 'does not auto-correct do-end if {} would change the meaning' do - src = "s.subspec 'Subspec' do |sp| end" - new_source = autocorrect_source(cop, src) - expect(new_source).to eq(src) - end + it 'accepts a multiline block with braces when chained' do + inspect_source(cop, ['map { |x|', + ' x', + '}.inspect']) + expect(cop.offenses).to be_empty + end - it 'does not auto-correct {} if do-end would change the meaning' do - src = ['foo :bar, :baz, qux: lambda { |a|', - ' bar a', - '}'].join("\n") - new_source = autocorrect_source(cop, src) - expect(new_source).to eq(src) - end + it 'accepts a multiline block with braces when passed to a ' \ + 'whitelisted method' do + inspect_source(cop, ['let(:foo) {', + ' x', + '}']) + expect(cop.offenses).to be_empty + end - context 'when there are braces around a multi-line block' do - it 'registers an offense in the simple case' do - inspect_source(cop, ['each { |x|', + it 'registers an offense for a multiline block with braces if the ' \ + 'return value is not used' do + inspect_source(cop, ['map { |x|', + ' x', '}']) expect(cop.messages) - .to eq(['Avoid using {...} for multi-line blocks.']) - end - - it 'accepts braces if do-end would change the meaning' do - src = ['scope :foo, lambda { |f|', - ' where(condition: "value")', - '}', - '', - 'expect { something }.to raise_error(ErrorClass) { |error|', - ' # ...', - '}', - '', - 'expect { x }.to change {', - ' Counter.count', - '}.from(0).to(1)'] - inspect_source(cop, src) - expect(cop.offenses).to be_empty + .to eq(['Prefer do...end over {...} for procedural blocks.']) end - it 'registers an offense for braces if do-end would not change ' \ - 'the meaning' do - src = ['scope :foo, (lambda { |f|', - ' where(condition: "value")', - '})', - '', - 'expect { something }.to(raise_error(ErrorClass) { |error|', - ' # ...', - '})'] - inspect_source(cop, src) - expect(cop.offenses.size).to eq(2) - end - - it 'can handle special method names such as []= and done?' do - src = ['h2[k2] = Hash.new { |h3,k3|', - ' h3[k3] = 0', - '}', - '', - 'x = done? list.reject { |e|', - ' e.nil?', - '}'] - inspect_source(cop, src) + it 'registers an offense for a multiline block with do-end if the ' \ + 'return value is assigned' do + inspect_source(cop, ['foo = map do |x|', + ' x', + 'end']) expect(cop.messages) - .to eq(['Avoid using {...} for multi-line blocks.']) + .to eq(['Prefer {...} over do...end for functional blocks.']) end - it 'auto-corrects { and } to do and end' do + it 'registers an offense for a multiline block with do-end if the ' \ + 'return value is passed as an argument' do + inspect_source(cop, ['puts (map do |x|', + ' x', + 'end)']) + expect(cop.messages) + .to eq(['Prefer {...} over do...end for functional blocks.']) + end + + it 'accepts a multiline block with do-end if it is the return value ' \ + 'of its scope' do + inspect_source(cop, ['block do', + ' map do |x|', + ' x', + ' end', + 'end']) + expect(cop.messages).to be_empty + end + + it 'accepts a multiline functional block with do-end if it is ' \ + 'whitelisted' do + inspect_source(cop, ['foo = bar.tap do |x|', + ' x.age = 3', + 'end']) + expect(cop.messages).to be_empty + end + + it 'auto-corrects { and } to do and end if it is a procedural block' do source = <<-END.strip_indent - each{ |x| - some_method - other_method + map { |x| + x } END expected_source = <<-END.strip_indent - each do |x| - some_method - other_method + map do |x| + x + end + END + + new_source = autocorrect_source(cop, source) + expect(new_source).to eq(expected_source) + end + + it 'does not auto-correct {} to do-end if it is a whitelisted method' do + source = <<-END.strip_indent + let(:foo) { |x| + x + } + END + + new_source = autocorrect_source(cop, source) + expect(new_source).to eq(source) + end + + it 'auto-corrects do-end to {} if it is a functional block' do + source = <<-END.strip_indent + foo = map do |x| + x end END + expected_source = <<-END.strip_indent + foo = map { |x| + x + } + END + new_source = autocorrect_source(cop, source) expect(new_source).to eq(expected_source) end - it 'does not auto-correct {} if do-end would introduce a syntax error' do - src = ['my_method :arg1, arg2: proc {', - ' something', - '}, arg3: :another_value'].join("\n") + it 'auto-corrects do-end to {} if it is a functional block and does ' \ + 'not change the meaning' do + source = <<-END.strip_indent + puts (map do |x| + x + end) + END + + expected_source = <<-END.strip_indent + puts (map { |x| + x + }) + END + + new_source = autocorrect_source(cop, source) + expect(new_source).to eq(expected_source) + end + end + + context 'multi-line style' do + let(:cop_config) { { 'EnforcedStyle' => 'multiline' } } + + it 'accepts a multiline block with do-end' do + inspect_source(cop, ['each do |x|', + 'end']) + expect(cop.offenses).to be_empty + end + + it 'registers an offense for a single line block with do-end' do + inspect_source(cop, 'each do |x| end') + expect(cop.messages) + .to eq(['Prefer {...} over do...end for single-line blocks.']) + end + + it 'accepts a single line block with braces' do + inspect_source(cop, 'each { |x| }') + expect(cop.offenses).to be_empty + end + + it 'auto-corrects do and end for single line blocks to { and }' do + new_source = autocorrect_source(cop, 'block do |x| end') + expect(new_source).to eq('block { |x| }') + end + + it 'does not auto-correct do-end if {} would change the meaning' do + src = "s.subspec 'Subspec' do |sp| end" + new_source = autocorrect_source(cop, src) + expect(new_source).to eq(src) + end + + it 'does not auto-correct {} if do-end would change the meaning' do + src = ['foo :bar, :baz, qux: lambda { |a|', + ' bar a', + '}'].join("\n") new_source = autocorrect_source(cop, src) expect(new_source).to eq(src) end + + context 'when there are braces around a multi-line block' do + it 'registers an offense in the simple case' do + inspect_source(cop, ['each { |x|', + '}']) + expect(cop.messages) + .to eq(['Avoid using {...} for multi-line blocks.']) + end + + it 'accepts braces if do-end would change the meaning' do + src = ['scope :foo, lambda { |f|', + ' where(condition: "value")', + '}', + '', + 'expect { something }.to raise_error(ErrorClass) { |error|', + ' # ...', + '}', + '', + 'expect { x }.to change {', + ' Counter.count', + '}.from(0).to(1)'] + inspect_source(cop, src) + expect(cop.offenses).to be_empty + end + + it 'registers an offense for braces if do-end would not change ' \ + 'the meaning' do + src = ['scope :foo, (lambda { |f|', + ' where(condition: "value")', + '})', + '', + 'expect { something }.to(raise_error(ErrorClass) { |error|', + ' # ...', + '})'] + inspect_source(cop, src) + expect(cop.offenses.size).to eq(2) + end + + it 'can handle special method names such as []= and done?' do + src = ['h2[k2] = Hash.new { |h3,k3|', + ' h3[k3] = 0', + '}', + '', + 'x = done? list.reject { |e|', + ' e.nil?', + '}'] + inspect_source(cop, src) + expect(cop.messages) + .to eq(['Avoid using {...} for multi-line blocks.']) + end + + it 'auto-corrects { and } to do and end' do + source = <<-END.strip_indent + each{ |x| + some_method + other_method + } + END + + expected_source = <<-END.strip_indent + each do |x| + some_method + other_method + end + END + + new_source = autocorrect_source(cop, source) + expect(new_source).to eq(expected_source) + end + + it 'does not auto-correct {} if do-end would introduce a syntax error' do + src = ['my_method :arg1, arg2: proc {', + ' something', + '}, arg3: :another_value'].join("\n") + new_source = autocorrect_source(cop, src) + expect(new_source).to eq(src) + end + end end end