Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Style/RedundantFreeze stop considering ENV values as immutable #10099

Merged
merged 1 commit into from Sep 22, 2021

Conversation

casperisfine
Copy link

Ref: #6784

ENV["foo"] returns a new mutable string on every call.

>> ENV["PATH"].frozen?
=> true
>> ENV["PATH"].object_id
=> 260
>> ENV["PATH"].object_id
=> 280

As such it's perfectly legitimate to freeze them in a pattern such as:

PATH = ENV["PATH"].freeze

@dvandersluis
Copy link
Member

Very interesting. Is this documented by ruby anywhere?

@casperisfine
Copy link
Author

Is this documented by ruby anywhere?

Not as far as I know. I checked ruby spec (https://github.com/ruby/spec/tree/master/core/env) but couldn't find anything about this.

But the documentations states:

Returns the value for the environment variable name if it exists:

Which depending on how you interpret it, suggest that it goes all the way to the actual process env (i.e. getenv(3)), and doesn't keep a cache of it. So it kinda implies a new string every time.

@dvandersluis
Copy link
Member

Thanks for digging into it. It'd probably be useful to document why ENV isn't included in the documentation for this cop IMO.

@casperisfine
Copy link
Author

To be honest it surprised me to see it included in the first place, I'm having trouble understanding the assumption here.

@dvandersluis
Copy link
Member

I think the assumption is that ENV works the same as a Hash, when that obviously isn’t the case. That being said, it probably is fine without explicit documentation.

Ref: rubocop#6784

`ENV["foo"]` returns a mutable string.

```ruby
>> ENV["PATH"].frozen?
=> true
>> ENV["PATH"].object_id
=> 260
>> ENV["PATH"].object_id
=> 280
```

As such it's perfectly legitimate to freeze them in a pattern such as:

```ruby
PATH = ENV["PATH"].freeze
```
@casperisfine
Copy link
Author

I think the assumption is that ENV works the same as a Hash

More than that, it assumed all the values were already frozen.

@dvandersluis dvandersluis merged commit 7fe8ae0 into rubocop:master Sep 22, 2021
@dvandersluis
Copy link
Member

Well you showed they are frozen right? 😆

In any case, thanks for this change!

@casperisfine
Copy link
Author

Well you showed they are frozen right? 😆

No they're not. They are mutable. You just get a new one on each access.

@dvandersluis
Copy link
Member

Why is ENV["PATH"].frozen? returning true then?

@casperisfine
Copy link
Author

Because I had a brainfart I guess -_-

@casperisfine
Copy link
Author

Ok, that explains why I didn't understand the initial reasoning...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants