Add new `Style/FetchEnvVar` cop #10502

j-miyake · 2022-04-04T11:01:13Z

Reading environment variables using brackets like ENV['SOME_KEY'] may unexpectedly be evaluated as nil if the variable is unset.

~~Because ENV behaves like a Hash, I think the style guide for Hash can be applied to ENV as well.~~
We can use ENV.fetch like Hash#fetch.
https://rubystyle.guide/#hash-fetch-defaults

This cop suggests ENV.fetch for the replacement of ENV[].

(Description updated)
Edited a confusing description where referencing to a style guide Hash#fetch.

Before submitting the PR make sure the following are checked:

The PR relates to only one subject with a clear title and description in grammatically correct, complete sentences.
Wrote good commit messages.
Commit message starts with [Fix #issue-number] (if the related issue exists).
Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Ran bundle exec rake default. It executes all tests and runs RuboCop on its own code.
Added an entry (file) to the changelog folder named {change_type}_{change_description}.md if the new code introduces user-observable changes. See changelog entry format for details.

…self

j-miyake · 2022-04-07T12:00:01Z

Seems this cop does not attract anyone; I’m closing this PR :)

bbatsov · 2022-04-07T12:08:36Z

We've got a pretty big backlog, so we're moving relatively slowly through all PRs. Lack of feedback, doesn't mean we're not interested in some proposal. I hope to get to review this over the weekend.

bbatsov · 2022-04-08T06:02:58Z

I just reviewed the cop and I like the idea overall. It seems, however, there's no cop for checking the same thing for hashes (likely because it's hard to infer what's a hash or not). I've been wondering if it makes sense to try to make a more generic cop that checks hashes and ENV (maybe with some configuration options to decide what exactly you want to check) or just limit this to what's currently being proposed. The current cop definitely has the advantage of being free from false positives which is nice. @rubocop/rubocop-core Any opinions?

j-miyake · 2022-04-08T11:48:17Z

@bbatsov Thank you for reviewing the code. (and sorry for my impatiently closing the PR)
As you said, in a static analysis, it's hard to know whether an object is a hash except in the case it is a hash literal. Plus, developers can implement their [] and []= methods; we could need to care about not having false positives for that case if we want to make a more generic cop.

bquorning · 2022-04-08T14:21:56Z

In my opinion, guarding against ENV[] is much more useful than guarding against Hash#[].

bbatsov · 2022-04-09T05:33:43Z

Okay, let's limit the scope just to env variables. One thing I'd like us to do is to come up with a shorter name for the cop, though - the currently suggest name is descriptive, but it's super long. Perhaps something like FetchEnvVarDefaults or EnvVarDefaults? I'm open to better suggestions, of course - naming is hard. :-)

bbatsov · 2022-04-09T05:34:36Z

.rubocop.yml

@@ -42,6 +42,15 @@ Style/IpAddresses:
  Exclude:
    - spec/rubocop/cop/style/ip_addresses_spec.rb

+Style/ReadingEnvVarWithoutDefaultValue:
+  ExcludedEnvVars:


I'd name this AllowedEnvVars or just AllowedVars.

bbatsov · 2022-04-09T05:38:09Z

https://rubystyle.guide/#hash-fetch-defaults

You can also add this to the cop metadata as a reference.

j-miyake · 2022-04-09T10:26:48Z

How about FetchEnvVar? There are cases where fetch without the default value fits more (e.g., raising an error against an unset environment variable as a circuit breaker), so I think we can suggest using fetch while probably should leave the developers to decide to use the default value.

For the same reason, I'm wondering if the error message below was better, while the auto-correction probably should give nil for the default value not to break the software's original behavior.

- Use `ENV.fetch('X', nil)` instead of `ENV['X']`.
+ Use `ENV.fetch('X')` or `ENV.fetch('X', nil)` instead of `ENV['X']`.

What do you think?

bbatsov · 2022-04-10T05:55:02Z

I like the proposed changes to the cop name and its message.

while the auto-correction probably should give nil for the default value not to break the software's original behavior.

Yeah, exactly.

j-miyake · 2022-04-10T08:09:37Z

Thanks, I updated this PR.

bquorning · 2022-04-10T15:56:08Z

.rubocop.yml

+    - XDG_CACHE_HOME
+    - XDG_CONFIG_HOME
+    - CIRCLE_STAGE
+    - CI


How were these 6 variable names chosen as exceptions to the new rule?

Thanks for pointing it out. They are the environment variables used in this project. I listed them here to get opinions on how these should be. I think we can replace almost all ENV[...] in this project with ENV.fetch(..., nil), except that here probably should be replaced with ENV.fetch(...) without the default values. Do you have any opinions?

I added a commit to show how the code will be.
1901117

bbatsov · 2022-04-10T15:56:32Z

lib/rubocop/cop/style/fetch_env_var.rb

+
+        # rubocop:disable Metrics/CyclomaticComplexity
+        def offensive?(node)
+          only_node_of_expression?(node) ||


I think it'd be a good idea to add some explanatory comment here, as it's not very clear why all those checks are needed.

Great point. I will write some comment for that.

BTW, this cop ignore some ENV[] in the below cases;

when an ENV[] is no more than a flag for a condition

when an ENV[] receives some message (This may be controversial though)

This feature is documented in the also good section of the documentation.
I’m going to mention about above as well, but would like to hear if this idea sounds good for others.

I added some comments and made the logic simpler.
17a52ef

bbatsov · 2022-04-12T20:25:43Z

Looks good. Just address the conflict in result_cache.rb and we're good to merge.

j-miyake · 2022-04-13T01:16:11Z

I resolved the conflict and fixed a method so that it can cover another case.
1270cc3

bbatsov · 2022-04-13T05:17:39Z

Thanks!

benpickles · 2022-04-22T13:16:31Z

The thing about ENV is that its values (and keys) will always be a (frozen) string so the Ruby Style Guide Hash#fetch recommendation isn't applicable because the false value case cannot occur. This means that ENV['foo'] and ENV.fetch('foo', nil) have the same behaviour and don't seem to protect against any meaningful issue (correct me if I'm wrong).

j-miyake · 2022-04-22T14:41:17Z

As you pointed out, the Style Guide Hash#fetch is to distinguish nil and false, so it's not applicable to ENV. The description of this PR may have been confusing because the actual purpose of this cop is different from it.

The purpose of this cop is to alert the developers that the code to read an environment variable is possibly nil on runtime. If the developer assumes that the environment variable is absolutely set, using ENV.fetch would be a better choice than ENV[] because fetch raises an error when the variable is not set and stops the execution immediately not expanding the incident. If they want to intentionally allow the case that the variable is nil, specifying the default value nil can help others know the developer's intention.
(This idea is based on my experience, but I know it depends on the situation or how the developers think.)

rmacklin · 2022-04-22T22:29:29Z

Hey there, wanted to point out something I noticed when running this cop's autocorrection.

Code like

value = ENV['EXAMPLE_KEY'] || 'default_value'

gets autocorrected to

value = ENV.fetch('EXAMPLE_KEY', nil) || 'default_value'

I ended up manually changing that to

value = ENV.fetch('EXAMPLE_KEY', 'default_value')

in my codebase. It'd be neat if the autocorrection could handle that pattern.

ShockwaveNN · 2022-04-24T08:53:44Z

@rmacklin I agree with you, I've replaced a lot of a = ENV['a'] || 'default' in my code base and I really like a = ENV.fetch('a', 'default') - it's look nicer and tider

j-miyake · 2022-04-24T16:18:45Z

That's true. I opened an issue for that enhancement.
#10578

Because `.rubocop.yml` is set to `NewCops: enable`, these newly added cops detect offenses when Rake Task is running. * rubocop/rubocop#10243 * rubocop/rubocop#10502

j-miyake added 4 commits April 4, 2022 19:56

Add new Style/ReadingEnvVarWithoutDefaultValue cop

a2f654c

add changelog

bc3ba3c

Move ExcludedEnvVars configuration for the inspection of rubocop it…

0cef3dd

…self

Add an offensive pattern

333ccc6

j-miyake marked this pull request as ready for review April 4, 2022 12:09

Refactoring and enforcing for the cases of using ||

c36ac01

j-miyake closed this Apr 7, 2022

bbatsov reopened this Apr 7, 2022

bbatsov reviewed Apr 9, 2022

View reviewed changes

j-miyake added 2 commits April 9, 2022 20:21

Rename an option

ba33155

Add a reference

51f19af

j-miyake added 2 commits April 10, 2022 16:16

Modify the error message format and the documentation

59648aa

Rename cop and its method

beabdf7

j-miyake changed the title ~~Add new Style/ReadingEnvVarWithoutDefaultValue cop~~ Add new Style/FetchEnvVar cop Apr 10, 2022

j-miyake changed the title ~~Add new Style/FetchEnvVar cop~~ Add new Style/FetchEnvVar cop Apr 10, 2022

bquorning reviewed Apr 10, 2022

View reviewed changes

bbatsov reviewed Apr 10, 2022

View reviewed changes

j-miyake added 2 commits April 11, 2022 20:46

Replace ENV[] by ENV.fetch in rubocop project

1901117

Simplify the logic, and add some comments

17a52ef

j-miyake force-pushed the reading_env_with_no_default_val branch from 2b6e80a to 17a52ef Compare April 12, 2022 01:07

j-miyake added 2 commits April 13, 2022 09:02

Merge branch 'master' into reading_env_with_no_default_val

0864c1a

Fix the message chaining detection

1270cc3

bbatsov merged commit 5564dd1 into rubocop:master Apr 13, 2022

j-miyake deleted the reading_env_with_no_default_val branch April 13, 2022 05:32

j-miyake mentioned this pull request Apr 24, 2022

Cop idea: Set the default value of ENV.fetch by autocorrect #10578

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new `Style/FetchEnvVar` cop #10502

Add new `Style/FetchEnvVar` cop #10502

j-miyake commented Apr 4, 2022 •

edited

j-miyake commented Apr 7, 2022

bbatsov commented Apr 7, 2022

bbatsov commented Apr 8, 2022

j-miyake commented Apr 8, 2022

bquorning commented Apr 8, 2022

bbatsov commented Apr 9, 2022

bbatsov Apr 9, 2022

bbatsov commented Apr 9, 2022

j-miyake commented Apr 9, 2022 •

edited

bbatsov commented Apr 10, 2022

j-miyake commented Apr 10, 2022

bquorning Apr 10, 2022

j-miyake Apr 11, 2022 •

edited

j-miyake Apr 11, 2022

bbatsov Apr 10, 2022

j-miyake Apr 11, 2022

j-miyake Apr 12, 2022

bbatsov commented Apr 12, 2022

j-miyake commented Apr 13, 2022

bbatsov commented Apr 13, 2022

benpickles commented Apr 22, 2022

j-miyake commented Apr 22, 2022

rmacklin commented Apr 22, 2022

ShockwaveNN commented Apr 24, 2022

j-miyake commented Apr 24, 2022 •

edited

Add new Style/FetchEnvVar cop #10502

Add new Style/FetchEnvVar cop #10502

Conversation

j-miyake commented Apr 4, 2022 • edited

j-miyake commented Apr 7, 2022

bbatsov commented Apr 7, 2022

bbatsov commented Apr 8, 2022

j-miyake commented Apr 8, 2022

bquorning commented Apr 8, 2022

bbatsov commented Apr 9, 2022

bbatsov Apr 9, 2022

Choose a reason for hiding this comment

bbatsov commented Apr 9, 2022

j-miyake commented Apr 9, 2022 • edited

bbatsov commented Apr 10, 2022

j-miyake commented Apr 10, 2022

bquorning Apr 10, 2022

Choose a reason for hiding this comment

j-miyake Apr 11, 2022 • edited

Choose a reason for hiding this comment

j-miyake Apr 11, 2022

Choose a reason for hiding this comment

bbatsov Apr 10, 2022

Choose a reason for hiding this comment

j-miyake Apr 11, 2022

Choose a reason for hiding this comment

j-miyake Apr 12, 2022

Choose a reason for hiding this comment

bbatsov commented Apr 12, 2022

j-miyake commented Apr 13, 2022

bbatsov commented Apr 13, 2022

benpickles commented Apr 22, 2022

j-miyake commented Apr 22, 2022

rmacklin commented Apr 22, 2022

ShockwaveNN commented Apr 24, 2022

j-miyake commented Apr 24, 2022 • edited

Add new `Style/FetchEnvVar` cop #10502

Add new `Style/FetchEnvVar` cop #10502

j-miyake commented Apr 4, 2022 •

edited

j-miyake commented Apr 9, 2022 •

edited

j-miyake Apr 11, 2022 •

edited

j-miyake commented Apr 24, 2022 •

edited