Skip to content

Commit

Permalink
[Fix rubocop#10788] Relax Style/FetchEnvVar to allow ENV[] in LHS…
Browse files Browse the repository at this point in the history 
… of `||`

Fixes rubocop#10788

This PR relaxes `Style/FetchEnvVar` cop to allow `ENV[]` in LHS of `||`.
`ENV[]` in LHS of `||` operator can be allowed by this cop because
the `||` is put by the developer who knows the LHS may be `nil`.
  • Loading branch information
@j-miyake
j-miyake committed Jul 18, 2022
1 parent 8fa156c commit 5610065
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 610 deletions.
1 change: 1 addition & 0 deletions changelog/fix_relax_stylefetchenvvar_to_allow_env_in.md
View file
@@ -0,0 +1 @@
* [#10788](https://github.com/rubocop/rubocop/issues/10788): Relax `Style/FetchEnvVar` to allow `ENV[]` in LHS of `||`. ([@j-miyake][])
187 changes: 10 additions & 177 deletions lib/rubocop/cop/style/fetch_env_var.rb
View file
Expand Up @@ -9,81 +9,36 @@ 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`.
#
# @example
# # bad
# ENV['X']
# 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', '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?`)
#
class FetchEnvVar < Base
extend AutoCorrector

# rubocop:disable Layout/LineLength
MSG_DEFAULT_NIL = 'Use `ENV.fetch(%<key>s)` or `ENV.fetch(%<key>s, nil)` instead of `ENV[%<key>s]`.'
MSG_DEFAULT_RHS_SECOND_ARG_OF_FETCH = 'Use `ENV.fetch(%<key>s, %<default>s)` instead of `ENV[%<key>s] || %<default>s`.'
MSG_DEFAULT_RHS_SINGLE_LINE_BLOCK = 'Use `ENV.fetch(%<key>s) { %<default>s }` instead of `ENV[%<key>s] || %<default>s`.'
MSG_DEFAULT_RHS_MULTILINE_BLOCK = 'Use `ENV.fetch(%<key>s)` with a block containing `%<default>s ...`'
# rubocop:enable Layout/LineLength
MSG = 'Use `ENV.fetch(%<key>s)` or `ENV.fetch(%<key>s, nil)` instead of `ENV[%<key>s]`.'

# @!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

# @!method offensive_nodes(node)
def_node_search :offensive_nodes, <<~PATTERN
[#env_with_bracket? #offensive?]
PATTERN

def on_send(node)
env_with_bracket?(node) do |name_node|
break unless offensive?(node)

if operand_of_or?(node)
target_node = offensive_nodes(or_chain_root(node)).to_a.last
target_name_node = env_with_bracket?(target_node)

if default_to_rhs?(target_node)
default_rhs(target_node, target_name_node)
else
default_nil(target_node, target_name_node)
end
else
default_nil(node, name_node)
message = format(MSG, key: name_node.source)
add_offense(node, message: message) do |corrector|
corrector.replace(node, new_code(name_node))
end
end
end
Expand Down Expand Up @@ -130,17 +85,6 @@ def offensive?(node)
!(allowed_var?(node) || allowable_use?(node))
end

def or_chain_root(node)
while operand_of_or?(ancestor_or ||= node.parent)
ancestor_or = ancestor_or.parent
end
ancestor_or
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?
Expand All @@ -157,8 +101,9 @@ def message_chained_with_dot?(node)
# it simply checks whether the variable is set.
# - Receiving a message with dot syntax, e.g. `ENV['X'].nil?`.
# - `ENV['key']` assigned by logical AND/OR assignment.
# - `ENV['key']` is the LHS of a `||`.
def allowable_use?(node)
used_as_flag?(node) || message_chained_with_dot?(node) || assigned?(node)
used_as_flag?(node) || message_chained_with_dot?(node) || assigned?(node) || or_lhs?(node)
end

# The following are allowed cases:
Expand All @@ -172,127 +117,15 @@ def assigned?(node)
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

def conterpart_rhs_of(node)
left_end_of_or_chains?(node) ? node.parent.rhs : node.parent.parent.rhs
end

def rhs_can_be_default_value?(node)
!rhs_is_block_control?(node)
end
def or_lhs?(node)
return false unless (parent = node.parent)&.or_type?

def rhs_is_block_control?(node)
block_control?(conterpart_rhs_of(node))
parent.lhs == node || parent.parent&.or_type?
end

def new_code_default_nil(name_node)
def new_code(name_node)
"ENV.fetch(#{name_node.source}, nil)"
end

def new_code_default_rhs_single_line(node, name_node)
parent = node.parent
if parent.rhs.basic_literal?
"ENV.fetch(#{name_node.source}, #{parent.rhs.source})"
else
"ENV.fetch(#{name_node.source}) { #{parent.rhs.source} }"
end
end

def new_code_default_rhs_multiline(node, name_node)
env_indent = indent(node.parent)
default = node.parent.rhs.source.split("\n").map do |line|
"#{env_indent}#{line}"
end.join("\n")
<<~NEW_CODE.chomp
ENV.fetch(#{name_node.source}) do
#{configured_indentation}#{default}
#{env_indent}end
NEW_CODE
end

def new_code_default_rhs(node, name_node)
if node.parent.rhs.single_line?
new_code_default_rhs_single_line(node, name_node)
else
new_code_default_rhs_multiline(node, name_node)
end
end

def default_rhs(node, name_node)
if left_end_of_or_chains?(node)
default_rhs_in_same_or(node, name_node)
else
default_rhs_in_outer_or(node, name_node)
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, name_node)
message = format(MSG_DEFAULT_NIL, key: name_node.source)

add_offense(node, message: message) do |corrector|
corrector.replace(node, new_code_default_nil(name_node))
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, name_node)
template = message_template_for(node.parent.rhs)
message = format(template,
key: name_node.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, name_node))
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, name_node)
parent = node.parent
grand_parent = parent.parent

template = message_template_for(grand_parent.rhs)
message = format(template,
key: name_node.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, name_node)
corrector.replace(grand_parent, "#{lhs_code} || #{rhs_code}")
end
end

def message_template_for(rhs)
if rhs.multiline?
MSG_DEFAULT_RHS_MULTILINE_BLOCK
elsif rhs.basic_literal?
MSG_DEFAULT_RHS_SECOND_ARG_OF_FETCH
else
MSG_DEFAULT_RHS_SINGLE_LINE_BLOCK
end
end

def configured_indentation
' ' * (config.for_cop('Layout/IndentationWidth')['Width'] || 2)
end

def first_line_of(source)
source.split("\n").first
end
end
end
end
Expand Down

0 comments on commit 5610065

Please sign in to comment.