diff --git a/changelog/new_add_new_fetch_env_var.md b/changelog/new_add_new_fetch_env_var.md new file mode 100644 index 000000000000..0db37f799a3f --- /dev/null +++ b/changelog/new_add_new_fetch_env_var.md @@ -0,0 +1 @@ +* [#10502](https://github.com/rubocop/rubocop/pull/10502): Add new `Style/FetchEnvVar` cop. ([@johnny-miyake][]) diff --git a/config/default.yml b/config/default.yml index 7c737f178b08..d3dddded2e6f 100644 --- a/config/default.yml +++ b/config/default.yml @@ -3526,6 +3526,16 @@ Style/ExponentialNotation: - engineering - integral +Style/FetchEnvVar: + Description: >- + This cop suggests `ENV.fetch` for the replacement of `ENV[]`. + Reference: + - https://rubystyle.guide/#hash-fetch-defaults + Enabled: pending + VersionAdded: '<>' + # Environment variables to be excluded from the inspection. + AllowedVars: [] + Style/FileRead: Description: 'Favor `File.(bin)read` convenience methods.' StyleGuide: '#file-read' diff --git a/lib/rubocop.rb b/lib/rubocop.rb index f9b529f94d8b..cfc61685b7e3 100644 --- a/lib/rubocop.rb +++ b/lib/rubocop.rb @@ -485,6 +485,7 @@ require_relative 'rubocop/cop/style/expand_path_arguments' require_relative 'rubocop/cop/style/explicit_block_argument' require_relative 'rubocop/cop/style/exponential_notation' +require_relative 'rubocop/cop/style/fetch_env_var' require_relative 'rubocop/cop/style/file_read' require_relative 'rubocop/cop/style/file_write' require_relative 'rubocop/cop/style/float_division' diff --git a/lib/rubocop/cli/command/suggest_extensions.rb b/lib/rubocop/cli/command/suggest_extensions.rb index b1b9b0339339..9f9159a0305e 100644 --- a/lib/rubocop/cli/command/suggest_extensions.rb +++ b/lib/rubocop/cli/command/suggest_extensions.rb @@ -42,7 +42,7 @@ def skip? # 1. On CI # 2. When given RuboCop options that it doesn't make sense for # 3. For all formatters except specified in `INCLUDED_FORMATTERS'` - ENV['CI'] || + ENV.fetch('CI', nil) || @options[:only] || @options[:debug] || @options[:list_target_files] || @options[:out] || @options[:stdin] || !INCLUDED_FORMATTERS.include?(current_formatter) diff --git a/lib/rubocop/cop/style/fetch_env_var.rb b/lib/rubocop/cop/style/fetch_env_var.rb new file mode 100644 index 000000000000..85d4d7d3b944 --- /dev/null +++ b/lib/rubocop/cop/style/fetch_env_var.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +module RuboCop + module Cop + module Style + # This cop suggests `ENV.fetch` for the replacement of `ENV[]`. + # `ENV[]` silently fails and returns `nil` when the environment variable is unset, + # which may cause unexpected behaviors when the developer forgets to set it. + # On the other hand, `ENV.fetch` raises KeyError or returns the explicitly + # specified default value. + # + # @example + # # bad + # ENV['X'] + # ENV['X'] || z + # x = ENV['X'] + # + # # good + # ENV.fetch('X') + # ENV.fetch('X', nil) || z + # x = ENV.fetch('X') + # + # # also good + # !ENV['X'] + # ENV['X'].some_method # (e.g. `.nil?`) + # + class FetchEnvVar < Base + extend AutoCorrector + + MSG = 'Use `ENV.fetch(%s)` or `ENV.fetch(%s, nil)` instead of `ENV[%s]`.' + + # @!method env_with_bracket?(node) + def_node_matcher :env_with_bracket?, <<~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) + + add_offense(node, message: format(MSG, key: expression.source)) do |corrector| + corrector.replace(node, "ENV.fetch(#{expression.source}, nil)") + end + end + end + + private + + def allowed_var?(expression) + expression.str_type? && cop_config['AllowedVars'].include?(expression.value) + end + + def used_as_flag?(node) + return false if node.root? + + node.parent.if_type? || (node.parent.send_type? && node.parent.prefix_bang?) + 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? + + node.parent.send_type? && node.parent.children.first == node && node.parent.dot? + end + + # Allow if used as a flag (e.g., `if ENV['X']` or `!ENV['X']`) because + # it simply checks whether the variable is set. + # Also allow if receiving a message with dot syntax, e.g. `ENV['X'].nil?`. + def allowable_use?(node) + used_as_flag?(node) || message_chained_with_dot?(node) + end + end + end + end +end diff --git a/lib/rubocop/result_cache.rb b/lib/rubocop/result_cache.rb index 13410dea239a..398ec302ebd8 100644 --- a/lib/rubocop/result_cache.rb +++ b/lib/rubocop/result_cache.rb @@ -66,12 +66,12 @@ def remove_files(files, dirs, remove_count) end def self.cache_root(config_store) - root = ENV['RUBOCOP_CACHE_ROOT'] + root = ENV.fetch('RUBOCOP_CACHE_ROOT', nil) root ||= config_store.for_pwd.for_all_cops['CacheRootDirectory'] root ||= if ENV.key?('XDG_CACHE_HOME') # Include user ID in the path to make sure the user has write # access. - File.join(ENV['XDG_CACHE_HOME'], Process.uid.to_s) + File.join(ENV.fetch('XDG_CACHE_HOME'), Process.uid.to_s) else # On FreeBSD, the /home path is a symbolic link to /usr/home # and the $HOME environment variable returns the /home path. @@ -81,7 +81,7 @@ def self.cache_root(config_store) # # To avoid raising warn log messages on FreeBSD, we retrieve # the real path of the home folder. - File.join(File.realpath(ENV['HOME']), '.cache') + File.join(File.realpath(ENV.fetch('HOME')), '.cache') end File.join(root, 'rubocop_cache') end diff --git a/lib/rubocop/rspec/shared_contexts.rb b/lib/rubocop/rspec/shared_contexts.rb index 972cef838d01..122e6a3153c8 100644 --- a/lib/rubocop/rspec/shared_contexts.rb +++ b/lib/rubocop/rspec/shared_contexts.rb @@ -5,8 +5,8 @@ RSpec.shared_context 'isolated environment', :isolated_environment do around do |example| Dir.mktmpdir do |tmpdir| - original_home = ENV['HOME'] - original_xdg_config_home = ENV['XDG_CONFIG_HOME'] + original_home = ENV.fetch('HOME', nil) + original_xdg_config_home = ENV.fetch('XDG_CONFIG_HOME', nil) # Make sure to expand all symlinks in the path first. Otherwise we may # get mismatched pathnames when loading config files later on. diff --git a/spec/rubocop/cli_spec.rb b/spec/rubocop/cli_spec.rb index 101413689b1e..1c95b4b56427 100644 --- a/spec/rubocop/cli_spec.rb +++ b/spec/rubocop/cli_spec.rb @@ -1837,8 +1837,8 @@ def find_suggestions # Ensure that these specs works in CI, since the feature is generally # disabled in when ENV['CI'] is set. - allow(ENV).to receive(:[]).and_call_original - allow(ENV).to receive(:[]).with('CI').and_return(false) + allow(ENV).to receive(:fetch).and_call_original + allow(ENV).to receive(:fetch).with('CI', nil).and_return(false) # Mock the lockfile to be parsed by bundler allow(Bundler).to receive(:default_lockfile) @@ -2024,7 +2024,7 @@ def find_suggestions end context 'when in CI mode' do - before { allow(ENV).to receive(:[]).with('CI').and_return(true) } + before { allow(ENV).to receive(:fetch).with('CI', nil).and_return(true) } it 'does not show the suggestion' do expect { cli.run(['example.rb']) }.not_to suggest_extensions diff --git a/spec/rubocop/cop/style/fetch_env_var_spec.rb b/spec/rubocop/cop/style/fetch_env_var_spec.rb new file mode 100644 index 000000000000..e05ca8971dbe --- /dev/null +++ b/spec/rubocop/cop/style/fetch_env_var_spec.rb @@ -0,0 +1,295 @@ +# frozen_string_literal: true + +RSpec.describe RuboCop::Cop::Style::FetchEnvVar, :config do + let(:cop_config) { { 'ExceptedEnvVars' => [] } } + + context 'when it is evaluated with no default values' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X', nil) + RUBY + + expect_offense(<<~RUBY) + ENV['X' + 'Y'] + ^^^^^^^^^^^^^^ Use `ENV.fetch('X' + 'Y')` or `ENV.fetch('X' + 'Y', nil)` instead of `ENV['X' + 'Y']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X' + 'Y', nil) + RUBY + end + end + + context 'with negation' do + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + !ENV['X'] + RUBY + end + end + + context 'when it receives a message' do + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + ENV['X'].some_method + RUBY + end + end + + context 'when it is compared with other object' do + it 'registers an offense' do + expect_offense(<<~RUBY) + ENV['X'] == 1 + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + ENV.fetch('X', nil) == 1 + RUBY + 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 it is an argument of a method' do + it 'registers an offense' do + expect_offense(<<~RUBY) + some_method(ENV['X']) + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + some_method(ENV.fetch('X', nil)) + RUBY + + expect_offense(<<~RUBY) + x.some_method(ENV['X']) + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + x.some_method(ENV.fetch('X', nil)) + RUBY + + expect_offense(<<~RUBY) + some_method( + 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'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + ) + RUBY + + expect_correction(<<~RUBY) + some_method( + ENV['A'].some_method, + ENV.fetch('B', nil) || ENV.fetch('C', nil), + ENV.fetch('X', nil), + ENV.fetch('Y', nil) + ) + RUBY + end + end + + context 'when it is assigned to a variable' do + it 'registers an offense when using single assignment' do + expect_offense(<<~RUBY) + x = ENV['X'] + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + x = ENV.fetch('X', nil) + RUBY + end + + it 'registers an offense when using multiple assignment' do + expect_offense(<<~RUBY) + x, y = ENV['X'], + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + ENV['Y'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + RUBY + + expect_correction(<<~RUBY) + x, y = ENV.fetch('X', nil), + ENV.fetch('Y', nil) + RUBY + end + end + + context 'when it is an array element' do + it 'registers an offense' do + expect_offense(<<~RUBY) + [ + ENV['X'], + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + ENV['Y'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + ] + RUBY + + expect_correction(<<~RUBY) + [ + ENV.fetch('X', nil), + ENV.fetch('Y', nil) + ] + RUBY + end + end + + context 'when it is a hash key' do + it 'registers an offense' do + expect_offense(<<~RUBY) + { + ENV['X'] => :x, + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + ENV['Y'] => :y + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + } + RUBY + + expect_correction(<<~RUBY) + { + ENV.fetch('X', nil) => :x, + ENV.fetch('Y', nil) => :y + } + RUBY + end + end + + context 'when it is a hash value' do + it 'registers an offense' do + expect_offense(<<~RUBY) + { + x: ENV['X'], + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + y: ENV['Y'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + } + RUBY + + expect_correction(<<~RUBY) + { + x: ENV.fetch('X', nil), + y: ENV.fetch('Y', nil) + } + RUBY + end + end + + context 'when it is used in an interpolation' do + it 'registers an offense' do + expect_offense(<<~RUBY) + "\#{ENV['X']}" + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + RUBY + + expect_correction(<<~RUBY) + "\#{ENV.fetch('X', nil)}" + RUBY + end + end + + context 'when using `fetch` instead of `[]`' do + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + ENV.fetch('X') + RUBY + + expect_no_offenses(<<~RUBY) + ENV.fetch('X', default_value) + RUBY + end + end + + context 'when it is used in a conditional expression' do + it 'registers no offenses with `if`' do + expect_no_offenses(<<~RUBY) + if ENV['X'] + puts x + end + RUBY + end + + it 'registers no offenses with `unless`' do + expect_no_offenses(<<~RUBY) + unless ENV['X'] + puts x + end + RUBY + end + + it 'registers no offenses with ternary operator' do + expect_no_offenses(<<~RUBY) + ENV['X'] ? x : y + RUBY + end + + it 'registers an offense with `case`' do + expect_offense(<<~RUBY) + case ENV['X'] + ^^^^^^^^ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`. + when ENV['Y'] + ^^^^^^^^ Use `ENV.fetch('Y')` or `ENV.fetch('Y', nil)` instead of `ENV['Y']`. + puts x + end + RUBY + + expect_correction(<<~RUBY) + case ENV.fetch('X', nil) + when ENV.fetch('Y', nil) + puts x + end + RUBY + end + end + + context 'when the env val is excluded from the inspection by the config' do + let(:cop_config) { { 'AllowedVars' => ['X'] } } + + it 'registers no offenses' do + expect_no_offenses(<<~RUBY) + ENV['X'] + RUBY + end + end +end diff --git a/spec/rubocop/result_cache_spec.rb b/spec/rubocop/result_cache_spec.rb index 5d71a97b40ac..73317a1212b8 100644 --- a/spec/rubocop/result_cache_spec.rb +++ b/spec/rubocop/result_cache_spec.rb @@ -376,7 +376,7 @@ def abs(path) it 'contains the given path and UID' do cacheroot = described_class.cache_root(config_store) - expect(cacheroot).to eq(File.join(ENV['XDG_CACHE_HOME'], puid, 'rubocop_cache')) + expect(cacheroot).to eq(File.join(ENV.fetch('XDG_CACHE_HOME'), puid, 'rubocop_cache')) end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 974822a88662..37e55257f834 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -70,11 +70,11 @@ config.after(:suite) { RuboCop::Cop::Registry.reset! } - if %w[ruby-head-ascii_spec ruby-head-spec].include? ENV['CIRCLE_STAGE'] + if %w[ruby-head-ascii_spec ruby-head-spec].include? ENV.fetch('CIRCLE_STAGE', nil) config.filter_run_excluding broken_on: :ruby_head end - if %w[jruby-9.3-ascii_spec jruby-9.3-spec].include? ENV['CIRCLE_STAGE'] + if %w[jruby-9.3-ascii_spec jruby-9.3-spec].include? ENV.fetch('CIRCLE_STAGE', nil) config.filter_run_excluding broken_on: :jruby end end