From 41b8400db89d0cfb1a80e30f4204726d6340784f Mon Sep 17 00:00:00 2001 From: Miyake J Takuma Date: Sun, 24 Apr 2022 08:47:11 +0900 Subject: [PATCH 1/6] Enhance the autocorrect for `RSpec/FetchEnvVar` --- ...hance_the_autocorrect_for_fetch_env_var.md | 1 + config/default.yml | 3 + lib/rubocop/cop/style/fetch_env_var.rb | 197 ++++++++- spec/rubocop/cop/style/fetch_env_var_spec.rb | 411 ++++++++++++++++-- 4 files changed, 574 insertions(+), 38 deletions(-) create mode 100644 changelog/change_enhance_the_autocorrect_for_fetch_env_var.md diff --git a/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md b/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md new file mode 100644 index 00000000000..7f4da693d06 --- /dev/null +++ b/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md @@ -0,0 +1 @@ +* [#10585](https://github.com/rubocop/rubocop/pull/10585): Enhance the autocorrect for `RSpec/FetchEnvVar`. ([@johnny-miyake][]) diff --git a/config/default.yml b/config/default.yml index 761593f25c9..014af1fb845 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3538,6 +3538,9 @@ Style/FetchEnvVar: VersionAdded: '1.28' # Environment variables to be excluded from the inspection. AllowedVars: [] + # - `SecondArgOfFetch` puts the RHS to the second argument of `ENV.fetch`. (default) + # - `BlockContent` puts the RHS into a block. + BasicLiteralRhs: 'SecondArgOfFetch' Style/FileRead: Description: 'Favor `File.(bin)read` convenience methods.' diff --git a/lib/rubocop/cop/style/fetch_env_var.rb b/lib/rubocop/cop/style/fetch_env_var.rb index 9ed2886883f..774e52e5a52 100644 --- a/lib/rubocop/cop/style/fetch_env_var.rb +++ b/lib/rubocop/cop/style/fetch_env_var.rb @@ -9,17 +9,37 @@ module Style # On the other hand, `ENV.fetch` raises KeyError or returns the explicitly # specified default value. # + # When an `ENV[]` is the LHS of `||`, the autocorrect makes the RHS + # the default value of `ENV.fetch`. + # In addition, when the RHS is a basic literal object (e.g., `'string'`, `true`, `1`), + # the autocorrect corrects the code according to the `BasicLiteralRhs` option. + # Available values + # - `SecondArgOfFetch` puts the RHS to the second argument of `ENV.fetch`. (default) + # - `BlockContent` puts the RHS into a block. + # # @example # # bad # ENV['X'] - # ENV['X'] || z + # ENV['X'] || 'string literal' + # ENV['X'] || some_method # x = ENV['X'] # + # ENV['X'] || y.map do |a| + # puts a * 2 + # end + # # # good # ENV.fetch('X') - # ENV.fetch('X', nil) || z + # ENV.fetch('X', 'string literal') + # ENV.fetch('X') { some_method } # x = ENV.fetch('X') # + # ENV.fetch('X') do + # y.map do |a| + # puts a * 2 + # end + # end + # # # also good # !ENV['X'] # ENV['X'].some_method # (e.g. `.nil?`) @@ -27,20 +47,43 @@ module Style class FetchEnvVar < Base extend AutoCorrector - MSG = 'Use `ENV.fetch(%s)` or `ENV.fetch(%s, nil)` instead of `ENV[%s]`.' + # rubocop:disable Layout/LineLength + MSG_DEFAULT_NIL = 'Use `ENV.fetch(%s)` or `ENV.fetch(%s, nil)` instead of `ENV[%s]`.' + MSG_DEFAULT_RHS_SINGLE_ARG_STYLE = 'Use `ENV.fetch(%s, %s)` instead of `ENV[%s] || %s`.' + MSG_DEFAULT_RHS_SINGLE_BLOCK_STYLE = 'Use `ENV.fetch(%s) { %s }` instead of `ENV[%s] || %s`.' + MSG_DEFAULT_RHS_MULTI = 'Use `ENV.fetch(%s)` with a block containing `%s ...`' + # rubocop:enable Layout/LineLength # @!method env_with_bracket?(node) def_node_matcher :env_with_bracket?, <<~PATTERN (send (const nil? :ENV) :[] $_) PATTERN + # @!method operand_of_or?(node) + def_node_matcher :operand_of_or?, <<~PATTERN + (^or ...) + PATTERN + + # @!method block_control?(node) + def_node_matcher :block_control?, <<~PATTERN + ({next | break | retry | redo}) + PATTERN + def on_send(node) env_with_bracket?(node) do |expression| break if allowed_var?(expression) break if allowable_use?(node) - add_offense(node, message: format(MSG, key: expression.source)) do |corrector| - corrector.replace(node, "ENV.fetch(#{expression.source}, nil)") + if operand_of_or?(node) + target_node, target_expr = rightmost_offense_in_or_chains(node) + + if right_end_of_or_chains?(target_node) || rhs_cannot_be_default_value?(target_node) + default_nil(target_node, target_expr) + else + default_rhs(target_node, target_expr) + end + else + default_nil(node, expression) end end end @@ -87,6 +130,150 @@ def assigned?(node) lhs, _method, _rhs = *parent node == lhs end + + def left_end_of_or_chains?(node) + return false unless operand_of_or?(node) + + node.parent.lhs == node + end + + def right_end_of_or_chains?(node) + !(left_end_of_or_chains?(node) || node.parent&.parent&.or_type?) + end + + # Returns the node and expression of the rightmost `ENV[]` in `||` chains. + # e.g., + # `ENV['X'] || y || z || ENV['A'] || b` + # ^^^^^^^^ Matches this one + def rightmost_offense_in_or_chains(node) + rmst_node = rmst_expr = nil + or_nodes = [node.parent] + + while (grand_parent = or_nodes.last&.parent)&.or_type? + or_nodes << grand_parent + end + + # Finds the rightmost `ENV[]` in `||` chains and yields it. + or_nodes.reverse.find do |or_node| + env_with_bracket?(or_node.rhs) do |expression| + rmst_node = or_node.rhs + rmst_expr = expression + end + end + return [rmst_node, rmst_expr] if rmst_node + + # Yields the node given to this method if no `ENV[]` is found in the above process. + expression = env_with_bracket?(node) + [node, expression] + end + + def rhs_cannot_be_default_value?(node) + rhs_is_block_control?(node) + end + + def rhs_is_block_control?(node) + rhs = left_end_of_or_chains?(node) ? node.parent.rhs : node.parent.parent.rhs + block_control?(rhs) + end + + def new_code_default_nil(expression) + "ENV.fetch(#{expression.source}, nil)" + end + + def new_code_default_rhs_single_line(node, expression) + parent = node.parent + if cop_config['BasicLiteralRhs'] == 'SecondArgOfFetch' && parent.rhs.basic_literal? + "ENV.fetch(#{expression.source}, #{parent.rhs.source})" + else + "ENV.fetch(#{expression.source}) { #{parent.rhs.source} }" + end + end + + def new_code_default_rhs_multiline(node, expression) + default = node.parent.rhs.source.split("\n").map do |line| + "#{indent(node.parent, offset: indentation_width)}#{line}" + end.join("\n") + <<~NEW_CODE.chomp + ENV.fetch(#{expression.source}) do + #{default} + end + NEW_CODE + end + + def new_code_default_rhs(node, expression) + if node.parent.rhs.single_line? + new_code_default_rhs_single_line(node, expression) + else + new_code_default_rhs_multiline(node, expression) + end + end + + def default_rhs(node, expression) + if left_end_of_or_chains?(node) + default_rhs_in_same_or(node, expression) + else + default_rhs_in_outer_or(node, expression) + end + end + + # Adds an offense and sets `nil` to the default value of `ENV.fetch`. + # `ENV['X']` --> `ENV.fetch('X', nil)` + def default_nil(node, expression) + message = format(MSG_DEFAULT_NIL, key: expression.source) + + add_offense(node, message: message) do |corrector| + corrector.replace(node, new_code_default_nil(expression)) + end + end + + # Adds an offense and makes the RHS the default value of `ENV.fetch`. + # `ENV['X'] || y` --> `ENV.fetch('X') { y }` + def default_rhs_in_same_or(node, expression) + template = message_template_for(node.parent.rhs) + message = format(template, + key: expression.source, + default: first_line_of(node.parent.rhs.source)) + + add_offense(node, message: message) do |corrector| + corrector.replace(node.parent, new_code_default_rhs(node, expression)) + end + end + + # Adds an offense and makes the RHS the default value of `ENV.fetch`. + # `z || ENV['X'] || y` --> `z || ENV.fetch('X') { y }` + def default_rhs_in_outer_or(node, expression) + parent = node.parent + grand_parent = parent.parent + + template = message_template_for(grand_parent.rhs) + message = format(template, + key: expression.source, + default: first_line_of(grand_parent.rhs.source)) + + add_offense(node, message: message) do |corrector| + lhs_code = parent.lhs.source + rhs_code = new_code_default_rhs(parent, expression) + corrector.replace(grand_parent, "#{lhs_code} || #{rhs_code}") + end + end + + def message_template_for(rhs) + if rhs.multiline? + MSG_DEFAULT_RHS_MULTI + elsif cop_config['BasicLiteralRhs'] == 'SecondArgOfFetch' && rhs.basic_literal? + MSG_DEFAULT_RHS_SINGLE_ARG_STYLE + else + MSG_DEFAULT_RHS_SINGLE_BLOCK_STYLE + end + end + + def indentation_width + config.for_cop('Layout/IndentationWidth')['Width'] || 2 + end + + def first_line_of(source) + source.split("\n").first + end end end end diff --git a/spec/rubocop/cop/style/fetch_env_var_spec.rb b/spec/rubocop/cop/style/fetch_env_var_spec.rb index be82c930898..d07cfb6f7fe 100644 --- a/spec/rubocop/cop/style/fetch_env_var_spec.rb +++ b/spec/rubocop/cop/style/fetch_env_var_spec.rb @@ -62,37 +62,6 @@ end end - context 'when the node is an operand of `||`' do - it 'registers no offenses with `||`' do - expect_offense(<<~RUBY) - ENV['X'] || y - ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. - RUBY - - expect_correction(<<~RUBY) - ENV.fetch('X', nil) || y - RUBY - - expect_offense(<<~RUBY) - y || ENV['X'] - ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. - RUBY - - expect_correction(<<~RUBY) - y || ENV.fetch('X', nil) - RUBY - - expect_offense(<<~RUBY) - z || ENV['X'] || y - ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. - RUBY - - expect_correction(<<~RUBY) - z || ENV.fetch('X', nil) || y - RUBY - end - end - context 'when the node is a receiver of `||=`' do it 'does not register an offense' do expect_no_offenses(<<~RUBY) @@ -154,7 +123,6 @@ ENV['A'].some_method, ENV['B'] || ENV['C'], ^^^^^^^^ Use `ENV.fetch('C')` or `ENV.fetch('C', nil)` instead of `ENV['C']`. - ^^^^^^^^ Use `ENV.fetch('B')` or `ENV.fetch('B', nil)` instead of `ENV['B']`. ENV['X'], ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. ENV['Y'] @@ -165,7 +133,7 @@ expect_correction(<<~RUBY) some_method( ENV['A'].some_method, - ENV.fetch('B', nil) || ENV.fetch('C', nil), + ENV.fetch('B') { ENV.fetch('C', nil) }, ENV.fetch('X', nil), ENV.fetch('Y', nil) ) @@ -336,4 +304,381 @@ RUBY end end + + context 'when the node is an operand of `||`' do + context 'and node is the right end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + y || ENV['X'] + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + y || ENV.fetch('X', nil) + RUBY + end + end + + context 'and followed by a basic literal' do + context 'when the node is the left end of `||` chains' do + context 'with BasicLiteralSecondArg style' do + let(:cop_config) { { 'BasicLiteralRhs' => 'SecondArgOfFetch' } } + + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || 'y' + ^^^^^^^^ Use `ENV.fetch('X', 'y')` instead of `ENV['X'] || 'y'`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X', 'y') + RUBY + end + end + + context 'with AlwaysUseBlock style' do + let(:cop_config) { { 'BasicLiteralRhs' => 'BlockContent' } } + + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || 'y' + ^^^^^^^^ Use `ENV.fetch('X') { 'y' }` instead of `ENV['X'] || 'y'`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { 'y' } + RUBY + end + end + end + + context 'when the node is between `||`s' do + context 'with BasicLiteralSecondArg style' do + let(:cop_config) { { 'BasicLiteralRhs' => 'SecondArgOfFetch' } } + + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || 'y' + ^^^^^^^^ Use `ENV.fetch('X', 'y')` instead of `ENV['X'] || 'y'`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X', 'y') + RUBY + end + end + + context 'with AlwaysUseBlock style' do + let(:cop_config) { { 'BasicLiteralRhs' => 'BlockContent' } } + + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || 'y' + ^^^^^^^^ Use `ENV.fetch('X') { 'y' }` instead of `ENV['X'] || 'y'`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { 'y' } + RUBY + end + end + end + end + + context 'and followed by a composite literal' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || { a: 1, b: 2 } + ^^^^^^^^ Use `ENV.fetch('X') { { a: 1, b: 2 } }` instead of `ENV['X'] || { a: 1, b: 2 }`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { { a: 1, b: 2 } } + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || { a: 1, b: 2 } + ^^^^^^^^ Use `ENV.fetch('X') { { a: 1, b: 2 } }` instead of `ENV['X'] || { a: 1, b: 2 }`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { { a: 1, b: 2 } } + RUBY + end + end + end + + context 'and followed by a variable or a no receiver method' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || y + ^^^^^^^^ Use `ENV.fetch('X') { y }` instead of `ENV['X'] || y`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { y } + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || y + ^^^^^^^^ Use `ENV.fetch('X') { y }` instead of `ENV['X'] || y`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { y } + RUBY + end + end + end + + context 'and followed by a method with its receiver' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || y.foo + ^^^^^^^^ Use `ENV.fetch('X') { y.foo }` instead of `ENV['X'] || y.foo`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { y.foo } + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || y.foo + ^^^^^^^^ Use `ENV.fetch('X') { y.foo }` instead of `ENV['X'] || y.foo`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { y.foo } + RUBY + end + end + end + + context 'and followed by another `ENV[]`' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || ENV['Y'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { ENV.fetch('Y', nil) } + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + a || ENV['X'] || ENV['Y'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + RUBY + + expect_correction(<<~RUBY) + a || ENV.fetch('X') { ENV.fetch('Y', nil) } + RUBY + end + end + end + + context 'and followed by `return`' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || return + ^^^^^^^^ Use `ENV.fetch('X') { return }` instead of `ENV['X'] || return`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { return } + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + a || ENV['X'] || return + ^^^^^^^^ Use `ENV.fetch('X') { return }` instead of `ENV['X'] || return`. + RUBY + + expect_correction(<<~RUBY) + a || ENV.fetch('X') { return } + RUBY + end + end + end + + context 'and followed by a block control' do + context 'on the right next' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || next + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X', nil) || next + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || next + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X', nil) || next + RUBY + end + end + end + + context 'beyond something' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || y || next + ^^^^^^^^ Use `ENV.fetch('X') { y }` instead of `ENV['X'] || y`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { y } || next + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || y || next + ^^^^^^^^ Use `ENV.fetch('X') { y }` instead of `ENV['X'] || y`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { y } || next + RUBY + end + end + end + end + + context 'and followed by multiple `ENV[]`s' do + context 'when the `ENV`s are separated by non-ENV nodes' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || y || ENV['Z'] || a || ENV['B'] + ^^^^^^^^ Use `ENV.fetch('B')` or `ENV.fetch('B', nil)` instead of `ENV['B']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { y } || ENV.fetch('Z') { a } || ENV.fetch('B', nil) + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || y || ENV['Z'] || a || ENV['B'] + ^^^^^^^^ Use `ENV.fetch('B')` or `ENV.fetch('B', nil)` instead of `ENV['B']`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { y } || ENV.fetch('Z') { a } || ENV.fetch('B', nil) + RUBY + end + end + end + + context 'when two `ENV[]`s are next to each other' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || ENV['Y'] || a + ^^^^^^^^ Use `ENV.fetch('Y') { a }` instead of `ENV['Y'] || a`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { ENV.fetch('Y') { a } } + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || ENV['Y'] || a + ^^^^^^^^ Use `ENV.fetch('Y') { a }` instead of `ENV['Y'] || a`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { ENV.fetch('Y') { a } } + RUBY + end + end + end + end + + context 'and followed by a mutiline expression' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || y.map do |a| + ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` + a * 2 + end + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') do + y.map do |a| + a * 2 + end + end + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || y.map do |a| + ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` + a * 2 + end + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') do + y.map do |a| + a * 2 + end + end + RUBY + end + end + end + end end From fa5557e90f26531d12c139c73d08cd37951a7de3 Mon Sep 17 00:00:00 2001 From: Miyake J Takuma Date: Fri, 29 Apr 2022 16:24:10 +0900 Subject: [PATCH 2/6] Fix the title in change.md --- changelog/change_enhance_the_autocorrect_for_fetch_env_var.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md b/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md index 7f4da693d06..711d12cfcd4 100644 --- a/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md +++ b/changelog/change_enhance_the_autocorrect_for_fetch_env_var.md @@ -1 +1 @@ -* [#10585](https://github.com/rubocop/rubocop/pull/10585): Enhance the autocorrect for `RSpec/FetchEnvVar`. ([@johnny-miyake][]) +* [#10585](https://github.com/rubocop/rubocop/pull/10585): Enhance the autocorrect for `Style/FetchEnvVar`. ([@johnny-miyake][]) From 37979149a76054a4879fc8121f74ab92c33bc1a3 Mon Sep 17 00:00:00 2001 From: Miyake J Takuma Date: Fri, 29 Apr 2022 16:39:23 +0900 Subject: [PATCH 3/6] Remove `BasicLiteralRhs` option --- config/default.yml | 3 - lib/rubocop/cop/style/fetch_env_var.rb | 21 +++--- spec/rubocop/cop/style/fetch_env_var_spec.rb | 70 +++++--------------- 3 files changed, 24 insertions(+), 70 deletions(-) diff --git a/config/default.yml b/config/default.yml index 014af1fb845..761593f25c9 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3538,9 +3538,6 @@ Style/FetchEnvVar: VersionAdded: '1.28' # Environment variables to be excluded from the inspection. AllowedVars: [] - # - `SecondArgOfFetch` puts the RHS to the second argument of `ENV.fetch`. (default) - # - `BlockContent` puts the RHS into a block. - BasicLiteralRhs: 'SecondArgOfFetch' Style/FileRead: Description: 'Favor `File.(bin)read` convenience methods.' diff --git a/lib/rubocop/cop/style/fetch_env_var.rb b/lib/rubocop/cop/style/fetch_env_var.rb index 774e52e5a52..23f6cec8fe1 100644 --- a/lib/rubocop/cop/style/fetch_env_var.rb +++ b/lib/rubocop/cop/style/fetch_env_var.rb @@ -11,11 +11,6 @@ module Style # # When an `ENV[]` is the LHS of `||`, the autocorrect makes the RHS # the default value of `ENV.fetch`. - # In addition, when the RHS is a basic literal object (e.g., `'string'`, `true`, `1`), - # the autocorrect corrects the code according to the `BasicLiteralRhs` option. - # Available values - # - `SecondArgOfFetch` puts the RHS to the second argument of `ENV.fetch`. (default) - # - `BlockContent` puts the RHS into a block. # # @example # # bad @@ -49,9 +44,9 @@ class FetchEnvVar < Base # rubocop:disable Layout/LineLength MSG_DEFAULT_NIL = 'Use `ENV.fetch(%s)` or `ENV.fetch(%s, nil)` instead of `ENV[%s]`.' - MSG_DEFAULT_RHS_SINGLE_ARG_STYLE = 'Use `ENV.fetch(%s, %s)` instead of `ENV[%s] || %s`.' - MSG_DEFAULT_RHS_SINGLE_BLOCK_STYLE = 'Use `ENV.fetch(%s) { %s }` instead of `ENV[%s] || %s`.' - MSG_DEFAULT_RHS_MULTI = 'Use `ENV.fetch(%s)` with a block containing `%s ...`' + MSG_DEFAULT_RHS_SECOND_ARG_OF_FETCH = 'Use `ENV.fetch(%s, %s)` instead of `ENV[%s] || %s`.' + MSG_DEFAULT_RHS_SINGLE_LINE_BLOCK = 'Use `ENV.fetch(%s) { %s }` instead of `ENV[%s] || %s`.' + MSG_DEFAULT_RHS_MULTILINE_BLOCK = 'Use `ENV.fetch(%s)` with a block containing `%s ...`' # rubocop:enable Layout/LineLength # @!method env_with_bracket?(node) @@ -182,7 +177,7 @@ def new_code_default_nil(expression) def new_code_default_rhs_single_line(node, expression) parent = node.parent - if cop_config['BasicLiteralRhs'] == 'SecondArgOfFetch' && parent.rhs.basic_literal? + if parent.rhs.basic_literal? "ENV.fetch(#{expression.source}, #{parent.rhs.source})" else "ENV.fetch(#{expression.source}) { #{parent.rhs.source} }" @@ -259,11 +254,11 @@ def default_rhs_in_outer_or(node, expression) def message_template_for(rhs) if rhs.multiline? - MSG_DEFAULT_RHS_MULTI - elsif cop_config['BasicLiteralRhs'] == 'SecondArgOfFetch' && rhs.basic_literal? - MSG_DEFAULT_RHS_SINGLE_ARG_STYLE + MSG_DEFAULT_RHS_MULTILINE_BLOCK + elsif rhs.basic_literal? + MSG_DEFAULT_RHS_SECOND_ARG_OF_FETCH else - MSG_DEFAULT_RHS_SINGLE_BLOCK_STYLE + MSG_DEFAULT_RHS_SINGLE_LINE_BLOCK end end diff --git a/spec/rubocop/cop/style/fetch_env_var_spec.rb b/spec/rubocop/cop/style/fetch_env_var_spec.rb index d07cfb6f7fe..96f595f7d92 100644 --- a/spec/rubocop/cop/style/fetch_env_var_spec.rb +++ b/spec/rubocop/cop/style/fetch_env_var_spec.rb @@ -321,66 +321,28 @@ context 'and followed by a basic literal' do context 'when the node is the left end of `||` chains' do - context 'with BasicLiteralSecondArg style' do - let(:cop_config) { { 'BasicLiteralRhs' => 'SecondArgOfFetch' } } - - it 'registers an offense' do - expect_offense(<<~RUBY) - ENV['X'] || 'y' - ^^^^^^^^ Use `ENV.fetch('X', 'y')` instead of `ENV['X'] || 'y'`. - RUBY - - expect_correction(<<~RUBY) - ENV.fetch('X', 'y') - RUBY - end - end - - context 'with AlwaysUseBlock style' do - let(:cop_config) { { 'BasicLiteralRhs' => 'BlockContent' } } - - it 'registers an offense' do - expect_offense(<<~RUBY) - ENV['X'] || 'y' - ^^^^^^^^ Use `ENV.fetch('X') { 'y' }` instead of `ENV['X'] || 'y'`. - RUBY + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || 'y' + ^^^^^^^^ Use `ENV.fetch('X', 'y')` instead of `ENV['X'] || 'y'`. + RUBY - expect_correction(<<~RUBY) - ENV.fetch('X') { 'y' } - RUBY - end + expect_correction(<<~RUBY) + ENV.fetch('X', 'y') + RUBY end end context 'when the node is between `||`s' do - context 'with BasicLiteralSecondArg style' do - let(:cop_config) { { 'BasicLiteralRhs' => 'SecondArgOfFetch' } } - - it 'registers an offense' do - expect_offense(<<~RUBY) - z || ENV['X'] || 'y' - ^^^^^^^^ Use `ENV.fetch('X', 'y')` instead of `ENV['X'] || 'y'`. - RUBY - - expect_correction(<<~RUBY) - z || ENV.fetch('X', 'y') - RUBY - end - end - - context 'with AlwaysUseBlock style' do - let(:cop_config) { { 'BasicLiteralRhs' => 'BlockContent' } } - - it 'registers an offense' do - expect_offense(<<~RUBY) - z || ENV['X'] || 'y' - ^^^^^^^^ Use `ENV.fetch('X') { 'y' }` instead of `ENV['X'] || 'y'`. - RUBY + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || 'y' + ^^^^^^^^ Use `ENV.fetch('X', 'y')` instead of `ENV['X'] || 'y'`. + RUBY - expect_correction(<<~RUBY) - z || ENV.fetch('X') { 'y' } - RUBY - end + expect_correction(<<~RUBY) + z || ENV.fetch('X', 'y') + RUBY end end end From 214f6f0b5e5cafa148aa4426e5ecba2a9d504ab9 Mon Sep 17 00:00:00 2001 From: Miyake J Takuma Date: Fri, 29 Apr 2022 21:26:17 +0900 Subject: [PATCH 4/6] Fix multiline indentation on autocorrect --- lib/rubocop/cop/style/fetch_env_var.rb | 11 +++--- spec/rubocop/cop/style/fetch_env_var_spec.rb | 36 +++++++++++++------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/lib/rubocop/cop/style/fetch_env_var.rb b/lib/rubocop/cop/style/fetch_env_var.rb index 23f6cec8fe1..a08ca4c85f6 100644 --- a/lib/rubocop/cop/style/fetch_env_var.rb +++ b/lib/rubocop/cop/style/fetch_env_var.rb @@ -185,13 +185,14 @@ def new_code_default_rhs_single_line(node, expression) end def new_code_default_rhs_multiline(node, expression) + env_indent = indent(node.parent) default = node.parent.rhs.source.split("\n").map do |line| - "#{indent(node.parent, offset: indentation_width)}#{line}" + "#{env_indent}#{line}" end.join("\n") <<~NEW_CODE.chomp ENV.fetch(#{expression.source}) do - #{default} - end + #{configured_indentation}#{default} + #{env_indent}end NEW_CODE end @@ -262,8 +263,8 @@ def message_template_for(rhs) end end - def indentation_width - config.for_cop('Layout/IndentationWidth')['Width'] || 2 + def configured_indentation + ' ' * (config.for_cop('Layout/IndentationWidth')['Width'] || 2) end def first_line_of(source) diff --git a/spec/rubocop/cop/style/fetch_env_var_spec.rb b/spec/rubocop/cop/style/fetch_env_var_spec.rb index 96f595f7d92..5511a37b5d7 100644 --- a/spec/rubocop/cop/style/fetch_env_var_spec.rb +++ b/spec/rubocop/cop/style/fetch_env_var_spec.rb @@ -607,16 +607,24 @@ context 'when the node is the left end of `||` chains' do it 'registers an offense' do expect_offense(<<~RUBY) - ENV['X'] || y.map do |a| - ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` - a * 2 + def foo + ENV['X'] || y.map do |a| + ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` + 3.times do |i| + i * 2 + end + end end RUBY expect_correction(<<~RUBY) - ENV.fetch('X') do - y.map do |a| - a * 2 + def foo + ENV.fetch('X') do + y.map do |a| + 3.times do |i| + i * 2 + end + end end end RUBY @@ -626,16 +634,20 @@ context 'when the node is between `||`s' do it 'registers an offense' do expect_offense(<<~RUBY) - z || ENV['X'] || y.map do |a| - ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` - a * 2 + def foo + z || ENV['X'] || y.map do |a| + ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` + a * 2 + end end RUBY expect_correction(<<~RUBY) - z || ENV.fetch('X') do - y.map do |a| - a * 2 + def foo + z || ENV.fetch('X') do + y.map do |a| + a * 2 + end end end RUBY From 8f940177fdf85702cccb06db25c84388bebeaa79 Mon Sep 17 00:00:00 2001 From: Miyake J Takuma Date: Fri, 29 Apr 2022 23:58:46 +0900 Subject: [PATCH 5/6] Look ahead the descendants of `||` operands --- lib/rubocop/cop/style/fetch_env_var.rb | 81 ++++++++++++++------ spec/rubocop/cop/style/fetch_env_var_spec.rb | 63 ++++++++++++++- 2 files changed, 117 insertions(+), 27 deletions(-) diff --git a/lib/rubocop/cop/style/fetch_env_var.rb b/lib/rubocop/cop/style/fetch_env_var.rb index a08ca4c85f6..e821161d318 100644 --- a/lib/rubocop/cop/style/fetch_env_var.rb +++ b/lib/rubocop/cop/style/fetch_env_var.rb @@ -64,18 +64,23 @@ class FetchEnvVar < Base ({next | break | retry | redo}) PATTERN + # @!method env_with_bracket_in_descendants?(node) + def_node_matcher :env_with_bracket_in_descendants?, <<~PATTERN + `(send (const nil? :ENV) :[] $_) + PATTERN + def on_send(node) env_with_bracket?(node) do |expression| - break if allowed_var?(expression) - break if allowable_use?(node) + break unless offensive?(node, expression) if operand_of_or?(node) - target_node, target_expr = rightmost_offense_in_or_chains(node) + target_node = lookahead_target_node_in_or_chains(node) + target_expr = env_with_bracket?(target_node) - if right_end_of_or_chains?(target_node) || rhs_cannot_be_default_value?(target_node) - default_nil(target_node, target_expr) - else + if default_to_rhs?(target_node) default_rhs(target_node, target_expr) + else + default_nil(target_node, target_expr) end else default_nil(node, expression) @@ -95,6 +100,14 @@ def used_as_flag?(node) node.parent.if_type? || (node.parent.send_type? && node.parent.prefix_bang?) end + def offensive?(node, expression) + !(allowed_var?(expression) || allowable_use?(node)) + end + + def default_to_rhs?(node) + operand_of_or?(node) && !right_end_of_or_chains?(node) && rhs_can_be_default_value?(node) + end + # Check if the node is a receiver and receives a message with dot syntax. def message_chained_with_dot?(node) return false if node.root? @@ -140,35 +153,57 @@ def right_end_of_or_chains?(node) # e.g., # `ENV['X'] || y || z || ENV['A'] || b` # ^^^^^^^^ Matches this one - def rightmost_offense_in_or_chains(node) - rmst_node = rmst_expr = nil - or_nodes = [node.parent] + def rightmost_offense_in_or_chains(base_node) + or_nodes = [base_node.parent] while (grand_parent = or_nodes.last&.parent)&.or_type? or_nodes << grand_parent end - # Finds the rightmost `ENV[]` in `||` chains and yields it. - or_nodes.reverse.find do |or_node| - env_with_bracket?(or_node.rhs) do |expression| - rmst_node = or_node.rhs - rmst_expr = expression - end + # Finds the rightmost `ENV[]` in `||` chains. + or_node = or_nodes.reverse.find do |n| + env_with_bracket?(n.rhs) end - return [rmst_node, rmst_expr] if rmst_node - # Yields the node given to this method if no `ENV[]` is found in the above process. - expression = env_with_bracket?(node) - [node, expression] + or_node ? or_node.rhs : base_node + end + + def no_env_with_bracket_in_descendants?(node) + !env_with_bracket_in_descendants?(node) + end + + def conterpart_rhs_of(node) + left_end_of_or_chains?(node) ? node.parent.rhs : node.parent.parent.rhs + end + + # Looks ahead to the `ENV[]` that must be corrected first, avoiding a cross correction. + # ``` + # ENV['X'] || y.map do |a| + # a.map do |b| + # ENV['Z'] + b + # ^^^^^^^^ This must be corrected first. + # end + # end + # ``` + def lookahead_target_node_in_or_chains(base_node) + return base_node unless operand_of_or?(base_node) + + candidate_node = rightmost_offense_in_or_chains(base_node) + return candidate_node if right_end_of_or_chains?(candidate_node) + + counterpart_rhs = conterpart_rhs_of(candidate_node) + return candidate_node if no_env_with_bracket_in_descendants?(counterpart_rhs) + + new_base_node = counterpart_rhs.each_descendant.find { |d| env_with_bracket?(d) } + lookahead_target_node_in_or_chains(new_base_node) end - def rhs_cannot_be_default_value?(node) - rhs_is_block_control?(node) + def rhs_can_be_default_value?(node) + !rhs_is_block_control?(node) end def rhs_is_block_control?(node) - rhs = left_end_of_or_chains?(node) ? node.parent.rhs : node.parent.parent.rhs - block_control?(rhs) + block_control?(conterpart_rhs_of(node)) end def new_code_default_nil(expression) diff --git a/spec/rubocop/cop/style/fetch_env_var_spec.rb b/spec/rubocop/cop/style/fetch_env_var_spec.rb index 5511a37b5d7..ec1292f94ff 100644 --- a/spec/rubocop/cop/style/fetch_env_var_spec.rb +++ b/spec/rubocop/cop/style/fetch_env_var_spec.rb @@ -601,6 +601,34 @@ end end end + + context 'when the right side of `||` is an expression containing another `ENV[]`' do + context 'when the node is the left end of `||` chains' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] || ENV['Y'] + z || a + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X') { ENV.fetch('Y', nil) + z } || a + RUBY + end + end + + context 'when the node is between `||`s' do + it 'registers an offense' do + expect_offense(<<~RUBY) + z || ENV['X'] || ENV['Y'] || a + ^^^^^^^^ Use `ENV.fetch('Y') { a }` instead of `ENV['Y'] || a`. + RUBY + + expect_correction(<<~RUBY) + z || ENV.fetch('X') { ENV.fetch('Y') { a } } + RUBY + end + end + end end context 'and followed by a mutiline expression' do @@ -610,8 +638,35 @@ def foo ENV['X'] || y.map do |a| ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` - 3.times do |i| - i * 2 + a.map do |b| + b * 2 + end + end + end + RUBY + + expect_correction(<<~RUBY) + def foo + ENV.fetch('X') do + y.map do |a| + a.map do |b| + b * 2 + end + end + end + end + RUBY + end + end + + context 'when the block on the right side of `||` contains another `ENV`' do + it 'registers an offense' do + expect_offense(<<~RUBY) + def foo + ENV['X'] || y.map do |a| + a.map do |b| + ENV['Z'] + b + ^^^^^^^^ Use `ENV.fetch('Z')` or `ENV.fetch('Z', nil)` instead of `ENV['Z']`. end end end @@ -621,8 +676,8 @@ def foo def foo ENV.fetch('X') do y.map do |a| - 3.times do |i| - i * 2 + a.map do |b| + ENV.fetch('Z', nil) + b end end end From 7392eda7b17d3030e73df99386f2306ca9719cd0 Mon Sep 17 00:00:00 2001 From: Miyake J Takuma Date: Sat, 30 Apr 2022 02:48:40 +0900 Subject: [PATCH 6/6] Fix false positive when ENV[] inside a block is in allowable use. --- lib/rubocop/cop/style/fetch_env_var.rb | 23 ++++++++----- spec/rubocop/cop/style/fetch_env_var_spec.rb | 35 ++++++++++++++++++++ 2 files changed, 49 insertions(+), 9 deletions(-) diff --git a/lib/rubocop/cop/style/fetch_env_var.rb b/lib/rubocop/cop/style/fetch_env_var.rb index e821161d318..91c0821f274 100644 --- a/lib/rubocop/cop/style/fetch_env_var.rb +++ b/lib/rubocop/cop/style/fetch_env_var.rb @@ -71,10 +71,10 @@ class FetchEnvVar < Base def on_send(node) env_with_bracket?(node) do |expression| - break unless offensive?(node, expression) + break unless offensive?(node) if operand_of_or?(node) - target_node = lookahead_target_node_in_or_chains(node) + target_node = lookahead_target_node(node) target_expr = env_with_bracket?(target_node) if default_to_rhs?(target_node) @@ -90,8 +90,9 @@ def on_send(node) private - def allowed_var?(expression) - expression.str_type? && cop_config['AllowedVars'].include?(expression.value) + def allowed_var?(node) + env_key_node = node.children.last + env_key_node.str_type? && cop_config['AllowedVars'].include?(env_key_node.value) end def used_as_flag?(node) @@ -100,8 +101,8 @@ def used_as_flag?(node) node.parent.if_type? || (node.parent.send_type? && node.parent.prefix_bang?) end - def offensive?(node, expression) - !(allowed_var?(expression) || allowable_use?(node)) + def offensive?(node) + !(allowed_var?(node) || allowable_use?(node)) end def default_to_rhs?(node) @@ -185,7 +186,7 @@ def conterpart_rhs_of(node) # end # end # ``` - def lookahead_target_node_in_or_chains(base_node) + def lookahead_target_node(base_node) return base_node unless operand_of_or?(base_node) candidate_node = rightmost_offense_in_or_chains(base_node) @@ -194,8 +195,12 @@ def lookahead_target_node_in_or_chains(base_node) counterpart_rhs = conterpart_rhs_of(candidate_node) return candidate_node if no_env_with_bracket_in_descendants?(counterpart_rhs) - new_base_node = counterpart_rhs.each_descendant.find { |d| env_with_bracket?(d) } - lookahead_target_node_in_or_chains(new_base_node) + new_base_node = counterpart_rhs.each_descendant.find do |d| + env_with_bracket?(d) && offensive?(d) + end + return candidate_node unless new_base_node + + lookahead_target_node(new_base_node) end def rhs_can_be_default_value?(node) diff --git a/spec/rubocop/cop/style/fetch_env_var_spec.rb b/spec/rubocop/cop/style/fetch_env_var_spec.rb index ec1292f94ff..30703632c85 100644 --- a/spec/rubocop/cop/style/fetch_env_var_spec.rb +++ b/spec/rubocop/cop/style/fetch_env_var_spec.rb @@ -686,6 +686,41 @@ def foo end end + context 'when the block on the right side of `||` contains an `ENV` that is used as a flag' do + it 'registers no offenses' do + expect_offense(<<~RUBY) + def foo + ENV['X'] || y.map do |a| + ^^^^^^^^ Use `ENV.fetch('X')` with a block containing `y.map do |a| ...` + a.map do |b| + if ENV['Z'] + b + else + 1 + end + end + end + end + RUBY + + expect_correction(<<~RUBY) + def foo + ENV.fetch('X') do + y.map do |a| + a.map do |b| + if ENV['Z'] + b + else + 1 + end + end + end + end + end + RUBY + end + end + context 'when the node is between `||`s' do it 'registers an offense' do expect_offense(<<~RUBY)