From 37cde0d83b26e25aeb3ba2931089e896c067eeb9 Mon Sep 17 00:00:00 2001 From: Justin Searls Date: Wed, 3 Mar 2021 15:38:17 -0500 Subject: [PATCH] Replace SemanticBlocks with custom BlockDelimiters This change will move Standard from attempting to enforce Weirich-style semantic blocks, because there will always be edge-cases we can't cover automatically, given how expressive Ruby is and the fact that certain cases are genuinely ambiguous, such that an automated tool cannot reliably determine the authorial intent of a block. Instead, the new rule will only enforce that single-line blocks be delimited with `{` and `}`, but otherwise allow users to use whatever multiline format they wish. --- CHANGELOG.md | 8 +- LICENSE.txt | 2 +- README.md | 10 +- config/base.yml | 35 +---- lib/standard.rb | 2 +- lib/standard/cop/block_delimiters.rb | 96 ++++++++++++ lib/standard/cop/semantic_blocks.rb | 165 --------------------- test/fixture/cli/autocorrectable-good.rb | 12 +- test/standard/cop/block_delimiters_test.rb | 56 +++++++ test/standard/cop/semantic_blocks_test.rb | 118 --------------- test/standard_test.rb | 2 +- 11 files changed, 176 insertions(+), 330 deletions(-) create mode 100644 lib/standard/cop/block_delimiters.rb delete mode 100644 lib/standard/cop/semantic_blocks.rb create mode 100644 test/standard/cop/block_delimiters_test.rb delete mode 100644 test/standard/cop/semantic_blocks_test.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 07c6b94b..a3a71add 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## unreleased changes + +* Relax multi-line block rules, moving away from enforcing semantic blocks to + instead allowing code to adhere to whatever multi-line format the author deems + best + ## 0.13.0 * Update rubocop from 1.7.0 to [1.10.0](https://github.com/rubocop-hq/rubocop/releases/tag/v1.10.0) enabling: @@ -12,7 +18,7 @@ * Update rubocop from 1.7.0 to [1.8.1](https://github.com/rubocop-hq/rubocop/releases/tag/v1.8.1) * Enabled [`Style/SlicingWithRange`](https://github.com/testdouble/standard/issues/175) -## 0.11.0 +## 0.11.0 * Update rubocop-performance from 1.9.1 to [1.9.2](https://github.com/rubocop-hq/rubocop-performance/releases/tag/v1.9.2) * Update rubocop from 1.4.2 to [1.7.0](https://github.com/rubocop-hq/rubocop/releases/tag/v1.7.0) diff --git a/LICENSE.txt b/LICENSE.txt index 5c3001b7..71df6f9b 100644 --- a/LICENSE.txt +++ b/LICENSE.txt @@ -2,7 +2,7 @@ Copyright (c) 2019 Test Double, LLC Portions of these files Copyright (c) 2012-18 Bozhidar Batsov: - config/base.yml - - lib/standard/cop/semantic_blocks.rb + - lib/standard/cop/block_delimiters.rb - test/cop_invoker.rb Permission is hereby granted, free of charge, to any person obtaining diff --git a/README.md b/README.md index c1335943..9796afc3 100644 --- a/README.md +++ b/README.md @@ -40,10 +40,12 @@ flag. you'll need interpolation in a string slows people down - **1.9 hash syntax** - When all the keys in a hash literal are symbols, Standard enforces Ruby 1.9's `{hash: syntax}` -- **Semantic blocks** - `{`/`}` for functional blocks that return a value, and - `do`/`end` for procedural blocks that have side effects. More - [here](http://www.virtuouscode.com/2011/07/26/the-procedurefunction-block-convention-in-ruby/) - and [here](https://github.com/rubocop-hq/ruby-style-guide/issues/162) +- **Braces for single-line blocks** - Require `{`/`}` for one-line blocks, but + allow either braces or `do`/`end` for multiline blocks. Like using `do`/`end` + for multiline blocks? Prefer `{`/`}` when chaining? A fan of expressing intent + with Jim Weirich's [semantic + block](http://www.virtuouscode.com/2011/07/26/the-procedurefunction-block-convention-in-ruby/) + approach? Standard lets you do you! - **Leading dots on multi-line method chains** - chosen for [these](https://github.com/testdouble/standard/issues/75) reasons. - **Spaces inside blocks, but not hash literals** - In Ruby, the `{` and `}` diff --git a/config/base.yml b/config/base.yml index d832a288..c91fdc8a 100644 --- a/config/base.yml +++ b/config/base.yml @@ -738,39 +738,8 @@ Security/YAMLLoad: Enabled: true SafeAutoCorrect: false -Standard/SemanticBlocks: - ProceduralMethods: - - benchmark - - bm - - bmbm - - tap - # Enumerable - - cycle - - each - - each_cons - - each_entry - - each_slice - - each_with_index - # Rails - - transaction - FunctionalMethods: - - given - - given! - - let - - let! - - subject - - watch - - Given - - Given! - - Invariant - - Then - - And - IgnoredMethods: - - lambda - - proc - - describe - - it - - When +Standard/BlockDelimiters: + Enabled: true Style/Alias: Enabled: true diff --git a/lib/standard.rb b/lib/standard.rb index 09446894..01d29f6e 100644 --- a/lib/standard.rb +++ b/lib/standard.rb @@ -7,7 +7,7 @@ require "standard/railtie" if defined?(Rails) && defined?(Rails::Railtie) require "standard/formatter" -require "standard/cop/semantic_blocks" +require "standard/cop/block_delimiters" module Standard end diff --git a/lib/standard/cop/block_delimiters.rb b/lib/standard/cop/block_delimiters.rb new file mode 100644 index 00000000..66aca08e --- /dev/null +++ b/lib/standard/cop/block_delimiters.rb @@ -0,0 +1,96 @@ +module RuboCop::Cop + module Standard + # Check for uses of braces around single line blocks, but allows either + # braces or do/end for multi-line blocks. + # + # @example + # # bad - single line block + # items.each do |item| item / 5 end + # + # # good - single line block + # items.each { |item| item / 5 } + # + class BlockDelimiters < RuboCop::Cop::Base + extend RuboCop::Cop::AutoCorrector + + def on_send(node) + return unless node.arguments? + return if node.parenthesized? + return if node.operator_method? || node.assignment_method? + + node.arguments.each do |arg| + get_blocks(arg) do |block| + # If there are no parentheses around the arguments, then braces + # and do-end have different meaning due to how they bind, so we + # allow either. + ignore_node(block) + end + end + end + + def on_block(node) + return if ignored_node?(node) + return if proper_block_style?(node) + + message = message(node) + add_offense(node.loc.begin, message: message) do |corrector| + autocorrect(corrector, node) + end + end + + private + + def get_blocks(node, &block) + case node.type + when :block + yield node + when :send + get_blocks(node.receiver, &block) if node.receiver + when :hash + # A hash which is passed as method argument may have no braces + # In that case, one of the K/V pairs could contain a block node + # which could change in meaning if do...end replaced {...} + return if node.braces? + + node.each_child_node { |child| get_blocks(child, &block) } + when :pair + node.each_child_node { |child| get_blocks(child, &block) } + end + end + + def proper_block_style?(node) + node.multiline? || node.braces? + end + + def message(node) + "Prefer `{...}` over `do...end` for single-line blocks." + end + + def autocorrect(corrector, node) + return if correction_would_break_code?(node) + + replace_do_end_with_braces(corrector, node.loc) + end + + def correction_would_break_code?(node) + return unless node.keywords? + + node.send_node.arguments? && !node.send_node.parenthesized? + end + + def replace_do_end_with_braces(corrector, loc) + b = loc.begin + e = loc.end + + corrector.insert_after(b, " ") unless whitespace_after?(b, 2) + + corrector.replace(b, "{") + corrector.replace(e, "}") + end + + def whitespace_after?(range, length = 1) + /\s/.match?(range.source_buffer.source[range.begin_pos + length, 1]) + end + end + end +end diff --git a/lib/standard/cop/semantic_blocks.rb b/lib/standard/cop/semantic_blocks.rb deleted file mode 100644 index ecac4dfb..00000000 --- a/lib/standard/cop/semantic_blocks.rb +++ /dev/null @@ -1,165 +0,0 @@ -module RuboCop::Cop - module Standard - class SemanticBlocks < RuboCop::Cop::Base - include RuboCop::Cop::IgnoredMethods - extend RuboCop::Cop::AutoCorrector - - def on_send(node) - return unless node.arguments? - return if node.parenthesized? || node.operator_method? - - node.arguments.each do |arg| - get_blocks(arg) do |block| - # If there are no parentheses around the arguments, then braces - # and do-end have different meaning due to how they bind, so we - # allow either. - ignore_node(block) - end - end - end - - def on_block(node) - return if ignored_node?(node) || - proper_block_style?(node) || - (!node.braces? && rescue_child_block?(node)) - - add_offense(node.loc.begin, message: message(node)) do |corrector| - return if correction_would_break_code?(node) - - if node.braces? - replace_braces_with_do_end(corrector, node.loc) - else - replace_do_end_with_braces(corrector, node.loc) - end - end - end - - private - - def message(node) - if node.single_line? - "Prefer `{...}` over `do...end` for single-line blocks." - elsif node.loc.begin.source == "{" - "Prefer `do...end` over `{...}` for procedural blocks." - else - "Prefer `{...}` over `do...end` for functional blocks." - end - end - - def replace_braces_with_do_end(corrector, loc) - b = loc.begin - e = loc.end - - corrector.insert_before(b, " ") unless whitespace_before?(b) - corrector.insert_before(e, " ") unless whitespace_before?(e) - corrector.insert_after(b, " ") unless whitespace_after?(b) - corrector.replace(b, "do") - corrector.replace(e, "end") - end - - def replace_do_end_with_braces(corrector, loc) - b = loc.begin - e = loc.end - - corrector.insert_after(b, " ") unless whitespace_after?(b, 2) - - corrector.replace(b, "{") - corrector.replace(e, "}") - end - - def whitespace_before?(range) - range.source_buffer.source[range.begin_pos - 1, 1] =~ /\s/ - end - - def whitespace_after?(range, length = 1) - range.source_buffer.source[range.begin_pos + length, 1] =~ /\s/ - end - - def get_blocks(node, &block) - case node.type - when :block - yield node - when :send - get_blocks(node.receiver, &block) if node.receiver - when :hash - # A hash which is passed as method argument may have no braces - # In that case, one of the K/V pairs could contain a block node - # which could change in meaning if do...end replaced {...} - return if node.braces? - - node.each_child_node { |child| get_blocks(child, &block) } - when :pair - node.each_child_node { |child| get_blocks(child, &block) } - end - end - - def proper_block_style?(node) - method_name = node.method_name - - if ignored_method?(method_name) - true - elsif node.single_line? - node.braces? - elsif node.braces? - !procedural_method?(method_name) && - (functional_method?(method_name) || functional_block?(node)) - else - procedural_method?(method_name) || !return_value_used?(node) - end - end - - def correction_would_break_code?(node) - rescue_child_block?(node) || non_parenthesized_keyword_args?(node) - end - - def rescue_child_block?(node) - node.children.any? { |node| node.rescue_type? || node.ensure_type? } - end - - def non_parenthesized_keyword_args?(node) - return unless node.keywords? - - node.send_node.arguments? && !node.send_node.parenthesized? - end - - def functional_method?(method_name) - cop_config["FunctionalMethods"].map(&:to_sym).include?(method_name) - end - - def functional_block?(node) - return_value_used?(node) || return_value_of_scope?(node) - end - - def procedural_method?(method_name) - cop_config["ProceduralMethods"].map(&:to_sym).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 - node.parent.assignment? || node.parent.send_type? - end - end - - def return_value_of_scope?(node) - return unless node.parent - - conditional?(node.parent) || array_or_range?(node.parent) || - node.parent.children.last == node - end - - def conditional?(node) - node.if_type? || node.or_type? || node.and_type? - end - - def array_or_range?(node) - node.array_type? || node.irange_type? || node.erange_type? - end - end - end -end diff --git a/test/fixture/cli/autocorrectable-good.rb b/test/fixture/cli/autocorrectable-good.rb index 9899a4f1..01618cf3 100644 --- a/test/fixture/cli/autocorrectable-good.rb +++ b/test/fixture/cli/autocorrectable-good.rb @@ -21,19 +21,19 @@ def do_stuff(a:, b:, c:) 5 + 6 - plus_stuff = STUFF.map { |e| + plus_stuff = STUFF.map do |e| e + 1 + maths_and_stuff - } + end STUFF.tap { |arr| arr.delete(0) } STUFF.each { |e| e.succ } - THINGS.tap do |things| + THINGS.tap { |things| if THINGS.is_a?(Hash) 42 + 8 end - end + } test = "hi" test2 = "hi" @@ -41,9 +41,9 @@ def do_stuff(a:, b:, c:) if test3 32 + 3 end - THINGS.keys.each do |key| + THINGS.keys.each { |key| THINGS[key] = plus_stuff[i] - end + } end def do_even_more_stuff diff --git a/test/standard/cop/block_delimiters_test.rb b/test/standard/cop/block_delimiters_test.rb new file mode 100644 index 00000000..65414db4 --- /dev/null +++ b/test/standard/cop/block_delimiters_test.rb @@ -0,0 +1,56 @@ +require "test_helper" +require "cop_invoker" + +class RuboCop::Cop::Standard::BlockDelimitersTest < UnitTest + include CopInvoker + + def setup + config = RuboCop::Config.new("Standard/BlockDelimiters" => { + "Enabled" => true + }) + @cop = RuboCop::Cop::Standard::BlockDelimiters.new(config) + end + + def test_single_line_with_braces + assert_no_offense @cop, <<-RUBY + result = [:a].map { |arg| arg } + RUBY + end + + def test_single_line_with_do_end + assert_offense @cop, <<-RUBY + result = [:a].map do |arg| arg end + ^^ Prefer `{...}` over `do...end` for single-line blocks. + RUBY + + assert_correction @cop, <<-RUBY + result = [:a].map { |arg| arg } + RUBY + end + + def test_multiline_with_braces + assert_no_offense @cop, <<-RUBY + lulz = [:a].map { + :lol + } + RUBY + end + + def test_multiline_with_do_end + assert_no_offense @cop, <<-RUBY + lulz = [:a].map do + :lol + end + RUBY + end + + def test_braces_with_rescue_does_not_fail + assert_no_offense @cop, <<-RUBY + lulz = [:a].map do + :lol + rescue StandardError + :nolol + end + RUBY + end +end diff --git a/test/standard/cop/semantic_blocks_test.rb b/test/standard/cop/semantic_blocks_test.rb deleted file mode 100644 index 86927c89..00000000 --- a/test/standard/cop/semantic_blocks_test.rb +++ /dev/null @@ -1,118 +0,0 @@ -require "test_helper" -require "cop_invoker" - -class RuboCop::Cop::Standard::SemanticBlocksTest < UnitTest - include CopInvoker - - def setup - standard_config = YAML.load_file("config/base.yml")["Standard/SemanticBlocks"] - config = RuboCop::Config.new("Standard/SemanticBlocks" => standard_config.merge( - "Enabled" => true - )) - @cop = RuboCop::Cop::Standard::SemanticBlocks.new(config) - end - - def test_functional_block_single_line_braces - assert_no_offense @cop, <<-RUBY - result = [:a].map { |arg| arg } - RUBY - end - - # This is an exception to "pure" semantic blocks - # See: https://github.com/testdouble/standard/pull/9 - def test_functional_block_single_line_do_end - assert_offense @cop, <<-RUBY - result = [:a].map do |arg| arg end - ^^ Prefer `{...}` over `do...end` for single-line blocks. - RUBY - end - - # This is an exception to "pure" semantic blocks - # See: https://github.com/testdouble/standard/pull/9 - def test_procedural_block_single_line_braces - assert_no_offense @cop, <<-RUBY - [:a].map { puts "procedural" } - RUBY - end - - # This is an exception to "pure" semantic blocks - # See: https://github.com/testdouble/standard/pull/9 - def test_procedural_block_single_line_do_end - assert_offense @cop, <<-RUBY - [:a].map do puts "procedural" end - ^^ Prefer `{...}` over `do...end` for single-line blocks. - RUBY - end - - def test_do_end_with_assignment_fails - assert_offense @cop, <<-RUBY - lulz = [:a].map do - ^^ Prefer `{...}` over `do...end` for functional blocks. - :lol - end - RUBY - - assert_correction @cop, <<-RUBY - lulz = [:a].map { - :lol - } - RUBY - end - - def test_braces_with_assignment_passes - assert_no_offense @cop, <<-RUBY - lulz = [:a].map { - :lol - } - RUBY - end - - def test_braces_with_rescue_does_not_fail - assert_no_offense @cop, <<-RUBY - lulz = [:a].map do - :lol - rescue StandardError - :nolol - end - RUBY - end - - def test_braces_with_side_effect_fails - assert_offense @cop, <<-RUBY - 42.tap { - ^ Prefer `do...end` over `{...}` for procedural blocks. - puts "cool" - } - RUBY - - assert_correction @cop, <<-RUBY - 42.tap do - puts "cool" - end - RUBY - end - - def test_do_end_with_side_effect_passes - assert_no_offense @cop, <<-RUBY - 42.tap do - puts "cool" - end - RUBY - end - - def test_method_with_arguments_without_parentheses_multi_line_block_curly_braces - assert_no_offense @cop, <<-RUBY - some_method argument, another_argument { |block_argument| - puts "curly braces should be allowed here" - } - RUBY - end - - def test_accepts_a_multiline_functional_block_with_do_end_if_it_is_an_ignored_method - assert_no_offense @cop, <<-RUBY - foo = lambda do - puts 42 - end - RUBY - end -end diff --git a/test/standard_test.rb b/test/standard_test.rb index 2db2c603..841e0819 100644 --- a/test/standard_test.rb +++ b/test/standard_test.rb @@ -4,7 +4,7 @@ class StandardTest < UnitTest def test_loads_stuff refute_nil RuboCop refute_nil Standard::Cli - refute_nil RuboCop::Cop::Standard::SemanticBlocks + refute_nil RuboCop::Cop::Standard::BlockDelimiters assert_instance_of Gem::Version, ::Standard::VERSION end end