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

Add `Style/RedundantFreeze` to remove redudant `.freeze` #32031

Merged
merged 1 commit into from Oct 1, 2018

Conversation

@yahonda
Contributor

yahonda commented Feb 17, 2018

Summary

Since Rails 6.0 will support Ruby 2.4.1 or higher # frozen_string_literal: true magic comment enabled by Style/FrozenStringLiteralComment cop is enough to make string object frozen.

Other Information

I'd like to hear from Rails committers for this change since Rails 5.2 will supports Ruby 2.2, this change itself should not be backported but also it could make other backports complex.

@rails-bot

This comment has been minimized.

rails-bot commented Feb 17, 2018

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@matthewd

This comment has been minimized.

Member

matthewd commented Feb 17, 2018

I wonder how many of these are extremely performance-critical. For most, it seems fine to backport to 5.2, just to make future maintenance easier: things being slightly slower on 5.2 + 2.2 doesn't sound like a big deal... I'm just not sure how we'd identify any where it makes a catastrophic difference.

Otherwise, we can wait a while and then merge this in a few months, when 5.2 backports have settled down.


I think we might want to keep the explicit freeze on the tests that specifically want a frozen string, for clarity. (Maybe?)

@yahonda

This comment has been minimized.

Contributor

yahonda commented Feb 17, 2018

Thanks for the review. I did not expect that backporting this change to 5-2-stable is possible.

I think we might want to keep the explicit freeze on the tests that specifically want a frozen string, for clarity. (Maybe?)

Agreed. I have excluded 3 test files which have tests expecting a frozen string in case of backporting and respecting the intention of the tests.

@jeremy

This comment has been minimized.

Member

jeremy commented Feb 17, 2018

Backporting to 5.2 is a nice idea. We could say Rails 5.2 supports Ruby 2.2, with this caveat, but prefers 2.3 or newer.

On the other hand, it's strange for our API to return differently mutable/frozen strings depending on the version of Ruby you're running.

@matthewd

This comment has been minimized.

Member

matthewd commented Feb 18, 2018

On the other hand, it's strange for our API to return differently mutable/frozen strings depending on the version of Ruby you're running.

We're already doing that on any other literals where we don't explicitly call .freeze, so it doesn't seem terrible... I guess there is a distinction between the performance-based freezings, and the ones where we actually care about making the value immutable (because it's being stored and reused in a constant, say).

I do think it's reasonable for us to strongly recommend a newer ruby than the oldest we support, and to bump that recommendation more freely: we want 5.2 to work on almost-already-EOL 2.2, but I don't see myself feeling bad it if doesn't work well.

@yahonda

This comment has been minimized.

Contributor

yahonda commented Feb 18, 2018

Excluded two more files changed by this commit. 8454aee

@yahonda

This comment has been minimized.

Contributor

yahonda commented Feb 23, 2018

Addressed conflicts. Since this pull request has been conflicting other commits
By separating commits by rubocop -a it is easy to rebase.

Will squash two commits when it is ready to merge.

@rafaelfranca rafaelfranca assigned matthewd and unassigned kaspth Feb 23, 2018

@yahonda

This comment has been minimized.

Contributor

yahonda commented Feb 23, 2018

It has been approved then I have squashed two commits.

@yahonda

This comment has been minimized.

Contributor

yahonda commented Sep 24, 2018

As #32971 has been merged to master I wanted this pull request reviewed again.

@@ -130,6 +130,15 @@ Style/FrozenStringLiteralComment:
- 'actionpack/test/**/*.ruby'
- 'activestorage/db/migrate/**/*.rb'
Style/RedundantFreeze:
Enabled: true
Exclude:

This comment has been minimized.

@matthewd

matthewd Sep 25, 2018

Member

In my 5 second test, Rubocop didn't seem to complain about redundant -"foo". If that's the case (and deliberate?), maybe we can consider switching to that for the first two. string_ext_test.rb seems more like it's missing a test for the unfrozen case (though I'd really prefer to see all those wrapped in an assert_deprecated anyway).

This comment has been minimized.

@kamipo

kamipo Sep 25, 2018

Member

Yeah, I also confirmed about redundant -"foo" is no offence for the cop when I commented.
Since we already replaced all String#dup to String#+@, replacing redundant String#freeze to String#-@ is fine to me.

This comment has been minimized.

@yahonda

yahonda Sep 29, 2018

Contributor

Thanks for the review. I have removed these files from the exclude list and replaced String#freeze with String#-@ where explicit string objects are necessary.

@@ -539,7 +539,7 @@ def translate_exception(exception, message)
end
end
COLLATE_REGEX = /.*\"(\w+)\".*collate\s+\"(\w+)\".*/i.freeze
COLLATE_REGEX = /.*\"(\w+)\".*collate\s+\"(\w+)\".*/i

This comment has been minimized.

@kamipo

kamipo Sep 24, 2018

Member

/.*\"(\w+)\".*collate\s+\"(\w+)\".*/i is not a string so this .freeze is not redundant.

This comment has been minimized.

@yahonda

yahonda Sep 24, 2018

Contributor

Thanks for the review. Let me confirm this review first. I think you mean regular expressions is not a string. It looks like Ruby itself does not support frozen regular expressions yet.
https://bugs.ruby-lang.org/issues/8948 I'm wondering if this .freeze is actually valid or not

This comment has been minimized.

@kamipo

kamipo Sep 25, 2018

Member

Looks like that https://bugs.ruby-lang.org/issues/8948 is a proposal for 3.times { /foo/f } returning the same regex object like a frozen string literal.
At least removing .freeze has an effect that changing the result of .frozen?.

irb(main):001:0> RUBY_VERSION
=> "2.5.1"
irb(main):002:0> /foo/.frozen?
=> false
irb(main):003:0> /foo/.freeze.frozen?
=> true
irb(main):004:0> /foo/.instance_variable_set(:@foo, "foo")
=> "foo"
irb(main):005:0> /foo/.freeze.instance_variable_set(:@foo, "foo")
Traceback (most recent call last):
        3: from /Users/kamipo/.rbenv/versions/2.5.1/bin/irb:11:in `<main>'
        2: from (irb):7
        1: from (irb):7:in `instance_variable_set'
FrozenError (can't modify frozen Regexp)

This comment has been minimized.

@matthewd

matthewd Sep 25, 2018

Member

While it does sound like Rubocop's being over-eager in declaring it redundant, I think this instance is fine in practice. It's not a value we return anywhere, so someone would have to be trying pretty hard to even find it... and at that point a const_set is just as easy.

This comment has been minimized.

@kamipo

kamipo Sep 25, 2018

Member

Yeah, this instance is fine in practice, and this is an only place where RuboCop's auto-correct affects in I reviewed all diff in this PR.
I suppose it is an issue for the cop, but I'm not sure whether RuboCop intentionally regard a regex literal like a string literal or not.

This comment has been minimized.

@yahonda

yahonda Sep 25, 2018

Contributor

I have opened a RuboCop issue rubocop-hq/rubocop#6331

This comment has been minimized.

@koic

koic Sep 27, 2018

Contributor

Thank you for opening the RuboCop issue. I think this is a false positive of RuboCop. I opened a PR rubocop-hq/rubocop#6333 to fix, but it may take some time to the next new release of RuboCop (maybe 0.59.3 or 0.60.0) and subsequent Code Climate support. Until then it will depend on hand craft.

- 'actionpack/test/controller/test_case_test.rb'
- 'activemodel/test/cases/type/string_test.rb'
- 'activesupport/test/core_ext/string_ext_test.rb'
- 'activesupport/lib/active_support/core_ext/string/strip.rb'

This comment has been minimized.

@kamipo

kamipo Sep 24, 2018

Member

This is not redundant freeze.

This comment has been minimized.

@kamipo

kamipo Sep 24, 2018

Member

This "".freeze is obviously redundant, there is no reason to exclude it.

gsub(/^#{scan(/^[ \t]*(?=\S)/).min}/, "".freeze).tap do |stripped|

@@ -130,6 +130,15 @@ Style/FrozenStringLiteralComment:
- 'actionpack/test/**/*.ruby'
- 'activestorage/db/migrate/**/*.rb'
Style/RedundantFreeze:
Enabled: true
Exclude:

This comment has been minimized.

@yahonda

yahonda Sep 29, 2018

Contributor

Thanks for the review. I have removed these files from the exclude list and replaced String#freeze with String#-@ where explicit string objects are necessary.

Show resolved Hide resolved .rubocop.yml
PATH = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/]/.freeze
FRAGMENT = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/\?]/
SEGMENT = /[^#{UNRESERVED}#{SUB_DELIMS}:@]/
PATH = /[^#{UNRESERVED}#{SUB_DELIMS}:@\/]/

This comment has been minimized.

@kamipo

kamipo Sep 29, 2018

Member

I re-searched by "/.freeze" in the diff and found extra affected regexs.

This comment has been minimized.

@yahonda

yahonda Sep 29, 2018

Contributor

Thank you for finding other false positives. Addressed and exclude 'actionpack/lib/action_dispatch/journey/router/utils.rb' also.

@kamipo

kamipo approved these changes Sep 29, 2018

Add `Style/RedundantFreeze` to remove redudant `.freeze`
Since Rails 6.0 will support Ruby 2.4.1 or higher
`# frozen_string_literal: true` magic comment is enough to make string object frozen.
This magic comment is enabled by `Style/FrozenStringLiteralComment` cop.

* Exclude these files not to auto correct false positive `Regexp#freeze`
 - 'actionpack/lib/action_dispatch/journey/router/utils.rb'
 - 'activerecord/lib/active_record/connection_adapters/sqlite3_adapter.rb'

It has been fixed by rubocop-hq/rubocop#6333
Once the newer version of RuboCop released and available at Code Climate these exclude entries should be removed.

* Replace `String#freeze` with `String#-@` manually if explicit frozen string objects are required

 - 'actionpack/test/controller/test_case_test.rb'
 - 'activemodel/test/cases/type/string_test.rb'
 - 'activesupport/lib/active_support/core_ext/string/strip.rb'
 - 'activesupport/test/core_ext/string_ext_test.rb'
 - 'railties/test/generators/actions_test.rb'

@kamipo kamipo merged commit b13a5cb into rails:master Oct 1, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

koic added a commit to koic/rubocop-rails_config that referenced this pull request Oct 1, 2018

@yahonda yahonda deleted the yahonda:remove_redundant_freeze branch Oct 27, 2018

toshimaru added a commit to toshimaru/rubocop-rails_config that referenced this pull request Oct 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment