From 2c20be10d83b75f099881d1f9be866a251676bc1 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`. As there are some methods that are functional or procedural but cannot be categorised as such from their usage alone, add configurable lists to permit methods such as `RSpec::Core::ExampleGroup.let` which is functional but appears procedural and `tap` which is procedural but appears functional. For methods which can be both functional and procedural (such as `lambda`) and cannot be categorised by usage, add a configurable list of ignored methods. 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 | 65 +++++ lib/rubocop/cop/style/blocks.rb | 126 ++++++++- spec/rubocop/cop/style/blocks_spec.rb | 378 ++++++++++++++++++++------ 4 files changed, 478 insertions(+), 94 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a9f1eb453523..84e75bd55117 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][]) @@ -1317,3 +1318,5 @@ [@vassilevsky]: https://github.com/vassilevsky [@gerry3]: https://github.com/gerry3 [@ypresto]: https://github.com/ypresto +[@clowder]: https://github.com/clowder +[@mudge]: https://github.com/mudge diff --git a/config/default.yml b/config/default.yml index 3e105619a761..85716a8cb4bd 100644 --- a/config/default.yml +++ b/config/default.yml @@ -148,6 +148,71 @@ Style/BarePercentLiterals: - percent_q - bare_percent +Style/Blocks: + EnforcedStyle: multiline + SupportedStyles: + # The `multiline` style enforces braces around single line blocks and + # do..end around multi-line blocks. + - multiline + # The `weirich` style enforces braces around functional blocks, where the + # primary purpose of the block is to return a value and do..end for + # procedural blocks, where the primary purpose of the block is its + # side-effects. + # + # This looks at the usage of a block's method to determine its type (e.g. is + # the result of a `map` assigned to a variable or passed to another + # method) but exceptions are permitted in the `ProceduralMethods`, + # `FunctionalMethods` and `IgnoredMethods` sections below. + - weirich + ProceduralMethods: + # Methods that are known to be procedural in nature but look functional from + # their usage, e.g. + # + # time = Benchmark.realtime do + # foo.bar + # end + # + # Here, the return value of the block is discarded but the return value of + # `Benchmark.realtime` is used. + - benchmark + - bm + - bmbm + - create + - each_with_object + - measure + - new + - realtime + - tap + - with_object + FunctionalMethods: + # Methods that are known to be functional in nature but look procedural from + # their usage, e.g. + # + # let(:foo) { Foo.new } + # + # Here, the return value of `Foo.new` is used to define a `foo` helper but + # doesn't appear to be used from the return value of `let`. + - let + - let! + - subject + IgnoredMethods: + # Methods that can be either procedural or functional and cannot be + # categorised from their usage alone, e.g. + # + # foo = lambda do |x| + # puts "Hello, #{x}" + # end + # + # foo = lambda do |x| + # x * 100 + # end + # + # Here, it is impossible to tell from the return value of `lambda` whether + # the inner block's return value is significant. + - lambda + - proc + - it + Style/BracesAroundHashParameters: EnforcedStyle: no_braces SupportedStyles: diff --git a/lib/rubocop/cop/style/blocks.rb b/lib/rubocop/cop/style/blocks.rb index 9e8a3e855937..90cde09f20eb 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,18 +25,35 @@ def on_send(node) def on_block(node) return if ignored_node?(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) + if proper_block_style?(node) + correct_style_detected + else + add_offense(node, :begin) { opposite_style_detected } 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 correction(node) lambda do |corrector| b, e = node.loc.begin, node.loc.end @@ -74,6 +89,97 @@ def parentheses?(send_node) def operator?(method_name) method_name =~ /^\W/ 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) + method_name = extract_method_name_from_block(node) + return true if ignored_method?(method_name) + + block_begin = node.loc.begin.source + + if block_begin == '{' + functional_method?(method_name) || functional_block?(node) + else + procedural_method?(method_name) || !return_value_used?(node) + end + end + + 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 extract_method_name_from_block(block) + node, _args, _body = *block + _receiver, method_name, *_args = *node + + method_name + end + + def ignored_method?(method_name) + ignored_methods.include?(method_name) + end + + def functional_method?(method_name) + functional_methods.include?(method_name) + end + + def functional_block?(node) + return_value_used?(node) || return_value_of_scope?(node) + end + + def procedural_method?(method_name) + procedural_methods.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 + + conditional?(node.parent) || node.parent.children.last == node + end + + def procedural_methods + cop_config['ProceduralMethods'].map(&:to_sym) + end + + def functional_methods + cop_config['FunctionalMethods'].map(&:to_sym) + end + + def ignored_methods + cop_config['IgnoredMethods'].map(&:to_sym) + end + + def conditional?(node) + node.if_type? || node.or_type? || node.and_type? + end end end end diff --git a/spec/rubocop/cop/style/blocks_spec.rb b/spec/rubocop/cop/style/blocks_spec.rb index cdd65450770b..14130ed235c0 100644 --- a/spec/rubocop/cop/style/blocks_spec.rb +++ b/spec/rubocop/cop/style/blocks_spec.rb @@ -2,107 +2,148 @@ 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 + cop_config = { + 'EnforcedStyle' => 'weirich', + 'ProceduralMethods' => %w(tap), + 'FunctionalMethods' => %w(let), + 'IgnoredMethods' => %w(lambda) + } - 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 + let(:cop_config) { cop_config } - 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 the return value is ' \ + 'assigned' do + inspect_source(cop, ['foo = map { |x|', + ' 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 '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 '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 passed to a method' do + inspect_source(cop, ['puts map { |x|', + ' x', + '}']) + 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 chained' do + inspect_source(cop, ['map { |x|', + ' x', + '}.inspect']) + expect(cop.offenses).to be_empty + end + + it 'accepts a multiline block with braces when passed to a known ' \ + 'functional 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 + it 'registers an offense for a multiline block with braces if the ' \ + 'return value is not used' do inspect_source(cop, ['each { |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 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(['Prefer {...} over do...end for functional blocks.']) + end + + it 'registers an offense for a multiline block with do-end if the ' \ + 'return value is passed to a method' 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 single line block with {} if used in an if statement' do + inspect_source(cop, 'return if any? { |x| x }') + expect(cop.messages).to be_empty + end + + it 'accepts a single line block with {} if used in a logical or' do + inspect_source(cop, 'any? { |c| c } || foo') + expect(cop.messages).to be_empty + end + + it 'accepts a single line block with {} if used in a logical and' do + inspect_source(cop, 'any? { |c| c } && foo') + expect(cop.messages).to be_empty + end + + it 'accepts a multiline functional block with do-end if it is ' \ + 'a known procedural method' do + inspect_source(cop, ['foo = bar.tap do |x|', + ' x.age = 3', + 'end']) + expect(cop.messages).to be_empty + end + + it 'accepts a multiline functional block with do-end if it is ' \ + 'an ignored method' do + inspect_source(cop, ['foo = lambda do', + ' puts 42', + 'end']) + expect(cop.messages).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) + it 'registers an offense for a single line procedural block' do + inspect_source(cop, 'each { |x| puts x }') expect(cop.messages) - .to eq(['Avoid using {...} for multi-line blocks.']) + .to eq(['Prefer do...end over {...} for procedural blocks.']) end - it 'auto-corrects { and } to do and end' do + it 'accepts a single line block with do-end if it is procedural' do + inspect_source(cop, 'each do |x| puts x; 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 + each { |x| + x } END expected_source = <<-END.strip_indent each do |x| - some_method - other_method + x end END @@ -110,12 +151,181 @@ 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 'does not auto-correct {} to do-end if it is a known functional ' \ + '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 'does not autocorrect do-end to {} if it is a known procedural ' \ + 'method' do + source = <<-END.strip_indent + foo = bar.tap do |x| + x.age = 1 + end + 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 '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